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

The Heading block "Change heading level" button should open a menu, not an additional toolbar #24797

Closed
afercia opened this issue Aug 25, 2020 · 4 comments
Labels
[Block] Heading Affects the Headings Block [Focus] Accessibility (a11y) Changes that impact accessibility and need corresponding review (e.g. markup changes). [Type] Regression Related to a regression in the latest release

Comments

@afercia
Copy link
Contributor

afercia commented Aug 25, 2020

Describe the bug

See related #24796 for the value to use for the aria-haspopup attribute.

In the Heading block, clicking the "Change heading level" button opens an additional UI with a group of buttons to change the heading level:

aria-haspopup for a toolbar

This additional UI uses an ARIA role=toolbar so basically users will get the following information while navigating through the UI:

  • the block toolbar is communicated as an ARIA toolbar
  • users expects the toolbar to contain controls that trigger actions or open additional menus
  • pressing the "Change heading level" button, users will get an additional toolbar instead:

Screenshot 2020-08-25 at 16 52 04

  • this is unexpected and potentially confusing
  • users would expect a menu instead
  • also, from a semantics perspective, the heading level buttons provide users with a single choice: they would be better represented by a menu with a set of role=menuitemradio

I'm not sure why the heading level buttons are wrapped in a "sub-toolbar" in the first place. Under all aspects, seems to me this should be a menu. The fact the design wants a group of buttons visually presented in an horizontal way doesn't matter: menus can be horizontal as well, and use an aria-orientation=horizontal attribute. https://www.w3.org/TR/wai-aria-1.1/#menu

@afercia afercia added [Type] Bug An existing feature does not function as intended [Focus] Accessibility (a11y) Changes that impact accessibility and need corresponding review (e.g. markup changes). labels Aug 25, 2020
@afercia
Copy link
Contributor Author

afercia commented Aug 25, 2020

Worth exploring whether the DropdownMenu component (which uses NavigableContainer internally) already has what is necessary to render a horizontal menu with left/right arrow keys navigation.

@ZebulanStanphill ZebulanStanphill added [Type] Regression Related to a regression in the latest release and removed [Type] Bug An existing feature does not function as intended labels Aug 25, 2020
@ZebulanStanphill
Copy link
Member

Thanks for opening this issue. When I made the change in #20246, I asked for implementation feedback and at the time it seemed like there were no issues. I would not have merged the PR if I was aware that there were accessibility regressions. So thanks for pointing them out. I had a feeling there was something not quite right about my implementation, but nobody found any issues with it at the time.

To be clear, the reason for making the heading level control act that way was to prepare for #22650... a PR which was ironically intended to help users choose more semantic/accessible heading levels. (And annoyingly, it is blocked at the moment due to the need for a document outline API... the same reason the Table of Contents block PR is stuck.)

I also wonder how the heading level validator from that PR ties into this. Can you still put the heading level validator into the dropdown if the dropdown is using a menu?

@afercia
Copy link
Contributor Author

afercia commented Aug 26, 2020

@ZebulanStanphill thanks for shedding some light on the history of this implementation, much appreciated. Yep, this is something we probably missed at that time.

While the current implementation works , this issue is more about what users would expect adn the best way to communicate what kind of additional UI a control opens. The UI needs to provide a consistent, predictable, experience to users.

In this case, ARIA gives us a way to communicate the availability of an additional UI via the aria-haspopup attribute, where the "popup" is the additional UI. As mentioned in #24796, the allowed values in ARIA 1.1 are:

false (default)	Indicates the element does not have a popup.
true            Indicates the popup is a menu.
menu            Indicates the popup is a menu.
listbox         Indicates the popup is a listbox.
tree            Indicates the popup is a tree.
grid            Indicates the popup is a grid.
dialog          Indicates the popup is a dialog.

Worth reminding most of the ARIA patterns replicate patterns that are in use in the various operating systems, so that users are supposed to be familiar with these patterns. A toolbar button that opens another toolbar isn't ideal because it's not expected. Amongst these values, the most common and easily understandable are "menu" and "dialog".

However, a "menu" should exclusively contain menu items: any other additional UI inside a menu would be unexpected and users would likely miss it completely, also because arrows navigation loops only through the menu items.

Looking at #22650, I'd tend to think the only value that would fit with the need of providing users with an additional UI made of different widgets would be the "dialog" one. For example:

  • users click the "Change heading level" button, which has an aria-haspopup="dialog" attribute
  • some visual UI (could be a "popover") opens and it's semantically a dialog
  • the dialog contains a set of buttons to change the heading level (semantics to be decided, but at this point I don't think it should be a menu)
  • the dialog contains also the heading level validator

I'd like to discuss this idea with the accessibility team though. This usage of a "dialog" would be a bit unorthodox but I can't think of another way to make the user experience as predictable as possible.

@afercia
Copy link
Contributor Author

afercia commented Jul 28, 2022

Closing, as now this uses a ToolbarDropdownMenu component with a role="menu" and menu items. #42273 recently added role="menuitemradio" to the menu items and aria-checked="true" to the selected heading level.

@afercia afercia closed this as completed Jul 28, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Block] Heading Affects the Headings Block [Focus] Accessibility (a11y) Changes that impact accessibility and need corresponding review (e.g. markup changes). [Type] Regression Related to a regression in the latest release
Projects
None yet
Development

No branches or pull requests

2 participants