Skip to content

fix: Make submenu hover focus remain on the trigger and collapse submenus when hovering root menu #8666

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.

Sign up for GitHub

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 3 commits into from
Aug 6, 2025
Merged

fix: Make submenu hover focus remain on the trigger and collapse submenus when hovering root menu #8666

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
b79e172
fix: hovering submenutrigger should keep focus on the trigger
LFDanLu Aug 1, 2025
d404bf5
make submenu close if any other item in the same menu is focused
LFDanLu Aug 5, 2025
b680b75
Merge branch 'main' into submenu_bugs
LFDanLu Aug 5, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 4 additions & 2 deletions packages/react-aria-components/src/Menu.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -131,10 +131,12 @@ export const SubmenuTrigger = /*#__PURE__*/ createBranchComponent('submenutrigg
<Provider
values={[
[MenuItemContext, {...submenuTriggerProps, onAction: undefined, ref: itemRef}],
[MenuContext, submenuProps],
[MenuContext, {
ref: submenuRef,
Copy link
Member

@snowystinger snowystinger Aug 5, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

i assume this was just correcting a mistake? or was this necessary so we don't include submenus in the parentMenuRef check?

Copy link
Member Author

Choose a reason for hiding this comment

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

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.

...submenuProps
}],
Comment on lines +134 to +137
Copy link
Member Author

@LFDanLu LFDanLu Aug 1, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

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

[OverlayTriggerStateContext, submenuTriggerState],
[PopoverContext, {
ref: submenuRef,
trigger: 'SubmenuTrigger',
triggerRef: itemRef,
placement: 'end top',
Expand Down
5 changes: 3 additions & 2 deletions packages/react-aria-components/src/Popover.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -190,11 +190,12 @@ function PopoverInner({state, isExiting, UNSTABLE_portalContainer, clearContexts
}, [ref, shouldBeDialog]);

// Focus the popover itself on mount, unless a child element is already focused.
// 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)) {
Copy link
Member

Choose a reason for hiding this comment

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

:-/
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

Copy link
Member Author

Choose a reason for hiding this comment

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

a bit unfortunate for sure

focusSafely(ref.current);
}
}, [isDialog, ref]);
}, [isDialog, ref, props.trigger]);

let children = useMemo(() => {
let children = renderProps.children;
Expand Down