Accessibility: Enhance The FormTokenField component's accesibility #1141

youknowriad · 2017-06-12T13:17:56Z

This PR tries to close #1107

My VoiceOver skills are not that high. So I'd appreciate some eyes to see if I got all the accessibility requirements right.

nylen · 2017-06-12T21:59:41Z

bootstrap-test.js

@@ -59,3 +59,7 @@ global.window._wpDateSettings = {
 		string: 'America/New_York',
 	},
 };
+global.wp = global.wp || {};


This is a separate section and should have a newline + comment before it.

nylen · 2017-06-12T22:02:50Z

components/form-token-field/index.js

@@ -314,6 +316,7 @@ class FormTokenField extends Component {

 	addNewToken( token ) {
 		this.addNewTokens( [ token ] );
+		this.speak( this.props.messages.added );


~~Why abstract out this.props.messages instead of just including the message here? Item added is generic enough to cover any situation.~~

Edit: never mind, I see that for tags, this is replaced with Tag added below.

Yeah, but this is still a good question to ask. Maybe we don't need them.

nylen · 2017-06-12T22:07:55Z

Is there functionality introduced here that should be tested as part of the TokenField tests?

RUN_SLOW_TESTS=y npm run test-unit -- --grep 'FormTokenField'

While these are not currently run on every build, I plan to introduce a task to automatically run these and other slow tests every hour or so.

youknowriad · 2017-06-13T09:50:52Z

Is there functionality introduced here that should be tested as part of the TokenField tests?

Nothing fancy here, just adding some aria attributes essentially, but let's wait for the accessibility review and test the markup once it's validated

afercia · 2017-06-14T20:34:45Z

Thanks very much for this PR :) I'm sorry I don't have so much time to test in depth, will try to do after WCEU. Couple things so far: testing with Safari+VoiceOver the highlighted option in the list gets announced and this is a great step forward! ❤️ There is a small issue with the bolded letters though. For example, typing "c" I get a list of suggested tags that contain "c" and the option markup is something like this:

<span><strong class="components-form-token-field__suggestion-match">c</strong>ategories</span>

Unfortunately, VoiceOver reads out this as two words: cee ategories. Not sure how to solve it other than removing the bolding. Of course it would ned to be tested with other browsers/screen readers combos too.

Re: wp.speak, if you don't want to use the core one (based on jQuery), at Yoast we've ported it to a JS module thanks to @omarreiss. See https://github.com/Yoast/a11y-speak/

youknowriad · 2017-06-15T09:22:18Z

@afercia Thanks for the review.

Unfortunately, VoiceOver reads out this as two words: cee ategories

I was able to fix this using aria-label. Is this the correct way to go?

Re: wp.speak, if you don't want to use the core one

I think Gutenberg should rely on the Core implementation. That said, the Core implementation should probably be updated to use Yoast package.

youknowriad · 2017-06-19T14:09:45Z

Can I have some 👀 here?

aduth · 2017-06-19T15:20:28Z

components/form-token-field/style.scss

@@ -189,3 +189,15 @@ input[type="text"].components-form-token-field__input {
 .components-form-token-field__suggestion-match {
 	color: $dark-gray-800;
 }
+
+.components-form-token__howto {


Should we apply the built-in .screen-reader-text class to avoid repeating it here?

aduth · 2017-06-19T15:21:21Z

components/form-token-field/index.js

 };

-export default FormTokenField;
+FormTokenField.instances = 0;


Why do we need to assign this property? Shouldn't this be handled by withInstanceId?

It should it wasn't available when I opened the PR :)

aduth · 2017-06-19T15:33:50Z

components/form-token-field/suggestions-list.js

-			<ul ref={ this.bindList } className={ classes } tabIndex="-1">
+			<ul
+				ref={ this.bindList }
+				className={ classes }


This element renders with an is-expanded modifier class. Should it also apply aria-expanded? Or is this the role of the input? I'm not really sure.

aduth · 2017-06-19T15:38:58Z

components/form-token-field/index.js

@@ -469,6 +481,9 @@ class FormTokenField extends Component {

 		return (
 			<div { ...tokenFieldProps } >
+				<label htmlFor={ `components-form-token-input-${ instanceId }` } className="components-form-token__howto">


Should we be rendering this if there's an empty placeholder ? Maybe want a more useful default placeholder ?

afercia · 2017-06-19T17:41:08Z

Looking a bit more in depth, there are a few issues to address.

I was able to fix this using aria-label. Is this the correct way to go?

Not 100% correct but I think it could be acceptable. However, chatting with @mtias at WCEU we were wondering if the filtering/search should work a bit differently. Currently, it searches also for tags that contain the search term. Not sure this is so useful. Say I'm typing de and in the suggestions I get also video. That's probably not what I want. Maybe it should search just for tags that start with the search term. If so, maybe the whole "bolding" wouldn't be so useful and could be removed.

I see an issue with the initial value of aria-activedescendant: when the input field is still empty, it gets a value even if no option is highlighted:

also, notice the aria-activedescendant value has a double hyphen.

Testing also on Windows with NVDA, the aria-activedescendant value seems wrong also in other cases, just playing with the up/down arrows to highlight the option, sometimes NVDA announces just "not selected". I guess because the aria-activedescendant value is wrong:

Please notice also it keeps saying "nn of 10" even if the number of options is less than 10. Not sure why this happens, I'd suggest to try to fix the aria-activedescendant value first and then test again.

One more thing noticed so far: I see there's an issue with the value of the aria-describedby that points to the visually hidden description:
aria-describedby="components-form-token-suggestions-howto-undefined"
notice the last part -undefined.

Also, the hidden description:
<div id="components-form-token-suggestions-howto-0" class="components-form-token__howto">Separate with commas</div>

could use the screen-reader-text CSS class instead of its own implementation. The custom one used at the moment is a bit outdated (still uses clip: rect(0 0 0 0); while the core one will be updated soon.

Lastly, I'd consider to remove the CSS class is-visible and use aria-selected for styling, using a CSS attribute selector. Definitely up to you, but maybe at this point is-visible is redundant and makes sense just if you want to keep things a bit more explicit.

afercia

Some issues to address, see comments. Once solved, it would need a second round of testing.

Fixed some of the issues above, I'm testing with VoiceOver and I'm not seeing any error with the count or with the value of aria-activedescendan. Let me know if it's better now.

afercia · 2017-06-21T12:38:49Z

Worth noting Safari+VoiceOver don't announce the count, while Firefox+NVDA do. I've tested a bit more on Windows and can still reproduce the count issue with Firefox+NVDA. Will try to understand what's going on here, my guess is it's something about timings. For example: when aria-activedescendant gets updated? Before or after the list gets re-rendered? Or other, similar timing issues. Couple of screenshots:

Note in the second screenshot: in the initial list, which is limited to 10 items, "audio" is in the 5th position. After typing some letters, the list is filtered to 2 results but "audio" is still announced as "5 of 10".

There are a few more things that would be nice to add, for feature parity with the current implementation, for example:

consider to show the suggestions list only after the second character gets entered in the field
announce the number of results together with short instructions (this is something that jQuery UI autocomplete does for free in the current implementation), see screenshot:

Please let me know if it's better I split these other things in a separate issue (I guess yes?)

youknowriad · 2017-06-21T14:06:04Z

consider to show the suggestions list only after the second character gets entered in the field

done

announce the number of results together with short instructions (this is something that jQuery UI autocomplete does for free in the current implementation), see screenshot:

done

Maybe we should investigate any other issue separately.

afercia · 2017-06-21T14:18:35Z

Aside: not sure this is really needed:

	handleMouseDown( e ) {
		// By preventing default here, we will not lose focus of <input> when clicking a suggestion
		e.preventDefault();
	}

as far as I see, the input always gets focus when FormTokenField updates.

afercia · 2017-06-21T14:45:24Z

Re: last commit, not sure it will work correctly across all browsers/screen reader combos. Couple of things:

minor: entering two spaces displays the list of 10 initial suggestions, I guess that's not what we want
aria-live regions need to be present in the DOM as soon as possible, ideally on DOM ready. Screen readers need to know what element they have to "monitor" for changes. Instead, injecting them doesn't work well for all screen readers. For example, while Safari+VoiceOver announce the messages "No Results" or "n results found.. etc.), Firefox+NVDA don't announce anything.
This came up during the first implementation of wp.speak and that's the reason why WP created the live regions on DOM ready. I'm afraid this should be reworked and use wp.speak.

Will split the other pending issues in a separate GH issue!

youknowriad · 2017-06-21T15:37:22Z

This came up during the first implementation of wp.speak and that's the reason why WP created the live regions on DOM ready. I'm afraid this should be reworked and use wp.speak.

I can still display it at initial render and update its content. That's how jQueryUI autocomplete works

afercia · 2017-06-21T16:25:13Z

@youknowriad thanks!. However, the core speak contains also fixes for specific edge cases, see for example https://core.trac.wordpress.org/changeset/40479
I'd consider to use the core implementation instead of reinventing the wheel, and have what we need in a centralized location. I think we're also going to open a Trac issue to propose to port the current jQuery-based implementation to a JS module.

…how an accessibility hint

youknowriad · 2017-06-21T16:45:27Z

updated to use speak should be ok now

afercia · 2017-06-21T17:15:10Z

Tested again on Windows, Firefox+NVDA and the live messages work nicely. Thanks @youknowriad

Ideally, the call to wp.a11y.speak should be debounced while users are still typing. This is important to avoid "bombarding" users with messages while they're still typing. What we've done at Yoast in similar cases is taking advantage, depending on the cases, of componentWillReceiveProps (if the search term is a prop, this is very handy) or componentDidUpdate to call a debounced function that then calls speak. This way we've been able (sort of) to detect when users stopped typing. Just wrapping speak in a lodash _debounce() or even using simply setTimeout (which is what lodash _debounce() internally does) could work. We've used a delay of 1000ms, but I guess this can be adjusted and maybe there are also better implementations.
Please let me know if you prefer I split this in a separate issue, I understand this PR is getting a bit too big! Thanks for your patience and the nice job.

youknowriad · 2017-06-22T10:34:06Z

Thanks for your patience and the nice job.

Sure thing, happy to make this component more accessible :)

Anyway, I've throttled the speak messages, Will probably merge this soon.

aduth · 2018-03-07T23:25:49Z

components/form-token-field/index.js

@@ -503,6 +558,11 @@ FormTokenField.defaultProps = {
 	isBorderless: false,
 	disabled: false,
 	tokenizeOnSpace: false,
+	messages: {
+		added: __( 'Item added.' ),


Given that these are assigned at load-time, I anticipate the strings are not correctly localized because setLocaleData for the editor is called as an inline script before wp-edit-post (but after wp-components).

youknowriad added the [Focus] Accessibility (a11y) Changes that impact accessibility and need corresponding review (e.g. markup changes). label Jun 12, 2017

youknowriad self-assigned this Jun 12, 2017

youknowriad requested a review from afercia June 12, 2017 13:17

youknowriad force-pushed the update/token-field-accessibility branch from f4fb9e7 to bc3c80e Compare June 12, 2017 15:34

nylen reviewed Jun 12, 2017

View reviewed changes

aduth reviewed Jun 19, 2017

View reviewed changes

afercia previously requested changes Jun 19, 2017

View reviewed changes

afercia mentioned this pull request Jun 21, 2017

Tags suggestions are always the first 10 tags #1338

Closed

afercia mentioned this pull request Jun 21, 2017

Tags "tokens" accessibility evaluation #1339

Closed

youknowriad added 7 commits June 21, 2017 17:37

Accessibility: Enhance The FormTokenField component's accesibility

8fbe462

Accessibility: Add aria-label for matched suggestion

39e34a7

Accessibility: Use screen-reader-text class for accessibility messages

8cb2c2f

Accessibility: Fix empty aria-activedescendant value

8dbd08e

Accessibility: Fix FormTokenField input describtion id

402820c

Accessibility: Only show the suggestions when we type two chars and s…

8682cb6

…how an accessibility hint

Accessibility: Always show the aria-live region

6b7cd44

Accessibility: Using speak instead of aria-live region

38b6edf

youknowriad force-pushed the update/token-field-accessibility branch from a83c0c6 to 38b6edf Compare June 21, 2017 16:45

Accessiblity: Throttle search results messages in FormTokenField

d57e486

youknowriad merged commit 8253709 into master Jun 26, 2017

youknowriad deleted the update/token-field-accessibility branch June 26, 2017 09:04

afercia mentioned this pull request Jun 27, 2017

Improve the Tags suggestions delayed audible messages (speak) #1510

Closed

afercia mentioned this pull request Jul 14, 2017

Reactify SnippetPreview editor Yoast/yoast-components#155

Closed

afercia mentioned this pull request Jul 29, 2017

The link modal posts list suggestion is not accessible #2083

Closed

aduth reviewed Mar 7, 2018

View reviewed changes

aduth mentioned this pull request Apr 3, 2018

i18n: Correctly translate FormTokenField placeholder #5937

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Accessibility: Enhance The FormTokenField component's accesibility #1141

Accessibility: Enhance The FormTokenField component's accesibility #1141

youknowriad commented Jun 12, 2017

nylen Jun 12, 2017

nylen Jun 12, 2017 •

edited

Loading

youknowriad Jun 13, 2017

nylen commented Jun 12, 2017

youknowriad commented Jun 13, 2017

afercia commented Jun 14, 2017

youknowriad commented Jun 15, 2017

youknowriad commented Jun 19, 2017

aduth Jun 19, 2017

aduth Jun 19, 2017

youknowriad Jun 19, 2017

aduth Jun 19, 2017

aduth Jun 19, 2017

afercia commented Jun 19, 2017

afercia left a comment

afercia commented Jun 21, 2017

youknowriad commented Jun 21, 2017

afercia commented Jun 21, 2017

afercia commented Jun 21, 2017

youknowriad commented Jun 21, 2017

afercia commented Jun 21, 2017

youknowriad commented Jun 21, 2017

afercia commented Jun 21, 2017

youknowriad commented Jun 22, 2017 •

edited

Loading

aduth Mar 7, 2018

Accessibility: Enhance The FormTokenField component's accesibility #1141

Accessibility: Enhance The FormTokenField component's accesibility #1141

Conversation

youknowriad commented Jun 12, 2017

Choose a reason for hiding this comment

nylen Jun 12, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nylen commented Jun 12, 2017

youknowriad commented Jun 13, 2017

afercia commented Jun 14, 2017

youknowriad commented Jun 15, 2017

youknowriad commented Jun 19, 2017

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

afercia commented Jun 19, 2017

afercia left a comment

Choose a reason for hiding this comment

afercia commented Jun 21, 2017

youknowriad commented Jun 21, 2017

afercia commented Jun 21, 2017

afercia commented Jun 21, 2017

youknowriad commented Jun 21, 2017

afercia commented Jun 21, 2017

youknowriad commented Jun 21, 2017

afercia commented Jun 21, 2017

youknowriad commented Jun 22, 2017 • edited Loading

Choose a reason for hiding this comment

nylen Jun 12, 2017 •

edited

Loading

youknowriad commented Jun 22, 2017 •

edited

Loading