[Joy][Menu] Menu stays open - `onClose` not called

enricoros commented 1 year ago

Duplicates

[X] I have searched the existing issues

Latest version

[X] I have tested the latest version

Steps to reproduce 🕹

Link to live example: https://codesandbox.io/s/bold-fog-dc2lz2?file=/Demo.tsx

Steps:

Click on the Button to open the menu
Click away from the menu to close the menu

Current behavior 😯

Menu stays open even when clicking away - onClose is not called:

Expected behavior 🤔

onClose is called, and the menu closes

Context 🔦

I've ported my application to the newly released Joy UI ^5.0.0-beta.0, and most menus in the application stay open. Selects close normally, but I cannot close menus. The repro shows the minimal code that used to work pre-beta.

Your environment 🌎

npx @mui/envinfo

``` System: OS: Windows 10 10.0.22621 Binaries: Node: 18.16.1 - C:\P\apps\nodejs\node.EXE Yarn: Not Found npm: 9.5.1 - C:\P\apps\nodejs\npm.CMD Browsers: Chrome: Not Found Edge: Spartan (44.22621.2070.0), Chromium (115.0.1901.188) npmPackages: @emotion/react: ^11.11.1 => 11.11.1 @emotion/styled: ^11.11.0 => 11.11.0 @mui/base: 5.0.0-beta.9 @mui/core-downloads-tracker: 5.14.3 @mui/icons-material: ^5.14.3 => 5.14.3 @mui/joy: ^5.0.0-beta.0 => 5.0.0-beta.0 @mui/material: 5.14.3 @mui/private-theming: 5.13.7 @mui/styled-engine: 5.13.2 @mui/system: 5.14.3 @mui/types: 7.2.4 @mui/utils: 5.14.3 @types/react: ^18.2.18 => 18.2.18 react: ^18.2.0 => 18.2.0 react-dom: ^18.2.0 => 18.2.0 typescript: ^5.1.6 => 5.1.6 ```

juliushuck commented 1 year ago

I also have the problem with a menu not closing. I have a custom button that has a loading state inside the menu. I want the menu to stay open until the user clicks somewhere else (not on the modal).

I'm currently using the new version with Dropdown, MenuButton, etc. But I also had the issue inside the beta version.

Also the disabled property does not seem to work on the MenuButton

Abhushit commented 1 year ago

I checked the problem with menu not closing. Using Dropdown and MenuButton, it is working fine.

juliushuck commented 1 year ago

@Abhushit Yes, "uncontrolled" also works for me. I tried to control the open state here: https://codesandbox.io/s/eloquent-feather-wfsj7f?file=/Demo.tsx But the menu is never firing onClose when I click away. Is this use case not supported? Or am I setting the wrong properties here?

Abhushit commented 1 year ago

Yes @juliushuck you are right. If you try to create a controlled function for onClose, it does not work with the Menu component. To get full control you can use MenuList component, It basically handles the focus state. And yes the disable property also does not work.

Please visit: https://codesandbox.io/s/2rc4wf?file=/Demo.js

sai6855 commented 1 year ago

Opened PR which fixes disable issue. https://github.com/mui/material-ui/pull/38342

juliushuck commented 1 year ago

@Abhushit Thank you for the solution - That works for now and maybe in the future, joy could support the use case a bit simpler.

@sai6855 I'm looking forward to that fix, thank you for the quick response time

enricoros commented 1 year ago

I see the requirement for Dropdown and MenuButton or ClickawayListener, but this is a large behavior change versus the former "onClose" approach.

Are we expected to port the code of the application to this new pattern because onClose is present but not firing?

Controllable menus are a requirement of real-world apps in my opinion, especially when you want the parent component to hold state, or an external Store to hold state (menus closeable by peer components, dynamic menu items, pluggable menus).

Please let us know the expected behavior (and/or remove the onClose call to force downstream migration away from the former API).

michaldudak commented 1 year ago

The open and onClose were moved from the Menu to the Dropdown (with onClose being changed to onOpenChange and firing on both opening and closing). These props should have been removed from the MenuProps interface. I'll fix it soon.

With the latest changes, the Dropdown is responsible for opening and closing the popup, while the Menu is only concerned with list navigation, highlighted item, etc.

This codesandbox shows how to implement a controlled menu.

Does this help?

enricoros commented 1 year ago

Thanks @michaldudak, I'll port to the new pattern asap and let you know if I have feedback.

My app has 4+ controllable menus, and some with complex controls (closeable by other parts of the UI as contents are pluggable).

I'll wait for the official merge and deploy of your patch that removes the onClose, to see if my code breaks anywhere, but likely not, once I port it.

enricoros commented 1 year ago

This codesandbox shows how to implement a controlled menu.

To be sure: we don't need a MenuButton, and the Anchor can be set from outside, correct?

enricoros commented 1 year ago

Test results: The change in behavior is very deep.

Issues introduced by the new menu paradigm

1. Breaks different triggers to open the menu

Deleting the <MenuButton> component from the example does not show the menu anymore. https://codesandbox.io/s/rough-pond-chf9c2?file=/Demo.tsx What if someone wants their own button: Button, IconButton, or any other way of showing menus programmatically?

2. Breaks dynamic menus / non-sibling anchors

What if the Anchor is not a sibling of the Menu, but in a very different part of the UI.

3. Breaks conditional rendering

React conditional rendering is now not supported - e.g. "{condition && " - as it will not fire the onOpenChange callback. https://codesandbox.io/s/wizardly-joji-tcqp2r?file=/Demo.tsx This would force the menu to be always present in the DOM, even in dynamic (content/origin unknown) cases.

Bottom line

I ported my full app to Joy-5.beta0 in 1 day (which is great, and love the Beta), but I am stuck porting my 5 menus - they simply cannot be ported to the new framework. The requirement of using "Dropdown" and "MenuButton" while seems to be a simplification in the easiest use case, it makes things more complex and prevents flexibility, denies use cases, or requires very expensive workarounds. What used to be <Menu open={...} onClose={..} anchorEl={...}> is now more elements, more opinionated, less production and more prototype. Please advise on the best path forward.

Apologies if this sounds critical of the change - I understand the spirit on delivering on an easy Dropdown experience - I read and followed the Migration Guide https://mui.com/joy-ui/migration/migrating-default-theme/, and was not expecting that after a day of work, my Joy App will be in an unfixable broken state - if I knew, I'd not have done the port. The Menu change goes deeper than style changes.

sai6855 commented 1 year ago

@enricoros https://codesandbox.io/s/cold-firefly-qz9vst?file=/Demo.tsx I think demo in this sandbox covers all 3 use cases you mentioned. Does this work for you?

@michaldudak what do you think about implementation? if you are fine, i can create demos.

enricoros commented 1 year ago

@sai6855 Hi, thanks for the code. The menu stays open (as per the original title of this issue); "clicking away" doesn't close it despite the code.

Regarding whether the 3 use cases are covered, the main issue is that the Dropdown, MenuButton and Menu are co-located in this demo. With the former <Menu anchor={... approach, we could have the anchor in a component, and the Menu could be defined in a very different part of the app. As my UI is "pluggable" and menus are "dynamic", this default pattern doesn't work for me - I'll explore some MenuList composition.

Imagine this use case: a menu that appears when you right-click. Before, it was possible with 1 line of code {!!anchor && <Menu anchor={...; now it is almost impossible.

I don't want to keep this issue open; I'm sharing my experience in detail because this new API imposes a large limitation for Joy/Base IMO, and I love this product.

enricoros commented 1 year ago

Found a workaround, this issue can be closed.

I wrote this component which is similar to the way the <Menu used to work:

https://github.com/enricoros/big-agi/blob/main/src/common/components/CloseableMenu.tsx - this doesn't work as well as the old Menu used to work (somehow it was closing on loss of focus for the window, not just for ClickAway, but it's a good compromise

siriwatknp commented 1 year ago

@enricoros Thanks a lot for the feedback.

cc @michaldudak I think it's worth taking https://github.com/mui/material-ui/issues/38324#issuecomment-1668715395 into consideration.

maftieu commented 1 year ago

Previously, using controlled Button+Menu, we could leave the menu opened when a menu item was clicked. Now, with Dropdown+MenuButton+Menu, I'm unable to have the same behavior. How to do this?

My use case is a dropdown menu, with items that I can enable/disable, and I want to leave the menu opened after a click on an item.

siriwatknp commented 1 year ago

Previously, using controlled Button+Menu, we could leave the menu opened when a menu item was clicked. Now, with Dropdown+MenuButton+Menu, I'm unable to have the same behavior. How to do this? My use case is a dropdown menu, with items that I can enable/disable, and I want to leave the menu opened after a click on an item.

I see 2 options:

Add a prop to Dropdown called disableCloseOnItemClick (or other names)
Add prevent behavior to the event when clicking on each item <MenuItem onClick={event => event.preventClosing() }>

cc @michaldudak

maftieu commented 1 year ago

I think the second solution offers more flexibility. It allows to mix items that closes the menu and items that don't.

michaldudak commented 1 year ago

We could make it more explicit and have a prop on the MenuItem called disableCloseOnClick.

I created a separate issue for this: mui/base-ui#46

siriwatknp commented 1 year ago

Previously, using controlled Button+Menu, we could leave the menu opened when a menu item was clicked. Now, with Dropdown+MenuButton+Menu, I'm unable to have the same behavior. How to do this?

My use case is a dropdown menu, with items that I can enable/disable, and I want to leave the menu opened after a click on an item.

@michaldudak Do you see any workaround to this? It sounds like a common use case to me and we should add it to the docs or demos.

enricoros commented 1 year ago

"Improworsement": is an improvement that makes things worse. The changes to the Menu component have transformed it from a React-controllable Menu, to a Dropdown. I don't write this message to get a reply, but just out of love for the product.

The new Dropdown is limited in many ways compared to the old Menu -- but it's still called "Menu". For instance, how can I show a dynamic menu (with programmatic items) when right-clicking some selected text?

Audience: you cater to react programmers, saving 1 anchorEl in the state is not hard for the audience (former behavior) Purpose: do you want to offer a Menu component that can be used broadly and in many use cases, or do you want to force the Dropdown paradigm and call it a menu? Flexibility: before anything was possible, but now some use cases are very complex or impossible to implement

I don't see why making menu state management opaque and non-controllable to the developers can improve the DX, if not for the basic use case of a dropdown on a button, but menus are used for much more than that.

siriwatknp commented 1 year ago

Here is my thought about the Menu:

general use cases for creating a menu that open/close when clicked: use the combination of Dropdown, MenuButton, and Menu.
more complex use cases when you want to control the behavior of the open/close action: use the controllable Menu (basically, the same behavior as the Menu before).

cc @michaldudak If you agree with this, I can take this over.

michaldudak commented 1 year ago

@enricoros, we've considered many different approaches when designing this solution. You can browse through mui/material-ui#32088 for more details.

I don't see a problem adding an explicit anchor prop to the Menu. I believe it could solve some of your problems, no? As for context menus, we thought of implementing them with a separate component. They would still use the Dropdown, but will be triggered differently (so not using MenuButton).

@siriwatknp I believe that more complex cases should also use the Dropdown, but not necessarily the MenuButton.

michaldudak commented 1 year ago

FYI, I added the anchor prop back to the Base UI's Menu in mui/material-ui#39284.

mui / material-ui