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

onehanddev · 2025-01-08T13:49:20Z

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.

…ergeReactProps to merge externalProps

…u component

netlify · 2025-01-08T13:51:29Z

✅ Deploy Preview for base-ui ready!

Name	Link
🔨 Latest commit	`79cc42c`
🔍 Latest deploy log	https://app.netlify.com/sites/base-ui/deploys/6788bee44568d100081499c9
😎 Deploy Preview	https://deploy-preview-1310--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.

mui-bot · 2025-01-08T13:52:33Z

Netlify deploy preview

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

Generated by 🚫 dangerJS against 03c6581

Signed-off-by: Hakimuddin <[email protected]>

onehanddev · 2025-01-08T14:06:39Z

@atomiks Please disregard the older PR and review this one instead.

atomiks

A test would be helpful to prevent future regressions

… when a submenu item is opened.

onehanddev · 2025-01-13T13:57:29Z

A test would be helpful to prevent future regressions

@atomiks Just did, Please check, Thanks !

atomiks · 2025-01-14T02:14:47Z

packages/react/src/menu/submenu-trigger/MenuSubmenuTrigger.test.tsx

+
+      submenuItems.forEach((item) => {
+        if (item === submenuItem1) {
+          expect(item).to.have.attribute('tabindex', '0');


Small nit: data-highlighted is the attribute used for styling so it'd be better to test that, even if they're functionally the same

@atomiks Done 👍

… /> tests

…://github.com/onehanddev/base-ui into menu/submenu-highlight-keyboard-focus-updated

atomiks · 2025-01-16T03:05:24Z

It looks like setting the active index to null has a problem with tabbing. When opening the submenu, tabbing backwards should tab to the submenu trigger first, and then the root trigger. Tabbing forward from the root trigger should focus the submenu trigger and then the last known highlighted item in the submenu popup. (This is what happens currently)

But this no longer happens since tabindex=0 is removed with this change when setting the activeIndex to null

onehanddev · 2025-01-16T08:11:35Z

It looks like setting the active index to null has a problem with tabbing. When opening the submenu, tabbing backwards should tab to the submenu trigger first, and then the root trigger. Tabbing forward from the root trigger should focus the submenu trigger and then the last known highlighted item in the submenu popup. (This is what happens currently)

But this no longer happens since tabindex=0 is removed with this change when setting the activeIndex to null

@atomiks That was a good find, I have made the change to update the tabindex while nulling the activeIndex, Please checkout my latest commit, Thanks !

atomiks · 2025-01-21T05:10:48Z

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

+          setIsSubmenuOpen(true);
+        }
+      };
+      return mergeReactProps(externalProps, {


mergeReactProps<'div'>(...) will be able to type the onKeyDown event automatically

atomiks · 2025-01-21T05:11:04Z

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

@@ -32,17 +41,37 @@ export function useMenuSubmenuTrigger(

  const menuTriggerRef = useForkRef(menuItemRef, setTriggerElement);

+  const direction = useDirection();
+
+  const [isSubmenuOpen, setIsSubmenuOpen] = useState(false);


How does this state get reset back to false when keepMounted=true is used?

Ah, at the moment, it seems we need to access the ultimate Menu.root open property or state. Based on this, we can reset the isSubmenuOpen to false. However, this could be challenging given that the menu could be nested any number of times. Do you have any approach or solution in mind for handling this?

onehanddev added 5 commits January 8, 2025 19:15

Added setActiveIndex to the MenuRoot and MenuSubmenuTrigger component…

0419c12

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

fix: ts errors

eb47bc2

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

c4aa02c

…ergeReactProps to merge externalProps

addressed PR comments

1700a03

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

03c6581

…u component

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

661c502

Signed-off-by: Hakimuddin <[email protected]>

onehanddev marked this pull request as ready for review January 8, 2025 14:01

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

onehanddev mentioned this pull request Jan 8, 2025

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

Closed

1 task

atomiks approved these changes Jan 10, 2025

View reviewed changes

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

onehanddev added 2 commits January 10, 2025 16:24

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

eecafbd

Added test cases to verify that the parent menu item remains inactive…

9ac8981

… when a submenu item is opened.

atomiks reviewed Jan 14, 2025

View reviewed changes

onehanddev added 3 commits January 14, 2025 08:32

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

08a8d3e

fix: replaced tabindex with data-highlighted for <Menu.SubmenuTrigger…

7f69c60

… /> tests

Merge branch 'menu/submenu-highlight-keyboard-focus-updated' of https…

6281f1b

…://github.com/onehanddev/base-ui into menu/submenu-highlight-keyboard-focus-updated

fix: retained tabindex of the submenutrigger once the submenu opens

79cc42c

atomiks reviewed Jan 21, 2025

View reviewed changes

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 #1310

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

onehanddev commented Jan 8, 2025

netlify bot commented Jan 8, 2025 •

edited

Loading

mui-bot commented Jan 8, 2025

onehanddev commented Jan 8, 2025

atomiks left a comment

onehanddev commented Jan 13, 2025

atomiks Jan 14, 2025

onehanddev Jan 14, 2025

atomiks commented Jan 16, 2025

onehanddev commented Jan 16, 2025

atomiks Jan 21, 2025

atomiks Jan 21, 2025

onehanddev Jan 21, 2025

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

Are you sure you want to change the base?

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

Conversation

onehanddev commented Jan 8, 2025

netlify bot commented Jan 8, 2025 • edited Loading

✅ Deploy Preview for base-ui ready!

mui-bot commented Jan 8, 2025

Netlify deploy preview

onehanddev commented Jan 8, 2025

atomiks left a comment

Choose a reason for hiding this comment

onehanddev commented Jan 13, 2025

atomiks Jan 14, 2025

Choose a reason for hiding this comment

onehanddev Jan 14, 2025

Choose a reason for hiding this comment

atomiks commented Jan 16, 2025

onehanddev commented Jan 16, 2025

atomiks Jan 21, 2025

Choose a reason for hiding this comment

atomiks Jan 21, 2025

Choose a reason for hiding this comment

onehanddev Jan 21, 2025

Choose a reason for hiding this comment

netlify bot commented Jan 8, 2025 •

edited

Loading