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

MenuItem must use aria-checked only when its role is menuitemcheckbox or menuitemradio #11431

Closed
afercia opened this issue Nov 2, 2018 · 5 comments
Assignees
Labels
[Focus] Accessibility (a11y) Changes that impact accessibility and need corresponding review (e.g. markup changes). [Type] Bug An existing feature does not function as intended

Comments

@afercia
Copy link
Contributor

afercia commented Nov 2, 2018

See #8279 and following changes. See #10545

The aria-checked state is used to indicate the "checked" state of checkboxes, radio buttons, and other widgets. It must be used exclusively with the following ARIA roles:

https://www.w3.org/TR/wai-aria-1.1/#aria-checked
used in roles:
checkbox
option
radio
switch

inherits in roles:
menuitemcheckbox
menuitemradio
treeitem

The MenuItem component has a default ARIA role of menuitem, and it's used also with the roles menuitemcheckbox and menuitemradio.

However, its aria-checked HTML attribute is set based on its isSelected prop, regardless of its role:

'aria-checked': isSelected,

The component should make sure aria-checked is set only when the role prop is menuitemcheckbox or menuitemradio.

Also, it should make sure that when a isSelected prop is passed, the actual intent of the developers is to communicate a "checked" state, thus a menuitemcheckbox or menuitemradio should be used.

Currently, seems to me the correct usage of this component is up to the developers knowledge, while I'd tend to think a component shouldn't assume such a deep accessibility knowledge.

Right now, there's at least one case where aria-checked is applied incorrectly on a role menuitem: the BlockNavigation component:

screenshot 2018-11-02 at 18 55 09

The menuitem role doesn't have any "checked" state, it's meant for generic items in a menu.

Reference:

menuitem role:

https://www.w3.org/TR/wai-aria-1.1/#menuitem

An option in a set of choices contained by a menu or menubar.

menuitemcheckbox role:

meant to indicate a menu item with checked state, where multiple choices are allowed:

https://www.w3.org/TR/wai-aria-1.1/#menuitemcheckbox

A menuitem with a checkable state

menuitemradio role:

meant for a menu item in a set of items where only one can be checked (single choice):

https://www.w3.org/TR/wai-aria-1.1/#menuitemradio

A checkable menuitem in a set of elements with the same role, only one of which can be checked at a time.

Worth also mentioning the implementation details in the ARIA Authoring Practices:
https://www.w3.org/TR/wai-aria-practices-1.1/#menu

Not sure what is the best path forward, but I'd recommend to consider to make developers life easier offering them a component that sets the correct ARIA roles and states/properties based on a simple prop. Maybe something like a type generic, singleChoice, and multipleChoice or something along those lines?

@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 Nov 2, 2018
@designsimply
Copy link
Member

I noticed at the top of https://www.w3.org/TR/wai-aria-1.1/#aria-checked it says:

aria-checked (state) indicates the current "checked" state of checkboxes, radio buttons, and other widgets.

Forgive me if these are silly questions. Could block navigation be considered a widget? I noticed both check boxes and menu items are listed in the definition of a widget on that same page. Would switching from menuitem to treeitem solve the problem for block navigation or are you saying that aria-checked should simply be removed for block navigation menu items because menuitem isn't explicitly mentioned in the characteristics table?

Aside from that, is there a use case that can show the problem you are trying to solve? For example, navigating block navigation using a Screenreader or VoiceOver is not working? Or is this strictly about adhering to W3C WAI recommendations and to be considered a code quality issue?

@afercia
Copy link
Contributor Author

afercia commented Nov 3, 2018

Absolutely. Thanks for your question, it gives me the opportunity to dive into details a bit better 🙂

When it comes to assistive technology software, it's like going back to how browsers used to behave 15 years ago 😬There are different implementations, inconsistencies, and support for some features greatly varies. The only way to maximize support is to strictly adhere to the specification, as that's the basement assistive technology software are built upon.

A menu is certainly a composite widget, see https://www.w3.org/TR/wai-aria-1.1/#widget_roles
Worth also noting "widget" is a term that's also generically used to refer to user interface elements that don't exist natively in HTML.

As per the consequences of not using ARIA roles and attributes correctly, they vary. In my testing, seems Safari and VoiceOver don't pay much attention to it and announce "ticked" anyways.

Instead, for example with Firefox and NVDA, there's an important difference:

menuitemcheckbox: the checked state is announced:

screenshot 122

menuitemradio: the checked state is announced

screenshot 123

menuitem: the checked state is not announced, thus not allowing users to understand which block is the selected one in the blocks navigation menu 😞

screenshot 124

Aside: the two screen readers also announce the same information in different ways: "ticked" vs. a more verbose wording. That's typical and I guess JAWS and other screen readers announce it in their own way as well.

@BE-Webdesign
Copy link
Contributor

I can work on this. Thank you for your knowledge @afercia!

@BE-Webdesign BE-Webdesign self-assigned this Nov 3, 2018
@afercia
Copy link
Contributor Author

afercia commented Nov 3, 2018

@BE-Webdesign @designsimply thanks!
Not sure what are the plans, but I've just realized that if this menu is meant to be used also with nested block and to represent their "indentation level", then an ARIA menu wouldn't be appropriate and we should consider an ARIA tree. I guess it's something that can go in next iterations, if necessary.

@BE-Webdesign
Copy link
Contributor

BE-Webdesign commented Nov 3, 2018

@afercia,

I think that would make another good issue. The aria-checked, although related, is separate from how the hierarchy/indentation level is structured.

talldan pushed a commit that referenced this issue Nov 8, 2018
…em. (#11459)

* Fixes #11431. Adjust aria-checked attribute to only work on matching aria
roles.

https://www.w3.org/TR/wai-aria-1.1/#aria-checked

* Modify BlockNavigationList to make proper use of aria roles.

* Modify the editor CHANGELOG.md to reflect a11y changes.

* Modify changelog for components package to reflect a11y changes.

* Add additional test case for MenuItem when aria-checked should be used.

* Test fix for missing function toBeTrue, use toBe( true ) instead

* Update README.md as well as update MenuItem to better fit coding aesthetics.

* Modify changelog to reflect new feature.
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). [Type] Bug An existing feature does not function as intended
Projects
None yet
Development

No branches or pull requests

3 participants