Conversation
|
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 Safari 10 + VoiceOver:
How
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
|
editor/components/inserter/menu.js
Outdated
| ) ) | ||
| } | ||
| </div> | ||
| <label htmlFor={ `editor-inserter__search-${ position }` } className="screen-reader-text"> |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
| color: #00aadc; | ||
| } | ||
|
|
||
| & .insert { |
There was a problem hiding this comment.
- We can drop the
&and the nesting would work just as well. - I had a hard time finding what
.insertwas in this case; found later that it's the Dashicon. For future readers, maybe being overly specific could clarify, e.g..dashicon.insert
| } | ||
|
|
||
| & .insert { | ||
| margin: 0 auto; /* Safari 10. */ |
There was a problem hiding this comment.
- 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.
| */ | ||
| const categories = [ | ||
| { slug: 'common', title: 'Common' } | ||
| { slug: 'common', title: 'Common blocks' } |
There was a problem hiding this comment.
Not necessarily in scope of this pull request, but the string ought to be translated.
There was a problem hiding this comment.
by the way, I guess this should be made translatable? 🙂
|
To clarify the
About the other buttons (the ones with a SVG icon and text), the button uses
Seems in some browsers this works, but Safari didn't align vertically the text, I had to add some |
|
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
left a comment
There was a problem hiding this comment.
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.
|
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. |
53a892a to
b00e551
Compare
|
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 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. |
|
Related to |
b00e551 to
01c9825
Compare
|
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.
|
Improves the inserter accessibility.
aria-haspopup="true"andaria-expandedto the inserter toggle buttonrole="menu"andaria-labelledbyto the inserter menu, and necessary IDstabIndex="0"to the menu to make it focusable, this way it gets announced as "menu" with the name provided byaria-labelledby; this will work with multiple menus too, each one will be announced with thecategory.titlerole="menuitem"to the inserter menu itemslabelto the search field, adding necessary IDImportant note:
when #515 will be done, the menu items will need
tabIndex="-1", as focus should be managed programmatically for the Tab and Arrows keysFixes #525