fix(material/dialog): content directives picking up wrong ref for stacked mixed type dialogs

crisbeto · crisbeto · commit bf95d4afcc9b · 2022-01-03T11:23:55.000+02:00
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.
diff --git 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<any>;
+
   constructor(
-    // The dialog title directive is always used in combination with a `MatDialogRef`.
-    // tslint:disable-next-line: lightweight-tokens
-    @Optional() public dialogRef: MatDialogRef<any>,
+    /**
+     * @deprecated `_dialogRef` parameter to be removed.
+     * @breaking-change 12.0.0
+     */
+    @Inject(ElementRef) _dialogRef: any,
     private _elementRef: ElementRef<HTMLElement>,
     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<unknown>;
+
   constructor(
-    // The dialog title directive is always used in combination with a `MatDialogRef`.
-    // tslint:disable-next-line: lightweight-tokens
-    @Optional() private _dialogRef: MatDialogRef<any>,
+    /**
+     * @deprecated `_dialogRef` parameter to be removed.
+     * @breaking-change 12.0.0
+     */
+    @Inject(ElementRef) _dialogRef: any,
     private _elementRef: ElementRef<HTMLElement>,
     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
@@ -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
+    <ng-template>
+      Top
+      <button class="close" mat-dialog-close>Close</button>
+    </ng-template>
+  `,
+})
+class MixedTypeStackedDialog {
+  @ViewChild(TemplateRef) template: TemplateRef<any>;
+
+  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
@@ -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<any>;
+
   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<any>,
+    @Inject(ElementRef) _dialogRef: any,
     private _elementRef: ElementRef<HTMLElement>,
     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<unknown>;
+
   constructor(
-    // The dialog title directive is always used in combination with a `MatDialogRef`.
-    // tslint:disable-next-line: lightweight-tokens
-    @Optional() private _dialogRef: MatDialogRef<any>,
+    /**
+     * @deprecated `_dialogRef` parameter to be removed.
+     * @breaking-change 12.0.0
+     */
+    @Inject(ElementRef) _dialogRef: any,
     private _elementRef: ElementRef<HTMLElement>,
     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
@@ -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
+    <ng-template>
+      Top
+      <button class="close" mat-dialog-close>Close</button>
+    </ng-template>
+  `,
+})
+class MixedTypeStackedDialog {
+  @ViewChild(TemplateRef) template: TemplateRef<any>;
+
+  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
@@ -132,7 +132,7 @@ export abstract class _MatDialogBase<C extends _MatDialogContainerBase> implemen
 // @public
 export class MatDialogClose implements OnInit, OnChanges {
     constructor(
-    dialogRef: MatDialogRef<any>, _elementRef: ElementRef<HTMLElement>, _dialog: MatDialog);
+    _dialogRef: any, _elementRef: ElementRef<HTMLElement>, _dialog: MatDialog);
     ariaLabel: string;
     // @deprecated
     dialogRef: MatDialogRef<any>;
@@ -149,7 +149,7 @@ export class MatDialogClose implements OnInit, OnChanges {
     // (undocumented)
     static ɵdir: i0.ɵɵDirectiveDeclaration<MatDialogClose, "[mat-dialog-close], [matDialogClose]", ["matDialogClose"], { "ariaLabel": "aria-label"; "type": "type"; "dialogResult": "mat-dialog-close"; "_matDialogClose": "matDialogClose"; }, {}, never>;
     // (undocumented)
-    static ɵfac: i0.ɵɵFactoryDeclaration<MatDialogClose, [{ optional: true; }, null, null]>;
+    static ɵfac: i0.ɵɵFactoryDeclaration<MatDialogClose, never>;
 }
 
 // @public
@@ -277,14 +277,15 @@ export const enum MatDialogState {
 
 // @public
 export class MatDialogTitle implements OnInit {
-    constructor(_dialogRef: MatDialogRef<any>, _elementRef: ElementRef<HTMLElement>, _dialog: MatDialog);
+    constructor(
+    _dialogRef: any, _elementRef: ElementRef<HTMLElement>, _dialog: MatDialog);
     id: string;
     // (undocumented)
     ngOnInit(): void;
     // (undocumented)
     static ɵdir: i0.ɵɵDirectiveDeclaration<MatDialogTitle, "[mat-dialog-title], [matDialogTitle]", ["matDialogTitle"], { "id": "id"; }, {}, never>;
     // (undocumented)
-    static ɵfac: i0.ɵɵFactoryDeclaration<MatDialogTitle, [{ optional: true; }, null, null]>;
+    static ɵfac: i0.ɵɵFactoryDeclaration<MatDialogTitle, never>;
 }
 
 // @public
