From bf95d4afcc9b2b28f1ef3bc4256ed76d61a68486 Mon Sep 17 00:00:00 2001 From: Kristiyan Kostadinov Date: Wed, 13 Jan 2021 21:25:20 +0100 Subject: [PATCH] fix(material/dialog): content directives picking up wrong ref for stacked mixed type dialogs The dialog content directives try to resolve their closest dialog using DI, and if it fails (e.g. inside a template dialog), they fall back to resolving it through the DOM. This becomes problematic if the `ng-template` was declared inside another dialog component, because it'll pick up the declaration dialog, not the one that the button exists in. These changes remove the resolution using DI and switch to only going through the DOM. Fixes #21554. --- .../mdc-dialog/dialog-content-directives.ts | 38 ++++++++-------- .../mdc-dialog/dialog.spec.ts | 39 +++++++++++++++++ .../dialog/dialog-content-directives.ts | 43 ++++++++++--------- src/material/dialog/dialog.spec.ts | 39 +++++++++++++++++ tools/public_api_guard/material/dialog.md | 9 ++-- 5 files changed, 125 insertions(+), 43 deletions(-) diff --git a/src/material-experimental/mdc-dialog/dialog-content-directives.ts b/src/material-experimental/mdc-dialog/dialog-content-directives.ts index 3daa9f0db283..470beabda527 100644 --- a/src/material-experimental/mdc-dialog/dialog-content-directives.ts +++ b/src/material-experimental/mdc-dialog/dialog-content-directives.ts @@ -9,10 +9,10 @@ import { Directive, ElementRef, + Inject, Input, OnChanges, OnInit, - Optional, SimpleChanges, } from '@angular/core'; import {_closeDialogVia} from '@angular/material/dialog'; @@ -47,23 +47,23 @@ export class MatDialogClose implements OnInit, OnChanges { @Input('matDialogClose') _matDialogClose: any; + /** Reference to the dialog that the close button is placed inside of. */ + dialogRef: MatDialogRef; + constructor( - // The dialog title directive is always used in combination with a `MatDialogRef`. - // tslint:disable-next-line: lightweight-tokens - @Optional() public dialogRef: MatDialogRef, + /** + * @deprecated `_dialogRef` parameter to be removed. + * @breaking-change 12.0.0 + */ + @Inject(ElementRef) _dialogRef: any, private _elementRef: ElementRef, private _dialog: MatDialog, ) {} ngOnInit() { - if (!this.dialogRef) { - // When this directive is included in a dialog via TemplateRef (rather than being - // in a Component), the DialogRef isn't available via injection because embedded - // views cannot be given a custom injector. Instead, we look up the DialogRef by - // ID. This must occur in `onInit`, as the ID binding for the dialog container won't - // be resolved at constructor time. - this.dialogRef = getClosestDialog(this._elementRef, this._dialog.openDialogs)!; - } + // Always resolve the closest dialog using the DOM, rather than DI, because DI won't work for + // `TemplateRef`-based dialogs and it may give us wrong results for stacked ones (see #21554). + this.dialogRef = getClosestDialog(this._elementRef, this._dialog.openDialogs)!; } ngOnChanges(changes: SimpleChanges) { @@ -101,18 +101,20 @@ export class MatDialogClose implements OnInit, OnChanges { export class MatDialogTitle implements OnInit { @Input() id: string = `mat-mdc-dialog-title-${dialogElementUid++}`; + private _dialogRef: MatDialogRef; + constructor( - // The dialog title directive is always used in combination with a `MatDialogRef`. - // tslint:disable-next-line: lightweight-tokens - @Optional() private _dialogRef: MatDialogRef, + /** + * @deprecated `_dialogRef` parameter to be removed. + * @breaking-change 12.0.0 + */ + @Inject(ElementRef) _dialogRef: any, private _elementRef: ElementRef, private _dialog: MatDialog, ) {} ngOnInit() { - if (!this._dialogRef) { - this._dialogRef = getClosestDialog(this._elementRef, this._dialog.openDialogs)!; - } + this._dialogRef = getClosestDialog(this._elementRef, this._dialog.openDialogs)!; if (this._dialogRef) { Promise.resolve().then(() => { diff --git a/src/material-experimental/mdc-dialog/dialog.spec.ts b/src/material-experimental/mdc-dialog/dialog.spec.ts index dbe52a813f74..6c9e384e2b27 100644 --- a/src/material-experimental/mdc-dialog/dialog.spec.ts +++ b/src/material-experimental/mdc-dialog/dialog.spec.ts @@ -70,6 +70,7 @@ describe('MDC-based MatDialog', () => { DialogWithoutFocusableElements, DirectiveWithViewContainer, ComponentWithContentElementTemplateRef, + MixedTypeStackedDialog, ], providers: [ {provide: Location, useClass: SpyLocation}, @@ -764,6 +765,25 @@ describe('MDC-based MatDialog', () => { }, )); + it('should close the correct dialog when stacked and using a template from another dialog', fakeAsync(() => { + const dialogRef = dialog.open(MixedTypeStackedDialog); + viewContainerFixture.detectChanges(); + + dialogRef.componentInstance.open(); + viewContainerFixture.detectChanges(); + + expect(overlayContainerElement.textContent).toContain('Bottom'); + expect(overlayContainerElement.textContent).toContain('Top'); + + (overlayContainerElement.querySelector('.close') as HTMLButtonElement).click(); + flushMicrotasks(); + viewContainerFixture.detectChanges(); + tick(500); + + expect(overlayContainerElement.textContent).toContain('Bottom'); + expect(overlayContainerElement.textContent).not.toContain('Top'); + })); + describe('passing in data', () => { it('should be able to pass in data', () => { let config = {data: {stringParam: 'hello', dateParam: new Date()}}; @@ -2127,3 +2147,22 @@ class DialogWithoutFocusableElements {} encapsulation: ViewEncapsulation.ShadowDom, }) class ShadowDomComponent {} + +@Component({ + template: ` + Bottom + + Top + + + `, +}) +class MixedTypeStackedDialog { + @ViewChild(TemplateRef) template: TemplateRef; + + constructor(private _dialog: MatDialog) {} + + open() { + this._dialog.open(this.template); + } +} diff --git a/src/material/dialog/dialog-content-directives.ts b/src/material/dialog/dialog-content-directives.ts index 5c782353e57a..183820971e03 100644 --- a/src/material/dialog/dialog-content-directives.ts +++ b/src/material/dialog/dialog-content-directives.ts @@ -11,9 +11,9 @@ import { Input, OnChanges, OnInit, - Optional, SimpleChanges, ElementRef, + Inject, } from '@angular/core'; import {MatDialog} from './dialog'; import {_closeDialogVia, MatDialogRef} from './dialog-ref'; @@ -45,28 +45,27 @@ export class MatDialogClose implements OnInit, OnChanges { @Input('matDialogClose') _matDialogClose: any; + /** + * Reference to the containing dialog. + * @deprecated `dialogRef` property to become private. + * @breaking-change 13.0.0 + */ + dialogRef: MatDialogRef; + constructor( /** - * Reference to the containing dialog. - * @deprecated `dialogRef` property to become private. - * @breaking-change 13.0.0 + * @deprecated `_dialogRef` parameter to be removed. + * @breaking-change 12.0.0 */ - // The dialog title directive is always used in combination with a `MatDialogRef`. - // tslint:disable-next-line: lightweight-tokens - @Optional() public dialogRef: MatDialogRef, + @Inject(ElementRef) _dialogRef: any, private _elementRef: ElementRef, private _dialog: MatDialog, ) {} ngOnInit() { - if (!this.dialogRef) { - // When this directive is included in a dialog via TemplateRef (rather than being - // in a Component), the DialogRef isn't available via injection because embedded - // views cannot be given a custom injector. Instead, we look up the DialogRef by - // ID. This must occur in `onInit`, as the ID binding for the dialog container won't - // be resolved at constructor time. - this.dialogRef = getClosestDialog(this._elementRef, this._dialog.openDialogs)!; - } + // Always resolve the closest dialog using the DOM, rather than DI, because DI won't work for + // `TemplateRef`-based dialogs and it may give us wrong results for stacked ones (see #21554). + this.dialogRef = getClosestDialog(this._elementRef, this._dialog.openDialogs)!; } ngOnChanges(changes: SimpleChanges) { @@ -105,18 +104,20 @@ export class MatDialogTitle implements OnInit { /** Unique id for the dialog title. If none is supplied, it will be auto-generated. */ @Input() id: string = `mat-dialog-title-${dialogElementUid++}`; + private _dialogRef: MatDialogRef; + constructor( - // The dialog title directive is always used in combination with a `MatDialogRef`. - // tslint:disable-next-line: lightweight-tokens - @Optional() private _dialogRef: MatDialogRef, + /** + * @deprecated `_dialogRef` parameter to be removed. + * @breaking-change 12.0.0 + */ + @Inject(ElementRef) _dialogRef: any, private _elementRef: ElementRef, private _dialog: MatDialog, ) {} ngOnInit() { - if (!this._dialogRef) { - this._dialogRef = getClosestDialog(this._elementRef, this._dialog.openDialogs)!; - } + this._dialogRef = getClosestDialog(this._elementRef, this._dialog.openDialogs)!; if (this._dialogRef) { Promise.resolve().then(() => { diff --git a/src/material/dialog/dialog.spec.ts b/src/material/dialog/dialog.spec.ts index 1bcd69157086..2e8ddd37c902 100644 --- a/src/material/dialog/dialog.spec.ts +++ b/src/material/dialog/dialog.spec.ts @@ -70,6 +70,7 @@ describe('MatDialog', () => { DialogWithoutFocusableElements, DirectiveWithViewContainer, ComponentWithContentElementTemplateRef, + MixedTypeStackedDialog, ], providers: [ {provide: Location, useClass: SpyLocation}, @@ -824,6 +825,25 @@ describe('MatDialog', () => { }, )); + it('should close the correct dialog when stacked and using a template from another dialog', fakeAsync(() => { + const dialogRef = dialog.open(MixedTypeStackedDialog); + viewContainerFixture.detectChanges(); + + dialogRef.componentInstance.open(); + viewContainerFixture.detectChanges(); + + expect(overlayContainerElement.textContent).toContain('Bottom'); + expect(overlayContainerElement.textContent).toContain('Top'); + + (overlayContainerElement.querySelector('.close') as HTMLButtonElement).click(); + flushMicrotasks(); + viewContainerFixture.detectChanges(); + tick(500); + + expect(overlayContainerElement.textContent).toContain('Bottom'); + expect(overlayContainerElement.textContent).not.toContain('Top'); + })); + describe('passing in data', () => { it('should be able to pass in data', () => { const config = { @@ -2174,3 +2194,22 @@ class DialogWithoutFocusableElements {} encapsulation: ViewEncapsulation.ShadowDom, }) class ShadowDomComponent {} + +@Component({ + template: ` + Bottom + + Top + + + `, +}) +class MixedTypeStackedDialog { + @ViewChild(TemplateRef) template: TemplateRef; + + constructor(private _dialog: MatDialog) {} + + open() { + this._dialog.open(this.template); + } +} diff --git a/tools/public_api_guard/material/dialog.md b/tools/public_api_guard/material/dialog.md index 35da0a953e73..84f6c4cbad76 100644 --- a/tools/public_api_guard/material/dialog.md +++ b/tools/public_api_guard/material/dialog.md @@ -132,7 +132,7 @@ export abstract class _MatDialogBase implemen // @public export class MatDialogClose implements OnInit, OnChanges { constructor( - dialogRef: MatDialogRef, _elementRef: ElementRef, _dialog: MatDialog); + _dialogRef: any, _elementRef: ElementRef, _dialog: MatDialog); ariaLabel: string; // @deprecated dialogRef: MatDialogRef; @@ -149,7 +149,7 @@ export class MatDialogClose implements OnInit, OnChanges { // (undocumented) static ɵdir: i0.ɵɵDirectiveDeclaration; // (undocumented) - static ɵfac: i0.ɵɵFactoryDeclaration; + static ɵfac: i0.ɵɵFactoryDeclaration; } // @public @@ -277,14 +277,15 @@ export const enum MatDialogState { // @public export class MatDialogTitle implements OnInit { - constructor(_dialogRef: MatDialogRef, _elementRef: ElementRef, _dialog: MatDialog); + constructor( + _dialogRef: any, _elementRef: ElementRef, _dialog: MatDialog); id: string; // (undocumented) ngOnInit(): void; // (undocumented) static ɵdir: i0.ɵɵDirectiveDeclaration; // (undocumented) - static ɵfac: i0.ɵɵFactoryDeclaration; + static ɵfac: i0.ɵɵFactoryDeclaration; } // @public