[menu] Fix closeParentOnEsc default value
#2493
Conversation
seongminn
requested review from
atomiks,
colmtuite and
michaldudak
as code owners
commit: |
Bundle size report
|
✅ Deploy Preview for base-ui ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
| returnFocus={finalFocus || returnFocus} | ||
| returnFocus={finalTriggerElement || returnFocus} |
There was a problem hiding this comment.
This prevents returnFocus from ever being passed, no?
It seems like finalTriggerElement needs to be part of the if branches above when calculating returnFocus
There was a problem hiding this comment.
Seems good now - also tested Menubar and the return focus issue this branch was addressing is still not present
| const { closeParentOnEsc = false } = props; | ||
|
|
||
| return ( | ||
| <MenuSubmenuRootContext.Provider value> | ||
| <MenuRoot {...props} /> | ||
| <MenuRoot closeParentOnEsc={closeParentOnEsc} {...props} /> |
There was a problem hiding this comment.
macOS closes them all simultaneously; not sure if this should change as a default
There was a problem hiding this comment.
According to the MDN documentation, pressing ESC in a submenu should return focus to the parent menu or menuitem.
I've implemented this as the default behavior for submenus based on that recommendation.
I'm curious to hear your thoughts on this.
There was a problem hiding this comment.
I'm not too fussed either way, and am ok with changing it if this default is better suited in the web paradigm cc: @colmtuite
oliviertassinari
changed the title
closeParentOnEsc default valuecloseParentOnEsc default value
oliviertassinari
changed the title
closeParentOnEsc default valuecloseParentOnEsc default value
LukasTy
left a comment
LukasTy
left a comment
There was a problem hiding this comment.
LGTM. 👌
Thank you for the contribution. 🙏
michaldudak
added
the
breaking change
Co-authored-by: Lukas Tyla <llukas.tyla@gmail.com>
6 participants
closed #2490
According to the MDN documentation, when the focus is on a submenu, pressing the ESC key should close the submenu and move focus to the submenu trigger.
Additionally, the Base UI documentation states that when the Esc key is pressed, the focus should return to the trigger element by default.
However, the current implementation has two issues:
closeParentOnEscis set tofalse, the root menu remains open, but the focus is lost completely.To resolve this, the following changes have been implemented:
false.