[Menu] Support Cascading Menus

[EsoterikStare]

Preview: https://deploy-preview-20591--material-ui.netlify.app/components/menus/#cascading-menu

I'd like to submit this solution to Cascading Menu support. Features of this solution include:

• Infinite nesting
• Supports any implementation you can do with current Menu and MenuItem
• Full Keyboard support
• ARIA support
• RTL support
• Very customizable
• Only native, already existing components: Menu and MenuItem
• Zero impact on pre-existing Menu and MenuItem implementations, i.e. No breaking changes

It uses a familiar and intuitive implementation pattern:

And best of all, it works.

Please take a look and let me know what you think. Thanks!

Closes #11723

TODO

• [x] Agree on the API
• [x] Replace assert with expect
• [x] Replace enzyme with testing-library
• [x] Keeping previously open sub-menu highlighted. For instance:

• [x] Fix focus visible style not correctly applied when closing a sub menu.

• [ ] Use a triangle of interactivity:

• [ ] Alternate placement when there isn't enough space: (might be deferred until Menu Popover is replaced with Popper)

[mui-pr-bot]

Details of bundle changes

@material-ui/core: parsed: +9.00% , gzip: +10.23%

Generated by :no_entry_sign: dangerJS against 16cb645922a4a3f6a86852a23e3d627ca8317478

[EsoterikStare]

I'm not sure what to do about the ci/circleci: test_static failure. It asks me to run prettier because of two files but, when I do that, prettier doesn't make any changes. Not sure how to proceed.

Edit: Strangely, if I run yarn prettier write then yarn prettier check-changed then yarn prettier write-changed, I can then search the sea of changes and just commit the things that test is complaining about. But, just running yarn prettier did nothing locally, while the remote test is still unhappy.

Any explanation of this would be appreciated as I still don't know why I had to do that, but at least it's happy now. =)

[EsoterikStare]

Also, regarding the test_browser-1 failures, it looks like one of the new unit tests is failing in several different browsers. One of the browsers it's failing in is Chrome, which is what I'm testing with, but I have no failing tests locally.

What is the best way to go about troubleshooting/recreating what's going on with the circleci test failures locally?

Edit: I re-read the contributing document and noticed that I can run yarn test:karma for this (missed that before) and reproduced the failure locally. I'll work on the test and, hopefully, the other browser failures will be addressed too.

[oliviertassinari]

In case you are bored, this one could be a fun challenge 😆 https://www.technologyreview.com/2013/03/09/179477/the-ingenious-engineering-trick-that-makes-amazon-menus-usable/

(macOS implements it)

[EsoterikStare]

In case you are bored, this one could be a fun challenge 😆 https://www.technologyreview.com/2013/03/09/179477/the-ingenious-engineering-trick-that-makes-amazon-menus-usable/

@oliviertassinari Oh man. That's brilliant. Maybe a future improvement! 😉

[eps1lon]

Any explanation of this would be appreciated as I still don't know why I had to do that, but at least it's happy now. =)

We recently switched to prettier v2. You probably had either an older version installed or an outdated branch.

[EsoterikStare]

Thanks, @oliviertassinari! Excited to be part of the v5 milestone!

Is there anything we can do to make it into the upcoming release? If there are any concerns or shortcomings that could be addressed that would get us over that line, we're more than willing to make those top priorities.

I know Cascading Menu support has been an open issue for a very long time and figured it would be nice to get that closed and on the list of improvements as soon as possible.

Thanks again for reviewing. =)

[sergiuiscoding]

Hi, @EsoterikStare,

Nice work with the cascading menu.

Quick fix for scroll events on subMenus. Currently subMenus cannot be scrolled if they don't fit in the available screen space.

The [classes.enablePointerEvents]: isSubMenu line in Menu.js should be moved from the MenuList to the PaperProps on Popover in order to let scroll events pass through, as Paper is the actuall scroll element.

[EsoterikStare]

Hi, @EsoterikStare,

Nice work with the cascading menu.

Quick fix for scroll events on subMenus. Currently subMenus cannot be scrolled if they don't fit in the available screen space.

The [classes.enablePointerEvents]: isSubMenu line in Menu.js should be moved from the MenuList to the PaperProps on Popover in order to let scroll events pass through, as Paper is the actuall scroll element.

Thanks! And, nice catch. I'll fix that up in the near term and push the fix.

[EsoterikStare]

Hi, @EsoterikStare,

Nice work with the cascading menu.

Quick fix for scroll events on subMenus. Currently subMenus cannot be scrolled if they don't fit in the available screen space.

The [classes.enablePointerEvents]: isSubMenu line in Menu.js should be moved from the MenuList to the PaperProps on Popover in order to let scroll events pass through, as Paper is the actuall scroll element.

I took a quick look at this while I was getting my fork updated and I can't reproduce any scrolling issue with the current implementation. I added a bunch of additional MenuItems to the zoom subMenu in the demo and it scrolls just fine. In fact, if I remove the class assignment from MenuList, it breaks the ability to open any subMenus past the first level even if I've added it to the PaperProps of Popover.

Are you encountering a problem you could document setup and reproduction steps for? If you are, does just adding the [classes.enablePointerEvents]: isSubMenu to PaperProps on Popover fix your problem instead of also removing the class from MenuList?

I'm using lastest Chrome on latest Windows 10, just in case it might be a browser/OS behavioral difference.

For sake of ease, here is what I added to the zoom subMenu to try and reproduce a scrolling problem:

const candies = [
    'Lifesavers',
    'Hersheys Kiss',
    'Skittles',
    'Twizzlers',
    'Ferrero Rocher',
    'Reeses Pieces',
    'Dum Dums Pop',
    'Starburst',
    'Swedish Fish',
    'Airheads',
    'Kitkat',
    'Almond Joy',
    'Twix',
    '3 Musketeers',
    'Milky Way',
    'Tootsie Roll',
    'Tootsie Pop',
    'Werthers',
    'Andes Mint',
    'Sour Patch Kids',
    'Milk Duds',
    'Sweet Tarts',
    'Nerds',
    'Laffy Taffy',
    'Gobstopper',
    'Mounds',
    'Snickers',
    'York Peppermint Pattie',
    'Heath Bar',
    'Jolly Rancher',
    'Blow Pop',
    '100 Grand',
    'Crunch',
    'Butterfinger',
    'Baby Ruth',
    'Dove Bar',
    'Lemonhead',
    'Warheads',
    '5th Avenue',
    'Bar None',
    'Clark Bar',
    'Krackel',
    'Bueno',
    'Lindt Chocolate Bar',
    'Lindt Lindor Truffles',
    'Mars Bar',
    'Mr. Goodbar',
    'Milka',
    'Pay Day',
    'Take 5',
    'Toblerone',
    'U-No Bar',
    'Wonka Bar',
    'Whatchamacallit',
    'Runts',
    'Bubble Tape',
    'Candy Buttons',
    'Candy Cigarettes',
    'Candy Corn',
    'Dots',
    'Fun Dip',
    'Junior Mints',
    'Peeps',
    'Pop Rocks',
    'Pixie Stix',
    'Pez',
    'Raisinets',
    'Razzles',
    'Smarties',
    'Whoppers',
    'Topic',
    'Hot Tamales',
    'Life Savers Gummies',
    'Cookie Dough Bites',
    'Spree',
    'Mentos',
    'Tic Tac',
    'Sugar Babies',
    'Haribo Starmix'
  ].map(candy => <MenuItem onClick={handleClose}>{candy}</MenuItem>)

and then:

<MenuItem
  subMenu={
    <Menu>
      {candies}
    </Menu>
  }
>
  Zoom
</MenuItem>

[sergiuiscoding]

@EsoterikStare, I've just tried your use-case, and I can still reproduce the problem. Adding the "enablePointerEvents" class on PaperProps fixes the problem without having to remove it from MenuList (but i think it doesn't make sense to keep it there in this case). I'm on latest Chrome, on macOS 10.15.4

Edit: Cannot reproduce the problem on Firefox, the scroll works fine.

[EsoterikStare]

@sergiuiscoding, Yeah, I'm not fond of leaving it in both places either but removing the class from MenuList definitely breaks functionality for at least some environments.

This sounds like it may be a MacOS specific issue in Chrome. I'll spare you the async verification duties and enlist a local friend to help me develop an approach that works in both ecosystems since I don't have a Mac. I'll make sure to @mention you when I push something I think will be a good path forward so you can verify it fixes you too.

In the meantime, if you (or anyone else) get(s) something going on your end, don't hesitate to PR my fork, or just drop me the details in another message.

Thanks for the additional details!

[EsoterikStare]

@eps1lon, @oliviertassinari, Can someone verify that argos failure? I think it's probably not a real failure...

[EsoterikStare]

@sergiuiscoding I think I fixed the scrolling issues you found by moving that class to PaperProps, as you suggested, without leaving it anywhere else. This seems to fix the scrolling issues we could reproduce. Will you pull and let me know if your scrolling issue is also fixed?

Thanks again for catching and reporting that. Cheers!

[EsoterikStare]

@sergiuiscoding, can you verify that your scrolling issues are gone, please? Thanks!

[sergiuiscoding]

@EsoterikStare, looks good, seems to be working fine.

[crampsalot]

Hello ! Any idea what the timeframe is for when this can appear in a release of material ui ? Also, any suggestions/tips for including whatever changes you've made here directly in my project (if it is possible at all) while waiting for it to be released publicly/officially ? Thanks.

[EsoterikStare]

Hello ! Any idea what the timeframe is for when this can appear in a release of material ui ? Also, any suggestions/tips for including whatever changes you've made here directly in my project (if it is possible at all) while waiting for it to be released publicly/officially ? Thanks.

@crampsalot,

The company I work for has been using this in our internal source by just copying the Menu and MenuItem source from my fork and implementing them as code we own, in place of the @material-ui/core components. It's a little risky because you'll need to keep an eye out for any other Menu or MenuItem changes coming from @material-ui/core and manually patch your files, but you'll only be forking/owning those two files, so to speak, so it's far less risky than depending on some random person's entire fork (me), which may or may not be up to date on the same schedule as the source project. =)

We've also found and fixed a few additional things along the way and have a few more in our backlog to address in the near term. When we've got more fixes ready, this MR will update and you'll probably also want to manually patch your code accordingly.

Hope that helps. I'm not sure when this will make it into the v5 code, but I'll be ready to quickly address feedback as it starts to flow in when the review of this MR begins in earnest.

[oliviertassinari]

Work on v5 has started, if we could rebase on the next branch, it would be great. I think that we can also look into options that require breaking changes, it's fine now.

[EsoterikStare]

@oliviertassinari I'll get that sorted asap.

[EsoterikStare]

@oliviertassinari I've updated my master branch (the unfortunate source of this PR) to the next branch and re-added the original code changes to clean up the fairly messy merge history of the previous source branch so I can more easily rebase and stay up to date. To everyone looking at this and seeing it comes from my master branch, I'm sorry. That was a poor choice of source branch for my PR. Just know that this is now based on the next branch despite the source branch name.

This is my second time managing an OSS contribution (first one of this scale) and I'm definitely making mistakes and learning from them. =)

[EsoterikStare]

One of the quirks of this pattern of re-using a component at a different level of a recursive structure is that the props api may not have the same requirements at different levels. For Menu, there is only one required prop: open, but when used as a subMenu it's not only not requred, but if you pass a value, it will not be respected since that is managed/orchestrated by its parent MenuItem, in the current implementation.

This creates an interesting problem for TS typing. The simplest solution is to just make the open prop optional in the d.ts, but I'm not sure if that would create more confusion. Another possibility I've been thinking about is to try and implement some of the fancy new conditional typing that TS now supports, but I'm not sure it would actually be a way forward.

I'd appreciate any feedback/thoughts about these two approaches or any other ideas for handling the difference of API requirements in different implementation contexts. TIA!

[mbrookes]

@eps1lon Any suggestions on the TS question?

[EsoterikStare]

I'd rather see this solved with a separate SubMenu component that is controlled via context rather than cloneElement. That way we don't have to overload the existing Menu component and typing i.e. reasoning will solve itself.

First, apologies for the extreme delay in my response to this feedback. I'll be on vacation for the next couple of weeks, but when I return, I should be able to be more responsive. In the meantime, my teammates have access to my fork of material-ui as contributors and will be able to address any burning issues while I'm away for the next couple weeks.

With that said:

I've made a thin wrapper component around the heavily modified Menu that basically just exists to omit that open prop from the d.ts. I imagine that you may be envisioning moving all of the Menu modifications into a SubMenu component, (same for the MenuItem?) but this doesn't address that yet. Hopefully, this is at least in the direction you were imagining.

In addition I've refactored the problematic, old ref pattern in MenuItem and removed that useless destructure and reference of key.

[EsoterikStare]

I have a regression in functionality that I can't seem to understand the source of. If someone could point me in the right direction, I'd be very grateful:

This line exists only to pass focus to the MenuItem that's the parent of the subMenu that's about to be closed by arrowing left with the keyboard. It worked as of about 2020-07-20, but when I rebased on the latest commits to the next branch, my unit test for that started failing (as you'll see in the checks), and, after investigating, I discovered the test correctly found a change in behavior.

Basically, that line used to apply the Mui-focusVisible class to the parent MenuItem. Now it doesn't. I'm not sure why and I can't seem to find any relevant changes. Any ideas?

[EsoterikStare]

@oliviertassinari, @eps1lon,

Sorry to @ you. I didn't get a response to my question, above, about the test failure when just generally asking the question and I'd like to get this green again if you have a few moments to help me understand what may have changed. It seems possibly related to the changes around focusVisible, but I can't find anything directly related to the change in how focus is applied in a Menu, MenuList, or MenuItem.

FWIW, I'm also working on improving the new passthrough SubMenu component to work with the react-docgen script since that's not happy either.

[eps1lon]

Sorry to @ you.

That's actually desired. I unsubscribed so that I won't get notified on every push.

I'd suggest using testing-library instead and look at the other tests asserting on focus-visible.

[oliviertassinari]

@eps1lon If you have been considering to switch to email notifications (instead of using GitHub UI notifications), here is one advantage: you can create a filter that deletes all notifications that are specific to commit pushes, I have discovered that recently, loved using it :)

[EsoterikStare]

Sorry to @ you.

That's actually desired. I unsubscribed so that I won't get notified on every push.

I'd suggest using testing-library instead and look at the other tests asserting on focus-visible.

So, the test isn't the problem, here. I can manually reproduce the failure that the test is catching.

Essentially, when calling .focus() on the ref to the parent MenuItem when closing a subMenu with keyboard, it still correctly focuses the correct MenuItem but it doesn't look focused. It's a cosmetic failure only, but it's an important one, I'd argue. The thing that changed is that just calling .focus() on that ref used to also apply the Mui-focusVisible class. Now it doesn't.

Nothing in my code changed; this happened after a rebase on the latest next changes. I've looked and looked but I cannot find the relevant change in the underlying code from next. I'm probably just missing something simple, but I was hoping that might just ring a bell for one of you since you're looking at/thinking about the larger set of changes all the time.

I may need to update the tests once the regression is fixed, but that's a different problem.

[EsoterikStare]

@eps1lon ☝️ (forgot to at you)

[JeremyGrieshop]

@EsoterikStare Do you know an exact commit change on next that breaks it? It's hard for me to imagine that focus(), a DOM function, would have ever applied a style property, unless focus() was ahead in the prototype chain, or an observable listened for it and applied it. I'm probably in over my head on this, but I'd like to see this PR get merged really badly and will do whatever it takes to help, lol.

[EsoterikStare]

@JeremyGrieshop I haven't been able to narrow it down to a specific commit by looking at the diffs. I'll have to revert to a working state and patch one commit at a time to get that info, most likely, and I haven't found the time to go through that tedium yet. I might be able to work on that soon.

Re: that class getting applied, yeah, I have no idea why that ever worked, looking back on it. But the fact remains that before updating I could just call .focus() and that element would get the focusVisible class applied and appear to be focused, then (crucially) the focusVisible class would be removed when focus changed. I'm not married to the current approach. We just need a way to make the focused element appear focused when you arrow left from a subMenu. /shrug.

It's a little tricky to do correctly because if you just naively apply the focusVisible class without some kind of a listener to clean it up when it's no longer focused, you end up with a weird visual that seems like you have more than one thing focused...

[JeremyGrieshop]

@EsoterikStare The commit made 3 days before you noticed your regression seems pretty notable ("Remove focus-visible if focus is re-targetted"):

https://github.com/mui-org/material-ui/commit/e58cc23df9e262a0f95c822504ac6c019b94407d#diff-05515339089dff752e7376b3a83dbe3b

The focus state for ButtonBase now uses a ref, and only sets the setFocusVisible() when it's not already determined to be focused via that ref.

[EsoterikStare]

@EsoterikStare The commit made 3 days before you noticed your regression seems pretty notable ("Remove focus-visible if focus is re-targetted"):

e58cc23#diff-05515339089dff752e7376b3a83dbe3b

The focus state for ButtonBase now uses a ref, and only sets the setFocusVisible() when it's not already determined to be focused via that ref.

Thanks! Serendipitous timing! I should have some time to look at this again for the first time in a few weeks. I'll take a look and see if that doesn't get me a little further.

[mbrookes]

@EsoterikStare git bisect might help you there.

[EsoterikStare]

@JeremyGrieshop, I Just confirmed 100% that the commit you suspected is the culprit. (e58cc23). It makes sense on a gut level that those changes could have resulted in this new behavior... but I still don't have my head around exactly what I can do about it. Still working on a solution.

@mbrookes, Thanks for suggesting git bisect! That's a sweet feature, but in this case it doesn't cut it because I need my commits on top of whatever state bisect puts my project into so I can see if the bug exists. Since it goes to a part of the timeline before my Cascading Menu changes are in place, I have no means of confirming if the bug exists there or not.

Ultimately, I poked at using git rebase --onto which seems like it may have worked if I had more time to sort through all the merge conflicts but, ultimately, I just ended up doing what I should have tried in the first place and directly reverted just the one commit @JeremyGrieshop pointed at and it immediately started working again.

@eps1lon, since your commit is the one that caused the behavior change, could you take another look and offer any advice on how to go about restoring the previous behavior described here: https://github.com/mui-org/material-ui/pull/20591#issuecomment-682021167

Thanks everyone!

[mbrookes]

in this case [git bisect] doesn't cut it because I need my commits on top of whatever state bisect puts my project into so I can see if the bug exists

Off, topic, but it's an interesting subject please indulge me 🙂 ... in that case I think you you could squash your commits into a single one on a new branch or detached HEAD (to preserve your working branch), then git cherry-pick that commit onto the detached head that git bisect gives you. You would still have to deal with merge conflicts, but that's generally easier with a single commit. When you bisect again your cherry-picked commit gets tossed. Rinse, repeat.

All moot though, since @JeremyGrieshop nailed it! 😁

[EsoterikStare]

Could someone with Safari try and figure out what's going on with that test_browser-1 failure? It seems like an assertion is failing and causing a hang that prevents done() from firing, so I'm not sure what the actual failure might be or how to troubleshoot without a local Safari browser... TIA!

EDIT: This turned out to be a transient failure. Originally Menu tests were timing out in Safari on the event callbacks > exiting > should fire callbacks test, then, after removing three of the four assertions in that test and pushing, I saw Autocomplete time out in Firefox but Menu happily passed all tests in all browsers... So I reverted the changes to the Menu tests, putting it back to the initial state and everything passed... /shrug. Maybe it's worth revisiting the 2000ms timeout on async operations in the karma tests?

[EsoterikStare]

@eps1lon, Seems like there is a transient failure in the argos tests here. If you can confirm, I think I've got a green build here. I had to comment out all the Cascading Menu tests and will be circling back on that today along with the remaining conversations that are ongoing in the review.

[EsoterikStare]

Great work so far! Could you get CI in a shape that it's marked as green and annotate the assertions that are not the intended behavior as // FIXME: @eps1lon so that it's easier for me to spot where I need to take a look?

@eps1lon, I added a // FIXME: @eps1lon to the portion of the test that was breaking after the change to focus behavior in e58cc23. I narrowed the change in behavior down to two files that are the relevant changes: ButtonBase.js and useIsFocusVisible.js. I suspect the relevant change is in the useIsFocusVisible.js file since the changes to ButtonBase.js seemed mostly to just be updating the consumption of that util, but I couldn't test one without the other in my confirmation and didn't get past that bit of confirmation before you responded and I switched gears to addressing feedback.

All of that said, none of my Cascading Menu tests are happy at the moment and that's what I plan to be working on for the last half of my day.

[EsoterikStare]

Also, I'm in the habit of letting the comment owner resolve the comment in a code review, so I'll leave those open unless you say otherwise or resolve them, @eps1lon.

[eps1lon]

General advise for tests: It can be ok to share helpers between tests e.g. a CascadingMenu component. However, sharing state between tests leads to very hard to debug tests e.g. sharing wrapper instances. Try to reduce you tests to the minimal code required and don't DRY your tests. When debugging tests it costs time to entangle what is required for the test and what isn't. So while it's faster to write the test initially it'll cost time when debugging. In almost all cases this results in a net loss of productivity because we only write once but debug more than once.

We learned the hard way the sharing wrapper instances between tests are problematic so we try to avoid them as much as possible. And for a unit test a ~70L component is just too big.

[eps1lon]

I looked at the test and from what I can tell the problem stems from TrapFocus still being active when you move the focus on ArrowLeft. Have you tried relying on our restore focus logic instead i.e. ArrowLeft simply closes and TrapFocus restores the focus to the previously focused item which should be the parent item?

[EsoterikStare]

General advise for tests: It can be ok to share helpers between tests e.g. a CascadingMenu component. However, sharing state between tests leads to very hard to debug tests e.g. sharing wrapper instances. Try to reduce you tests to the minimal code required and don't DRY your tests. When debugging tests it costs time to entangle what is required for the test and what isn't. So while it's faster to write the test initially it'll cost time when debugging. In almost all cases this results in a net loss of productivity because we only write once but debug more than once.

We learned the hard way the sharing wrapper instances between tests are problematic so we try to avoid them as much as possible. And for a unit test a ~70L component is just too big.

Good to know. We certainly don't need all of that depth in every test, so it would allow us to only be as complex as we need on a per test basis, which seems nice. I'm already having to fix those tests anyways, so maybe I'll try refactoring a bit while I'm in there to move away from the shared wrapper setup.

[EsoterikStare]

I looked at the test and from what I can tell the problem stems from TrapFocus still being active when you move the focus on ArrowLeft. Have you tried relying on our restore focus logic instead i.e. ArrowLeft simply closes and TrapFocus restores the focus to the previously focused item which should be the parent item?

I haven't. I wasn't aware of that functionality. I'm pretty sure the entire reason we did what we did there with the more imperative ref.focus() was because we tried just closing it before and it didn't appear focused. Maybe you fixed that behavior with your changes and consequently broke our workaround. =)

I'll give that a try once we generally have working tests again and let you know. Thanks for taking a look at that.

[EsoterikStare]

@eps1lon not sure who to @ about argos verifications, but I'm getting the same argos failure as before. Looks like there's a rogue phone icon in the docs-components-icons/FontAwesomeIconSize.png snapshot... Not sure what to make of it, but feels unrelated to any of the changes here...

Looks like this is the only thing preventing a green build.

[eps1lon]

Don't mind argos if it seems unrelated. Visual regression tests just a bit flaky at times.

[EsoterikStare]

I looked at the test and from what I can tell the problem stems from TrapFocus still being active when you move the focus on ArrowLeft. Have you tried relying on our restore focus logic instead i.e. ArrowLeft simply closes and TrapFocus restores the focus to the previously focused item which should be the parent item?

I haven't. I wasn't aware of that functionality. I'm pretty sure the entire reason we did what we did there with the more imperative ref.focus() was because we tried just closing it before and it didn't appear focused. Maybe you fixed that behavior with your changes and consequently broke our workaround. =)

I'll give that a try once we generally have working tests again and let you know. Thanks for taking a look at that.

@eps1lon So I played with this idea a bit and it doesn't seem to work like I expected. Also, that's basically already what's being done when ArrowLeft is triggered: it just tells the parent menu to close the submenu that's open, which should trigger that trapFocus logic you mentioned. So, it seems like something else may be going on.

The rest of the tests are now working again, so it's just the remaining regression related to that that one assertion near the // FIX ME tag that needs a solution.

I'm planning on addressing some of the remaining feedback items on Monday as well. Have a good weekend everyone!

[EsoterikStare]

Hey @eps1lon, If you have a bit more time today, I think I could use some more of your thoughts on the focus behavior regression. I see what you're saying about the TrapFocus component potentially being in play, but the restoreFocus behavior in there doesn't seem to be working like I expect and I haven't been able to figure out a way to address that. I'm not even sure if there's maybe just a good reason it wouldn't work here that I'm missing.

Could you or someone else with knowledge around this weigh in a little more when you get a chance? I'm still struggling to fix this and my window for being able to devote a large amount of my time to this is closing, so I'd like to address this and just have the remaining review feedback to worry about, if possible. Thanks in advance!

[EsoterikStare]

It looks like the main reason all of the rest of the arrow key navigation looks focused correctly is because of the MenuList moveFocus() utility function and the autoFocus behaviors that fire when a Menu is intially rendered. Is there a way we could re-fire MenuList logic that fires when the Menu initializes without actually causing a rerender or anything janky like closing and opening the parent menu again very quickly?