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

crisbeto · crisbeto · commit a6502dfd2255 · 2021-01-13T21:25:20.000+01: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,22 +47,22 @@ 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>,
-      private _elementRef: ElementRef<HTMLElement>,
-      private _dialog: MatDialog) {}
+    /**
+     * @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) {
@@ -97,17 +97,19 @@ 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
@@ -721,6 +721,26 @@ describe('MDC-based MatDialog', () => {
        expect(resolver.resolveComponentFactory).toHaveBeenCalled();
      }));
 
+  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()}};
@@ -1852,6 +1872,26 @@ class DialogWithInjectedData {
 class DialogWithoutFocusableElements {
 }
 
+@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);
+  }
+}
+
+
 // Create a real (non-test) NgModule as a workaround for
 // https://github.com/angular/angular/issues/10760
 const TEST_DIRECTIVES = [
@@ -1864,6 +1904,7 @@ const TEST_DIRECTIVES = [
   DialogWithInjectedData,
   DialogWithoutFocusableElements,
   ComponentWithContentElementTemplateRef,
+  MixedTypeStackedDialog,
 ];
 
 @NgModule({
@@ -1877,6 +1918,7 @@ const TEST_DIRECTIVES = [
     ContentElementDialog,
     DialogWithInjectedData,
     DialogWithoutFocusableElements,
+    MixedTypeStackedDialog,
   ],
 })
 class DialogTestModule {
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,22 +45,22 @@ 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) {
@@ -95,17 +95,19 @@ export class MatDialogClose implements OnInit, OnChanges {
 export class MatDialogTitle implements OnInit {
   @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
@@ -780,6 +780,26 @@ describe('MatDialog', () => {
       expect(resolver.resolveComponentFactory).toHaveBeenCalled();
     }));
 
+  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 = {
@@ -1929,6 +1949,25 @@ class DialogWithInjectedData {
 @Component({template: '<p>Pasta</p>'})
 class DialogWithoutFocusableElements {}
 
+@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);
+  }
+}
+
 // Create a real (non-test) NgModule as a workaround for
 // https://github.com/angular/angular/issues/10760
 const TEST_DIRECTIVES = [
@@ -1941,6 +1980,7 @@ const TEST_DIRECTIVES = [
   DialogWithInjectedData,
   DialogWithoutFocusableElements,
   ComponentWithContentElementTemplateRef,
+  MixedTypeStackedDialog,
 ];
 
 @NgModule({
@@ -1954,6 +1994,7 @@ const TEST_DIRECTIVES = [
     ContentElementDialog,
     DialogWithInjectedData,
     DialogWithoutFocusableElements,
+    MixedTypeStackedDialog,
   ],
 })
 class DialogTestModule { }
diff --git a/tools/public_api_guard/material/dialog.d.ts b/tools/public_api_guard/material/dialog.d.ts
@@ -88,12 +88,13 @@ export declare class MatDialogClose implements OnInit, OnChanges {
     dialogRef: MatDialogRef<any>;
     dialogResult: any;
     type: 'submit' | 'button' | 'reset';
-    constructor(dialogRef: MatDialogRef<any>, _elementRef: ElementRef<HTMLElement>, _dialog: MatDialog);
+    constructor(
+    _dialogRef: any, _elementRef: ElementRef<HTMLElement>, _dialog: MatDialog);
     _onButtonClick(event: MouseEvent): void;
     ngOnChanges(changes: SimpleChanges): void;
     ngOnInit(): void;
     static ɵdir: i0.ɵɵDirectiveDefWithMeta<MatDialogClose, "[mat-dialog-close], [matDialogClose]", ["matDialogClose"], { "ariaLabel": "aria-label"; "type": "type"; "dialogResult": "mat-dialog-close"; "_matDialogClose": "matDialogClose"; }, {}, never>;
-    static ɵfac: i0.ɵɵFactoryDef<MatDialogClose, [{ optional: true; }, null, null]>;
+    static ɵfac: i0.ɵɵFactoryDef<MatDialogClose, never>;
 }
 
 export declare class MatDialogConfig<D = any> {
@@ -169,10 +170,11 @@ export declare const enum MatDialogState {
 
 export declare class MatDialogTitle implements OnInit {
     id: string;
-    constructor(_dialogRef: MatDialogRef<any>, _elementRef: ElementRef<HTMLElement>, _dialog: MatDialog);
+    constructor(
+    _dialogRef: any, _elementRef: ElementRef<HTMLElement>, _dialog: MatDialog);
     ngOnInit(): void;
     static ɵdir: i0.ɵɵDirectiveDefWithMeta<MatDialogTitle, "[mat-dialog-title], [matDialogTitle]", ["matDialogTitle"], { "id": "id"; }, {}, never>;
-    static ɵfac: i0.ɵɵFactoryDef<MatDialogTitle, [{ optional: true; }, null, null]>;
+    static ɵfac: i0.ɵɵFactoryDef<MatDialogTitle, never>;
 }
 
 export declare function throwMatDialogContentAlreadyAttachedError(): void;
