Nested DropdownMenu stays open at deeper levels

[r100-stack]

Changes

TL;DR

• Fixes DropdownMenu close issue #1924
• MenuItem triggers increased. i.e. more affordances to navigate the DropdownMenu.

Trigger	Before	After
click	❌	✅
hover	✅	✅
focus	✅	❌ (since focus not always ≠ intention to open)
→/← arrow keys	❌	✅ (since arrow keys always = intention to open)
Esc	Closes one level of submenu	Closes entire DropdownMenu (keyboard users no longer need to press Esc multiple times)

• Since click trigger is added, passing a non-empty submenuItems array and onClick to MenuItem at the same time is no longer supported. This is because when a non-empty submenuItems array is passed, clicking the MenuItem toggles the submenu visibility.
  • If both passed, shows warning in console
• DropdownMenu and MenuItem refactored to remove hacky code

---

After a lot of investigation into what was causing the submenus to close unexpectedly, I was still unable to find a strong explanation. Despite not knowing the root cause, I noticed that the submenus seem to close as expected after wrapping the tree with a <FloatingTree> and the submenus with a <FloatingNode>. So, this PR makes that change.

Since I did quite a lot of investigation into this, I also decided (along with @mayank99) to use this opportunity to make some DropdownMenu behavioral improvements in this PR itself. Mayank and I decided upon these target behaviors:

Target DropdownMenu behaviors

• only one menu active at a time

  Details

  Before:

  Enregistrement.de.l.ecran.2024-04-08.a.6.49.31.AM.mov

  After:

  Enregistrement.de.l.ecran.2024-04-08.a.6.50.26.AM.mov

• you can hover out of a submenu and it stays open, as long as the focus was moved inside the submenu

  Details

  Before: all submenus would close if hovering out of the deepest level submenu. This is inconvenient since if a user hovers out accidentally, they lose all their submenu navigation.

  Enregistrement.de.l.ecran.2024-04-08.a.6.58.07.AM.mov

  After: If hovered out of a submenu after hovering into it, that submenu stays open.

  Enregistrement.de.l.ecran.2024-04-08.a.6.57.29.AM.mov

  And if the focus is inside the submenu, the submenu now remains still open.

  Before:

  Enregistrement.de.l.ecran.2024-04-08.a.7.09.21.AM.mov

  After:

  Enregistrement.de.l.ecran.2024-04-08.a.7.07.50.AM.mov

• arrow-right/arrow-left moves focus and opens/closes the menu

  Details

  Before: Submenu stays open even after pressing ←. This is not ideal since usually a ← means that the user wants to close the submenu.

  Enregistrement.de.l.ecran.2024-04-08.a.7.13.08.AM.mov

  After: Submenu closes upon pressing ←. Pressing → reopens the submenu and moves the focus inside.

  Enregistrement.de.l.ecran.2024-04-08.a.7.10.57.AM.mov

• escape key closes everything

  Details

  Before: Pressing Esc used to close one submenu at a time. This not ideal for keyboard users who want to close the entire dropdown menu. If they indeed want to close one menu at a time, they can use the left arrow.

  Enregistrement.de.l.ecran.2024-04-08.a.7.26.42.AM.mov

  After: Pressing Esc closes the entire dropdown. This is now consistent with some commonly used dropdown menus (E.g. MacOS context menu) and also the dropdown menus from other libraries (RadixUI, React ARIA).

  Enregistrement.de.l.ecran.2024-04-08.a.7.26.05.AM.mov

• Hovering a MenuItem focuses it. This is helpful when a user wants to stop using the mouse and instead continue navigating the Menu hierarchy using the keyboard, and vice versa.

  Details

  Before:

  Enregistrement.de.l.ecran.2024-04-26.a.15.37.28.mov

  After:

  Enregistrement.de.l.ecran.2024-04-26.a.15.36.00.mov

This PR implements these planned behaviors in an attempt to improve the overall DropdownMenu behavior.

Some other behavior changes/improvements

• When focusing on a trigger with a closed DropdownMenu, arrow ↓/↑ keys open the DropdownMenu and focus the first/last focusable node respectively. (The optional keyboard behavior mentioned in W3C Menu button pattern)

  Details

  Before:

  Enregistrement.de.l.ecran.2024-04-26.a.15.40.10.mov

  After:

  Enregistrement.de.l.ecran.2024-04-26.a.15.44.01.mov

• Disabled MenuItems no longer show their submenus (Nested DropdownMenu stays open at deeper levels #1942 (comment)).

• Since click trigger is added, passing a non-empty submenuItems array and onClick to MenuItem at the same time is no longer supported. This is because when a non-empty submenuItems array is passed, clicking the MenuItem toggles the submenu visibility.

  • If both passed, shows warning in console

Notable implementation points

• Wrapped DropdownMenu with FloatingTree and each MenuItem with FloatingNode
  • isNestedSubmenuVisible hack removed as FloatingTree + FloatingNode automatically handles keeping ancestor submenus open in deep submenu hierarchies.
  • Since isNestedSubmenuVisible is removed, isSubmenuOpen is now the source of truth.
• Added the useListNavigation interaction.
  • focusOnSubmenu hack removed as arrow navigation is implemented automatically in useListNavigation.
  • I tried to use it to also focus the MenuItem upon hover by adding focusItemOnHover: true. But this didn't work. So, I manually focus the MenuItems in onMouseEnter.
• MenuItemContext should be wrapped around the Menu containing the MenuItems. This contains parts that are important for useListNavigation.
• Renamed the triggers prop in usePopover to now be named as the interactions props. This prop allows booleans to turn on/off a particular interaction, or an object to override the default interaction props that usePopover passes to useFloating. Since usePopover is private, this does not result in a public-facing change.
• When adding the useClick trigger, I noticed it required two clicks to activate the submenu for the first time. I worked around this Floating UI behavior by manually toggling the submenu visibility in the onClick (Nested DropdownMenu stays open at deeper levels #1942 (comment)).
• Using tree events (docs) to ensure correct state of the DropdownMenu at all times
  • Only one active submenu in any menu at any given time.
  • Focusing a node "X" with submenu "Y" closes the submenus of "Y".

Self TODOs

• Undo all changes and then redo them to make sure all git precommit steps report no errors/warnings

• Hovering within the menus and submenus and then out always keeps the last used submenu open. But sometimes hovering from outside all the menu into the last opened submenu closes the menu unexpectedly. Currently working on this, but the PR is ready to review.

  Details

  Enregistrement.de.l.ecran.2024-04-08.a.12.10.50.PM.mov

Testing

• Tested in the vite playground (App.tsx).
• Added/Updated unit tests.

Docs

• Created changesets.

After PR TODOs

• Try replacing the cloneElements with contexts (#1942 (comment)) cloneElement from this PR is no longer being used.
• Try moving submenu logic from MenuItem into Menu.
  • (Important) Logic in Menu for the keyboard and the focusedIndex can be replaced by useListNavigation.

    • Handling with Private Menu component code optimization #2021

• See why safePolygon is only working for the last menu item (#1942 (comment)).

  • Handling with Floating UI safePolygon for non-last MenuItems #2026

• In ComboBox, use ListItem instead of MenuItem. Other similar changes also needed (#1942 (comment))
• Deprecate MenuItem (#1942 (comment))

[r100-stack]

@mayank99 and I had removed this in a previous commit (3861266). But I brought it back since I now remember why I had it in the first place. It was because if we don't update the active index upon hover/focus, then Floating UI's useListNavigation keeps the stale activeIndex.

Screen recording

Enregistrement.de.l.ecran.2024-04-25.a.14.52.41.mov

One small difference is that instead of updating the parent's activeIndex in onFocus, I am now doing it in onMouseEnter, where it is really needed because that is where we're manually changing the focus instead of letting useListNavigation handle it.

[r100-stack]

I realized the I was not using the listNavigation interaction in the Menu in DropdownMenu. Thus, the previously mentioned issue existed in the first-level Menu in the DropdownMenu.

Screen recording

Enregistrement.de.l.ecran.2024-04-25.a.16.08.21.mov

This is now fixed in 8a1bc78. Since there now are many contexts, open to suggestions (if any) for context names. In a follow up PR, I can try using jotai to reduce the number of contexts, similar to #2011.

[mayank99]

Does this change mean you can remove the keyboard navigation logic from Menu?

[mayank99]

Since there now are many contexts

I'm not sure I understand why the contexts needed to be split? setHasFocusedNodeInSubmenu could be made optional instead?

[r100-stack]

I'm not sure I understand why the contexts needed to be split? setHasFocusedNodeInSubmenu could be made optional instead?

Ohh, yes, I can do that instead. Merged MenuItemIndexContext and MenuItemsParentMenuContext into one context with the old name of MenuItemContext (ac857ca).

[r100-stack]

Does this change mean you can remove the keyboard navigation logic from Menu?

Yes, it can be removed if the children of Menu is always going to be a MenuItem. This is because MenuItem handles setting the parent's listItemsRef (code) which is needed for Floating UI'suseListNavigation to work.

E.g. I played around with doing this in SplitButton (8723bc0).

But since Menu supports non-MenuItem children too, I guess some cloning will be required to add a ref function that updates listItemsRef. So, we would need to add this to all internal uses of the Menu component that support non-MenuItem children and add this cloning logic to the children.

Since some more investigation might be required, I think this can be done after this PR. If so, I can add this as an after PR TODO.

[mayank99]

But since Menu supports non-MenuItem children too

This is a contrived case that will never happen.

I think this can be done after this PR

I'm ok with this, but it should be done sooner rather than later, since we now have duplicated logic for keyboard navigation.

[r100-stack]

This is a contrived case that will never happen.

I see, then should we update the JSDocs in a few places? Because it gives the notion that non-MenuItems in Menu are fully supported/allowed. Examples:

iTwinUI/packages/itwinui-react/src/core/Menu/Menu.tsx

Line 17 in d1af8d0

* Menu items. Recommended to use `MenuItem` components.

iTwinUI/packages/itwinui-react/src/core/ComboBox/ComboBox.tsx

Line 120 in d1af8d0

* If a `MenuItem` is returned, then focus styling is automatically handled.

iTwinUI/packages/itwinui-react/src/core/Select/Select.tsx

Line 514 in d1af8d0

* Custom renderer for an item in the dropdown list. `MenuItem` item props are going to be populated if not provided.

I'm ok with this, but it should be done sooner rather than later

Sounds good, added an after PR TODO with the prefix "(Important)" to prioritize it.

[mayank99]

ComboBox and Select (public) have completely different semantics from Menu (private), so they should not even be using MenuItem in the first place. It makes more sense to use ListItem.

No jsdoc changes necessary in this PR, but here's what I think makes the most sense as a plan for near-future:

1. Remove redundant logic from Menu and abstract some parts of DropdownMenu/MenuItem into Menu.
2. Replace incorrect usages of Menu (mainly in ComboBox and Select) with a new private component (could be called Listbox).
3. Deprecate MenuItem and replace with something like DropdownMenu.Item (and maybe ComboBox.Item/Select.Item).
4. Update all documentation to show the correct usage of all components.

[r100-stack]

Update: I think I've addressed all review comments, all tests are now passing, and I have also updated the PR description based on the latest developments in this PR.

[mayank99]

LGTM.

I'm not a fan of the useEffect and getFocusableNodes in 4f30668, but I'm ok with it as a stopgap since all tests are now passing and our immediate plan likely involves removing all this code.