Skip to content

Commit 8b7a29b

Browse files
crisbetocrisbeto
committed
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.
1 parent 4a943f1 commit 8b7a29b

File tree

5 files changed

+131
-42
lines changed

5 files changed

+131
-42
lines changed

‎src/material-experimental/mdc-dialog/dialog-content-directives.ts

Lines changed: 22 additions & 20 deletions
Original file line numberDiff line numberDiff line change
@@ -9,10 +9,10 @@
99
import {
1010
Directive,
1111
ElementRef,
12+
Inject,
1213
Input,
1314
OnChanges,
1415
OnInit,
15-
Optional,
1616
SimpleChanges,
1717
} from '@angular/core';
1818
import {_closeDialogVia} from '@angular/material/dialog';
@@ -47,22 +47,22 @@ export class MatDialogClose implements OnInit, OnChanges {
4747

4848
@Input('matDialogClose') _matDialogClose: any;
4949

50+
/** Reference to the dialog that the close button is placed inside of. */
51+
dialogRef: MatDialogRef<any>;
52+
5053
constructor(
51-
// The dialog title directive is always used in combination with a `MatDialogRef`.
52-
// tslint:disable-next-line: lightweight-tokens
53-
@Optional() public dialogRef: MatDialogRef<any>,
54-
private _elementRef: ElementRef<HTMLElement>,
55-
private _dialog: MatDialog) {}
54+
/**
55+
* @deprecated `_dialogRef` parameter to be removed.
56+
* @breaking-change 12.0.0
57+
*/
58+
@Inject(ElementRef) _dialogRef: any,
59+
private _elementRef: ElementRef<HTMLElement>,
60+
private _dialog: MatDialog) {}
5661

5762
ngOnInit() {
58-
if (!this.dialogRef) {
59-
// When this directive is included in a dialog via TemplateRef (rather than being
60-
// in a Component), the DialogRef isn't available via injection because embedded
61-
// views cannot be given a custom injector. Instead, we look up the DialogRef by
62-
// ID. This must occur in `onInit`, as the ID binding for the dialog container won't
63-
// be resolved at constructor time.
64-
this.dialogRef = getClosestDialog(this._elementRef, this._dialog.openDialogs)!;
65-
}
63+
// Always resolve the closest dialog using the DOM, rather than DI, because DI won't work for
64+
// `TemplateRef`-based dialogs and it may give us wrong results for stacked ones (see #21554).
65+
this.dialogRef = getClosestDialog(this._elementRef, this._dialog.openDialogs)!;
6666
}
6767

6868
ngOnChanges(changes: SimpleChanges) {
@@ -97,17 +97,19 @@ export class MatDialogClose implements OnInit, OnChanges {
9797
export class MatDialogTitle implements OnInit {
9898
@Input() id: string = `mat-mdc-dialog-title-${dialogElementUid++}`;
9999

100+
private _dialogRef: MatDialogRef<unknown>;
101+
100102
constructor(
101-
// The dialog title directive is always used in combination with a `MatDialogRef`.
102-
// tslint:disable-next-line: lightweight-tokens
103-
@Optional() private _dialogRef: MatDialogRef<any>,
103+
/**
104+
* @deprecated `_dialogRef` parameter to be removed.
105+
* @breaking-change 12.0.0
106+
*/
107+
@Inject(ElementRef) _dialogRef: any,
104108
private _elementRef: ElementRef<HTMLElement>,
105109
private _dialog: MatDialog) {}
106110

107111
ngOnInit() {
108-
if (!this._dialogRef) {
109-
this._dialogRef = getClosestDialog(this._elementRef, this._dialog.openDialogs)!;
110-
}
112+
this._dialogRef = getClosestDialog(this._elementRef, this._dialog.openDialogs)!;
111113

112114
if (this._dialogRef) {
113115
Promise.resolve().then(() => {

‎src/material-experimental/mdc-dialog/dialog.spec.ts

Lines changed: 42 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -742,6 +742,26 @@ describe('MDC-based MatDialog', () => {
742742
expect(resolver.resolveComponentFactory).toHaveBeenCalled();
743743
}));
744744

745+
it('should close the correct dialog when stacked and using a template from another dialog',
746+
fakeAsync(() => {
747+
const dialogRef = dialog.open(MixedTypeStackedDialog);
748+
viewContainerFixture.detectChanges();
749+
750+
dialogRef.componentInstance.open();
751+
viewContainerFixture.detectChanges();
752+
753+
expect(overlayContainerElement.textContent).toContain('Bottom');
754+
expect(overlayContainerElement.textContent).toContain('Top');
755+
756+
(overlayContainerElement.querySelector('.close') as HTMLButtonElement).click();
757+
flushMicrotasks();
758+
viewContainerFixture.detectChanges();
759+
tick(500);
760+
761+
expect(overlayContainerElement.textContent).toContain('Bottom');
762+
expect(overlayContainerElement.textContent).not.toContain('Top');
763+
}));
764+
745765
describe('passing in data', () => {
746766
it('should be able to pass in data', () => {
747767
let config = {data: {stringParam: 'hello', dateParam: new Date()}};
@@ -1905,6 +1925,26 @@ class DialogWithoutFocusableElements {
19051925
})
19061926
class ShadowDomComponent {}
19071927

1928+
@Component({
1929+
template: `
1930+
Bottom
1931+
<ng-template>
1932+
Top
1933+
<button class="close" mat-dialog-close>Close</button>
1934+
</ng-template>
1935+
`,
1936+
})
1937+
class MixedTypeStackedDialog {
1938+
@ViewChild(TemplateRef) template: TemplateRef<any>;
1939+
1940+
constructor(private _dialog: MatDialog) {}
1941+
1942+
open() {
1943+
this._dialog.open(this.template);
1944+
}
1945+
}
1946+
1947+
19081948
// Create a real (non-test) NgModule as a workaround for
19091949
// https://github.com/angular/angular/issues/10760
19101950
const TEST_DIRECTIVES = [
@@ -1918,6 +1958,7 @@ const TEST_DIRECTIVES = [
19181958
DialogWithoutFocusableElements,
19191959
ComponentWithContentElementTemplateRef,
19201960
ShadowDomComponent,
1961+
MixedTypeStackedDialog,
19211962
];
19221963

19231964
@NgModule({
@@ -1931,6 +1972,7 @@ const TEST_DIRECTIVES = [
19311972
ContentElementDialog,
19321973
DialogWithInjectedData,
19331974
DialogWithoutFocusableElements,
1975+
MixedTypeStackedDialog,
19341976
],
19351977
})
19361978
class DialogTestModule {

‎src/material/dialog/dialog-content-directives.ts

Lines changed: 20 additions & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -11,9 +11,9 @@ import {
1111
Input,
1212
OnChanges,
1313
OnInit,
14-
Optional,
1514
SimpleChanges,
1615
ElementRef,
16+
Inject,
1717
} from '@angular/core';
1818
import {MatDialog} from './dialog';
1919
import {_closeDialogVia, MatDialogRef} from './dialog-ref';
@@ -45,22 +45,22 @@ export class MatDialogClose implements OnInit, OnChanges {
4545

4646
@Input('matDialogClose') _matDialogClose: any;
4747

48+
/** Reference to the dialog that the close button is placed inside of. */
49+
dialogRef: MatDialogRef<any>;
50+
4851
constructor(
49-
// The dialog title directive is always used in combination with a `MatDialogRef`.
50-
// tslint:disable-next-line: lightweight-tokens
51-
@Optional() public dialogRef: MatDialogRef<any>,
52+
/**
53+
* @deprecated `_dialogRef` parameter to be removed.
54+
* @breaking-change 12.0.0
55+
*/
56+
@Inject(ElementRef) _dialogRef: any,
5257
private _elementRef: ElementRef<HTMLElement>,
5358
private _dialog: MatDialog) {}
5459

5560
ngOnInit() {
56-
if (!this.dialogRef) {
57-
// When this directive is included in a dialog via TemplateRef (rather than being
58-
// in a Component), the DialogRef isn't available via injection because embedded
59-
// views cannot be given a custom injector. Instead, we look up the DialogRef by
60-
// ID. This must occur in `onInit`, as the ID binding for the dialog container won't
61-
// be resolved at constructor time.
62-
this.dialogRef = getClosestDialog(this._elementRef, this._dialog.openDialogs)!;
63-
}
61+
// Always resolve the closest dialog using the DOM, rather than DI, because DI won't work for
62+
// `TemplateRef`-based dialogs and it may give us wrong results for stacked ones (see #21554).
63+
this.dialogRef = getClosestDialog(this._elementRef, this._dialog.openDialogs)!;
6464
}
6565

6666
ngOnChanges(changes: SimpleChanges) {
@@ -95,17 +95,19 @@ export class MatDialogClose implements OnInit, OnChanges {
9595
export class MatDialogTitle implements OnInit {
9696
@Input() id: string = `mat-dialog-title-${dialogElementUid++}`;
9797

98+
private _dialogRef: MatDialogRef<unknown>;
99+
98100
constructor(
99-
// The dialog title directive is always used in combination with a `MatDialogRef`.
100-
// tslint:disable-next-line: lightweight-tokens
101-
@Optional() private _dialogRef: MatDialogRef<any>,
101+
/**
102+
* @deprecated `_dialogRef` parameter to be removed.
103+
* @breaking-change 12.0.0
104+
*/
105+
@Inject(ElementRef) _dialogRef: any,
102106
private _elementRef: ElementRef<HTMLElement>,
103107
private _dialog: MatDialog) {}
104108

105109
ngOnInit() {
106-
if (!this._dialogRef) {
107-
this._dialogRef = getClosestDialog(this._elementRef, this._dialog.openDialogs)!;
108-
}
110+
this._dialogRef = getClosestDialog(this._elementRef, this._dialog.openDialogs)!;
109111

110112
if (this._dialogRef) {
111113
Promise.resolve().then(() => {

‎src/material/dialog/dialog.spec.ts

Lines changed: 41 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -800,6 +800,26 @@ describe('MatDialog', () => {
800800
expect(resolver.resolveComponentFactory).toHaveBeenCalled();
801801
}));
802802

803+
it('should close the correct dialog when stacked and using a template from another dialog',
804+
fakeAsync(() => {
805+
const dialogRef = dialog.open(MixedTypeStackedDialog);
806+
viewContainerFixture.detectChanges();
807+
808+
dialogRef.componentInstance.open();
809+
viewContainerFixture.detectChanges();
810+
811+
expect(overlayContainerElement.textContent).toContain('Bottom');
812+
expect(overlayContainerElement.textContent).toContain('Top');
813+
814+
(overlayContainerElement.querySelector('.close') as HTMLButtonElement).click();
815+
flushMicrotasks();
816+
viewContainerFixture.detectChanges();
817+
tick(500);
818+
819+
expect(overlayContainerElement.textContent).toContain('Bottom');
820+
expect(overlayContainerElement.textContent).not.toContain('Top');
821+
}));
822+
803823
describe('passing in data', () => {
804824
it('should be able to pass in data', () => {
805825
let config = {
@@ -1981,6 +2001,25 @@ class DialogWithoutFocusableElements {}
19812001
})
19822002
class ShadowDomComponent {}
19832003

2004+
@Component({
2005+
template: `
2006+
Bottom
2007+
<ng-template>
2008+
Top
2009+
<button class="close" mat-dialog-close>Close</button>
2010+
</ng-template>
2011+
`,
2012+
})
2013+
class MixedTypeStackedDialog {
2014+
@ViewChild(TemplateRef) template: TemplateRef<any>;
2015+
2016+
constructor(private _dialog: MatDialog) {}
2017+
2018+
open() {
2019+
this._dialog.open(this.template);
2020+
}
2021+
}
2022+
19842023
// Create a real (non-test) NgModule as a workaround for
19852024
// https://github.com/angular/angular/issues/10760
19862025
const TEST_DIRECTIVES = [
@@ -1994,6 +2033,7 @@ const TEST_DIRECTIVES = [
19942033
DialogWithoutFocusableElements,
19952034
ComponentWithContentElementTemplateRef,
19962035
ShadowDomComponent,
2036+
MixedTypeStackedDialog,
19972037
];
19982038

19992039
@NgModule({
@@ -2007,6 +2047,7 @@ const TEST_DIRECTIVES = [
20072047
ContentElementDialog,
20082048
DialogWithInjectedData,
20092049
DialogWithoutFocusableElements,
2050+
MixedTypeStackedDialog,
20102051
],
20112052
})
20122053
class DialogTestModule { }

‎tools/public_api_guard/material/dialog.d.ts

Lines changed: 6 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -88,12 +88,13 @@ export declare class MatDialogClose implements OnInit, OnChanges {
8888
dialogRef: MatDialogRef<any>;
8989
dialogResult: any;
9090
type: 'submit' | 'button' | 'reset';
91-
constructor(dialogRef: MatDialogRef<any>, _elementRef: ElementRef<HTMLElement>, _dialog: MatDialog);
91+
constructor(
92+
_dialogRef: any, _elementRef: ElementRef<HTMLElement>, _dialog: MatDialog);
9293
_onButtonClick(event: MouseEvent): void;
9394
ngOnChanges(changes: SimpleChanges): void;
9495
ngOnInit(): void;
9596
static ɵdir: i0.ɵɵDirectiveDefWithMeta<MatDialogClose, "[mat-dialog-close], [matDialogClose]", ["matDialogClose"], { "ariaLabel": "aria-label"; "type": "type"; "dialogResult": "mat-dialog-close"; "_matDialogClose": "matDialogClose"; }, {}, never>;
96-
static ɵfac: i0.ɵɵFactoryDef<MatDialogClose, [{ optional: true; }, null, null]>;
97+
static ɵfac: i0.ɵɵFactoryDef<MatDialogClose, never>;
9798
}
9899

99100
export declare class MatDialogConfig<D = any> {
@@ -169,10 +170,11 @@ export declare const enum MatDialogState {
169170

170171
export declare class MatDialogTitle implements OnInit {
171172
id: string;
172-
constructor(_dialogRef: MatDialogRef<any>, _elementRef: ElementRef<HTMLElement>, _dialog: MatDialog);
173+
constructor(
174+
_dialogRef: any, _elementRef: ElementRef<HTMLElement>, _dialog: MatDialog);
173175
ngOnInit(): void;
174176
static ɵdir: i0.ɵɵDirectiveDefWithMeta<MatDialogTitle, "[mat-dialog-title], [matDialogTitle]", ["matDialogTitle"], { "id": "id"; }, {}, never>;
175-
static ɵfac: i0.ɵɵFactoryDef<MatDialogTitle, [{ optional: true; }, null, null]>;
177+
static ɵfac: i0.ɵɵFactoryDef<MatDialogTitle, never>;
176178
}
177179

178180
export declare function throwMatDialogContentAlreadyAttachedError(): void;

0 commit comments

Comments
 (0)