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

DropdownMenu v2 #50459

Open
30 of 53 tasks
ciampo opened this issue May 9, 2023 · 23 comments
Open
30 of 53 tasks

DropdownMenu v2 #50459

ciampo opened this issue May 9, 2023 · 23 comments
Assignees
Labels
[Package] Components /packages/components [Type] Enhancement A suggestion for improvement. [Type] Tracking Issue Tactical breakdown of efforts across the codebase and/or tied to Overview issues.

Comments

@ciampo
Copy link
Contributor

ciampo commented May 9, 2023

Related to #49271

We would like to build a mode modular, accessible, and feature-rich (eg. with support for sub-menus) version of the DropdownMenu component.

Given how tricky it can be to implement this feature in an accessible and usable way, we're going to leverage ariakit and its Menu component. We initially tested Radix UI but found some blockers.

Choosing an API approach

After some initial discussion, we've landed on an approach:

  • Keep ariakit as an internal implementation detail as much as possible
  • Expose as few props as needed
  • Expose low-level, granular components, to allow for greater flexibility
  • Consider adding as many guidelines, examples, and any other tools to help consumers use the component correctly.

Next steps:

This may close #18537

@alexstine
Copy link
Contributor

Please let me know when ready for accessibility testing. Added a couple required checks above to ensure it is not missed before publishing the package.

@ciampo
Copy link
Contributor Author

ciampo commented May 23, 2023

Hey all, an experimental version of the new DropdownMenu component has been added to the repository:

These are still very early days, but it is a great time to gather some early feedback on all aspects:

In order to keep this issue as a tracking issue, it would be great if y'all could open separate issues — we can link them all in this issue's description.

In the upcoming days, I'm going to start testing the new component in the editor, in order to spot any issues and missing features, and I'll start work on adding support for using the component with SlotFill.

Thanks!

@jameskoster
Copy link
Contributor

That's looking great already ✨

Seems like the visual design is based on some rough explorations I worked on in Figma. Here's the link to that in case anyone else would like to riff on the appearance.

@alexstine
Copy link
Contributor

@ciampo This seems to be testing well on Windows. Would be nice to get a Mac review. CC: @afercia .

@SaxonF
Copy link
Contributor

SaxonF commented May 24, 2023

Nice work!

(Edit by @ciampo : moved the feedback to a separate issue: #50910)

@andrewhayward
Copy link
Contributor

Was playing around with the Storybook example, and I noticed that pressing escape collapses the entire menu. This is fine at the top level, but submenus should collapse up to their parent. I checked the underlying Radix UI component, and it behaves the same, so I'm guessing it's a problem there rather than with this. Will need to raise a ticket on that project to resolve.

@andrewhayward
Copy link
Contributor

Will need to raise a ticket on that project to resolve.

Looks like a ticket was already raised, and closed as "won't fix". Not a blocker, by any stretch, but probably worth making note of.

@alexstine
Copy link
Contributor

I would prefer the escape to take me out of submenus before closing the entire menu especially if you were multiple levels deep. The argument about Mac OS in that issue is invalid for Windows. Opening a Windows context menu, then submenu, and pressing escape will focus the submenu trigger.

I am not sure it should be a blocker for the first implementation but I am not a fan of how the contributors dismissed that issue. This could come back to haunt us later. It's the risk you take with 3rd-party.

@ciampo
Copy link
Contributor Author

ciampo commented May 25, 2023

It's the risk you take with 3rd-party.

Yup, and it is a calculated one. Using a well-known and actively maintained third-party library allowed us to iterate on this otherwise fairly complex component quite quickly and with very good results. We can definitely expect situations like this one, but the hope is that there won't be many and that the trade-off will be advantageous for the project.

@andrewhayward
Copy link
Contributor

andrewhayward commented May 25, 2023

I think we've got a few options at this point:

  1. Leave things as they are, and accept the situation for what it is.
  2. Work with the upstream project, if not to change the current behaviour (evidently that's unlikely to happen), then at least to contribute something that would give us more flexibility to do things to our preference.
  3. Fork the upstream project and make adjustments.
  4. Make adjustments within our component. Given that we're not exposing any upstream primitives, we can override specific event handlers without causing complexity for component consumers.

I'm not a huge fan of 1, but it might be sufficient for the immediate term to get something working. 2 runs the risk of being rejected, at which point we'd be back to this conversation. 3 is (from my perspective anyway) a total non-starter. And 4 is arguably something of a hack, but could probably be done relatively cleanly, at least from a code perspective.

@mirka
Copy link
Member

mirka commented May 25, 2023

Leave things as they are, and accept the situation for what it is

To be honest, I was kind of persuaded by all the reasoning that was given in closing the upstream issue as wontfix. The left arrow key will always return you to the closest parent, whereas Escape is the only key that will escape you out completely no matter what. (I'm imagining a situation where the focus is in a submenu nested two or more levels deep.)

So given that there is a functional advantage, plus the ARIA APG being ambiguous, plus two major reference implementations (Windows and macOS) diverging on this, I don't see the current Escape behavior as a clearly bad thing we should fix.

@alexstine
Copy link
Contributor

Yes, I think it is likely fine the way it is and should not be blocked. I just wanted to highlight the fact that if we do go down the road of implementing more 3rd-party components, these types of issues might not be so clear-cut. There is also a reason why people with disabilities continue to keep Windows at the top of the operating system list. Anyone here who has tried to use Mac Voiceover I am sure understands why.

Anyway, super excited for this to continue. This will be a nice submenu implementation that very closely mocks Google Drive and now Gmail context menus.

Thanks for the work here.

@andrewhayward
Copy link
Contributor

Ultimately the upstream dependency isn't wrong, as there's no normative right in this context, and their reasoning isn't way off base.

I think using MacOS as the primary example isn't great, and more people would probably expect escape to go up a level rather than collapsing the whole thing. But it does work, so I think it's fine to leave it as is until/unless we get overwhelming feedback otherwise.

@alexstine
Copy link
Contributor

I did check the submenu implementation in Google Drive and escape does collapse the whole menu. I agree, non-standard ARIA can be annoying.

@mtias
Copy link
Member

mtias commented May 26, 2023

Thank you all for driving and engaging on this one. Can't wait to leverage it to improve several areas of the current user interface that are unnecessarily long and disorganized.

@richtabor
Copy link
Member

richtabor commented Jun 15, 2023

@ciampo are you thinking this may make it into 6.3 (re #51400)?

@ciampo
Copy link
Contributor Author

ciampo commented Jun 16, 2023

@ciampo are you thinking this may make it into 6.3 (re #51400)?

Hey Rich, I've just posted an update directly on the PR. The TL;DR is that we've hit a bit of a wall around how the DropdownMenu enforces its "modality" and how it interacts with other Popover-based components.

I'll continue exploring more solutions in the next days, but as of today I'm not sure we'll be able to land #51400 before the 6.3 release.

In the meantime, it would be great to start getting some feedback about that PR anyway!

@ciampo
Copy link
Contributor Author

ciampo commented Sep 29, 2023

Quick update: after #51400 surfaced a few critical limitations with using radix-ui for the new DropdownMenu component in the block editor, I tested ariakit as an alternative with encouraging results.

In the upcoming days, I will go ahead and try to put together a version of DropdownMenu based off ariakit, and then I'll try it again in the block settings dropdown — hopefully we should be on a good path 🤞

I will also update this issue's TODOs to better reflect the new plan of action.

@afercia
Copy link
Contributor

afercia commented Dec 6, 2023

I'd like to propose to consider to make the new dropdown component handle automatically the case where the passed items are only 1 item. See #56792 for the current droopdown.
We should avoid the component to render a dropdown that contains only one item as that's not a great UX. Could this be handled automatically? Cc @jorgefilipecosta

@andrewhayward
Copy link
Contributor

We should avoid the component to render a dropdown that contains only one item as that's not a great UX. Could this be handled automatically?

A couple of thoughts...
Two rows of action buttons, side by side. The first set shows ellipsis action menus, one being open with a single 'edit' item. The second shows 'edit' icons in place of two of the action menus, with the third menu remaining.

First, I'm not sure how we could reasonably translate any generic menu item into a button in an automated way, given the number of variables involved. In the example above, we've going from a menu item with text and no icon, to an action button with an icon but no text. Not saying it's not possible, but certainly not straightforward. The API for doing so could get quite complex quite quickly.

Second, while I agree that we should ideally avoid action menus with one item, there's also an argument for consistency. There's a lot out there on macro consistency (including SC 3.2.3 and SC 3.2.4), but not so much on micro consistency. So this is isn't a suggestion either way, but we should definitely think about whether (in cases like this specifically) it's actually more confusing to have a mix of different modalities.

@youknowriad
Copy link
Contributor

Just a comment as a regular user. If I have in the same table, two rows and maybe one row is editable and the other not. which would mean, for one row, you have two items in the "more menu" and for the other only one. I'd definitely favor consistency over having the button in different places across the two rows. So IMO, it's not something to be handled at the DropdownMenu component level.

@jameskoster
Copy link
Contributor

FWIW it should be very rare for the ellipsis menu in data views to only contain a single action. The idea is for it to also contain quick edit actions like 'rename', 'view revisions', 'make sticky'... essentially everything you can do in the 'quick edit' experience in wp-admin. An older mockup for pages:

Screenshot 2023-12-06 at 13 49 11

@ciampo ciampo added the [Type] Tracking Issue Tactical breakdown of efforts across the codebase and/or tied to Overview issues. label Dec 22, 2023
@jorgefilipecosta
Copy link
Member

I'd like to propose to consider to make the new dropdown component handle automatically the case where the passed items are only 1 item. See #56792 for the current droopdown. We should avoid the component to render a dropdown that contains only one item as that's not a great UX. Could this be handled automatically? Cc @jorgefilipecosta

I guess automatically avoiding rendering dropdown menus with only one item is not feasible. It depends on what triggers the dropdown menu to open. It may be a simple button, it may be a small toolbar button, etc. But when using dropdown menus if the possibility of having only one item exists we should add the logic to avoid that.

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. [Type] Tracking Issue Tactical breakdown of efforts across the codebase and/or tied to Overview issues.
Projects
None yet
Development

No branches or pull requests