Improve the inserter a11y

[afercia]

Improves the inserter accessibility.

• renames 'Common' to 'Common blocks', to give better context (by the way, I guess this should be made translatable?)
• adds aria-haspopup="true" and aria-expanded to the inserter toggle button
• adds role="menu" and aria-labelledby to the inserter menu, and necessary IDs
• adds tabIndex="0" to the menu to make it focusable, this way it gets announced as "menu" with the name provided by aria-labelledby; this will work with multiple menus too, each one will be announced with the category.title
• adds role="menuitem" to the inserter menu items
• adds a properly associated label to the search field, adding necessary ID
• fixes toggle button position in Safari 10
• adjusts menu max-height (I guess this would need further adjustments)
• improves menu items text alignment in Safari 10
• adds focus style on the menu items, same as hover

Important note:
when #515 will be done, the menu items will need tabIndex="-1", as focus should be managed programmatically for the Tab and Arrows keys

Fixes #525

[afercia]

Giving the inserter menu and items proper ARIA roles, will allow keyboard events to correctly work when using screen readers, see #515.

Making the menu focusable and using aria-labelledby will make screen readers announce it as "menu" and with the name used for the visible menu "title". To avoid the title gets announced twice, the visible one has a aria-hidden="true" attribute. aria-labelledby works even if it targets and element with aria-hidden="true". Screenshots:

Safari 10 + VoiceOver:

How aria-labelledby works: see in the Safari inspector:

or in IE 11 and AViewer:

Notice above the SVG titles are announced as part of the menu items in IE11, will open a separate issue specific to SVG accessibility.

The menu and menuitem roles give also one more bonus: some browsers and screen readers get able to announce the number of items, see Firefox + NVDA:

[aduth]

Can you clarify whether the following would work just as well to associate the label with the input?

<label>
	<span className="screen-reader-text">
		{ wp.i18n.__( 'Search blocks' ) }
	</span>
	<input />
</label>

I only raise in an effort to avoid id, since it's hard to guarantee that a component will only occur once in a page.

Same applies to the above aria-labelledby, but an alternative is less clear to me. In cases where I've required an ID in a React component, it can work to store a static incrementing number that's assigned in mount:

class Foo extends Component {
	constructor() {
		super( ...arguments );
		this.instanceId = this.constructor.instances++;
	}

	render() {
		return <span id={ 'foo-' + this.instanceId } />;
	}
}

Foo.instances = 0;

But, evidenced by this example, it's a bit cumbersome to manage.

[afercia]

There are two ways to associate labels to form controls: implicit and explicit. Implicit: the label wraps the control and doesn't use a for attribute. Explicit: label and control are separate elements (no wrapping) and they use for/id. The latter is the recommended one for accessibility. Never do a mix of the two techniques 🙂

Same applies to the above aria-labelledby

ARIA attributes often need IDs. We'll need lots of IDs 🙂 Happy to implement any best practice, but as you pointed out in some cases it will be a bit hard.

[aduth]

The latter is the recommended one for accessibility. Never do a mix of the two techniques 🙂

For my own curiosity, could you clarify the "why" of both of these two points?

[afercia]

In short, explicit labels are the ones better supported by browsers/screen readers combos. See also: https://make.wordpress.org/core/handbook/best-practices/coding-standards/accessibility-coding-standards/#labeling

A mix of the two techniques is against the spec and in some combos could make the label being announced twice.

[aduth]

• We can drop the & and the nesting would work just as well.
• I had a hard time finding what .insert was in this case; found later that it's the Dashicon. For future readers, maybe being overly specific could clarify, e.g. .dashicon.insert

[aduth]

• Would we want the comment to appear in the generated stylesheet? With /* it will, otherwise SASS's // could prevent it from being included
• What specific to Safari 10 is this addressing? Might help future maintainers decide whether this can be dropped.

[aduth]

Not necessarily in scope of this pull request, but the string ought to be translated.

[afercia]

by the way, I guess this should be made translatable? 🙂

[afercia]

To clarify the .dashicon.insert change, seems Safari 10 doesn't like flex whit SVG. Without margin: 0 auto the icon is not centered:

About the other buttons (the ones with a SVG icon and text), the button uses display: flex but one of the flex-items is actually a text node:

Seems in some browsers this works, but Safari didn't align vertically the text, I had to add some line-height. As far as I know, flex applies to elements, not to text nodes. Not sure this is actually an issue, but maybe something worth considering.

[afercia]

IDs are now generated. See in the screenshot below, with the top and bottom inserters open at the same time (actually, there should be only one instantiated at a time, inserters should close when clicking anywhere outside of them, see #515 (comment)):

[aduth]

This looks great 👍 It seems the translation files have drifted from master a bit, so I'm going to rebase and force push an update shortly before merging.

[afercia]

Yep I was in doubt what to do with the gutenberg.pot, wouldn't be better to ecxlude it from commits?

[aduth]

Yep I was in doubt what to do with the gutenberg.pot, wouldn't be better to ecxlude it from commits?

We probably need a better way to automate this. I'd considered a bot generating and committing in response to a master commits webhook, but there's not a great long-term core reconciliation story to this approach.

[aduth]

In response to noticing that it wasn't just the Inserter button that was misaligned but also Undo / Redo, I started down a rabbit hole of adjusting the icon centering approach by using padding on the underlying IconButton. However this had unintended consequences on the toolbar controls, specifically those with adjacent subscript text (heading levels, quote styles). I'd argue it more surfaced some preexisting issues. One being that there is icon-specific whitespace for some icons and not others (like the heading H icon).

The latest commit works somewhat well; could probably do for a design review @jasmussen . If it's too much of a detour, we could come up with a simpler alternative as a short-term solution, especially since it's not particularly in scope to the intent of the pull request.

[aduth]

Related to .pot discussion: #604

[aduth]

Rebased and plan to merge this as-is. Design concerns around icon button centering can be addressed separately, though for what it's worth, the same issue exists already in master with quote style subscript proximity to the icon itself.

master	update/better-inserter-a11y