Skip to content
This repository has been archived by the owner on Jul 12, 2024. It is now read-only.

Rework advanced filters screen reader text generation #1099

Merged
merged 8 commits into from
Jan 8, 2019
Merged

Rework advanced filters screen reader text generation #1099

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
472fb96
Rework NumberFilter screen reader text.
jeffstieler Dec 15, 2018
5c7d932
NumberFilter: automatically hide non-interpolated strings in title la…
jeffstieler Dec 18, 2018
2d43744
NumberFilter: remove dynamic legend text, just use “add” label.
jeffstieler Dec 18, 2018
3307b6d
NumberFilter: a11y speak a description of the filter once all inputs …
jeffstieler Dec 18, 2018
e1e3feb
NumberFilter: don’t hide anything from accessibility tree, render des…
jeffstieler Dec 18, 2018
b1975dd
Render descriptive screen reader text for SearchFilter and SelectFilt…
jeffstieler Dec 18, 2018
3ed6be1
Add tests for textContent().
jeffstieler Dec 18, 2018
079ff48
Fix type in textContent() test case label.
jeffstieler Dec 18, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
89 changes: 73 additions & 16 deletions packages/components/src/filters/advanced/number-filter.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,28 +7,41 @@ import { SelectControl, TextControl } from '@wordpress/components';
import { get, find, partial } from 'lodash';
import interpolateComponents from 'interpolate-components';
import classnames from 'classnames';
import { _x } from '@wordpress/i18n';
import { sprintf, __, _x } from '@wordpress/i18n';

/**
* Internal dependencies
*/
import TextControlWithAffixes from '../../text-control-with-affixes';
import { textContent } from './utils';

/**
* WooCommerce dependencies
*/
import { formatCurrency } from '@woocommerce/currency';

class NumberFilter extends Component {
getBetweenString() {
return _x(
'{{rangeStart /}}{{span}}and{{/span}}{{rangeEnd /}}',
'{{rangeStart /}}{{span}} and {{/span}}{{rangeEnd /}}',
'Numerical range inputs arranged on a single line',
'wc-admin'
);
}

getLegend( filter, config ) {
getScreenReaderText( filter, config ) {
const inputType = get( config, [ 'input', 'type' ], 'number' );
const rule = find( config.rules, { value: filter.rule } ) || {};
let [ rangeStart, rangeEnd ] = ( filter.value || '' ).split( ',' );

// Return nothing if we're missing input(s)
if (
! rangeStart ||
( 'between' === rule.value && ! rangeEnd )
) {
return '';
}

if ( 'currency' === inputType ) {
rangeStart = formatCurrency( rangeStart );
rangeEnd = formatCurrency( rangeEnd );
Expand All @@ -47,16 +60,16 @@ class NumberFilter extends Component {
} );
}

return interpolateComponents( {
return textContent( interpolateComponents( {
mixedString: config.labels.title,
components: {
filter: <span>{ filterStr }</span>,
rule: <span>{ rule.label }</span>,
filter: <Fragment>{ filterStr }</Fragment>,
rule: <Fragment>{ rule.label }</Fragment>,
},
} );
} ) );
}

getFormControl( type, value, onChange ) {
getFormControl( { type, value, label, onChange } ) {
if ( 'currency' === type ) {
const currencySymbol = get( wcSettings, [ 'currency', 'symbol' ] );
const symbolPosition = get( wcSettings, [ 'currency', 'position' ] );
Expand All @@ -68,13 +81,15 @@ class NumberFilter extends Component {
className="woocommerce-filters-advanced__input"
type="number"
value={ value || '' }
aria-label={ label }
onChange={ onChange }
/>
: <TextControlWithAffixes
prefix={ <span dangerouslySetInnerHTML={ { __html: currencySymbol } } /> }
className="woocommerce-filters-advanced__input"
type="number"
value={ value || '' }
aria-label={ label }
onChange={ onChange }
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

/>
);
Expand All @@ -85,6 +100,7 @@ class NumberFilter extends Component {
className="woocommerce-filters-advanced__input"
type="number"
value={ value || '' }
aria-label={ label }
onChange={ onChange }
/>
);
Expand All @@ -105,11 +121,24 @@ class NumberFilter extends Component {
onFilterChange( filter.key, 'value', rangeStart || rangeEnd );
}

return this.getFormControl(
inputType,
rangeStart || rangeEnd,
partial( onFilterChange, filter.key, 'value' )
);
let labelFormat = '';

if ( 'lessthan' === filter.rule ) {
/* eslint-disable-next-line max-len */
/* translators: Sentence fragment, "maximum amount" refers to a numeric value the field must be less than. Screenshot for context: https://cloudup.com/cmv5CLyMPNQ */
labelFormat = _x( '%(field)s maximum amount', 'maximum value input', 'wc-admin' );
} else {
/* eslint-disable-next-line max-len */
/* translators: Sentence fragment, "minimum amount" refers to a numeric value the field must be more than. Screenshot for context: https://cloudup.com/cmv5CLyMPNQ */
labelFormat = _x( '%(field)s minimum amount', 'minimum value input', 'wc-admin' );
}

return this.getFormControl( {
type: inputType,
value: rangeStart || rangeEnd,
label: sprintf( labelFormat, { field: get( config, [ 'labels', 'add' ] ) } ),
onChange: partial( onFilterChange, filter.key, 'value' ),
} );
}

getRangeInput() {
Expand All @@ -130,8 +159,28 @@ class NumberFilter extends Component {
return interpolateComponents( {
mixedString: this.getBetweenString(),
components: {
rangeStart: this.getFormControl( inputType, rangeStart, rangeStartOnChange ),
rangeEnd: this.getFormControl( inputType, rangeEnd, rangeEndOnChange ),
rangeStart: this.getFormControl( {
type: inputType,
value: rangeStart,
label: sprintf(
/* eslint-disable-next-line max-len */
/* translators: Sentence fragment, "range start" refers to the first of two numeric values the field must be between. Screenshot for context: https://cloudup.com/cmv5CLyMPNQ */
__( '%(field)s range start', 'wc-admin' ),
{ field: get( config, [ 'labels', 'add' ] ) }
),
onChange: rangeStartOnChange,
} ),
rangeEnd: this.getFormControl( {
type: inputType,
value: rangeEnd,
label: sprintf(
/* eslint-disable-next-line max-len */
/* translators: Sentence fragment, "range end" refers to the second of two numeric values the field must be between. Screenshot for context: https://cloudup.com/cmv5CLyMPNQ */
__( '%(field)s range end', 'wc-admin' ),
{ field: get( config, [ 'labels', 'add' ] ) }
),
onChange: rangeEndOnChange,
} ),
span: <span className="separator" />,
},
} );
Expand Down Expand Up @@ -165,11 +214,14 @@ class NumberFilter extends Component {
),
},
} );

const screenReaderText = this.getScreenReaderText( filter, config );

/*eslint-disable jsx-a11y/no-noninteractive-tabindex*/
return (
<fieldset tabIndex="0">
<legend className="screen-reader-text">
{ this.getLegend( filter, config ) }
{ labels.add || '' }
</legend>
<div
className={ classnames( 'woocommerce-filters-advanced__fieldset', {
Expand All @@ -178,6 +230,11 @@ class NumberFilter extends Component {
>
{ children }
</div>
{ screenReaderText && (
<span className="screen-reader-text">
{ screenReaderText }
</span>
) }
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 😁

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

via tabbing? Or is there another method?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cool, thank you for the clarification.

</fieldset>
);
/*eslint-enable jsx-a11y/no-noninteractive-tabindex*/
Expand Down
28 changes: 21 additions & 7 deletions packages/components/src/filters/advanced/search-filter.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@
/**
* External dependencies
*/
import { Component } from '@wordpress/element';
import { Component, Fragment } from '@wordpress/element';
import { SelectControl } from '@wordpress/components';
import { find, isEqual, partial } from 'lodash';
import PropTypes from 'prop-types';
Expand All @@ -13,6 +13,7 @@ import classnames from 'classnames';
* Internal dependencies
*/
import Search from '../../search';
import { textContent } from './utils';

class SearchFilter extends Component {
constructor( { filter, config, query } ) {
Expand Down Expand Up @@ -51,18 +52,23 @@ class SearchFilter extends Component {
onFilterChange( filter.key, 'value', idList );
}

getLegend( filter, config ) {
getScreenReaderText( filter, config ) {
const { selected } = this.state;

if ( 0 === selected.length ) {
return '';
}

const rule = find( config.rules, { value: filter.rule } ) || {};
const filterStr = selected.map( item => item.label ).join( ', ' );

return interpolateComponents( {
return textContent( interpolateComponents( {
mixedString: config.labels.title,
components: {
filter: <span>{ filterStr }</span>,
rule: <span>{ rule.label }</span>,
filter: <Fragment>{ filterStr }</Fragment>,
rule: <Fragment>{ rule.label }</Fragment>,
},
} );
} ) );
}

render() {
Expand Down Expand Up @@ -95,11 +101,14 @@ class SearchFilter extends Component {
),
},
} );

const screenReaderText = this.getScreenReaderText( filter, config );

/*eslint-disable jsx-a11y/no-noninteractive-tabindex*/
return (
<fieldset tabIndex="0">
<legend className="screen-reader-text">
{ this.getLegend( filter, config, selected ) }
{ labels.add || '' }
</legend>
<div
className={ classnames( 'woocommerce-filters-advanced__fieldset', {
Expand All @@ -108,6 +117,11 @@ class SearchFilter extends Component {
>
{ children }
</div>
{ screenReaderText && (
<span className="screen-reader-text">
{ screenReaderText }
</span>
) }
</fieldset>
);
/*eslint-enable jsx-a11y/no-noninteractive-tabindex*/
Expand Down
34 changes: 27 additions & 7 deletions packages/components/src/filters/advanced/select-filter.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,13 +2,18 @@
/**
* External dependencies
*/
import { Component } from '@wordpress/element';
import { Component, Fragment } from '@wordpress/element';
import { SelectControl, Spinner } from '@wordpress/components';
import { find, partial } from 'lodash';
import PropTypes from 'prop-types';
import interpolateComponents from 'interpolate-components';
import classnames from 'classnames';

/**
* Internal dependencies
*/
import { textContent } from './utils';

/**
* WooCommerce dependencies
*/
Expand Down Expand Up @@ -41,16 +46,21 @@ class SelectFilter extends Component {
return options;
}

getLegend( filter, config ) {
getScreenReaderText( filter, config ) {
if ( '' === filter.value ) {
return '';
}

const rule = find( config.rules, { value: filter.rule } ) || {};
const value = find( config.input.options, { value: filter.value } ) || {};
return interpolateComponents( {

return textContent( interpolateComponents( {
mixedString: config.labels.title,
components: {
filter: <span>{ value.label }</span>,
rule: <span>{ rule.label }</span>,
filter: <Fragment>{ value.label }</Fragment>,
rule: <Fragment>{ rule.label }</Fragment>,
},
} );
} ) );
}

render() {
Expand Down Expand Up @@ -83,17 +93,27 @@ class SelectFilter extends Component {
),
},
} );

const screenReaderText = this.getScreenReaderText( filter, config );

/*eslint-disable jsx-a11y/no-noninteractive-tabindex*/
return (
<fieldset tabIndex="0">
<legend className="screen-reader-text">{ this.getLegend( filter, config ) }</legend>
<legend className="screen-reader-text">
{ labels.add || '' }
</legend>
<div
className={ classnames( 'woocommerce-filters-advanced__fieldset', {
'is-english': isEnglish,
} ) }
>
{ children }
</div>
{ screenReaderText && (
<span className="screen-reader-text">
{ screenReaderText }
</span>
) }
</fieldset>
);
/*eslint-enable jsx-a11y/no-noninteractive-tabindex*/
Expand Down
Loading