From de963dd4fbca0e55da8da431f20ebe3582c45a28 Mon Sep 17 00:00:00 2001 From: crisbeto Date: Sat, 23 Mar 2019 17:24:18 +0100 Subject: [PATCH] fix(dialog): aria-label not being removed if text content changes after init Currently we set an `aria-label` on the `mat-dialog-close` button if it's an icon button or it doesn't have text, however it wasn't accounting for the case where the text comes in at a later point. Fixes #15542. --- src/lib/dialog/BUILD.bazel | 2 + src/lib/dialog/dialog-content-directives.ts | 44 +++++++++++++++++++-- src/lib/dialog/dialog.spec.ts | 31 ++++++++++++++- tools/public_api_guard/lib/dialog.d.ts | 6 ++- 4 files changed, 76 insertions(+), 7 deletions(-) diff --git a/src/lib/dialog/BUILD.bazel b/src/lib/dialog/BUILD.bazel index d0d850c2ad2f..686168edf930 100644 --- a/src/lib/dialog/BUILD.bazel +++ b/src/lib/dialog/BUILD.bazel @@ -19,6 +19,7 @@ ng_module( "//src/cdk/keycodes", "//src/cdk/overlay", "//src/cdk/portal", + "//src/cdk/observers", "//src/lib/core", ], ) @@ -50,6 +51,7 @@ ng_test_library( "//src/cdk/overlay", "//src/cdk/scrolling", "//src/cdk/testing", + "//src/cdk/observers", ":dialog", ] ) diff --git a/src/lib/dialog/dialog-content-directives.ts b/src/lib/dialog/dialog-content-directives.ts index edca30a06c2b..8d48ea37e88a 100644 --- a/src/lib/dialog/dialog-content-directives.ts +++ b/src/lib/dialog/dialog-content-directives.ts @@ -14,7 +14,13 @@ import { Optional, SimpleChanges, ElementRef, + ChangeDetectorRef, + NgZone, + OnDestroy, } from '@angular/core'; +import {ContentObserver} from '@angular/cdk/observers'; +import {of as observableOf, Subscription} from 'rxjs'; +import {startWith} from 'rxjs/operators'; import {MatDialog} from './dialog'; import {MatDialogRef} from './dialog-ref'; @@ -33,7 +39,7 @@ let dialogElementUid = 0; 'type': 'button', // Prevents accidental form submits. } }) -export class MatDialogClose implements OnInit, OnChanges { +export class MatDialogClose implements OnInit, OnChanges, OnDestroy { /** Screenreader label for the button. */ @Input('aria-label') ariaLabel: string = 'Close dialog'; @@ -48,10 +54,21 @@ export class MatDialogClose implements OnInit, OnChanges { */ _hasAriaLabel?: boolean; + /** Subscription to changes in the button's content. */ + private _contentChanges = Subscription.EMPTY; + constructor( @Optional() public dialogRef: MatDialogRef, private _elementRef: ElementRef, - private _dialog: MatDialog) {} + private _dialog: MatDialog, + + /** + * @deprecated @breaking-change 9.0.0 + * _contentObserver, _ngZone and _changeDetectorRef parameters to be made required. + */ + private _contentObserver?: ContentObserver, + private _ngZone?: NgZone, + private _changeDetectorRef?: ChangeDetectorRef) {} ngOnInit() { if (!this.dialogRef) { @@ -69,8 +86,23 @@ export class MatDialogClose implements OnInit, OnChanges { if (element.hasAttribute('mat-icon-button')) { this._hasAriaLabel = true; } else { - const buttonTextContent = element.textContent; - this._hasAriaLabel = !buttonTextContent || buttonTextContent.trim().length === 0; + // @breaking-change 9.0.0 Remove null checks for _contentObserver, _ngZone and + // _changeDetectorRef once they are made into required parameters. + const contentChangesStream = this._contentObserver ? + this._contentObserver.observe(this._elementRef) : observableOf(); + + // Toggle whether the button should have an aria-label, based on its content. + this._contentChanges = contentChangesStream.pipe(startWith(null)).subscribe(() => { + const buttonTextContent = element.textContent; + this._hasAriaLabel = !buttonTextContent || buttonTextContent.trim().length === 0; + + // The content observer runs outside the `NgZone` so we need to bring it back in. + if (this._ngZone && this._changeDetectorRef) { + this._ngZone.run(() => { + this._changeDetectorRef!.markForCheck(); + }); + } + }); } } } @@ -87,6 +119,10 @@ export class MatDialogClose implements OnInit, OnChanges { this._hasAriaLabel = !!changes['ariaLabel'].currentValue; } } + + ngOnDestroy() { + this._contentChanges.unsubscribe(); + } } /** diff --git a/src/lib/dialog/dialog.spec.ts b/src/lib/dialog/dialog.spec.ts index 246a1863ba0f..10b93fa42c83 100644 --- a/src/lib/dialog/dialog.spec.ts +++ b/src/lib/dialog/dialog.spec.ts @@ -36,6 +36,7 @@ import { MAT_DIALOG_DEFAULT_OPTIONS } from './index'; import {Subject} from 'rxjs'; +import {MutationObserverFactory} from '@angular/cdk/observers'; describe('MatDialog', () => { @@ -47,8 +48,10 @@ describe('MatDialog', () => { let testViewContainerRef: ViewContainerRef; let viewContainerFixture: ComponentFixture; let mockLocation: SpyLocation; + let mutationCallbacks: Function[]; beforeEach(fakeAsync(() => { + mutationCallbacks = []; TestBed.configureTestingModule({ imports: [MatDialogModule, DialogTestModule], providers: [ @@ -56,6 +59,19 @@ describe('MatDialog', () => { {provide: ScrollDispatcher, useFactory: () => ({ scrolled: () => scrolledSubject.asObservable() })}, + { + provide: MutationObserverFactory, + useValue: { + create: (callback: Function) => { + mutationCallbacks.push(callback); + + return { + observe: () => {}, + disconnect: () => {} + }; + } + } + } ], }); @@ -1148,6 +1164,14 @@ describe('MatDialog', () => { expect(button.getAttribute('aria-label')).toBeTruthy(); })); + it('should not have an aria-label if a button has bound text', fakeAsync(() => { + let button = overlayContainerElement.querySelector('.close-with-text-binding')!; + mutationCallbacks.forEach(callback => callback()); + viewContainerFixture.detectChanges(); + + expect(button.getAttribute('aria-label')).toBeFalsy(); + })); + it('should not have an aria-label if a button has text', fakeAsync(() => { let button = overlayContainerElement.querySelector('[mat-dialog-close]')!; expect(button.getAttribute('aria-label')).toBeFalsy(); @@ -1511,6 +1535,7 @@ class PizzaMsg { + +