fix: Make submenu hover focus remain on the trigger and collapse submenus when hovering root menu #8666
Conversation
also makes it so ArrowRight still moves to the submenu after hovering, not sure if this breaks VO moving focus to the dismiss button though...
| [MenuContext, { | ||
| ref: submenuRef, | ||
| ...submenuProps | ||
| }], |
There was a problem hiding this comment.
Fixes ArrowRight moving focus into the SubMenu after hovering it. Might cause iOS VO to close the submenu prematurely when focusing the dismiss button
EDIT: seems ok on my iPad but that is only on 17 since it is too old
|
Build successful! 🎉 |
|
The other bug where all the submenus don't close when hovering the root menu item is a bit tricker. It broke due to the change to ignore blur/focus events bubbling through the portal via https://github.com/adobe/react-spectrum/blame/8f6724f09621327d78f77c997d6c7423e06b7b84/packages/%40react-aria/interactions/src/useFocusWithin.ts#L77-L81 which seems correct as a general change. However, this means we need to know to close all submenus up to a certain level without relying on useOverlay's Most solutions will need to provide the root menu state to menu items in general I think which is kinda gross... Would allow us to essentially do something like "if focus lands on a regular menu item, close all submenus that have a level greater than said menu item." Might be nice to have additional information on the item node for this. Alternatively, we can special case the ignoring of blur/focus event bubbling through portals strictly for submenus. This seems to work but unsure what side effects this might have EDIT: Fixed, see PR change |
this fixes the case where the user hovers a root menu level item when there are multiple submenu levels open but only the last one closes
LFDanLu
changed the title
|
Build successful! 🎉 |
|
Build successful! 🎉 |
| // Skip this for submenus since hovering a submenutrigger should keep focus on the trigger | ||
| useEffect(() => { | ||
| if (isDialog && ref.current && !ref.current.contains(document.activeElement)) { | ||
| if (isDialog && props.trigger !== 'SubmenuTrigger' && ref.current && !ref.current.contains(document.activeElement)) { |
There was a problem hiding this comment.
:-/
I see we use this check in a few places though, so I'm ok with it. at some point we'll need to make this a more generic/defined behaviour check
There was a problem hiding this comment.
a bit unfortunate for sure
| [MenuItemContext, {...submenuTriggerProps, onAction: undefined, ref: itemRef}], | ||
| [MenuContext, submenuProps], | ||
| [MenuContext, { | ||
| ref: submenuRef, |
There was a problem hiding this comment.
i assume this was just correcting a mistake? or was this necessary so we don't include submenus in the parentMenuRef check?
There was a problem hiding this comment.
just something we forgot to update I believe when we moved to automatically making these menus wrapped in dialogs. This resulted in the ref being passed to useSubmenuTrigger resolving to the dialog instead of the submenu, thus meaning ArrowRight wouldnt cause focus to move to the first menu item since we weren't actually focusing the menu.
4 participants
Closes RSP Component Milestones (view)
✅ Pull Request Checklist:
📝 Test Instructions:
🧢 Your Project:
RSP