-
-
Notifications
You must be signed in to change notification settings - Fork 82
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
[Menu] Fixed Menu item not toggling highlighted prop when submenu popup is opened using keyboard #1285
base: master
Are you sure you want to change the base?
[Menu] Fixed Menu item not toggling highlighted prop when submenu popup is opened using keyboard #1285
Changes from 4 commits
b6f179b
d1aba74
8e93ab9
0969a08
a3c9339
b511990
62220ba
436a1e4
79da511
a7bb068
b1e9b73
b15795d
f7bccad
da29724
ce4bbb7
dbc3ff7
771c279
d63eea4
36b6191
7ffc041
f3bae72
09cbbf5
bc8450f
5c403b0
5d7d71e
7008045
1931c76
0f09c92
3181e2e
ef1e2a7
616775b
d289505
1ba3e4d
2d50a69
5337c0e
80ff4db
04534a4
7de8074
8cd3fcb
88ad014
30f0ef0
44f0557
e1ab1b7
3c394dd
1e18fc8
caedfec
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -4,6 +4,11 @@ import { FloatingEvents } from '@floating-ui/react'; | |
import { useMenuItem } from '../item/useMenuItem'; | ||
import { useForkRef } from '../../utils/useForkRef'; | ||
import { GenericHTMLProps } from '../../utils/types'; | ||
import { mergeReactProps } from '../../utils/mergeReactProps'; | ||
|
||
type MenuKeyboardEvent = { | ||
key: 'ArrowRight' | 'ArrowLeft' | 'ArrowUp' | 'ArrowDown'; | ||
} & React.KeyboardEvent; | ||
|
||
export function useMenuSubmenuTrigger( | ||
parameters: useSubmenuTrigger.Parameters, | ||
|
@@ -17,6 +22,9 @@ export function useMenuSubmenuTrigger( | |
setTriggerElement, | ||
allowMouseUpTriggerRef, | ||
typingRef, | ||
onKeyDown, | ||
setActiveIndex, | ||
onClick, | ||
} = parameters; | ||
|
||
const { getRootProps: getMenuItemProps, rootRef: menuItemRef } = useMenuItem({ | ||
|
@@ -34,15 +42,28 @@ export function useMenuSubmenuTrigger( | |
|
||
const getRootProps = React.useCallback( | ||
(externalProps?: GenericHTMLProps) => { | ||
return { | ||
return mergeReactProps(externalProps, { | ||
...getMenuItemProps({ | ||
'aria-haspopup': 'menu' as const, | ||
...externalProps, | ||
onKeyDown: (event: MenuKeyboardEvent) => { | ||
if (event.key === 'ArrowRight' && highlighted) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This works well overall, but in I wonder if it's more robust to check for focus/blur, or the submenu opening? Explicitly checking for opening actions like onClick/ArrowLeft/Right can work ok, but there's edge cases to consider since it's not as general There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I see, Is it fine to just check the direction and use There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
I'm not certain, sometimes edge cases can pop up in the future. That will likely be good enough, though. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes, I agree. I debugged further and found that we get the latest open state for the submenu in the useMenuRoot hook. Even when we use it in useMenuSubmenuTrigger, it only works for keyboard events, not hover ones. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I implemented the direction check using useDirection and toggled the keyboard event key accordingly. Apologies for the commit mess in this PR—it happened while pulling the latest changes and resolving conflicts. To keep things clean, I created a fresh PR here: #1310. Please have a look at it when you get a chance, @atomiks. Thanks! |
||
// Clear parent menu's highlight state when entering submenu | ||
// This prevents multiple highlighted items across menu levels | ||
setActiveIndex(null); | ||
} | ||
onKeyDown?.(event); | ||
}, | ||
onClick: (event: React.MouseEvent) => { | ||
if (highlighted) { | ||
setActiveIndex(null); | ||
} | ||
onClick?.(event); | ||
}, | ||
}), | ||
ref: menuTriggerRef, | ||
}; | ||
}); | ||
}, | ||
[getMenuItemProps, menuTriggerRef], | ||
[getMenuItemProps, menuTriggerRef, onKeyDown, onClick, highlighted, setActiveIndex], | ||
); | ||
|
||
return React.useMemo( | ||
|
@@ -82,6 +103,21 @@ export namespace useSubmenuTrigger { | |
* A ref that is set to `true` when the user is using the typeahead feature. | ||
*/ | ||
typingRef: React.RefObject<boolean>; | ||
/** | ||
* Callback function that is triggered on key down events. | ||
* @param event - The keyboard event that triggered the callback. | ||
*/ | ||
onKeyDown?: (event: MenuKeyboardEvent) => void; | ||
/** | ||
* Callback function that is triggered on click events. | ||
* @param event - The click event that triggered the callback. | ||
*/ | ||
onClick?: (event: React.MouseEvent) => void; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. These aren't necessary with There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Missed it, Its being removed in latest commit. |
||
/** | ||
* Callback to update the active (highlighted) item index. | ||
* Set to null to remove highlighting from all items. | ||
*/ | ||
setActiveIndex: (index: number | null) => void; | ||
} | ||
|
||
export interface ReturnValue { | ||
|
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.
There's also regular
onClick
to check forThere 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.
Added, Please check.