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

Change aria-checked to only be used on matching ARIA roles for MenuItem. #11459

Merged
merged 8 commits into from Nov 8, 2018

Conversation

Projects
None yet
3 participants
@BE-Webdesign
Contributor

BE-Webdesign commented Nov 3, 2018

Fixes #11431.

Description

Changes aria-checked to only be used on matching ARIA roles for MenuItem. Fixes an a11y issue where Screen readers can not properly understand the BlockNavigationList for which block is currently active.

How has this been tested?

Via Jest and manually: see here

Types of changes

Updates the MenuItem component from the general components library to only use aria-checked when the aria-role is either menuitemcheckbox, or menuitemradio. This also changes BlockNavigationList to use the menuitemradio role, so screen readers can properly understand which block is currently active.

Checklist:

  • My code is tested.
  • My code follows the WordPress code style.
  • My code follows the accessibility standards.
  • My code has proper inline documentation.
@talldan

Hi @BE-Webdesign - thanks for working on this, it's not a straightforward issue, but this looks like an improvement to the current code.

I think there are some things to address, so I left comments.

I'd also like to invite @afercia in to review this.

@@ -74,15 +74,21 @@ export function MenuItem( {
props.icon = icon;
}
const atts = {

This comment has been minimized.

@talldan

talldan Nov 6, 2018

Contributor

Convention is that this should be called 'props' instead of atts. I think childProps is a good name given props is already used in this scope. Alternatively could avoid extracting the props into a variable and keep them inline:

const supportsAriaChecked = role === 'menuitemcheckbox' || role === 'menuitemradio';
const ariaChecked = supportsAriaChecked  ? isSelected : undefined;
// ... snip
return createElement(
		tagName,
		{
			'aria-label': label,	
			'aria-checked': ariaChecked,	
			role,	
			className,	
			...props,	
		},
};
// Make sure aria-checked matches spec https://www.w3.org/TR/wai-aria-1.1/#aria-checked
if ( role === 'menuitemcheckbox' || role === 'menuitemradio' ) {

This comment has been minimized.

@talldan

talldan Nov 6, 2018

Contributor

I've worked on this before and wondered what the right behaviour should be.

Quoting the original issue:

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

This PR satisfies the above logic.

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.

However I think this request in the issue still has a question mark—a developer could still easily use the wrong role, only now they'd not have aria-checked output at all.

This PR does at least document the constraint in the form of code, so that's good. I think it'd be good to update the README.md for the MenuItem detailing the intended behaviour of the props as well.

<MenuItem role="menuitem"><div /></MenuItem>
);
expect( wrapper.prop( 'aria-checked' ) ).toBeUndefined();

This comment has been minimized.

@talldan

talldan Nov 6, 2018

Contributor

I don't think this test is quite right since it would have passed with the previous code as well (aria-checked would be undefined whenever isSelected is undefined), so there's no assertion that the behaviour has changed. The MenuItem should have an isSelected prop to demonstrate that the prop is not included in the rendered output.

@talldan talldan requested a review from afercia Nov 6, 2018

@BE-Webdesign

This comment has been minimized.

Contributor

BE-Webdesign commented Nov 7, 2018

@talldan All set.

@talldan

talldan approved these changes Nov 8, 2018

Thanks for addressing those things. This looks good code-wise for me. Would still love an a11y check, to make sure it addresses the issue.

@afercia

afercia approved these changes Nov 8, 2018

From an a11y perspective LGTM. Love to see specifications enforced through code, thank you.

screenshot 131

The Menu component has now a good a11y, can be used for generic items, single choice, and multiple choice with a proper feedback for assistive technologies.

Re: the blocks navigation menu, worth noting if it's meant to support nested blocks too (e.g. blocks within Columns) then I guess it should be changed in something else than a menu. Will discuss this with the accessibility team and report the feedback,

@talldan

This comment has been minimized.

Contributor

talldan commented Nov 8, 2018

Great, thanks for checking @afercia. Let's merge this.

@talldan talldan added this to the 4.3 milestone Nov 8, 2018

@talldan talldan merged commit 53209f4 into master Nov 8, 2018

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

@talldan talldan deleted the fix/11431-a11y-menu-checkbox branch Nov 8, 2018

grey-rsi pushed a commit to OnTheGoSystems/gutenberg that referenced this pull request Nov 22, 2018

Change aria-checked to only be used on matching ARIA roles for MenuIt…
…em. (WordPress#11459)

* Fixes WordPress#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