WordPress / gutenberg

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

The `ContentOnlySettingsMenu` breaks the ARIA menu pattern #65514

Open afercia opened 1 month ago

afercia commented 1 month ago

Description

https://github.com/WordPress/gutenberg/pull/62354 introduced a ContentOnlySettingsMenu component that is then used in a few places in the editor. This component adds a menu item and a paragraph with some extr ainformation inside a few dropdown menus.

The dropdown menus are ARIA menus. They can only contain specific roles and they cannot contain paragraphs.

There have been ohter cases in the past where extraneous content was added inside ARIA menus and they all have been raised as issues and fixed. I'm surprised extraneous content has been added to ARIA menus again as this has been reported a few times already for previous cases. See for example: https://github.com/WordPress/gutenberg/issues/58313 https://github.com/WordPress/gutenberg/issues/13390 https://github.com/WordPress/gutenberg/issues/12505 https://github.com/WordPress/gutenberg/issues/5953

It's worth reminding:

[!IMPORTANT] An ARIA menu can only contain elements with role group, separator, menuitem and the menuitems variants. It can not contain anything else.

Reference:

https://www.w3.org/TR/wai-aria-1.2/#menubar Required Owned Elements:

The presence of the paragraph with extra information:

Note: For the ones that only test with VoiceOver on macOS, the behavior described above only happens with Windows screen readers. VoiceOv er has a different interaction model which is, in a way, non-standard.

Also important: I seem to recall there were checks in place to make sure an UI with role=menu only contains the expected roles. Not sure if something has changed in that regard or maybe the checks I'm thinking at are specific to some other component. Cc @youknowriad

Regardless, this extraneous content inside the ARIA menus must be removed as it totally breaks the expected semantics and interaction.

From a design perspective, I'd argue this isn't a great design pattern as some important information is buried down inside a dropdown menu, which is just a good way to hide important information,

@WordPress/gutenberg-components given this happened a few times already, it appeaers to me there should be some kind of mechanism to ensure an ARIA menu doesn't contain extraneous content. Is there any way to make sure an ARIA menu only contains menu items? Thanks.

Cc @ramonjd @talldan @kevin940726

Step-by-step reproduction instructions

To see an example of this pattern follo the steps below. Other example are in the screenshot in the original PR https://github.com/WordPress/gutenberg/pull/62354

Screenshots, screen recording, code snippet

Screenshot 2024-09-20 at 09 18 07

Environment info

No response

Please confirm that you have searched existing issues in the repo.

Please confirm that you have tested with all plugins deactivated except Gutenberg.

ciampo commented 1 month ago

given this happened a few times already, it appeaers to me there should be some kind of mechanism to ensure an ARIA menu doesn't contain extraneous content. Is there any way to make sure an ARIA menu only contains menu items? Thanks.

This kind of runtime checks are not easy to write, can be costly and they are not perfect — developers who really want to do something usually find a way regardless. Especially if the "invalid" items are injected via a slot/fill.

The best solution, IMO, is to add e2e tests that check these menus

kevin940726 commented 3 weeks ago

The accessibility improvement is meant to be an immediate follow-up, but I guess we never get to it 🤦. I think we could implement some special cases for screen readers, if we want to keep the design intact.

The best solution, IMO, is to add e2e tests that check these menus

I think we can run an automatic accessibility check via Axe (if they can catch this sort of bug). We might actually use to do this with Puppeteer, but probably left out in Playwright 🤔.

afercia commented 3 weeks ago

I think we could implement some special cases for screen readers, if we want to keep the design intact.

The design is just wrong and it's not accessible. It needs to be changed. The arguments in this report are valid. The UI needs to be designed in an accessible way to start with, and breaking patterns isn't the best way to do that.

kevin940726 commented 3 weeks ago

Curious to hear what @jameskoster thinks about the alternative UI 🙏.

jameskoster commented 3 weeks ago

This feature doesn't seem to be working as intended any more. I see the paragraph and associated actions spread throughout the menu:

Screenshot 2024-09-26 at 12 24 14

This makes the experience more confusing.

It may be simplest to remove the text for now.