Skip to content

[NavigationMenu] Fix menu reopening on mouse move when closed with Esc #1579

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 9 commits into from
Sep 30, 2022
Merged

[NavigationMenu] Fix menu reopening on mouse move when closed with Esc #1579

merged 9 commits into from
Sep 30, 2022

Conversation

rortan134
Copy link
Contributor

Fixes #1529

Makes the menu stay closed after pressing the Esc key

@benoitgrelard
Copy link
Contributor

Hey @rortan134,

Not sure why the checks haven't run, would you be able to push a change or force-push to re-trigger them?

Thanks 🙏

Copy link
Contributor

@benoitgrelard benoitgrelard left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry just one final nit before I merge!

Co-authored-by: Benoît Grélard <[email protected]>
Copy link
Contributor

@benoitgrelard benoitgrelard left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unfortunately after testing this, I've just realised that this approach won't work.
To clarify it works if the trigger is focused, but not if you're just hovering with your mouse.
We cannot rely on the fact that the trigger is focused.
Instead we are going to have to rely on the onEscapeKeydown event handler from DismissableLayer.

So the way to go is:

  • lift up wasEscapeCloseRef definition into NavigationMenuItem
  • pass it down through context (NavigationMenuItemContextProvider)
  • set the ref to true in onEscapeKeydown on DismissableLayer in NavigationMenuContentImpl instead of in the trigger keydown event

Hopefully that's enough for you to get going, but let me know otherwise and I can take it from here otherwise! 🙏

@benoitgrelard benoitgrelard added the PR: In Review This PR is in the process of being reviewed by the team label Sep 29, 2022
@rortan134
Copy link
Contributor Author

rortan134 commented Sep 30, 2022

I made the changes to make a shallower approach, though the root cause of the focus issue seems to be related to the document focus variable in UseEscapeKeydown and the fact that Storybook has two documents (One inside an iframe). Quoting from the jQuery Docs:

[the keydown event] can be attached to any element, but the event is only sent to the element that has the focus. Focusable elements can vary between browsers

Therefore, since Storybook uses an iframe, there are two documents and only one of them can be focused, and if the external one (Sidebar + Header) is focused then the UseEscapeKeydown handler won't trigger and the dropdown won't close, which is likely what you saw happening.

This issue is likely to be exclusive to Storybook though, since regular web pages usually don't have more than one document, and it won't be a problem.
An easy way to test this behavior is adding a console.log(ownerDocument.hasFocus()) inside the useEffect on the useEscapeKeydown hook, you'll see that when you focus the sidebar and proceed to hover on a menu it'll say false and Escape won't close the menu, though if you focus the inner document (iframe, and also where the component is rendered) then it'll say true and the menu will work fine. Tell me if you find any issues 😄

Copy link
Contributor

@benoitgrelard benoitgrelard left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

though the root cause of the focus issue seems to be related to the document focus variable in UseEscapeKeydown and the fact that Storybook has two documents
Therefore, since Storybook uses an iframe, there are two documents and only one of them can be focused, and if the external one (Sidebar + Header) is focused then the UseEscapeKeydown handler won't trigger and the dropdown won't close, which is likely what you saw happening.

I think you are getting your wires crossed a bit. Just to clarify, this isn't what I was referring to in my earlier comment.

To reproduce the issue I was discussing before with the naïve trigger onKeyDown approach:

  • click somewhere on the page to make sure the document is focused (or open the frame directly itself)
  • hover a menu item (don't focus it with keyboard) -> menu opens
  • press escape -> menu closes
  • mouse mouse (still over the item) -> item reopens

This is because we were marking the prevention in the ref only in the onKeyDown of the trigger, which in this case would never happen because the trigger isn't focus so wouldn't receive a keydown event.

Hence why I requested the changes.
This is all working great now, thanks!

@benoitgrelard benoitgrelard merged commit af0a2e9 into radix-ui:main Sep 30, 2022
@benoitgrelard benoitgrelard removed the PR: In Review This PR is in the process of being reviewed by the team label Sep 30, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[NavigationMenu] Menu closed with Esc immediately reopens on mouse move
2 participants