- 
                Notifications
    You must be signed in to change notification settings 
- Fork 15
fix(container-menu): improve menu dropdown expansion and focus behavior on blur #691
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
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Controlled usage can't escape the trigger with the keyboard.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| if (!isMenuOrTriggerFocused) { | ||
| const nextElementIsFocusable = | ||
| !!event.relatedTarget /* [1] */ && | ||
| event.relatedTarget?.nodeName !== '#document'; /* [2] */ | 
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
On our Gatsby site, when a menu dropdown is open and you click outside, focus shifts to a <div> with tabIndex="-1" instead of the trigger. Should I handle that case?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm, I doubt it, because the updated behavior is so much better than current https://garden.zendesk.com/components/menu – which jumps to the currently expanded menu when you trigger a menu further down the page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can't quite pin it down, but I run into some obscure combos where the state feels just a bit off. However, overall I think we can call this a real nice quality-of-life boost for the menu hook. And the accompanying tests + comments are an excellent guide if we need to revisit for future tweaks. Nice work @ze-flo 🙌
| if (!isMenuOrTriggerFocused) { | ||
| const nextElementIsFocusable = | ||
| !!event.relatedTarget /* [1] */ && | ||
| event.relatedTarget?.nodeName !== '#document'; /* [2] */ | 
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm, I doubt it, because the updated behavior is so much better than current https://garden.zendesk.com/components/menu – which jumps to the currently expanded menu when you trigger a menu further down the page.
Co-authored-by: Jonathan Zempel <jzempel@gmail.com>

Description
This PR addresses a bug where the Menu’s dropdown remained open when the user clicked outside its iframe. The dropdown now correctly closes when focus is moved outside the iframe.
Detail
This PR implements the fix in
Menufor manual testing in Storybook.Checklist
npm start)