Skip to content
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

DropdownMenuV2: inherit placement for nested submenus #56213

Closed

Conversation

ciampo
Copy link
Contributor

@ciampo ciampo commented Nov 16, 2023

What?

This PR proposes an additional piece of logic to the new experimental DropdownMenu component, by letting nested submenus inherit the placement from their parent menus.

Why?

The idea is that having nested submenus opening in the same "direction" is easier for the users both in terms of how the UI looks, but also in terms of UX and a11y (especially keyboard navigation) — here's an example scenario.

How?

See above

Testing Instructions

  1. Apply the changes from this PR to your local machine
  2. Load the "With Submenu" example in a standalone tab (link)
  3. Open all nested menus, notice how the all open to the right (without any placement prop applied)
  4. Apply this change to move the menu trigger button to the right part of the screen in the Storybook examples
Click to expand
diff --git a/packages/components/src/dropdown-menu-v2-ariakit/stories/index.story.tsx b/packages/components/src/dropdown-menu-v2-ariakit/stories/index.story.tsx
index a6319c6cfd..2802d7f960 100644
--- a/packages/components/src/dropdown-menu-v2-ariakit/stories/index.story.tsx
+++ b/packages/components/src/dropdown-menu-v2-ariakit/stories/index.story.tsx
@@ -119,7 +119,11 @@ export const Default: StoryFn< typeof DropdownMenu > = ( props ) => (
 );
 Default.args = {
 	trigger: (
-		<Button __next40pxDefaultSize variant="secondary">
+		<Button
+			__next40pxDefaultSize
+			variant="secondary"
+			style={ { float: 'right' } }
+		>
 			Open menu
 		</Button>
 	),
  1. Reload the same submenu example
  2. Notice how all submenus open to the left (still without any explicit placement prop)
  3. Modify the storybook example and set the placement prop on the various DropdownMenu components, make sure that the prop always takes priority on the default behaviour

Screenshots

Before After (this PR)
Screenshot 2023-11-16 at 20 15 24 Screenshot 2023-11-16 at 20 15 53

@ciampo ciampo self-assigned this Nov 16, 2023
@ciampo ciampo added [Type] Enhancement A suggestion for improvement. [Package] Components /packages/components labels Nov 16, 2023
@ciampo ciampo requested review from youknowriad, jameskoster, ntsekouras and a team November 16, 2023 19:18
@ciampo ciampo marked this pull request as ready for review November 16, 2023 19:18
@ciampo ciampo requested a review from ajitbohra as a code owner November 16, 2023 19:18
Copy link
Contributor

@jameskoster jameskoster left a comment

Choose a reason for hiding this comment

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

Neat!

I don't suppose it's related to this change, but just in case; the trigger seems to jump when it's aligned right:

jump

@andrewhayward
Copy link
Contributor

I'm a bit confused by both outcomes, to be honest! Either way, there are some issues with the keyboard navigation, which really are confusing, and which I would block this on.

When the trigger is on the left...

  1. Pressing down on the trigger opens the menu, and focuses the first menu item ("Level 1 item")
  2. Pressing down again moves to the second menu item ("Submenu trigger")
  3. Pressing right opens the submenu (visually to the right), and focuses the first submenu item ("Level 2 item")
  4. Pressing down twice moves to the third submenu item ("Submenu trigger")
  5. Pressing right opens the sub-submenu (visually to the right), and focuses the first sub-submenu item ("Level 3 item")
  6. Pressing right does nothing
  7. Pressing left closes the sub-submenu, and moves back to the submenu trigger item ("Submenu trigger")
  8. Pressing left closes the submenu, and moves back to the menu trigger item ("Submenu trigger")
  9. Pressing left again does nothing

When the trigger is on the right...

  1. Pressing down on the trigger opens the menu, and focuses the first menu item ("Level 1 item")
  2. Pressing down again moves to the second menu item ("Submenu trigger")
  3. Pressing right opens the submenu (visually to the left), and focuses the first submenu item ("Level 2 item")
  4. Pressing down twice moves to the third submenu item ("Submenu trigger")
  5. Pressing right opens the does nothing 🤔
  6. Pressing left opens the sub-submenu (visually to the left), and focuses the first sub-submenu item ("Level 3 item")
    1. Pressing left does nothing closes both the sub-submenu and the submenu, and focuses the menu trigger item ("Submenu trigger") 🤔
  7. Pressing right closes the sub-submenu, and moves back to the submenu trigger item ("Submenu trigger")
  8. Pressing right again closes the does nothing 🤔
    1. Pressing left closes the opens the sub-submenu (visually to the left), and focuses the first sub-submenu item ("Level 3 item") 🤔
  9. Pressing down moves the focus to the first submenu item ("Level 2 item")
  10. Pressing right closes the does nothing 🤔
  11. Pressing left closes the submenu, and moves back to the menu trigger item ("Submenu trigger")

Ariakit assigns arrow keys based on the placement prop for the submenu, and doesn't take into account that visually the menu may render with a different placement thanks to floating UI.

Evidently Ariakit tries to be smart about arrow keys, which I'm guessing is causing all of these issues. I'm not entirely sure why. I'd expect left and right to behave consistently (and flipped but still consistent when RTL), regardless of where the menu is visually.

@ciampo
Copy link
Contributor Author

ciampo commented Nov 17, 2023

@jameskoster

I don't suppose it's related to this change, but just in case; the trigger seems to jump when it's aligned right:

That seems to be triggered by Ariakit, since opening the DropdownMenu when modal also prevents the body from scrolling. So not related to this PR, but thank you for noticing!

@andrewhayward :

I'm a bit confused by both outcomes, to be honest! Either way, there are some issues with the keyboard navigation, which really are confusing, and which I would block this on.

I had flagged this behaviour in ariakit/ariakit#3029, but it seems like this behaviour was intentional and aimed at aligning with how native submenus work on both macOS and Windows. Does that make sense to you? Feel free to reply to the ariakit issue in case you have any feedback to add.

To solve this very same issue, in #55625 (comment) I had decided to apply an explicit placement="left-start" to the first submenu, which should at least ensure a coherent behaviour when navigating that instance of dropdown menu (all submenus open with left arrow key, etc) — do you think it's a good enough solution, in case?

@andrewhayward
Copy link
Contributor

...it seems like this behaviour was intentional and aimed at aligning with how native submenus work on both macOS and Windows. Does that make sense to you?

I can't find anything specific from Apple on navigating menus with keyboards, but when a menu is close to the edge of the screen, right still opens submenus (visually to the left), and left closes them.

Microsoft is also a little ambiguous on the matter:

Arrow keys. The arrow keys move input focus among the controls within a group. Pressing the right arrow key moves input focus to the next control in tab order, whereas pressing the left arrow moves input focus to the previous control.
...
Within a tab stop, the arrow key order should flow from left to right, top to bottom, without exceptions. The arrow keys should cycle through all items in both directions without stopping.

IBM however is rather more explicit:

Keyboard
Once the menu opens by pressing Return or Enter on the menu trigger, users can:
- Press Up and Down arrows to navigate between menu options
- Press Return or Enter or Right arrow on the item with the caret icon to reveal additional options
- Press Return or Enter on an item to select or deselect it
- Press Escape to close it

As is the W3:

Right Arrow:
- When focus is in a menu and on a menuitem that has a submenu, opens the submenu and places focus on its first item.
...
Left Arrow:
- When focus is in a submenu of an item in a menu, closes the submenu and returns focus to the parent menuitem.

I'm not aware of an OS pattern that swaps this, outside of RTL contexts.

The documentation is at least consistent in this though!

Apple: Use submenus sparingly. Each submenu adds complexity to the interface and hides the items it contains.
Microsoft: Avoid using submenus unnecessarily. Submenus require more physical effort to use and generally make the menu items more difficult to locate.
IBM: While submenus can help tidy up a context menu and make its options clearer, introducing multiple levels of submenus can make the user experience more complex and harder to navigate. Avoid multiple levels of nesting when it comes to submenus.

@ciampo
Copy link
Contributor Author

ciampo commented Nov 17, 2023

Thank you, @andrewhayward . I left a reply in the related ariakit issue.

Specifically to this PR, I would argue that the issues with the keyboard navigation UX already affect DropdownMenu as it is on trunk — if anything, this PR would improve the situation by applying more consistency to the placement (and therefore, the default keyboard behaviour) of nested submenus.

In other words, I wonder if we could review this PR and potentially merge it without waiting on potential changes on the ariakit side.

@diegohaz
Copy link
Member

@jameskoster

I don't suppose it's related to this change, but just in case; the trigger seems to jump when it's aligned right:

That seems to be triggered by Ariakit, since opening the DropdownMenu when modal also prevents the body from scrolling. So not related to this PR, but thank you for noticing!

That's because there's padding in Storybook's body element, and Ariakit adjusts this padding to compensate for the scrollbar width. Currently, it's not checking the current padding (as it could be a padding added by another parent/sibling modal). Generally, this isn't an issue because sites don't typically apply padding to the body. However, it's definitely something we should take into account in Ariakit.

@ciampo
Copy link
Contributor Author

ciampo commented Nov 30, 2023

Hey folks, do we think this PR can be landed, or is there too much skepticism around the proposed behaviour?

@jameskoster
Copy link
Contributor

I don't have a strong feeling either way to be honest. The overlapping menus is a bit awkward, but not the end of the world, especially if we can improve our elevation styles (shadows). Perhaps most importantly it feels consistent with the keyboard behavior which is something I don't think we should mess with – the W3 guidelines seem very sensible to me.

@ciampo
Copy link
Contributor Author

ciampo commented Dec 1, 2023

Sounds good. I'm going to be closing this issue based on the overall skepticism around the keyboard interaction, and in the hope that we can deal better with overlapping submenus by tweaking the styles.

@ciampo ciampo closed this Dec 1, 2023
@johnbillion johnbillion deleted the feat/dropdown-menu-ariakit-placement-inheritance branch February 10, 2025 16:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Package] Components /packages/components [Type] Enhancement A suggestion for improvement.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants