Rework advanced filters screen reader text generation by jeffstieler · Pull Request #1099 · woocommerce/woocommerce-admin

jeffstieler · 2018-12-15T01:59:40Z

Based on conversation here: #1089 (comment)

This PR seeks to:

Only describe full filter if all values are set
Describe each input more precisely

Notes:

There is an opportunity to DRY up the markup in the render of NumberFilter, SearchFilter, and SelectFilter, but I've left that undone as it might be a premature optimization. I can certainly turn that into another component if we decide that's cleanup worth doing.

Also, this PR adds a utility method called textContent(). It's essentially react-addons-text-content - I made the decision to just copy pasta the code into our project since the dependency is rather small and hasn't been updated in 3 years.

To test:

Navigate to devdocs, filters component
Add a filter of each type (order status, products, item quantity)
Verify that the <legend> for each doesn't change when interacting the the component
Verify that a <div> with class screen-reader-text is rendered only when all of the filter inputs have values

psealock

Looking good @jeffstieler !

I noted a couple small things and a comment on unread screenReaderText.

psealock · 2018-12-18T20:07:23Z

packages/components/src/filters/advanced/test/utils.js

+import { textContent } from '../utils';
+
+describe( 'textContent()', () => {
+	test( 'should be got text `Hello World`', () => {


should be got text => should get text

Heh, copy pasta'd from https://github.com/rwu823/react-addons-text-content/blob/master/test/react-addons-text-content.spec.js#L5.

I'll fix it.

Fixed in 9ce4d9e.

psealock · 2018-12-18T20:18:57Z

packages/components/src/filters/advanced/number-filter.js

 					type="number"
 					value={ value }
+					aria-label={ label }
 					onChange={ onChange }


Looks like you can add a prop min="0" to prevent negative numbers.

Let's handle this in a separate PR - the NumberFilter component doesn't have any sort of input validation yet.

psealock · 2018-12-18T20:33:08Z

packages/components/src/filters/advanced/number-filter.js

+					<span className="screen-reader-text">
+						{ screenReaderText }
+					</span>
+				) }


Using Chrome and VoiceOver, this screenReaderText never gets read. I see it appropriately added to the DOM when the sentence is complete, however.

Sorry if this has been previously discussed, but would it not make sense to add screenReaderText to the legend?

Is it not read even when you move to it?

It's not meant to be automatically read, as that interrupts the content entry - having it in legend is too noisy, and why @jeffstieler is doing all this work 😁

It can't be tabbed to, but if you use the VO controls you'll navigate to it after the last filter input, before the remove button.

but if you use the VO controls you'll navigate to it after the last filter input

via tabbing? Or is there another method?

Ok, I figured out how to navigate to the text using VO controls. I'm not a VO power user (yet), so forgive my novice comment, but I'm not experiencing the input being too noisy on master -- the legend only gets read when the fieldset has focus.

I assumed VO users would tab their way through a form, instead of using the arrow controls to read text in a span, and by doing so skip right over the text being added in this PR. Is this assumption wrong?

I assumed VO users would tab their way through a form, instead of using the arrow controls to read text in a span, and by doing so skip right over the text being added in this PR. Is this assumption wrong?

I'm not knowledgeable enough to say - I'm new to a11y 🙂. I defer to @ryelle.

Is this assumption wrong?

It's over-simplifying, not exactly wrong – screen reader users will tab to move through interactive components (forms, buttons, etc), but they'll use the arrow nav to move through the page's full contents (otherwise they'd never read non-interactive page content). They might also use other navigation methods (skipping through headings, or a list of links, or a list of buttons, etc).

That said, I don't know what a real user would do in this scenario - but this approach is better than interrupting the user while they're typing.

Cool, thank you for the clarification.

ryelle

Tested with VoiceOver + Safari, and it's working as expected. I'm not exactly sure why we need textContent, though, can you explain that a little more?

jeffstieler · 2018-12-18T22:50:31Z

I'm not exactly sure why we need textContent, though, can you explain that a little more?

I initially introduced that so we'd be passing a string to wp.a11y.speak(). Let me double check to see if that's still needed..

jeffstieler · 2018-12-18T23:02:14Z

I'm not exactly sure why we need textContent, though, can you explain that a little more?

I initially introduced that so we'd be passing a string to wp.a11y.speak(). Let me double check to see if that's still needed..

In testing, removing textContent() results in the following markup:

<span class="screen-reader-text">Item Quantity is <span>Less Than</span> <span>1</span></span>

Which VoiceOver requires several interactions to completely read - 3 in total for this example. Using textContent() to extract only the text content results in VO reading the full sentence ("Item Quantity is Less Than 1") once the component is navigated to.

Edit: even using <Fragment /> instead of <span /> for the interpolated components results in the same behavior.

Only describe full filter if all values are set. Describe each input more precisely. Based on conversation here: #1089 (comment)

…bel from accessibility tree.

…are populated.

…criptive span for screen reader instead of using speak().

…er components.

jeffstieler · 2019-01-03T20:17:57Z

Bumping for a resolution to the textContent() question (#). cc: @ryelle

ryelle · 2019-01-03T20:24:59Z

@jeffstieler That confused me a little because spans are not supposed to create breaks like that. This seems to be a Safari vs Chrome difference - Safari + VO doesn't break the nested spans apart, but Chrome + VO does. If you switch the wrapper (the element with screen-reader-text) to a div, it reads correctly in both cases.

<div class="screen-reader-text">Item Quantity is <span>Less Than</span> <span>1</span></div>

jeffstieler · 2019-01-04T00:16:51Z

@ryelle it seems that VO (even in Safari) doesn't cope well with line breaks in the HTML. In my testing (on this PR) using an outer <div> with inner <span>s still takes multiple key presses to read the full phrase of "Item Quantity is Less Than 1".

Here's a screen capture: https://cloudup.com/c_Ne8_uyoj3 (you can see the selected VO element, the VO overlay screen, and the markup of the screen-reader-text div all in frame)

I'll look in/around the interpolateComponents() to see if there's a culprit for the line breaks.

ryelle · 2019-01-04T00:29:09Z

Hm, even with linebreaks in a plain HTML page, VoiceOver reads it as one line for me

<div class="screen-reader-text">
	Item Quantity is 
	<span>Less Than</span> 
	<span>1</span>
</div>

can you push up your latest changes so I can test it out myself?

jeffstieler · 2019-01-04T00:34:01Z

Perhaps then I'm not navigating correctly? I'm using Control + Option + Right arrow to move between readable elements.

Pushed up my changes - 2532539.

ryelle · 2019-01-04T16:08:02Z

I figured it out – the wp core CSS for .screen-reader-text also puts styles on .screen-reader-text span, and that triggers VO to read the spans as individual items 🙃 In my testing, I was just creating plain HTML pages with no CSS, to test the markup… not thinking that the screen-reader-text class did that.

Edit: You should try out using the class ui-helper-hidden-accessible instead of screen-reader-text – it should work but I think something is still interacting with it, but I don't know if that's just a local bug. You might have to bring back textContent after all.

jeffstieler · 2019-01-04T20:10:37Z

@ryelle (Following up on a Slack convo we had)

Changing to the ui-helper-hidden-accessible didn't solve the problem - apparently WP's line-height (#) is causing the problem.

Even while resetting the line-height to either initial or normal seems to fix the Safari VO issue, the text of the spans are all concatenated, ignoring padding whitespace. This results in the screen reader reading Item Quantity is Between$4.00and$6.00 which causes it to read "between dollar sign four point zero zero ..." instead of "between four dollars and zero zero cents ...".

I don't believe any of these issues were present with the textContent() utility function I introduced. I'm going to go back to that logic and test these scenarios. If they are no longer problems, I think we should move forward with that solution.

jeffstieler · 2019-01-04T20:28:28Z

Tested with the (formerly) previous commit - everything is fine there. I've deleted the commit that removed the usage of textContent().

ryelle

This is reading correctly now, no pauses/broken sentences. Thanks for all your work on this!

jeffstieler added the status: in progress label Dec 15, 2018

jeffstieler self-assigned this Dec 15, 2018

jeffstieler requested a review from ryelle December 15, 2018 01:59

probot-autolabeler bot added focus: components Issues for woocommerce components Packages labels Dec 15, 2018

jeffstieler mentioned this pull request Dec 15, 2018

Add Advanced Numerical Value Filter #1089

Merged

jeffstieler force-pushed the fix/advanced-filters-legend-text branch from 9fa1d5f to 579968b Compare December 18, 2018 18:07

jeffstieler added [Status] Needs Review focus: accessibility The issue/PR is related to accessibility. and removed status: in progress labels Dec 18, 2018

jeffstieler requested a review from a team December 18, 2018 18:08

psealock reviewed Dec 18, 2018

View reviewed changes

ryelle reviewed Dec 18, 2018

View reviewed changes

emdashcodes mentioned this pull request Dec 20, 2018

Add remaining filters to the downloads report #1141

Merged

1 task

jeffstieler added 8 commits January 3, 2019 13:13

Rework NumberFilter screen reader text.

472fb96

Only describe full filter if all values are set. Describe each input more precisely. Based on conversation here: #1089 (comment)

NumberFilter: automatically hide non-interpolated strings in title la…

5c7d932

…bel from accessibility tree.

NumberFilter: remove dynamic legend text, just use “add” label.

2d43744

NumberFilter: a11y speak a description of the filter once all inputs …

3307b6d

…are populated.

NumberFilter: don’t hide anything from accessibility tree, render des…

e1e3feb

…criptive span for screen reader instead of using speak().

Render descriptive screen reader text for SearchFilter and SelectFilt…

b1975dd

…er components.

Add tests for textContent().

3ed6be1

Fix type in textContent() test case label.

079ff48

jeffstieler force-pushed the fix/advanced-filters-legend-text branch from 9ce4d9e to 079ff48 Compare January 3, 2019 20:16

jeffstieler force-pushed the fix/advanced-filters-legend-text branch from 2532539 to 079ff48 Compare January 4, 2019 20:27

jeffstieler mentioned this pull request Jan 5, 2019

Advanced Filters: Add DatePicker #1146

Merged

ryelle approved these changes Jan 8, 2019

View reviewed changes

jeffstieler added [Status] Ready to Merge and removed [Status] Needs Review labels Jan 8, 2019

jeffstieler merged commit eaffb0d into master Jan 8, 2019

jeffstieler deleted the fix/advanced-filters-legend-text branch January 8, 2019 15:45

Conversation

jeffstieler commented Dec 15, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

psealock left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ryelle left a comment

Choose a reason for hiding this comment

Uh oh!

jeffstieler commented Dec 18, 2018

Uh oh!

jeffstieler commented Dec 18, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jeffstieler commented Jan 3, 2019

Uh oh!

ryelle commented Jan 3, 2019

Uh oh!

jeffstieler commented Jan 4, 2019

Uh oh!

ryelle commented Jan 4, 2019

Uh oh!

jeffstieler commented Jan 4, 2019

Uh oh!

ryelle commented Jan 4, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jeffstieler commented Jan 4, 2019

Uh oh!

jeffstieler commented Jan 4, 2019

Uh oh!

ryelle left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

jeffstieler commented Dec 15, 2018 •

edited

Loading

jeffstieler commented Dec 18, 2018 •

edited

Loading

ryelle commented Jan 4, 2019 •

edited

Loading