Fix closing Menu when other Menu is opened
#3726
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
| // In a perfect world this is pushed / popped when we open / close the Dialog | ||
| // for within an event listener. But since the state is controlled by the | ||
| // user, this is the next best thing to do. | ||
| let stackMachine = stackMachines.get(null) |
There was a problem hiding this comment.
I believe this should be safe because we are using a DefaultMap in module scope and this will always return the exact same reference.
| useIsoMorphicEffect(() => { | ||
| if (!enabled) return | ||
|
|
||
| stackMachine.actions.push(id) | ||
| return () => stackMachine.actions.pop(id) | ||
| }, [stackMachine, id, enabled]) |
There was a problem hiding this comment.
Unfortunately the Dialog is state driven and not based on user events. So the only way we know that it sits on top or not is with an effect.
RobinMalfait
force-pushed
the
fix/issue-3701
branch
from
515b119 to
a3b10cf
Compare
So the specific `mousedown` code can be removed.
This will allow us to act immediately when a new item is pushed on top of the stack. In case that happens the `Menu`, `Listbox`, `Combobox` will immediately close. The `Dialog` will just push (or pop) onto the stack when it's opened.
RobinMalfait
force-pushed
the
fix/issue-3701
branch
from
a3b10cf to
2b7e1e5
Compare
packages/@headlessui-react/src/components/listbox/listbox-machine.ts
Outdated
Show resolved
Hide resolved
packages/@headlessui-react/src/components/combobox/combobox-machine-glue.tsx
Outdated
Show resolved
Hide resolved
| // Handle outside click | ||
| let outsideClickEnabled = comboboxState === ComboboxState.Open | ||
| let outsideClickEnabled = isTopLayer | ||
| useOutsideClick(outsideClickEnabled, [buttonElement, inputElement, optionsElement], () => |
There was a problem hiding this comment.
I'd just drop the extra var and use isTopLayer directly
There was a problem hiding this comment.
This also applies to a couple of other spots in the file btw
| ? window.document.activeElement | ||
| : null | ||
| }) | ||
| }) |
There was a problem hiding this comment.
I'm guessing we didn't need to use the capture phase for this?
There was a problem hiding this comment.
You know, I deleted them all for cleanup, but didn't test this case; Going to revert this part.
Fixes: #3701
This PR fixes an issue where an open
Menuis not closed when opening a newMenu. This is also fixed forListboxandComboboxthat used the same techniques.This happened because we recently shipped an improvement where the
Menuopens onpointerdowninstead of onclick. This means that theuseOutsideClickhook was not correct anymore because it relies onclick.We could try and figure out that we should already close on
pointerdownbut this might not be expected for other components. Instead we want to simplify things a bit and ideally not even worry about what event caused a specific state change.Instead of trying to fight timing issues when certain events happen, this PR takes a slightly different approach.
We already had the concept of a "top-layer" similar to the browser's
#top-layer(when using nativedialog). This essentially lets us know which component sits on top of the hierarchy.This top-layer is important because when you have the following structure:
Assuming that both the
DialogandMenuare open, clicking outside or pressing escape should only close theMenu. Once theMenuis closed, we should close theDialog.In this case, we can enable/disable the
useOutsideClickhook based on whether the current component is the top-layer or not.Some components like the
Menu,ListboxandComboboxshould immediately close when they are not the top-layer anymore. ADialogcan stay open, because you can have interactable elements like the example above in theDialog.Luckily, these components that should immediately close already use their own state machine. This allows us to listen to the
OpenMenu(orOpenListbox,OpenCombobox) event, and if that happens, we can push the current component on the shared stack machine.This now means that it doesn't matter how the
Menuis opened, but the moment a user event (click, enter, ...) opens theMenu, we now that we are on top of the stack.All other components could listen to push events on the stack. Once those happen, we can close the current component immediately. This has the nice side effect that we don't have to use a
useEffectto check for state changes. We can just act immediately when an event happens.The
useOutsideClickhooks is still used and useful in situations where you literally just clicked somewhere else. But in case you are opening anotherMenuor anotherListbox, we can immediately close the one that was open before.Test plan
Before:
Screen.Recording.2025-05-12.at.19.19.42.mov
After:
Screen.Recording.2025-05-12.at.19.19.05.mov