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

Fix Menu component ARIA pattern #67078

Open
2 of 6 tasks
afercia opened this issue Nov 18, 2024 · 5 comments
Open
2 of 6 tasks

Fix Menu component ARIA pattern #67078

afercia opened this issue Nov 18, 2024 · 5 comments
Labels
[Focus] Accessibility (a11y) Changes that impact accessibility and need corresponding review (e.g. markup changes). [Package] Components /packages/components [Type] Bug An existing feature does not function as intended

Comments

@afercia
Copy link
Contributor

afercia commented Nov 18, 2024

Description

Spotted while inspecting the new 'Preview' menu introduced in the global styles after #66376

The Menu component is based on Ariakit. At a very first inspection, it appears there's at least two accessibility failures:

1
The menu may render a visually hidden button labeled 'Dismiss popup'. This breaks the ARIA menu pattern.

A menu should only contain menuitems, their variant menuitemradio/checkbox, and groups or separators.
The 'Dismiss popup' button can't be tabbed to or focused by using arrows anyways. However, screen reader users can still move to it.
I doubt the button label 'Dismiss popup' is translatable.
Regardless, this button shouldn't be there in the first place.

Screenshots, where I'm also revealing the visually hidden button by removing some CSS:

Image

Image

It appears the Ariakit Menu uses the Ariakit Dialog internally, and this button rendering is controlled by the modal prop.

I'm not into discussing the Ariakit internal implementation, which I find arguable, but this is not OK. This button should be removed.
At the very least, the Gutenberg implementation should explore to set the modal prop to false. However, I'm not sure about potential side effects when changing this prop value.
Alternatively, the implementation should be fixed upstream.

2
The Menu may rander an aria-labelledby attribute on the element with role=menu that apparently points to an invalid ID. Actually, the referenced ID is in the DOM so that the labeling is supposed to work, theoretically. This was interesting to debug and it took some time.

By default, the aria-labelledby of the element with role=menu points to the trigger button that opens the menu. As such, the menu is supposed to be labeled with whatever the accessible name of the trigger button is. It's a sensible default.

However, when the modal prop is true, when the Ariakit menu opens it adds (via the internal DIalog component) an inert attribute to the sibling elements, which in WordPress happens to be: <div id="wpwrap" inert>.

This triggers an inconsistent behavior between menus with a trigger button that uses an icon+aria-label and the ones that use a trigger button with visible text. The inconsistency appears to related to browsers behavior as well. I was able to reproduce this bug with Chrome but wasn't with Firefox.

When inspecting the DOM via the Chrome dev tools > Elements > Accessibility panel:

Screenshot:

Image

This isn't surprising because the trigger button, which is responsible to provide the labeling for the menu, is actually inside an inert part of the DOM. Browsers are supposed to make any inert part not perceivable. As such, at least in Chrome, the aria-labelledby points to an element that is in an inert area. I guess this explains the invalid source warning in Chrome.

Why it works for menus with an icon trigger button then? I can only guess that icon buttons have a tooltip that renders outside the inert part of the DOM. It appears this helps Chrome locate the referenced element and compute the accessible name.

I'm not sure what is the expected browsers behavior regarding referenced elements that live within an inert area. Historically, even if a referenced element is visually hidden with CSS or the hidden attribute, browsers are expected to be able to reference them for things like aria-labelledby or aria-describedby. However, the inert case seems ot be different, at least in Chrome.

Regardless, this appears to be one more occurrences of untested usage of the inert attribute. In previous issues and PRs, there have been already reports that the inert attribute is problematic for accessibility and should be used with extreme care. In all cases, its usage should be thoughtfully tested which doesn't appear to be the case for the Ariakit implementation.

Since the inert attribute is only added when the modal prop is true, I suggest to reconsider the usage of this prop or implementa more safe mechanism to make the rest of the page not preceivable. The Gutenberg Modal component uses the aria-hidden attribute for a reason.

Step-by-step reproduction instructions

1

  • Go to the Site editor > Styles
  • Click the Preview button in the middle panel that opens on the right of the navigation panel.
  • Inspect the dropdown menu.
  • Observe the element with role=menu contains a visually hidden button labeled 'Dismiss popup`.

2

  • Observe the element with role=menu does have an aria-labelledby attribute that correctly points to the trigger button.
  • Inspect the Chrome dev tools Elements > Accessibility panel.
  • Observe the aria menu accessible name is an empty string and the aria-labelledby value is reported as 'invalid source'.
  • Go to the Site Editor > Pages.
  • Click the Layout icon button in the middle panel that opens on the right of the navigation panel.
  • Inspect the dropdown menu.
  • Observe the aria menu accessible name is correctly computed by Chrome (see screenshot below).

Image

Screenshots, screen recording, code snippet

No response

Environment info

No response

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

  • Yes

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

  • Yes

Please confirm which theme type you used for testing.

  • Block
  • Classic
  • Hybrid (e.g. classic with theme.json)
  • Not sure
@afercia afercia added [Focus] Accessibility (a11y) Changes that impact accessibility and need corresponding review (e.g. markup changes). [Package] Components /packages/components [Type] Bug An existing feature does not function as intended labels Nov 18, 2024
@diegohaz
Copy link
Member

I created ariakit/ariakit#4270 to track this issue upstream.

@diegohaz
Copy link
Member

diegohaz commented Dec 1, 2024

I'd like to suggest some workarounds that Gutenberg can use to address these issues before they're fixed upstream, and I'd appreciate your feedback.

1
The menu may render a visually hidden button labeled 'Dismiss popup'.

Ariakit automatically renders a "Dismiss popup" button within modals as a fallback if the developer doesn't render one themselves. This is essential because mobile screen reader users lack other methods to close a modal. They can't press Esc or click outside. Without such an option, these users would be stuck in the menu once it's open, forcing them to refresh the page or find another way to navigate back.

As stated in the docs:

A visually hidden dismiss button will be rendered if the DialogDismiss component hasn't been used. This allows screen reader users to close the dialog.

If removing the modal prop is not viable, Gutenberg can explicitly render the dismiss button and decide if it should be accessible to everyone:

<Ariakit.MenuDismiss>
  {__("Dismiss popup")}
</Ariakit.MenuDismiss>

This isn't an option for Ariakit because it involves design decisions beyond the library's scope. However, we can definitely add a console warning about that.

To fix the accessibility tree, pass role="dialog" to the Ariakit.Menu component and wrap menu items in a <div role="menu">:

<Ariakit.Menu modal role="dialog">
  <Ariakit.MenuDismiss>
    {__("Dismiss popup")}
  </Ariakit.MenuDismiss>
  <div role="menu" aria-labelledby="...">
    {/* menu items */}

Alternatively, if the design allows, the dismiss button could be a regular menu item accessible to everyone:

<Ariakit.Menu modal>
  {children}
  <Ariakit.MenuItem render={<Ariakit.MenuDismiss />}>
    {__("Close")}
  </Ariakit.MenuItem>
</Ariakit.Menu>

2
The Menu may rander an aria-labelledby attribute on the element with role=menu that apparently points to an invalid ID.

Again, if removing the modal prop isn't an option, Gutenberg could explicitly add an aria-label or aria-labelledby attribute.

Alternatively, the menu button can be made accessible with the getPersistentElements prop:

<Ariakit.Menu
  modal
  getPersistentElements={() => {
    // Adds the menu button to the "modal context"
    const { disclosureElement } = menuStore.getState();
    if (!disclosureElement) return [];
    return [disclosureElement];
  }}
>
  {/* Prevents Ariakit from rendering a visually hidden dismiss button */}
  <Ariakit.MenuDismiss hidden />

This would address both (1) and (2) because all users could interact with the menu button and close the modal, making a dismiss button inside the menu not required.

In previous issues and PRs, there have been already reports that the inert attribute is problematic for accessibility and should be used with extreme care.

Are any of those related to Ariakit usage? If so, could you please tag me or link them here so I can take a closer look?

@afercia
Copy link
Contributor Author

afercia commented Dec 2, 2024

Are any of those related to Ariakit usage? If so, could you please tag me or link them here so I can take a closer look?

I'm not sure they are specifically related to Ariakit. Anyways one of the discussions about inert happened on #54369 where the most important part to me is that when using inert, screen reader users do not have an equal experience to sighted users. It really depends on the usage, which should always be very thoughtful.

@ciampo
Copy link
Contributor

ciampo commented Dec 2, 2024

Thank you @afercia for reporting, and @diegohaz for looking into the matter so swiftly!

Here are some additional considerations:

  • I doubt that we'll want to add an explicit "Close menu" item in all menus (cc @WordPress/gutenberg-design )
  • Re. modality, we've been discussing it in @wordpress/components: modality of popover-based components #63674. In short, it looks like we should limit the amount of modal popovers. As such, I'm soon going to test making Menu's popover non-modal by default. Having said that, we still plan on exposing the modal prop, which means that we still need to find a solutions that works well if the popover is marked as modal.
  • Re. the specific solutions, I'll defer to @afercia and @diegohaz 's feedback on the solutions proposed so far. I'm happy to create PRs as needed to test those solutions, though.

cc @WordPress/gutenberg-components

@afercia
Copy link
Contributor Author

afercia commented Dec 3, 2024

Regarding the inert attribute preventing the labelling of the menu to work as expected, honestly I'm not sure whether it's a specific Chrome bug or just a gray are ain the specs. That should be investigated.

To recap:

  • The menu is labelled by the means of an aria-labelledby attribute that points to the trigger button that opens the menu. In this case, the trigger menu name is 'Preview'.
  • Whe the menu opens, the trigger button is inside a wrapper that gets an inert attribute.
  • At this point, Chrome seems to ignore the trigger button to compoute the label of the menu.
  • However, usually the referenced ID for aria-labelledby or aria-describedby do work even if the referenced element is hidden with CSS or by the means of an HTML hidden attribuge.
  • Instead, they don't work with inert.
  • Firefox does compute the menu labeling so it appears it doesn't care if the trigger button is inside an inert. This is ht ebehavior I would expect.

Regardless whether it's a Chrome bug or a gray area in the specs. the point is the labeling of the menu doesn't work in the most used browser. We should fix it or make sure the Chrome is aware of this problem to potentially consider a fix.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Focus] Accessibility (a11y) Changes that impact accessibility and need corresponding review (e.g. markup changes). [Package] Components /packages/components [Type] Bug An existing feature does not function as intended
Projects
None yet
Development

No branches or pull requests

3 participants