From 083d287ff116d39b38655af67c99d2d3a16352bb Mon Sep 17 00:00:00 2001 From: crisbeto Date: Mon, 15 Jan 2018 06:33:21 +0100 Subject: [PATCH] fix(aria-describer): exception when attempting to describe a non-element node * Fixes an error that was being thrown when trying to describe a non-element node. This could happen if the consumer is attempting to describe an `ng-container` which corresponds to a comment node. * Switches a few cases, that had hard-coded the `Node.ELEMENT_NODE` constant for to Universal compatibility, to take the constant from the `Document`. Relates to #8724. --- src/cdk/a11y/aria-describer.spec.ts | 5 +++++ src/cdk/a11y/aria-describer.ts | 4 ++-- src/cdk/a11y/focus-trap.ts | 10 ++-------- src/lib/icon/icon-registry.ts | 8 +++++--- 4 files changed, 14 insertions(+), 13 deletions(-) diff --git a/src/cdk/a11y/aria-describer.spec.ts b/src/cdk/a11y/aria-describer.spec.ts index 93164d3a518c..34d1b629f652 100644 --- a/src/cdk/a11y/aria-describer.spec.ts +++ b/src/cdk/a11y/aria-describer.spec.ts @@ -107,6 +107,11 @@ describe('AriaDescriber', () => { expectMessages(['My Message']); expectMessage(component.element1, 'My Message'); }); + + it('should not throw when attempting to describe a non-element node', () => { + const node: any = document.createComment('Not an element node'); + expect(() => ariaDescriber.describe(node, 'This looks like an element')).not.toThrow(); + }); }); function getMessagesContainer() { diff --git a/src/cdk/a11y/aria-describer.ts b/src/cdk/a11y/aria-describer.ts index f6e0094e9c88..d0c99e4f9e56 100644 --- a/src/cdk/a11y/aria-describer.ts +++ b/src/cdk/a11y/aria-describer.ts @@ -60,7 +60,7 @@ export class AriaDescriber { * message element. */ describe(hostElement: Element, message: string) { - if (!message.trim()) { + if (hostElement.nodeType !== this._document.ELEMENT_NODE || !message.trim()) { return; } @@ -75,7 +75,7 @@ export class AriaDescriber { /** Removes the host element's aria-describedby reference to the message element. */ removeDescription(hostElement: Element, message: string) { - if (!message.trim()) { + if (hostElement.nodeType !== this._document.ELEMENT_NODE || !message.trim()) { return; } diff --git a/src/cdk/a11y/focus-trap.ts b/src/cdk/a11y/focus-trap.ts index 5e1413bf2379..56880fc9f0eb 100644 --- a/src/cdk/a11y/focus-trap.ts +++ b/src/cdk/a11y/focus-trap.ts @@ -21,12 +21,6 @@ import {take} from 'rxjs/operators/take'; import {InteractivityChecker} from './interactivity-checker'; import {DOCUMENT} from '@angular/common'; -/** - * Node type of element nodes. Used instead of Node.ELEMENT_NODE - * which is unsupported in Universal. - * @docs-private - */ -const ELEMENT_NODE_TYPE = 1; /** * Class that allows for trapping focus within a DOM element. @@ -229,7 +223,7 @@ export class FocusTrap { let children = root.children || root.childNodes; for (let i = 0; i < children.length; i++) { - let tabbableChild = children[i].nodeType === ELEMENT_NODE_TYPE ? + let tabbableChild = children[i].nodeType === this._document.ELEMENT_NODE ? this._getFirstTabbableElement(children[i] as HTMLElement) : null; @@ -251,7 +245,7 @@ export class FocusTrap { let children = root.children || root.childNodes; for (let i = children.length - 1; i >= 0; i--) { - let tabbableChild = children[i].nodeType === ELEMENT_NODE_TYPE ? + let tabbableChild = children[i].nodeType === this._document.ELEMENT_NODE ? this._getLastTabbableElement(children[i] as HTMLElement) : null; diff --git a/src/lib/icon/icon-registry.ts b/src/lib/icon/icon-registry.ts index e0174b9870f5..f75db9de8685 100644 --- a/src/lib/icon/icon-registry.ts +++ b/src/lib/icon/icon-registry.ts @@ -78,6 +78,8 @@ class SvgIconConfig { */ @Injectable() export class MatIconRegistry { + private _document: Document; + /** * URLs and cached SVG elements for individual icons. Keys are of the format "[namespace]:[icon]". */ @@ -108,8 +110,9 @@ export class MatIconRegistry { constructor( @Optional() private _httpClient: HttpClient, private _sanitizer: DomSanitizer, - @Optional() @Inject(DOCUMENT) private _document?: any) { + @Optional() @Inject(DOCUMENT) document?: any) { // TODO(crisbeto): make _document required next major release. + this._document = document; } /** @@ -438,8 +441,7 @@ export class MatIconRegistry { let svg = this._svgElementFromString(''); for (let i = 0; i < element.childNodes.length; i++) { - // Note: 1 corresponds to `Node.ELEMENT_NODE` which we can't use in Universal. - if (element.childNodes[i].nodeType === 1) { + if (element.childNodes[i].nodeType === this._document.ELEMENT_NODE) { svg.appendChild(element.childNodes[i].cloneNode(true)); } }