Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Polish a11y package. #21148

Merged
merged 1 commit into from
Mar 31, 2020
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
10 changes: 5 additions & 5 deletions packages/a11y/README.md
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,7 @@ Create the live regions.
<a name="speak" href="#speak">#</a> **speak**

Allows you to easily announce dynamic interface updates to screen readers using ARIA live regions.
This module is inspired by the `speak` function in wp-a11y.js
This module is inspired by the `speak` function in `wp-a11y.js`.

_Usage_

Expand All @@ -39,15 +39,15 @@ speak( 'The message you want to send to the ARIA live region', 'assertive' );

_Parameters_

- _message_ `string`: The message to be announced by Assistive Technologies.
- _ariaLive_ `string`: Optional. The politeness level for aria-live. Possible values: polite or assertive. Default polite.
- _message_ `string`: The message to be announced by assistive technologies.
- _ariaLive_ `[string]`: The politeness level for aria-live; default: 'polite'.


<!-- END TOKEN(Autogenerated API docs) -->

### Background

For context I'll quote [this article on WordPress.org](https://make.wordpress.org/accessibility/2015/04/15/let-wordpress-speak-new-in-wordpress-4-2/) by [@joedolson](https://github.com/joedolson):
For context Ill quote [this article on WordPress.org](https://make.wordpress.org/accessibility/2015/04/15/let-wordpress-speak-new-in-wordpress-4-2/) by [@joedolson](https://github.com/joedolson):

> #### Why.
>
Expand All @@ -71,4 +71,4 @@ For context I'll quote [this article on WordPress.org](https://make.wordpress.or

See <https://make.wordpress.org/design/handbook/design-guide/browser-support/>

<br/><br/><p align="center"><img src="https://s.w.org/style/images/codeispoetry.png?1" alt="Code is Poetry." /></p>
<br /><br /><p align="center"><img src="https://s.w.org/style/images/codeispoetry.png?1" alt="Code is Poetry." /></p>
Original file line number Diff line number Diff line change
@@ -1,15 +1,13 @@
/**
* Build the live regions markup.
*
* @param {string} ariaLive Optional. Value for the 'aria-live' attribute, default 'polite'.
* @param {string} [ariaLive] Value for the 'aria-live' attribute; default: 'polite'.
*
* @return {HTMLDivElement} The ARIA live region HTML element.
*/
const addContainer = function( ariaLive ) {
ariaLive = ariaLive || 'polite';
Copy link
Member

Choose a reason for hiding this comment

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

Technically this would previously fall back to 'polite' for any falsy value given (null, false, 0, etc), and now it will only fall back when explicitly undefined (omitted). Generally speaking, I think this is a fine approach, but it may be something to consider for potential side-effects.

Copy link
Member Author

@ZebulanStanphill ZebulanStanphill Mar 26, 2020

Choose a reason for hiding this comment

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

Yeah, I generally lean towards enforcing that functions only be called with certain types, so in this case I'd consider calling this function with anything other than one of the 2 valid strings or undefined to be an error that should be caught by the linter/compiler.

I initially tried to use a type union of ('assertive'|'polite') as the ariaLive JSDoc type, but despite something similar appearing as an example in our docs, the auto-generated docs weren't handling it properly and was just outputting (|). I'm going to make another PR for that change later and see if anyone can figure out how to fix the issue.

Copy link
Member

Choose a reason for hiding this comment

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

I initially tried to use a type union of ('assertive'|'polite') as the ariaLive JSDoc type, but despite something similar appearing as an example in our docs, the auto-generated docs weren't handling it properly and was just outputting (|) I'm going to make another PR for that change later and see if anyone can figure out how to fix the issue.

Yeah, this is related to the issue described at #18045, and tangentially #19952 (#19952 (comment)).


export default function addContainer( ariaLive = 'polite' ) {
const container = document.createElement( 'div' );
container.id = 'a11y-speak-' + ariaLive;
container.id = `a11y-speak-${ ariaLive }`;
container.className = 'a11y-speak-region';

container.setAttribute(
Expand All @@ -30,12 +28,10 @@ const addContainer = function( ariaLive ) {
container.setAttribute( 'aria-relevant', 'additions text' );
container.setAttribute( 'aria-atomic', 'true' );

const body = document.querySelector( 'body' );
const { body } = document;
if ( body ) {
body.appendChild( container );
}

return container;
};

export default addContainer;
}
9 changes: 4 additions & 5 deletions packages/a11y/src/clear.js
Original file line number Diff line number Diff line change
@@ -1,11 +1,10 @@
/**
* Clear the a11y-speak-region elements.
*/
const clear = function() {
const regions = document.querySelectorAll( '.a11y-speak-region' );
export default function clear() {
const regions = document.getElementsByClassName( 'a11y-speak-region' );

for ( let i = 0; i < regions.length; i++ ) {
regions[ i ].textContent = '';
}
};

export default clear;
}
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@ let previousMessage = '';
*
* @return {string} The filtered message.
*/
const filterMessage = function( message ) {
export default function filterMessage( message ) {
/*
* Strip HTML tags (if any) from the message string. Ideally, messages should
* be simple strings, carefully crafted for specific use with A11ySpeak.
Expand All @@ -24,6 +24,4 @@ const filterMessage = function( message ) {
previousMessage = message;

return message;
};

export default filterMessage;
}
31 changes: 15 additions & 16 deletions packages/a11y/src/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -6,26 +6,26 @@ import domReady from '@wordpress/dom-ready';
/**
* Internal dependencies
*/
import addContainer from './addContainer';
import addContainer from './add-container';
import clear from './clear';
import filterMessage from './filterMessage';
import filterMessage from './filter-message';

/**
* Create the live regions.
*/
export const setup = function() {
const containerPolite = document.getElementById( 'a11y-speak-polite' );
export function setup() {
const containerAssertive = document.getElementById(
'a11y-speak-assertive'
);
const containerPolite = document.getElementById( 'a11y-speak-polite' );

if ( containerPolite === null ) {
addContainer( 'polite' );
}
if ( containerAssertive === null ) {
addContainer( 'assertive' );
}
};
if ( containerPolite === null ) {
addContainer( 'polite' );
}
}

/**
* Run setup on domReady.
Expand All @@ -34,11 +34,10 @@ domReady( setup );

/**
* Allows you to easily announce dynamic interface updates to screen readers using ARIA live regions.
* This module is inspired by the `speak` function in wp-a11y.js
* This module is inspired by the `speak` function in `wp-a11y.js`.
*
* @param {string} message The message to be announced by Assistive Technologies.
* @param {string} ariaLive Optional. The politeness level for aria-live. Possible values:
* polite or assertive. Default polite.
* @param {string} message The message to be announced by assistive technologies.
* @param {string} [ariaLive] The politeness level for aria-live; default: 'polite'.
*
* @example
* ```js
Expand All @@ -51,20 +50,20 @@ domReady( setup );
* speak( 'The message you want to send to the ARIA live region', 'assertive' );
* ```
*/
export const speak = function( message, ariaLive ) {
export function speak( message, ariaLive ) {
// Clear previous messages to allow repeated strings being read out.
clear();

message = filterMessage( message );

const containerPolite = document.getElementById( 'a11y-speak-polite' );
const containerAssertive = document.getElementById(
'a11y-speak-assertive'
);
const containerPolite = document.getElementById( 'a11y-speak-polite' );

if ( containerAssertive && 'assertive' === ariaLive ) {
if ( containerAssertive && ariaLive === 'assertive' ) {
containerAssertive.textContent = message;
} else if ( containerPolite ) {
containerPolite.textContent = message;
}
};
}
11 changes: 5 additions & 6 deletions packages/a11y/src/index.native.js
Original file line number Diff line number Diff line change
@@ -1,19 +1,18 @@
/**
* Internal dependencies
*/
import filterMessage from './filterMessage';
import filterMessage from './filter-message';

/**
* Update the ARIA live notification area text node.
*
* @param {string} message The message to be announced by Assistive Technologies.
* @param {string} ariaLive Optional. The politeness level for aria-live. Possible values:
* polite or assertive. Default polite.
* @param {string} [ariaLive] The politeness level for aria-live; default: 'polite'.
*/
export const speak = function( message, ariaLive ) {
export function speak( message, ariaLive ) {
message = filterMessage( message );
//TODO: Use native module to speak message
if ( 'assertive' === ariaLive ) {
if ( ariaLive === 'assertive' ) {
} else {
}
};
}
Original file line number Diff line number Diff line change
@@ -1,14 +1,14 @@
/**
* Internal dependencies
*/
import addContainer from '../addContainer';
import addContainer from '../add-container';

describe( 'addContainer', () => {
describe( 'with polite param', () => {
it( 'should create an aria-live element with aria-live attr set to polite', () => {
const container = addContainer( 'polite' );

expect( container ).not.toBe( null );
expect( container ).not.toBeNull();
expect( container.className ).toBe( 'a11y-speak-region' );
expect( container.id ).toBe( 'a11y-speak-polite' );
expect( container.getAttribute( 'style' ) ).not.toBeNull();
Expand All @@ -24,7 +24,7 @@ describe( 'addContainer', () => {
it( 'should create an aria-live element with aria-live attr set to assertive', () => {
const container = addContainer( 'assertive' );

expect( container ).not.toBe( null );
expect( container ).not.toBeNull();
expect( container.className ).toBe( 'a11y-speak-region' );
expect( container.id ).toBe( 'a11y-speak-assertive' );
expect( container.getAttribute( 'style' ) ).not.toBeNull();
Expand All @@ -40,7 +40,7 @@ describe( 'addContainer', () => {
it( 'should default to creating an aria-live element with aria-live attr set to polite', () => {
const container = addContainer( 'polite' );

expect( container ).not.toBe( null );
expect( container ).not.toBeNull();
expect( container.className ).toBe( 'a11y-speak-region' );
expect( container.id ).toBe( 'a11y-speak-polite' );
expect( container.getAttribute( 'style' ) ).not.toBeNull();
Expand Down
4 changes: 2 additions & 2 deletions packages/a11y/src/test/clear.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -8,12 +8,12 @@ describe( 'clear', () => {
const container1 = document.createElement( 'div' );
container1.className = 'a11y-speak-region';
container1.textContent = 'not empty';
document.querySelector( 'body' ).appendChild( container1 );
document.body.appendChild( container1 );

const container2 = document.createElement( 'div' );
container2.className = 'a11y-speak-region';
container2.textContent = 'not empty';
document.querySelector( 'body' ).appendChild( container2 );
document.body.appendChild( container2 );

clear();
expect( container1.textContent ).toBe( '' );
Expand Down
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
/**
* Internal dependencies
*/
import filterMessage from '../filterMessage';
import filterMessage from '../filter-message';

describe( 'filterMessage', () => {
describe( 'when a clean message is passed in', () => {
Expand All @@ -15,7 +15,7 @@ describe( 'filterMessage', () => {
it( 'should add a space to the message to make sure it is announced again', () => {
filterMessage( 'repeated message.' );
const actual = filterMessage( 'repeated message.' );
expect( actual ).toBe( 'repeated message.' + '\u00A0' );
expect( actual ).toBe( 'repeated message.\u00A0' );
} );
} );

Expand Down
20 changes: 9 additions & 11 deletions packages/a11y/src/test/index.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@ import domReady from '@wordpress/dom-ready';
*/
import { setup, speak } from '../';
import clear from '../clear';
import filterMessage from '../filterMessage';
import filterMessage from '../filter-message';

jest.mock( '../clear', () => {
return jest.fn();
Expand All @@ -18,7 +18,7 @@ jest.mock( '@wordpress/dom-ready', () => {
callback();
} );
} );
jest.mock( '../filterMessage', () => {
jest.mock( '../filter-message', () => {
return jest.fn( ( message ) => {
return message;
} );
Expand Down Expand Up @@ -82,9 +82,9 @@ describe( 'speak', () => {
it( 'should set the textcontent of the polite aria-live region', () => {
speak( 'message', 'assertive' );
expect( containerPolite.textContent ).toBe( 'message' );
expect( document.getElementById( 'a11y-speak-assertive' ) ).toBe(
null
);
expect(
document.getElementById( 'a11y-speak-assertive' )
).toBeNull();
} );
} );

Expand All @@ -103,12 +103,10 @@ describe( 'speak', () => {
} );

it( 'should set the textcontent of the polite aria-live region', () => {
expect( document.getElementById( 'a11y-speak-polite' ) ).toBe(
null
);
expect( document.getElementById( 'a11y-speak-assertive' ) ).toBe(
null
);
expect( document.getElementById( 'a11y-speak-polite' ) ).toBeNull();
expect(
document.getElementById( 'a11y-speak-assertive' )
).toBeNull();
} );
} );

Expand Down