From 7ace81ce3bf1df21a1df3f9890a2181881834b5f Mon Sep 17 00:00:00 2001 From: crisbeto Date: Wed, 1 Apr 2020 23:08:18 +0200 Subject: [PATCH] fix(aria-describer): messages not being read out in IE and Edge Fixes the messages from the `AriaDescriber` not being read out in Edge or IE, because the message container is `aria-hidden` and has `display: none`. Fixes #12298. --- .../aria-describer/aria-describer.spec.ts | 60 ++++++++++++++++--- src/cdk/a11y/aria-describer/aria-describer.ts | 19 +++++- tools/public_api_guard/cdk/a11y.d.ts | 3 +- 3 files changed, 70 insertions(+), 12 deletions(-) diff --git a/src/cdk/a11y/aria-describer/aria-describer.spec.ts b/src/cdk/a11y/aria-describer/aria-describer.spec.ts index 2062072a9120..aa2857b29ed9 100644 --- a/src/cdk/a11y/aria-describer/aria-describer.spec.ts +++ b/src/cdk/a11y/aria-describer/aria-describer.spec.ts @@ -1,62 +1,74 @@ import {A11yModule, CDK_DESCRIBEDBY_HOST_ATTRIBUTE} from '../index'; import {AriaDescriber, MESSAGES_CONTAINER_ID} from './aria-describer'; -import {async, ComponentFixture, TestBed} from '@angular/core/testing'; -import {Component, ElementRef, ViewChild} from '@angular/core'; +import {ComponentFixture, TestBed} from '@angular/core/testing'; +import {Component, ElementRef, ViewChild, Provider} from '@angular/core'; +import {Platform} from '@angular/cdk/platform'; describe('AriaDescriber', () => { let ariaDescriber: AriaDescriber; let component: TestApp; let fixture: ComponentFixture; - beforeEach(async(() => { + function createFixture(providers: Provider[] = []) { TestBed.configureTestingModule({ imports: [A11yModule], declarations: [TestApp], - providers: [AriaDescriber], + providers: [AriaDescriber, ...providers], }).compileComponents(); - })); - beforeEach(() => { fixture = TestBed.createComponent(TestApp); component = fixture.componentInstance; ariaDescriber = component.ariaDescriber; fixture.detectChanges(); - }); + } afterEach(() => { ariaDescriber.ngOnDestroy(); }); it('should initialize without the message container', () => { + createFixture(); expect(getMessagesContainer()).toBeNull(); }); it('should be able to create a message element', () => { + createFixture(); ariaDescriber.describe(component.element1, 'My Message'); expectMessages(['My Message']); }); it('should be able to describe using an element', () => { + createFixture(); const descriptionNode = fixture.nativeElement.querySelector('#description-with-existing-id'); ariaDescriber.describe(component.element1, descriptionNode); expectMessage(component.element1, 'Hello'); }); + it('should hide the message container', () => { + createFixture(); + ariaDescriber.describe(component.element1, 'My Message'); + expect(getMessagesContainer().classList).toContain('cdk-visually-hidden'); + }); + it('should not register empty strings', () => { + createFixture(); ariaDescriber.describe(component.element1, ''); expect(getMessageElements()).toBe(null); }); it('should not register non-string values', () => { + createFixture(); expect(() => ariaDescriber.describe(component.element1, null!)).not.toThrow(); expect(getMessageElements()).toBe(null); }); it('should not throw when trying to remove non-string value', () => { + createFixture(); expect(() => ariaDescriber.removeDescription(component.element1, null!)).not.toThrow(); }); it('should de-dupe a message registered multiple times', () => { + createFixture(); ariaDescriber.describe(component.element1, 'My Message'); ariaDescriber.describe(component.element2, 'My Message'); ariaDescriber.describe(component.element3, 'My Message'); @@ -67,6 +79,7 @@ describe('AriaDescriber', () => { }); it('should de-dupe a message registered multiple via an element node', () => { + createFixture(); const descriptionNode = fixture.nativeElement.querySelector('#description-with-existing-id'); ariaDescriber.describe(component.element1, descriptionNode); ariaDescriber.describe(component.element2, descriptionNode); @@ -77,6 +90,7 @@ describe('AriaDescriber', () => { }); it('should be able to register multiple messages', () => { + createFixture(); ariaDescriber.describe(component.element1, 'First Message'); ariaDescriber.describe(component.element2, 'Second Message'); expectMessages(['First Message', 'Second Message']); @@ -85,6 +99,7 @@ describe('AriaDescriber', () => { }); it('should be able to unregister messages', () => { + createFixture(); ariaDescriber.describe(component.element1, 'My Message'); expectMessages(['My Message']); @@ -104,6 +119,7 @@ describe('AriaDescriber', () => { }); it('should not remove nodes that were set as messages when unregistering', () => { + createFixture(); const descriptionNode = fixture.nativeElement.querySelector('#description-with-existing-id'); expect(document.body.contains(descriptionNode)) @@ -123,6 +139,7 @@ describe('AriaDescriber', () => { }); it('should keep nodes set as descriptions inside their original position in the DOM', () => { + createFixture(); const descriptionNode = fixture.nativeElement.querySelector('#description-with-existing-id'); const initialParent = descriptionNode.parentNode; @@ -141,6 +158,7 @@ describe('AriaDescriber', () => { }); it('should be able to unregister messages while having others registered', () => { + createFixture(); ariaDescriber.describe(component.element1, 'Persistent Message'); ariaDescriber.describe(component.element2, 'My Message'); expectMessages(['Persistent Message', 'My Message']); @@ -159,12 +177,14 @@ describe('AriaDescriber', () => { }); it('should be able to append to an existing list of aria describedby', () => { + createFixture(); ariaDescriber.describe(component.element4, 'My Message'); expectMessages(['My Message']); expectMessage(component.element4, 'My Message'); }); it('should be able to handle multiple regisitrations of the same message to an element', () => { + createFixture(); ariaDescriber.describe(component.element1, 'My Message'); ariaDescriber.describe(component.element1, 'My Message'); expectMessages(['My Message']); @@ -172,11 +192,13 @@ describe('AriaDescriber', () => { }); it('should not throw when attempting to describe a non-element node', () => { + createFixture(); const node: any = document.createComment('Not an element node'); expect(() => ariaDescriber.describe(node, 'This looks like an element')).not.toThrow(); }); it('should clear any pre-existing containers', () => { + createFixture(); const extraContainer = document.createElement('div'); extraContainer.id = MESSAGES_CONTAINER_ID; document.body.appendChild(extraContainer); @@ -192,6 +214,7 @@ describe('AriaDescriber', () => { }); it('should not describe messages that match up with the aria-label of the element', () => { + createFixture(); component.element1.setAttribute('aria-label', 'Hello'); ariaDescriber.describe(component.element1, 'Hello'); ariaDescriber.describe(component.element1, 'Hi'); @@ -199,6 +222,7 @@ describe('AriaDescriber', () => { }); it('should assign an id to the description element, if it does not have one', () => { + createFixture(); const descriptionNode = fixture.nativeElement.querySelector('[description-without-id]'); expect(descriptionNode.getAttribute('id')).toBeFalsy(); ariaDescriber.describe(component.element1, descriptionNode); @@ -206,6 +230,7 @@ describe('AriaDescriber', () => { }); it('should not overwrite the existing id of the description element', () => { + createFixture(); const descriptionNode = fixture.nativeElement.querySelector('#description-with-existing-id'); expect(descriptionNode.id).toBe('description-with-existing-id'); ariaDescriber.describe(component.element1, descriptionNode); @@ -213,6 +238,7 @@ describe('AriaDescriber', () => { }); it('should not remove pre-existing description nodes on destroy', () => { + createFixture(); const descriptionNode = fixture.nativeElement.querySelector('#description-with-existing-id'); expect(document.body.contains(descriptionNode)) @@ -231,6 +257,7 @@ describe('AriaDescriber', () => { }); it('should remove the aria-describedby attribute if there are no more messages', () => { + createFixture(); const element = component.element1; expect(element.hasAttribute('aria-describedby')).toBe(false); @@ -242,10 +269,27 @@ describe('AriaDescriber', () => { expect(element.hasAttribute('aria-describedby')).toBe(false); }); + it('should set `aria-hidden` on the container by default', () => { + createFixture([{provide: Platform, useValue: {BLINK: true}}]); + ariaDescriber.describe(component.element1, 'My Message'); + expect(getMessagesContainer().getAttribute('aria-hidden')).toBe('true'); + }); + + it('should disable `aria-hidden` on the container in IE', () => { + createFixture([{provide: Platform, useValue: {TRIDENT: true}}]); + ariaDescriber.describe(component.element1, 'My Message'); + expect(getMessagesContainer().getAttribute('aria-hidden')).toBe('false'); + }); + + it('should disable `aria-hidden` on the container in Edge', () => { + createFixture([{provide: Platform, useValue: {EDGE: true}}]); + ariaDescriber.describe(component.element1, 'My Message'); + expect(getMessagesContainer().getAttribute('aria-hidden')).toBe('false'); + }); }); function getMessagesContainer() { - return document.querySelector(`#${MESSAGES_CONTAINER_ID}`); + return document.querySelector(`#${MESSAGES_CONTAINER_ID}`)!; } function getMessageElements(): Node[] | null { diff --git a/src/cdk/a11y/aria-describer/aria-describer.ts b/src/cdk/a11y/aria-describer/aria-describer.ts index 9af03d02a73b..ae31a20f575e 100644 --- a/src/cdk/a11y/aria-describer/aria-describer.ts +++ b/src/cdk/a11y/aria-describer/aria-describer.ts @@ -9,6 +9,7 @@ import {DOCUMENT} from '@angular/common'; import {Inject, Injectable, OnDestroy} from '@angular/core'; import {addAriaReferencedId, getAriaReferenceIds, removeAriaReferencedId} from './aria-reference'; +import {Platform} from '@angular/cdk/platform'; /** @@ -50,7 +51,12 @@ let messagesContainer: HTMLElement | null = null; export class AriaDescriber implements OnDestroy { private _document: Document; - constructor(@Inject(DOCUMENT) _document: any) { + constructor( + @Inject(DOCUMENT) _document: any, + /** + * @breaking-change 8.0.0 `_platform` parameter to be made required. + */ + private _platform?: Platform) { this._document = _document; } @@ -153,6 +159,8 @@ export class AriaDescriber implements OnDestroy { /** Creates the global container for all aria-describedby messages. */ private _createMessagesContainer() { if (!messagesContainer) { + // @breaking-change 8.0.0 `_platform` null check can be removed once the parameter is required + const canBeAriaHidden = !this._platform || (!this._platform.EDGE && !this._platform.TRIDENT); const preExistingContainer = this._document.getElementById(MESSAGES_CONTAINER_ID); // When going from the server to the client, we may end up in a situation where there's @@ -165,8 +173,13 @@ export class AriaDescriber implements OnDestroy { messagesContainer = this._document.createElement('div'); messagesContainer.id = MESSAGES_CONTAINER_ID; - messagesContainer.setAttribute('aria-hidden', 'true'); - messagesContainer.style.display = 'none'; + messagesContainer.classList.add('cdk-visually-hidden'); + + // IE and Edge won't read out the messages if they're in an `aria-hidden` container. + // We only disable `aria-hidden` for these platforms, because it comes with the + // disadvantage that people might hit the messages when they've navigated past + // the end of the document using the arrow keys. + messagesContainer.setAttribute('aria-hidden', canBeAriaHidden + ''); this._document.body.appendChild(messagesContainer); } } diff --git a/tools/public_api_guard/cdk/a11y.d.ts b/tools/public_api_guard/cdk/a11y.d.ts index dbd61352f48c..2fc528587753 100644 --- a/tools/public_api_guard/cdk/a11y.d.ts +++ b/tools/public_api_guard/cdk/a11y.d.ts @@ -10,7 +10,8 @@ export declare class ActiveDescendantKeyManager extends ListKeyManager