Skip to content

a11y: add service to add aria-describedby labels #6168

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

Merged
merged 19 commits into from
Sep 11, 2017

Conversation

andrewseguin
Copy link
Contributor

@andrewseguin andrewseguin commented Jul 31, 2017

Service adds a new element in the document body that contains the message. This is pointed to by the host element using aria-describedby for screenreaders to read out.

Used by tooltip and improved its accessibility. (add show on focus, escape keyboard shortcut)

@googlebot googlebot added the cla: yes PR author has agreed to Google's Contributor License Agreement label Jul 31, 2017

// Remove the a11y message if this was its last unique instance.
const a11yMessageElement = a11yMessages.get(message);
if (a11yMessageElement && --a11yMessageElement.count == 0) {
Copy link
Member

Choose a reason for hiding this comment

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

This might be a little more readable as if (a11yMessageElement && a11yMessageElement.count === 1) and then doing the decrement separately.


// If the global messages container no longer has any children, remove it.
if (!this._getA11yMessagesContainer()!.childNodes.length) {
document.body.removeChild(this._getA11yMessagesContainer()!);
Copy link
Member

Choose a reason for hiding this comment

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

You can use the Renderer2 to create/add/remove these elements.


const messageElement = document.createElement('div');
messageElement.id = `md-tooltip-message-${latestA11yMessageId++}`;
messageElement.innerHTML = message;
Copy link
Member

Choose a reason for hiding this comment

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

This looks a little dangerous. Perhaps we should use textContent instead since the spec doesn't allow rich text inside tooltips, IIRC?


const messageElement = document.createElement('div');
messageElement.id = `md-tooltip-message-${latestA11yMessageId++}`;
messageElement.innerHTML = message;
Copy link
Member

Choose a reason for hiding this comment

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

This one also needs to be updated if the tooltip text changes.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In the set message function, we'll unregister the old message and register the new one

if (this._message) { this._unregisterA11yMessage(this._message); }

// If the message is not a string (e.g. number), convert it to a string and trim it.
this._message = value ? `${value}`.trim() : '';
Copy link
Member

Choose a reason for hiding this comment

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

Might be a good idea to have a test for this case in particular (passing in a non-string to the message). We have another pending PR that deals with the same issue: #6146

private _createA11yMessagesContainer() {
const a11yMessagesContainer = document.createElement('div');
a11yMessagesContainer.id = A11Y_MESSAGES_CONTAINER_ID;
a11yMessagesContainer.className = 'cdk-visually-hidden';
Copy link
Member

Choose a reason for hiding this comment

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

It's unlikely for this to clobber any classes, but it's still better to use the renderer or classList.

@andrewseguin
Copy link
Contributor Author

Good call on the renderer stuff, wasn't even thinking about it

export const A11Y_MESSAGES_CONTAINER_ID = 'md-tooltip-a11y-messages';

/** Global incremental identifier for each registered a11y message. */
let latestA11yMessageId = 0;
Copy link
Member

Choose a reason for hiding this comment

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

I'd be okay with just calling this nextId (which is what we typically name these).

if (this._message) { this._unregisterA11yMessage(this._message); }

// If the message is not a string (e.g. number), convert it to a string and trim it.
this._message = value ? `${value}`.trim() : '';
Copy link
Member

Choose a reason for hiding this comment

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

this._message = `${value || ''}`.trim();

?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is done to address an issue where numbers are being used as Input and are throwing an error because they do not have trim(). This converts the input to string and trims it.

Copy link
Member

Choose a reason for hiding this comment

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

My suggestion still stringifies the value but omits the ternary.


/** Creates the global container for all tooltip a11y messages. */
private _createA11yMessagesContainer() {
const a11yMessagesContainer = this._renderer.createElement('div');
Copy link
Member

Choose a reason for hiding this comment

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

This needs aria-hidden as well. Otherwise screen-reader users will run into this content at the end of the document without context

* If an element has already been created for this message, increase that registered message's
* reference count.
*/
private _registerA11yMessage() {
Copy link
Member

Choose a reason for hiding this comment

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

What do you think about breaking all the code around this approach into a separate class called something like AriaDescriber? It would be good to capture all of this logic in one place separate from the tooltip and we could potentially re-use it in the future.

}

/** Returns the global container for tooltip a11y messages. */
private _getA11yMessagesContainer() {
Copy link
Member

Choose a reason for hiding this comment

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

Why not just remember the element?

/** Sets the trigger's aria-describedby attribute to the tooltip message element. */
private _setAriaDescribedBy() {
// Return if an aria-describedby already exists and it is not referencing a tooltip message.
const ariaDescribedBy = this._elementRef.nativeElement.getAttribute('aria-describedby');
Copy link
Member

Choose a reason for hiding this comment

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

Something I realized just now is that rather than clobbering an existing aria-describedby, we should augment it with the new ID (the attribute can have multiple, comma-delimited IDs). I talked to @mmalerba about doing the same thing for inputs with md-error and md-hint. We should create a couple utility functions in cdk/a11y to add/remove an ID to an element's aria attribute. Something like...

addAriaReferencedId(element, id, 'aria-describedby');
removeAriaReferencedId(element, id, 'aria-describedby');

(open to coming up with a better name)

@andrewseguin
Copy link
Contributor Author

Updated now so that the majority of the change is by adding aria-describer. Usage from tooltip is still included to show how it is used and its minimal code. Ready for review

@andrewseguin andrewseguin changed the title a11y(tooltip): add message element for screenreaders a11y: add service to add aria-describedby labels Aug 7, 2017
* to the Renderer.
*/
@Injectable()
export class AriaDescriber {
Copy link
Member

Choose a reason for hiding this comment

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

Let's make this @docs-private for now; I don't think this will be a public API before we're out of beta

constructor(private _platform: Platform,
@Optional() private _renderer: Renderer2) {
if (!_renderer) {
throw Error('AriaDescriber must be provided through a component\'s provider decorator ' +
Copy link
Member

Choose a reason for hiding this comment

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

Can use " or ` for the string instead of escaping the '

let nextId = 0;

/** Global map of all registered message elements that have been placed into the document. */
const registeredMessages = new Map<string, RegisteredMessage>();
Copy link
Member

Choose a reason for hiding this comment

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

messageRegistry?

*/
export interface RegisteredMessage {
messageElement: HTMLElement;
hostElements: HTMLElement[];
Copy link
Member

Choose a reason for hiding this comment

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

Why do you need to keep track of these elements? I'm thinking this may cause a memory leak if references to elements are preserved here when the elements have been removed.

Could you accomplish what you need by adding an attribute to the described element?

*/
private _createMessageElement(message: string): HTMLElement {
const messageElement = this._renderer.createElement('div');
this._renderer.setAttribute(messageElement, 'id', `cdk-aria-describedby-message-${nextId++}`);
Copy link
Member

Choose a reason for hiding this comment

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

How about just cdk-describedby-x?

Copy link
Contributor Author

@andrewseguin andrewseguin Aug 8, 2017

Choose a reason for hiding this comment

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

Went with cdk-describedby-message-## for the messages, and cdk-described-host for the elements that are described.

*/

/** IDs are deliminated by an empty space, as per the spec. */
const ID_DELIMINATOR = ' ';
Copy link
Member

Choose a reason for hiding this comment

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

We should probably be tolerant of any amount of whitespace between IDs rather than a single space (even though the spec calls for a single space)

const ID_DELIMINATOR = ' ';

/** Adds an aria reference ID to an element's aria property if it is not already present. */
export function addAriaReferencedId(el: HTMLElement, id: string, attr: string) {
Copy link
Member

Choose a reason for hiding this comment

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

element, attributeName, id feels like a more natural argument order

el.setAttribute(attr, filteredIds.join(ID_DELIMINATOR));
}

/** Returns a list of an element's aria reference IDs for the provided aria attribute. */
Copy link
Member

Choose a reason for hiding this comment

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

Gets the list of IDs referenced by the given ARIA attribute on an element. Used for attributes such as aria-labelledby, aria-owns, etc.

el.setAttribute(attr, ids.join(ID_DELIMINATOR));
}

/** Removes an aria reference ID from an element's aria property. */
Copy link
Member

Choose a reason for hiding this comment

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

Removes the given ID from the specified ARIA attribute on an element. Used for attributes such as aria-labelledby, aria-owns, etc.

/** IDs are deliminated by an empty space, as per the spec. */
const ID_DELIMINATOR = ' ';

/** Adds an aria reference ID to an element's aria property if it is not already present. */
Copy link
Member

Choose a reason for hiding this comment

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

Adds the given ID to the specified ARIA attribute on an element. Used for attributes such as aria-labelledby, aria-owns, etc.

if (!this._platform.isBrowser || !message.trim()) { return; }

if (!registeredMessages.get(message)) {
const messageElement = this._createMessageElement(message);
Copy link
Contributor

Choose a reason for hiding this comment

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

Is there no way to use existing DOM elements as description messages? For the input we always already have a md-hint or md-error element with the message.

Maybe you could make a directive that uses this service under the hood for existing elements to use?

Something like:

<span [cdkDescribes]="listOfElementsIDescribe">...</span>

Or the other way around:

<span [cdkDescribedBy]="listOfElementsThatDescribeMe">...</span>

function expectMessages(messages: string[]) {
const messageElements = getMessageElements();
if (!messageElements) {
fail(`Expected messages ${messages} but there were no message elements`);
Copy link
Member

Choose a reason for hiding this comment

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

Why not have a regular expect here?

}

const messages: string[] = [];
ariaDescribedBy.split(' ').forEach(referenceId => {
Copy link
Member

Choose a reason for hiding this comment

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

You can condense this logic with an array map or reduce.

constructor(private _platform: Platform,
@Optional() private _renderer: Renderer2) {
if (!_renderer) {
throw Error('AriaDescriber must be provided through a component\'s provider decorator ' +
Copy link
Member

Choose a reason for hiding this comment

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

You can get access to a renderer without going through a component. See #5840

* message element.
*/
addDescription(hostElement: HTMLElement, message: string) {
if (!this._platform.isBrowser || !message.trim()) { return; }
Copy link
Member

Choose a reason for hiding this comment

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

The message.trim will fail if the value is not a string.

addDescription(hostElement: HTMLElement, message: string) {
if (!this._platform.isBrowser || !message.trim()) { return; }

if (!registeredMessages.get(message)) {
Copy link
Member

Choose a reason for hiding this comment

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

It's pretty much the same, but you can use registeredMessages.has(message) here.

* Removes the host element's aria-describedby reference to the message element.
*/
removeDescription(hostElement: HTMLElement, message: string) {
if (!this._platform.isBrowser || !message.trim()) { return; }
Copy link
Member

Choose a reason for hiding this comment

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

This trim has the potential to fail as well.

registeredMessage.hostElements = registeredMessage.hostElements.filter(el => el != hostElement);
removeAriaReferencedId(hostElement, registeredMessage.messageElement.id, 'aria-describedby');

if (registeredMessage.hostElements.length == 0) {
Copy link
Member

Choose a reason for hiding this comment

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

Either triple equals or !registeredMessage.hostElements.length here and below.

}

/** Unregisters all created message elements and removes the message container. */
_unregisterAllMessages() {
Copy link
Member

Choose a reason for hiding this comment

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

This one could be renamed to ngOnDestroy which has the advantage of being called by Angular when appropriate.

@andrewseguin
Copy link
Contributor Author

Updated based on the comments, thanks

}

const registeredMessage = messageRegistry.get(message)!;
registeredMessage.referenceCount++;
Copy link
Member

Choose a reason for hiding this comment

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

Only update the reference count if the element is not already described by the message? (can check the existing IDs)

* Used for attributes such as aria-labelledby, aria-owns, etc.
*/
export function getAriaReferenceIds(el: Element, attr: string): string[] {
const idsValue = (el.getAttribute(attr) || '').trim();
Copy link
Member

Choose a reason for hiding this comment

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

attributeValue?

Since you're matching all non-whitespace, do you need the trim?

const idsValue = (el.getAttribute(attr) || '').trim();

// Get string array of all individual ids (whitespace deliminated) in the attribute value
const idsArray = idsValue.match(/\S+/g) || [];
Copy link
Member

Choose a reason for hiding this comment

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

Just ids?

Since you're matching all non-whitespace, do you need the map to trim?

*/
export function removeAriaReferencedId(el: Element, attr: string, id: string) {
const ids = getAriaReferenceIds(el, attr);
const filteredIds = ids.filter(val => val != id.trim());
Copy link
Member

Choose a reason for hiding this comment

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

How would you end up with an empty string here since getAriaReferenceIds only targets non-whitespace?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Looks like this needed to be revisited based on the regex, before the single space deliminator didn't trim up as much as I liked. The regex approach works much nicer

*/
function expectIds(ids: string[]) {
expect(getAriaReferenceIds(testElement!, 'aria-describedby')).toEqual(ids);
expect(testElement!.getAttribute('aria-describedby')).toBe(ids.length ? ids.join(' ') : '');
Copy link
Member

Choose a reason for hiding this comment

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

This is a bit too much logic in the test for my taste. I'd generally prefer to have the assertion on aria-describedby to be hard-coded into each test

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Changed it around a bit, kept expectIds but takes in an attr. Removed the helper functions for adding/removing the references

* @docs-private
*/
@Injectable()
export class AriaDescriber {
Copy link
Member

Choose a reason for hiding this comment

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

Need to add a provider for AriaDescriber similar to LIVE_ANNOUNCER_PROVIDER
(provides the existing instances if there is one)

}

function removeCdkDescribedByReferenceIds(element: Element) {
// Remove all aria-describedby reference IDs that are prefixed by CDK_DESCRIBEDBY_ID_PREFIX
Copy link
Member

Choose a reason for hiding this comment

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

Make this a function description?

}

// If the message is not a string (e.g. number), convert it to a string and trim it.
this._message = value ? `${value}`.trim() : '';
Copy link
Member

Choose a reason for hiding this comment

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

Just `${value}`.trim() ?

Copy link
Member

Choose a reason for hiding this comment

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

Wouldn't that end up with the string undefined if value === undefined?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

As mentioned by @crisbeto, an undefined message will evaluate `${value}`.trim() as 'undefined'

Copy link
Member

Choose a reason for hiding this comment

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

Ah, right; that's why my original comment was `${value || ''}`.trim()

Copy link
Member

@crisbeto crisbeto left a comment

Choose a reason for hiding this comment

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

LGTM

}

// If the message is not a string (e.g. number), convert it to a string and trim it.
this._message = value ? `${value}`.trim() : '';
Copy link
Member

Choose a reason for hiding this comment

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

Wouldn't that end up with the string undefined if value === undefined?

@andrewseguin
Copy link
Contributor Author

Phew, ready found another round

Copy link
Member

@jelbourn jelbourn left a comment

Choose a reason for hiding this comment

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

LGTM

@jelbourn jelbourn added pr: lgtm action: merge The PR is ready for merge by the caretaker labels Aug 9, 2017
@kara kara added pr: needs rebase and removed action: merge The PR is ready for merge by the caretaker labels Aug 21, 2017
@kara
Copy link
Contributor

kara commented Aug 21, 2017

@andrewseguin Rebase?

@andrewseguin andrewseguin added action: merge The PR is ready for merge by the caretaker and removed pr: needs rebase labels Aug 28, 2017
@jelbourn
Copy link
Member

jelbourn commented Sep 1, 2017

@andrewseguin this is failing the prerender task

@jelbourn jelbourn removed the action: merge The PR is ready for merge by the caretaker label Sep 1, 2017
@andrewseguin andrewseguin added action: merge The PR is ready for merge by the caretaker and removed action: merge The PR is ready for merge by the caretaker labels Sep 6, 2017
@andrewseguin andrewseguin added the action: merge The PR is ready for merge by the caretaker label Sep 6, 2017
@andrewseguin
Copy link
Contributor Author

Fixed prerender (the aria describer's ngOnDestroy was referencing the document). Added a11y docs for review as well @tinayuangao

@mmalerba mmalerba merged commit 3be4da6 into angular:master Sep 11, 2017
josephperrott pushed a commit to josephperrott/components that referenced this pull request Sep 15, 2017
* checkin

* changes

* a11y(tooltip): add message element for tooltip a11y

* comments

* remove extra line

* add test for tooltip message as number

* remove fit

* use renderer

* always decrement

* add aria-describer

* add test

* tests

* md to cdk

* Add aria-hidden to container

* fix aot

* comments

* comments

* rebase

* fix prerender; add a11y docs
@andrewseguin andrewseguin deleted the tooltip-a11y branch November 28, 2017 20:40
@angular-automatic-lock-bot
Copy link

This issue has been automatically locked due to inactivity.
Please file a new issue if you are encountering a similar or related problem.

Read more about our automatic conversation locking policy.

This action has been performed automatically by a bot.

@angular-automatic-lock-bot angular-automatic-lock-bot bot locked and limited conversation to collaborators Sep 7, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
action: merge The PR is ready for merge by the caretaker cla: yes PR author has agreed to Google's Contributor License Agreement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants