Fix issue with top bar menu having unique ARIA roles #13059

Firestorm980 · 2018-12-21T14:40:46Z

Description

Closes #12505. Adds role options to menus so that the items within the main more menu can now be groups within the menu. Should fix issues with keyboard navigation as well as roles. The menu options/props default to what they used to be, so if it isn't specified and these menus are used elsewhere, it should fallback to the old functionality.

How has this been tested?

Manual keyboard navigation testing in the admin with VoiceOver.
Ran test-e2e within Gutenberg docker.

Types of changes

Updates menu group component to have a default role, label, and keyboard event control
Changes menus used in the top nav to use group instead of menu and wraps within a parent menu
Changes menus used in the top nav to let the parent menu handle overall keyboard event navigation

Checklist:

My code is tested.
My code follows the WordPress code style.
My code follows the accessibility standards.
My code has proper inline documentation.
I've included developer documentation if appropriate.

WP Updates

packages/components/src/menu-group/index.js

jorgefilipecosta

Hi @Firestorm980 thank you for your contribution. I don't notice any visual changes and it looks like things are working correctly.
Previously on the ellipsis menu, it was not possible to use the arrow down to navigate between all the menu items and now it is (which is an improvament).
cc: @afercia, @tofumatt would it be possible to have a look at this PR regarding the a11y point of view?

jorgefilipecosta · 2018-12-21T17:36:19Z

packages/components/src/menu-group/index.js

@@ -33,9 +37,14 @@ export function MenuGroup( {
 	return (
 		<div className={ classNames }>
 			{ label &&
-				<div className="components-menu-group__label" id={ labelId }>{ label }</div>
+				<div
+					className={ `components-menu-group__label ${ ( isScreenReaderLabel ) ? 'screen-reader-text' : '' }` }


Here if isScreenReaderLabel is false we add an unnecessary space. What if we use the classnames util e.g:

className={ classnames( 'components-menu-group__label', { ['screen-reader-text']: isScreenReaderLabel } }

afercia · 2018-12-27T08:25:57Z

packages/components/src/menu-group/index.js

 			}
-			<NavigableMenu orientation="vertical" aria-labelledby={ labelId }>
+			<NavigableMenu orientation="vertical" aria-labelledby={ labelId } role={ role } useEventToOffset={ useEventToOffset }>


Seems to me the wrapper element (the one with role=menu) has now an aria-labelledby attribute that points to a non existing ID (while the groups are OK). It makes sense to use the visible titles for the groups but also the menu should be labelled.

In the W3C example, the menu uses an aria-label="Text Formatting". In the same way, this menu could use an aria-label when the role === menu.

afercia · 2018-12-27T08:29:09Z

@Firestorm980 thanks so much for working on this 🙂 I've left one minor comment (the whole menu should be labelled). Keyboard interaction is greatly improved 👍

gziolo · 2019-01-31T14:56:11Z

@Firestorm980 - do you plan to address feedback left? It looks like it is very close to be ready to ship. We would like to include it in the plugin's release planned for the middle of February.

Edit: It now has some conflicts with master branch that must be resolved.

Firestorm980 · 2019-01-31T15:15:44Z

@gziolo I should be able to get to this one in the next couple days.

WP Updates

This reverts commit c52db84.

Firestorm980 · 2019-02-03T03:10:08Z

@gziolo @afercia Fixes should be included now.

afercia · 2019-02-16T19:16:25Z

packages/components/src/menu-group/index.js

@@ -19,6 +19,10 @@ export function MenuGroup( {
 	className = '',
 	instanceId,
 	label,
+	role = 'menu',
+	labelRole = ( role === 'group' ) ? { role: 'presentation' } : {},


I'd remove this, as role=presentation is useful to "flatten" the accessibility tree but there's no need for it, as the div it applies to is already ignored.

afercia · 2019-03-19T17:06:49Z

@Firestorm980 hello. Do you have time to address the pending issues? If not, please let us know. These changes would greatly improve keyboard interaction in the more menu. If you don't have time, we can always copy your branch to the main repo preserving commits and attributions, and proceed from there. Thanks!

gziolo · 2019-03-20T10:45:22Z

@afercia in case we don't hear back from @Firestorm980 soon, I opened #14525 whit all commits transferred. In addition, I rebased new PR with all the changes applied in master branch. Feel free to work on new PR.

chrisvanpatten · 2019-03-22T18:51:00Z

Removed the "stale" label as it seems it no longer is :)

Firestorm980 · 2019-03-22T20:15:05Z

Sorry about the delay in communication on this one. Hopefully will have another commit up soon with fixes.

gziolo

See my comments. In general, this PR goes in the right direction but it would be nice to explore whether we should introduce new common component Menu which represents the modified version of MenuGroup. While those two are similar, they are conceptually different.

gziolo · 2019-03-27T12:07:53Z

packages/components/src/navigable-container/container.js

@@ -68,6 +68,11 @@ class NavigableContainer extends Component {
 		const { getFocusableContext } = this;
 		const { cycle = true, eventToOffset, onNavigate = noop, stopNavigationEvents } = this.props;


Suggested change

const { cycle = true, eventToOffset, onNavigate = noop, stopNavigationEvents } = this.props;

const { cycle = true, eventToOffset = noop, onNavigate = noop, stopNavigationEvents } = this.props;

This should be enough to make this work instead of the check:

// This avoids a console error when eventToOffset is null

gziolo · 2019-03-27T12:10:36Z

packages/edit-post/src/components/header/more-menu/index.js

-				<PluginMoreMenuGroup.Slot fillProps={ { onClose } } />
-				<ToolsMoreMenuGroup.Slot fillProps={ { onClose } } />
-				<MenuGroup>
+				<MenuGroup label={ menuLabel } isLabelHidden={ true }>


Fragment is no longer necessary to wrap components.

gziolo · 2019-03-27T12:18:47Z

packages/components/src/menu-group/index.js

@@ -19,6 +19,9 @@ export function MenuGroup( {
 	className = '',
 	instanceId,
 	label,
+	role = 'menu',


It feels wrong to see the default role set menu for the component which is called MenuGroup and then see it overridden back to group in other places. It seems like we need to introduce another component called Menu which will have role set to menu, its class name which will hide the label for the menu, and will always have useEventToOffset set to true. This will make it possible to leave MenuGroup usage unchanged.

@gziolo should we spin up a new issue for that?

I would feel comfortable doing it as part of this PR because in the current form you might see unexpected role="menu" in all plugins using MenuGroup.

aduth · 2019-04-08T13:31:35Z

In general, this PR goes in the right direction but it would be nice to explore whether we should introduce new common component Menu which represents the modified version of MenuGroup. While those two are similar, they are conceptually different.

This will make it possible to leave MenuGroup usage unchanged.

Arriving here via #14843 and considering the recommendations from #12505, I wonder what it is that MenuGroup is supposed to represent that we're trying to leave untouched. From its current documentation, it's not clear to me why it implements behaviors (navigating by arrow keys) and not instead rely on that to be handled by the context in which it's rendered. Indeed, this seems to be the recommendation in #12505:

Basically, the MenuGroup component should be just a group of menu items and not render a NavigableMenu. Then, the groups should be wrapped by an ARIA menu, i.e. a single NavigableMenu.

The challenge is in how it's used today, the "More Options" menu doesn't implement any of these behaviors on the wrapping dropdown. It seems like something we'd expect out of the DropdownMenu component, but as it exists today, it's not possible to actually use groupings there. #14843 is a step in the direction of making this possible, though.

I would expect:

The "More Options" menu implements using NavigableMenu, probably where today it renders just a Fragment
Each of the sections ("Writing", etc) render as their own MenuGroup.
A MenuGroup is just a div with grouping attributes as prescribed in The top bar More Menu should be a unique ARIA menu #12505 (i.e. no NavigableMenu).
After UI Components: Refactor BlockSettingsMenu and MoreMenu as DropdownMenu #14843, "More Options" component could be refactored to use DropdownMenu instead of Dropdown so that it doesn't need to handle NavigableMenu behaviors.

tl;dr We seem to be trying to let MenuGroup to have some behaviors for a standalone context, and I'm not sure why?

Firestorm980 · 2019-04-08T14:26:12Z

@gziolo @aduth just a heads up that my schedule is getting pretty full so I don't have a lot of spare time to tackle this one, especially now that the scope of the ticket appears to be changing somewhat.

gziolo · 2019-04-10T12:09:12Z

@gziolo @aduth just a heads up that my schedule is getting pretty full so I don't have a lot of spare time to tackle this one, especially now that the scope of the ticket appears to be changing somewhat.

Thank you for your contribution so far, it helped us better understand what would be the best approach here. I'm personally leaning towards including the changes proposed here in #14843. It means there is going to be more aggressive refactoring required to make it work as @afercia described in #12505. However, it seems like we would be able to address all concerns shared in the original issue. In addition, this will give us more control over DropdownMenu and will improve the default usage of this component in the context of its accessibility.

Firestorm980 and others added 10 commits November 9, 2018 14:30

Merge pull request #1 from WordPress/master

001a3f3

WP Updates

Merge pull request #2 from WordPress/master

a42e5d6

WP Updates

Merge pull request #3 from WordPress/master

75fc100

WP Updates

Merge pull request #4 from WordPress/master

7811b8f

WP Updates

Merge pull request #5 from WordPress/master

7f9ea08

WP Updates

Updates roles for appropriate a11y in the header more menu

874ab92

linting updates for header menu components

92e4ef4

Updated snapshots for menu items

ac8eb39

Merge pull request #6 from WordPress/master

7bf8a34

WP Updates

Merge branch 'master' into fix/issue-12505

c58588b

Firestorm980 mentioned this pull request Dec 21, 2018

The top bar More Menu should be a unique ARIA menu #12505

Closed

jorgefilipecosta reviewed Dec 21, 2018

View reviewed changes

packages/components/src/menu-group/index.js Show resolved Hide resolved

jorgefilipecosta reviewed Dec 21, 2018

View reviewed changes

afercia reviewed Dec 27, 2018

View reviewed changes

gziolo added the [Focus] Accessibility (a11y) Changes that impact accessibility and need corresponding review (e.g. markup changes). label Jan 31, 2019

gziolo added this to the 5.1 (Gutenberg) milestone Jan 31, 2019

Firestorm980 and others added 8 commits February 1, 2019 13:36

Merge pull request #7 from WordPress/master

13120e6

WP Updates

Merge branch 'master' into fix/issue-12505

2517c36

Fix extra spacing in class attribute and eslint errors

a47ea27

Add a label for the tools and options menu

d99dbb7

Add a check for eventToOffset to prevent errors

ae906f4

Updated test snapshot

c52db84

Revert "Updated test snapshot"

be2b57c

This reverts commit c52db84.

Add proper conditional for eventToOffset

0ea3031

gziolo self-requested a review February 7, 2019 13:27

afercia reviewed Feb 16, 2019

View reviewed changes

gziolo added this to the 5.3 (Gutenberg) milestone Mar 4, 2019

youknowriad removed this from the 5.3 (Gutenberg) milestone Mar 18, 2019

gziolo mentioned this pull request Mar 20, 2019

Fix issue with top bar menu having unique ARIA roles (2) #14525

Closed

5 tasks

gziolo added the [Status] Stale Gives the original author opportunity to update before closing. Can be reopened as needed. label Mar 20, 2019

Firestorm980 added 2 commits March 22, 2019 13:48

Merge remote-tracking branch 'upstream/master'

a123632

Merge branch 'master' into fix/issue-12505

8eabcbf

Firestorm980 requested review from aduth, ajitbohra, chrisvanpatten, noisysocks, talldan and youknowriad as code owners March 22, 2019 18:49

chrisvanpatten removed the [Status] Stale Gives the original author opportunity to update before closing. Can be reopened as needed. label Mar 22, 2019

fix roles as props, update options to be menu item, fix a11y tree

264510e

update tests

b23da67

gziolo requested changes Mar 27, 2019

View reviewed changes

gziolo added the General Interface Parts of the UI which don't fall neatly under other labels. label Mar 27, 2019

gziolo assigned aduth and Firestorm980 and unassigned aduth Mar 27, 2019

gziolo closed this Apr 10, 2019

aduth mentioned this pull request Apr 10, 2019

UI Components: Refactor BlockSettingsMenu and MoreMenu as DropdownMenu #14843

Merged

afercia mentioned this pull request May 1, 2019

Single visible menu is made up of multiple menus #15318

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix issue with top bar menu having unique ARIA roles #13059

Fix issue with top bar menu having unique ARIA roles #13059

Firestorm980 commented Dec 21, 2018 •

edited by gziolo

Loading

jorgefilipecosta left a comment

jorgefilipecosta Dec 21, 2018

afercia Dec 27, 2018

afercia commented Dec 27, 2018

gziolo commented Jan 31, 2019 •

edited

Loading

Firestorm980 commented Jan 31, 2019

Firestorm980 commented Feb 3, 2019 •

edited

Loading

afercia Feb 16, 2019

afercia commented Mar 19, 2019

gziolo commented Mar 20, 2019

chrisvanpatten commented Mar 22, 2019

Firestorm980 commented Mar 22, 2019

gziolo left a comment

gziolo Mar 27, 2019

gziolo Mar 27, 2019

gziolo Mar 27, 2019

timwright12 Apr 1, 2019

gziolo Apr 2, 2019

aduth commented Apr 8, 2019

Firestorm980 commented Apr 8, 2019

gziolo commented Apr 10, 2019

		@@ -68,6 +68,11 @@ class NavigableContainer extends Component {
		const { getFocusableContext } = this;
		const { cycle = true, eventToOffset, onNavigate = noop, stopNavigationEvents } = this.props;

	const { cycle = true, eventToOffset, onNavigate = noop, stopNavigationEvents } = this.props;
	const { cycle = true, eventToOffset = noop, onNavigate = noop, stopNavigationEvents } = this.props;

Fix issue with top bar menu having unique ARIA roles #13059

Fix issue with top bar menu having unique ARIA roles #13059

Conversation

Firestorm980 commented Dec 21, 2018 • edited by gziolo Loading

Description

How has this been tested?

Types of changes

Checklist:

jorgefilipecosta left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

afercia commented Dec 27, 2018

gziolo commented Jan 31, 2019 • edited Loading

Firestorm980 commented Jan 31, 2019

Firestorm980 commented Feb 3, 2019 • edited Loading

Choose a reason for hiding this comment

afercia commented Mar 19, 2019

gziolo commented Mar 20, 2019

chrisvanpatten commented Mar 22, 2019

Firestorm980 commented Mar 22, 2019

gziolo left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aduth commented Apr 8, 2019

Firestorm980 commented Apr 8, 2019

gziolo commented Apr 10, 2019

Firestorm980 commented Dec 21, 2018 •

edited by gziolo

Loading

gziolo commented Jan 31, 2019 •

edited

Loading

Firestorm980 commented Feb 3, 2019 •

edited

Loading