[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

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

• 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 🚫 dangerJS against 16cb645

[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]

Hey everyone. It's been a while since there's been any significant movement on this PR and I'm not sure what next steps look like. Is this still something that's desired at some point? If so, what needs to happen to get this feature to a place where it could be discussed some more and eventually accepted?

The last feedback I remember had more to do with just the prioritization of work for the v5 release effort, but there were also some understandable concerns about modifying the internals of the existing Menu components' code. If the CascadingMenu components were more of a wrapper around the existing Menu components (if possible), would that be more palatable?

Thanks for any feedback. Just trying to decide what to do with this since it's been languishing for a while now.

[JeremyGrieshop]

Hey everyone. It's been a while since there's been any significant movement on this PR and I'm not sure what next steps look like. Is this still something that's desired at some point? If so, what needs to happen to get this feature to a place where it could be discussed some more and eventually accepted?

The last feedback I remember had more to do with just the prioritization of work for the v5 release effort, but there were also some understandable concerns about modifying the internals of the existing Menu components' code. If the CascadingMenu components were more of a wrapper around the existing Menu components (if possible), would that be more palatable?

Thanks for any feedback. Just trying to decide what to do with this since it's been languishing for a while now.

I ended up doing my own internal implementation in the meantime, using Popper. I assume this may be where Menu heads towards as well.

I thought perhaps this use case was for a small target audience, but if you sort all PR's by thumbs up, heart, whatever, it's consistently at the very top and has been for a long time.

[EsoterikStare]

I ended up doing my own internal implementation in the meantime, using Popper. I assume this may be where Menu heads towards as well.

I was under the impression Menu would be migrating to Popper internally at some point. I think it was mentioned by Olivier in this PR discussion early on.

I thought perhaps this use case was for a small target audience, but if you sort all PR's by thumbs up, heart, whatever, it's consistently at the very top and has been for a long time.

Yeah, I took note of how long this feature had been a request when I decided to try and contribute a solution *checks watch* about 2 years ago, now (wow). The request for a CascadingMenu has been around for ~4yrs now across various issues and PR attempts over that span of time, iirc.

I'm curious what the perceived priority level of a first-party solution to this is internally and I'd love some guidance on exactly what would be necessary from an approach and features standpoint to get a CascadingMenu solution merged in. Could any of the maintainers speak to this in any detail? I'm willing to completely start over on this if we can define a set of requirements from the beginning that will lead to an accepted PR.

[mnajdova]

@EsoterikStare first of all thank you for the tremendous effort. It took me some time to go over all the conversations happening on the PR :)

To talk a bit about what would take for the cascading menus to be accepted. It would have been the best if we would have started with features requirements first. To summarize the list that everyone has come up with in this PR:

1. Infinite nesting
2. Supports any implementation you can do with current Menu and MenuItem
3. Full Keyboard support
   You did very good job here!
4. ARIA support
5. RTL support
6. Very customizable
7. Only native, already existing components: Menu and MenuItem
   I am not sure about this one, I would rather have a dedicated component for the submenu
8. Zero impact on pre-existing Menu and MenuItem implementations, i.e. No breaking changes
   This is the most important requirement. This should have lead us to implement the new behavior in the lab even from the start. One way we could do this is by extracting the logic of the Menu in a hook, and reusing it in a brand new component in the lab.
9. Agree on the API
   This is important. For example, in the current implementation, we have a more or less empty SubMenu component, which in my opinion doesn't make much sense. We are using cloneElement which we saw in the past introduced bunch of problems, so I would propose we use context instead.
   There is still some work to be done here even in terms of API, for example there are props like onArrowRightKeydown instead of onKeyDown. This is just one thing I noticed, haven't look too deep in the API to be honest.
10. Keeping previously open sub-menu highlighted.
11. Fix focus visible style not correctly applied when closing a sub menu.
    There is definitely some bug in the isFocusVisible hook. Something similar was reported in the Slider too - Focus-visible applying with mouse on Slider #29506
12. Use a triangle of interactivity
13. Alternate placement when there isn't enough space: (might be deferred until Menu Popover is replaced with Popper)

---

Currently @michaldudak is working on providing an unstyled Menu component and useMenu hook, that will be part of the @mui/base package. What I think would be a great next step is, once the hook is done, to create a new component in the lab package, that would use this hook, and try to build a Submenu with it, without necessarily changing the components we already have in the @mui/material package (not sure if it would be possible, but at least to try). The benefits I see with this path is:

1. we test out the headless hooks
2. we have an implementation that does not impact existing @mui/material users (for example we won't break anything that currently works)
3. we can merge more often and iterate in the lab and even do breaking changes (we could have faster feedback loop)

What is great with this PR we have right now is that we already have bunch of tests we can use in order to test the behavior, which definitely means a lot. cc @mui-org/core anyone has anything to add/propose?

---

References/benchmarks:

• https://height.app/blog/guide-to-build-context-menus

[tonysepia]

I just wanted to thank @EsoterikStare for pursuing this, as I am also eagerly anticipating an official MUI way to do this!

[EsoterikStare]

@mnajdova Thanks for the quick and thorough response! I totally agree that it would have been better to do some more requirements gathering before this attempt was made. It was one of those things that started as a Spike, then a POC that turned into a burning need as we got close to something serviceable, and, though we were wanting to contribute the whole time, we weren't sure we'd be allowed to legally until pretty late in all that work.. Hind sight being what it is, of course changing the internals of a stable component was cause for concern, among other things. Having this be a separate component that could live and evolve in Lab makes way more sense. Lets collaborate a bit better this time around. =)

Having the Menu logic extracted into a hook does seem like a pretty nice starting point this time around. I assume all of the moving parts (Menu, MenuList, MenuItem) will be encompassed in that effort? Any delivery estimate on that work so I can roughly plan to start looking at that?

Finally, does it make sense to just close this MR? Since I'll basically be starting over, and it seems pretty clear this won't be getting merged in anything like it's current state, I'm not sure if it makes sense to leave this open. (Also, I made a silly mistake in my haste and naivete and made this PR against my fork's master branch, which has been a pain for the last two years and is a mistake I won't ever make again, lol.)

[mnajdova]

Having the Menu logic extracted into a hook does seem like a pretty nice starting point this time around. I assume all of the moving parts (Menu, MenuList, MenuItem) will be encompassed in that effort? Any delivery estimate on that work so I can roughly plan to start looking at that?

@michaldudak should have a PR up soon, I would say it should not take more than two weeks from now roughly. Michal, correct me if I am wrong :)

Finally, does it make sense to just close this MR? Since I'll basically be starting over, and it seems pretty clear this won't be getting merged in anything like it's current state, I'm not sure if it makes sense to leave this open.

Sure, if it is easier to start from scratch let’s close it. I’ve summarized all requirement that popped out while reviewing, so we should be good.

Also, I made a silly mistake in my haste and naivete and made this PR against my fork's master branch, which has been a pain for the last two years and is a mistake I won't ever make again, lol.

And us changing the main branch we did sure didn’t help :))

Having all these said I believe we have an agreement: wait for Michal to implement the headless hook for the menu, and start working on a component inside the lab, having in mind the requirements listed above.

[michaldudak]

Hi @EsoterikStare! The implementation I'm working on still has a few rough edges, but I should be able to create a PR next week. I will let you know when it's published so you can take a look and give your feedback on it, or even contribute yourself if you'd like to.

As for the scope - there will be hooks and unstyled components to build menus (MenuUnstyled and MenuItemUnstyled). The MenuList was rewritten as useListbox hook and it's already merged to master (it's powering the SelectUnstyled).

Note that since this is basically a rewrite, the new components will likely need some time to mature and won't be production-ready from the start. We will not change the Menu implementation in @mui/material to use the new hooks right away either (it'll likely happen around v6). As @mnajdova suggested, using them to create a component in the lab should be a safe option, though.

[EsoterikStare]

@michaldudak Sounds good. Thanks for the heads up and the extra info about what to expect in terms of the structure!

Just to set expectations, I won't promise any kind of timeline on this second effort since I'll mostly be doing it on my own time this time around, it seems like, and that time is precious to me. But I'll start hacking away on an MVP when I can and hopefully get somewhere with that at some point in the not-too-distant future.

@mnajdova I'll close this PR when I bump my fork's master to latest mui to begin work on the new effort. I'll leave this open until then so it can remain a discussion forum leading up to that unless you feel it needs to be closed sooner.

Thanks everyone. =)

[michaldudak]

Just to set expectations, I won't promise any kind of timeline on this second effort since I'll mostly be doing it on my own time this time around, it seems like, and that time is precious to me. But I'll start hacking away on an MVP when I can and hopefully get somewhere with that at some point in the not-too-distant future.

No worries! We don't expect you to create a solution. You're free to do so if you wish, but it's entirely up to you.

[EsoterikStare]

No worries! We don't expect you to create a solution. You're free to do so if you wish, but it's entirely up to you.

Oh, I want to be the person that figures this out and finally gets a solution available in MUI. I just want to be clear up-front about the amount of time I'm able to commit to this at the moment. Life is a little more exciting than usual for me right now and for the next few months. =)

[mnajdova]

Oh, I want to be the person that figures this out and finally gets a solution available in MUI. I just want to be clear up-front about the amount of time I'm able to commit to this at the moment. Life is a little more exciting than usual for me right now and for the next few months. =)

No worries :) feel free to drop me your email (via Twitter or email) and I can add you to our Slack channel, it would be easier to reach out to everyone in case we don't respond on GitHub :)

[mnajdova]

We talked with @oliviertassinari on this matter too. It may be even better if we try to build this functionality directly in the unstyled Menu (and useMenu hooks), instead of building new component in the lab. For v6 , the Material Design components will anyway be built on top of the unstyled components (hooks), so we will have the functionality there too. Of course, this would go after Michal has the first implementation ready in the @mui/base package.

[EsoterikStare]

@mnajdova That sounds like a worthwhile thing to explore. Seems like keeping all that logic in the hooks would be ideal since that's kind of the point of having hooks-based components. I'll shoot you an email from mine so you can add me to the Slack group. I appreciate the offer! =)

[EsoterikStare]

Alright. The time has come to put this PR to rest. I've archived all of the work that was done up to this point in this branch over on my fork, in case anyone wants to reference it. I'll begin work on a new effort, based on the new unstyledMenu hooks, soon, but it may be a little while before that's ready.

Thanks everyone!

[JeremyGrieshop]

Alright. The time has come to put this PR to rest. I've archived all of the work that was done up to this point in this branch over on my fork, in case anyone wants to reference it. I'll begin work on a new effort, based on the new unstyledMenu hooks, soon, but it may be a little while before that's ready.

Thanks everyone!

If I can help in any way, over in your new branch, let me know. I'd be happy to contribute code, tests, etc.