[Menu] Fixed Menu item not toggling highlighted prop when submenu popup is opened using keyboard #1285

onehanddev · 2025-01-05T14:17:03Z

Improve Menu Navigation and Fix Highlighting Issue

Fixed bug where parent expandable menu item is getting highlighted along with the its sub menu with keyboard navigation.
Improved navigation by resetting parent menu's highlight state when entering a submenu.

I have followed (at least) the PR section of the contributing guide.

…s, allowing for dynamic updates to the active (highlighted) item index. This fixes not updating hover out behaviour of navigation for keyboard events.

mui-bot · 2025-01-05T14:19:52Z

Netlify deploy preview

https://deploy-preview-1285--base-ui.netlify.app/

Generated by 🚫 dangerJS against caedfec

atomiks · 2025-01-05T23:52:59Z

packages/react/src/menu/submenu-trigger/useMenuSubmenuTrigger.ts

        ...getMenuItemProps({
          'aria-haspopup': 'menu' as const,
          ...externalProps,
+          onKeyDown: (event: MenuKeyboardEvent) => {
+            if (event.key === 'ArrowRight' && highlighted) {
+              // Clear parent menu's highlight state when entering submenu
+              // This prevents multiple highlighted items across menu levels
+              setActiveIndex(null);
+            }
+            onKeyDown?.(event);
+            externalProps?.onKeyDown?.(event);
+          },


Using the mergeReactProps util will work better here

Done, Please check.

atomiks · 2025-01-05T23:54:49Z

packages/react/src/menu/submenu-trigger/useMenuSubmenuTrigger.ts

@@ -38,11 +44,20 @@ export function useMenuSubmenuTrigger(
        ...getMenuItemProps({
          'aria-haspopup': 'menu' as const,
          ...externalProps,
+          onKeyDown: (event: MenuKeyboardEvent) => {
+            if (event.key === 'ArrowRight' && highlighted) {


There's also regular onClick to check for

Added, Please check.

…ergeReactProps to merge externalProps

atomiks · 2025-01-06T08:21:21Z

packages/react/src/menu/submenu-trigger/useMenuSubmenuTrigger.ts

+    /**
+     * 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;


These aren't necessary with mergeReactProps, it will automatically call them from the external props

Missed it, Its being removed in latest commit.

atomiks · 2025-01-06T11:52:13Z

packages/react/src/menu/submenu-trigger/useMenuSubmenuTrigger.ts

+          onKeyDown: (event: MenuKeyboardEvent) => {
+            if (event.key === 'ArrowRight' && highlighted) {


This works well overall, but in rtl mode, it's ArrowLeft instead of ArrowRight

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

I see, Is it fine to just check the direction and use ArrowLeft | ArrowRight ? What are the other edge cases which could potentially not respect these events ?

const direction = useDirection() is the hook for checking the direction, yep.

I'm not certain, sometimes edge cases can pop up in the future. That will likely be good enough, though.

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.

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!

… props (mui#1194)

Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com> Co-authored-by: Michał Dudak <[email protected]>

Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>

Signed-off-by: Olivier Tassinari <[email protected]>

Co-authored-by: Michał Dudak <[email protected]>

…1226)

…ergeReactProps to merge externalProps

…u component

…com/onehanddev/base-ui into menu/submenu-highlight-keyboard-focus

netlify · 2025-01-08T13:40:53Z

✅ Deploy Preview for base-ui ready!

Name	Link
🔨 Latest commit	`caedfec`
🔍 Latest deploy log	https://app.netlify.com/sites/base-ui/deploys/677e8002d9f0d200082d5b1f
😎 Deploy Preview	https://deploy-preview-1285--base-ui.netlify.app
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

onehanddev added 2 commits January 5, 2025 19:40

Added setActiveIndex to the MenuRoot and MenuSubmenuTrigger component…

b6f179b

…s, allowing for dynamic updates to the active (highlighted) item index. This fixes not updating hover out behaviour of navigation for keyboard events.

Merge branch 'master' into menu/submenu-highlight-keyboard-focus

d1aba74

fix: ts errors

8e93ab9

onehanddev marked this pull request as ready for review January 5, 2025 14:55

onehanddev requested review from atomiks, colmtuite and michaldudak as code owners January 5, 2025 14:55

atomiks reviewed Jan 5, 2025

View reviewed changes

Handled click event as well for setting active item for menu, added m…

0969a08

…ergeReactProps to merge externalProps

atomiks reviewed Jan 6, 2025

View reviewed changes

addressed PR comments

a3c9339

zannager added the component: menu This is the name of the generic UI component, not the React module! label Jan 6, 2025

atomiks reviewed Jan 6, 2025

View reviewed changes

github-actions bot added the PR: out-of-date The pull request has merge conflicts and can't be merged label Jan 8, 2025

mj12albert and others added 14 commits January 8, 2025 19:06

[ScrollArea] Read DirectionProvider and use logical positioning CSS…

b511990

… props (mui#1194)

Bump @mui/monorepo to v6.3.0 (mui#1252)

62220ba

Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com> Co-authored-by: Michał Dudak <[email protected]>

[docs-infra] Remove Next.js production profiler (mui#1242)

436a1e4

Bump @stefanprobst/rehype-extract-toc to ^2.2.1 (mui#1253)

79da511

Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>

Bump @types/node to ^18.19.69 (mui#1254)

a7bb068

Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>

Bump prettier to ^3.4.2 (mui#1258)

b1e9b73

Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>

Bump react-router-dom to ^6.28.1 (mui#1259)

b15795d

Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>

Bump shiki to ^1.25.1 (mui#1260)

f7bccad

Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>

Bump chalk to ^5.4.1 (mui#1266)

da29724

Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>

Bump framer-motion to ^11.15.0 (mui#1267)

ce4bbb7

Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>

Bump github/codeql-action action to v3.28.0 (mui#1268)

dbc3ff7

Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>

Bump markdownlint-cli2 to ^0.17.1 (mui#1269)

771c279

Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>

Bump piscina to ^4.8.0 (mui#1271)

d63eea4

Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>

Bump pnpm to 9.15.2 (mui#1272)

36b6191

Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>

renovate bot and others added 27 commits January 8, 2025 19:06

Bump stylelint to ^16.12.0 (mui#1273)

7ffc041

Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>

Bump @types/prop-types to ^15.7.14 (mui#1255)

f3bae72

Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>

Bump webpack to ^5.97.1 (mui#1276)

09cbbf5

Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>

Bump tsx to ^4.19.2 (mui#1275)

bc8450f

Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>

Bump @argos-ci/core to ^2.12.0 (mui#1262)

5c403b0

Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>

Bump Emotion to ^11.14.0 (mui#1264)

5d7d71e

Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>

Bump terser to ^5.37.0 (mui#1274)

7008045

Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>

[core] Remove tsbuildinfo from the built package (mui#1152)

1931c76

[core] Use stable React packages (mui#1113)

0f09c92

Bump eslint (mui#1257)

3181e2e

Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>

Bump babel (mui#1265)

ef1e2a7

Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>

Bump terser-webpack-plugin to ^5.3.11 (mui#1261)

616775b

Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>

Bump @next/mdx to ^15.1.3 (mui#1263)

d289505

Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>

Bump @octokit/rest to ^21.0.2 (mui#1277)

1ba3e4d

Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>

Bump Public packages' dependencies (mui#1158)

2d50a69

Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>

Bump MUI (mui#1157)

5337c0e

Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>

[useButton] Fix Enter key preventDefault when rendering links (mui#1251)

80ff4db

[docs] Add security coverage (mui#1150)

04534a4

Signed-off-by: Olivier Tassinari <[email protected]>

[Select] Fix highlight flash on Safari (mui#1233)

7de8074

[Slider] Set position: relative on range slider indicator (mui#1175)

8cd3fcb

[test] Use vite and vitest for e2e and regression tests (mui#1127)

88ad014

Co-authored-by: Michał Dudak <[email protected]>

[docs-infra] Always document the className and render props (mui#…

30f0ef0

…1226)

fix: ts errors

44f0557

Handled click event as well for setting active item for menu, added m…

e1ab1b7

…ergeReactProps to merge externalProps

addressed PR comments

3c394dd

added keyboard support for rtl direction for submenu toggling for Men…

1e18fc8

…u component

Merge branch menu/submenu-highlight-keyboard-focus of https://github.…

caedfec

…com/onehanddev/base-ui into menu/submenu-highlight-keyboard-focus

onehanddev mentioned this pull request Jan 8, 2025

[Menu] Fixed Menu item not toggling highlighted prop when submenu popup is opened using keyboard #1310

Open

1 task

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Menu] Fixed Menu item not toggling highlighted prop when submenu popup is opened using keyboard #1285

[Menu] Fixed Menu item not toggling highlighted prop when submenu popup is opened using keyboard #1285

onehanddev commented Jan 5, 2025 •

edited

Loading

mui-bot commented Jan 5, 2025 •

edited

Loading

atomiks Jan 5, 2025

onehanddev Jan 6, 2025

atomiks Jan 5, 2025

onehanddev Jan 6, 2025

atomiks Jan 6, 2025

onehanddev Jan 6, 2025

atomiks Jan 6, 2025

onehanddev Jan 6, 2025

atomiks Jan 7, 2025

onehanddev Jan 7, 2025

onehanddev Jan 8, 2025

netlify bot commented Jan 8, 2025

		onKeyDown: (event: MenuKeyboardEvent) => {
		if (event.key === 'ArrowRight' && highlighted) {

[Menu] Fixed Menu item not toggling highlighted prop when submenu popup is opened using keyboard #1285

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

Conversation

onehanddev commented Jan 5, 2025 • edited Loading

mui-bot commented Jan 5, 2025 • edited Loading

Netlify deploy preview

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

netlify bot commented Jan 8, 2025

✅ Deploy Preview for base-ui ready!

onehanddev commented Jan 5, 2025 •

edited

Loading

mui-bot commented Jan 5, 2025 •

edited

Loading