search

WordPress / gutenberg

The Block Editor project for WordPress and beyond. Plugin is available from the official repository.
https://wordpress.org/gutenberg/
Other
10.4k stars 4.16k forks source link

DropdownMenu v2 #50459

Open ciampo opened 1 year ago

ciampo commented 1 year ago

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:

Next steps:

This may close #18537

alexstine commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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

SaxonF commented 1 year ago

Nice work!

(Edit by @ciampo : moved the feedback to a separate issue: https://github.com/WordPress/gutenberg/issues/50910)

andrewhayward commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

@ciampo are you thinking this may make it into 6.3 (re https://github.com/WordPress/gutenberg/pull/51400)?

ciampo commented 1 year ago

@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 commented 1 year ago

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 commented 10 months ago

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 https://github.com/WordPress/gutenberg/pull/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 commented 10 months ago

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 commented 10 months ago

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 commented 10 months ago

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
jorgefilipecosta commented 8 months ago

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.