Skip to content

Commit e701706

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 d090617 commit e701706

File tree

5 files changed

+132
-46
lines changed

5 files changed

+132
-46
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: 22 additions & 22 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,28 +45,26 @@ export class MatDialogClose implements OnInit, OnChanges {
4545

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

48-
constructor(
48+
/**
49+
* Reference to the containing dialog.
50+
* @deprecated `dialogRef` property to become private.
51+
* @breaking-change 13.0.0
52+
*/
53+
dialogRef: MatDialogRef<any>;
4954

55+
constructor(
5056
/**
51-
* Reference to the containing dialog.
52-
* @deprecated `dialogRef` property to become private.
53-
* @breaking-change 13.0.0
57+
* @deprecated `_dialogRef` parameter to be removed.
58+
* @breaking-change 12.0.0
5459
*/
55-
// The dialog title directive is always used in combination with a `MatDialogRef`.
56-
// tslint:disable-next-line: lightweight-tokens
57-
@Optional() public dialogRef: MatDialogRef<any>,
60+
@Inject(ElementRef) _dialogRef: any,
5861
private _elementRef: ElementRef<HTMLElement>,
5962
private _dialog: MatDialog) {}
6063

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

7270
ngOnChanges(changes: SimpleChanges) {
@@ -102,17 +100,19 @@ export class MatDialogTitle implements OnInit {
102100
/** Unique id for the dialog title. If none is supplied, it will be auto-generated. */
103101
@Input() id: string = `mat-dialog-title-${dialogElementUid++}`;
104102

103+
private _dialogRef: MatDialogRef<unknown>;
104+
105105
constructor(
106-
// The dialog title directive is always used in combination with a `MatDialogRef`.
107-
// tslint:disable-next-line: lightweight-tokens
108-
@Optional() private _dialogRef: MatDialogRef<any>,
106+
/**
107+
* @deprecated `_dialogRef` parameter to be removed.
108+
* @breaking-change 12.0.0
109+
*/
110+
@Inject(ElementRef) _dialogRef: any,
109111
private _elementRef: ElementRef<HTMLElement>,
110112
private _dialog: MatDialog) {}
111113

112114
ngOnInit() {
113-
if (!this._dialogRef) {
114-
this._dialogRef = getClosestDialog(this._elementRef, this._dialog.openDialogs)!;
115-
}
115+
this._dialogRef = getClosestDialog(this._elementRef, this._dialog.openDialogs)!;
116116

117117
if (this._dialogRef) {
118118
Promise.resolve().then(() => {

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

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

815+
it('should close the correct dialog when stacked and using a template from another dialog',
816+
fakeAsync(() => {
817+
const dialogRef = dialog.open(MixedTypeStackedDialog);
818+
viewContainerFixture.detectChanges();
819+
820+
dialogRef.componentInstance.open();
821+
viewContainerFixture.detectChanges();
822+
823+
expect(overlayContainerElement.textContent).toContain('Bottom');
824+
expect(overlayContainerElement.textContent).toContain('Top');
825+
826+
(overlayContainerElement.querySelector('.close') as HTMLButtonElement).click();
827+
flushMicrotasks();
828+
viewContainerFixture.detectChanges();
829+
tick(500);
830+
831+
expect(overlayContainerElement.textContent).toContain('Bottom');
832+
expect(overlayContainerElement.textContent).not.toContain('Top');
833+
}));
834+
815835
describe('passing in data', () => {
816836
it('should be able to pass in data', () => {
817837
const config = {
@@ -1998,6 +2018,25 @@ class DialogWithoutFocusableElements {}
19982018
})
19992019
class ShadowDomComponent {}
20002020

2021+
@Component({
2022+
template: `
2023+
Bottom
2024+
<ng-template>
2025+
Top
2026+
<button class="close" mat-dialog-close>Close</button>
2027+
</ng-template>
2028+
`,
2029+
})
2030+
class MixedTypeStackedDialog {
2031+
@ViewChild(TemplateRef) template: TemplateRef<any>;
2032+
2033+
constructor(private _dialog: MatDialog) {}
2034+
2035+
open() {
2036+
this._dialog.open(this.template);
2037+
}
2038+
}
2039+
20012040
// Create a real (non-test) NgModule as a workaround for
20022041
// https://github.com/angular/angular/issues/10760
20032042
const TEST_DIRECTIVES = [
@@ -2011,6 +2050,7 @@ const TEST_DIRECTIVES = [
20112050
DialogWithoutFocusableElements,
20122051
ComponentWithContentElementTemplateRef,
20132052
ShadowDomComponent,
2053+
MixedTypeStackedDialog,
20142054
];
20152055

20162056
@NgModule({
@@ -2024,6 +2064,7 @@ const TEST_DIRECTIVES = [
20242064
ContentElementDialog,
20252065
DialogWithInjectedData,
20262066
DialogWithoutFocusableElements,
2067+
MixedTypeStackedDialog,
20272068
],
20282069
})
20292070
class DialogTestModule { }

‎tools/public_api_guard/material/dialog.md

Lines changed: 5 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -127,7 +127,7 @@ export abstract class _MatDialogBase<C extends _MatDialogContainerBase> implemen
127127
// @public
128128
export class MatDialogClose implements OnInit, OnChanges {
129129
constructor(
130-
dialogRef: MatDialogRef<any>, _elementRef: ElementRef<HTMLElement>, _dialog: MatDialog);
130+
_dialogRef: any, _elementRef: ElementRef<HTMLElement>, _dialog: MatDialog);
131131
ariaLabel: string;
132132
// @deprecated
133133
dialogRef: MatDialogRef<any>;
@@ -144,7 +144,7 @@ export class MatDialogClose implements OnInit, OnChanges {
144144
// (undocumented)
145145
static ɵdir: i0.ɵɵDirectiveDeclaration<MatDialogClose, "[mat-dialog-close], [matDialogClose]", ["matDialogClose"], { "ariaLabel": "aria-label"; "type": "type"; "dialogResult": "mat-dialog-close"; "_matDialogClose": "matDialogClose"; }, {}, never>;
146146
// (undocumented)
147-
static ɵfac: i0.ɵɵFactoryDeclaration<MatDialogClose, [{ optional: true; }, null, null]>;
147+
static ɵfac: i0.ɵɵFactoryDeclaration<MatDialogClose, never>;
148148
}
149149

150150
// @public
@@ -272,14 +272,15 @@ export const enum MatDialogState {
272272

273273
// @public
274274
export class MatDialogTitle implements OnInit {
275-
constructor(_dialogRef: MatDialogRef<any>, _elementRef: ElementRef<HTMLElement>, _dialog: MatDialog);
275+
constructor(
276+
_dialogRef: any, _elementRef: ElementRef<HTMLElement>, _dialog: MatDialog);
276277
id: string;
277278
// (undocumented)
278279
ngOnInit(): void;
279280
// (undocumented)
280281
static ɵdir: i0.ɵɵDirectiveDeclaration<MatDialogTitle, "[mat-dialog-title], [matDialogTitle]", ["matDialogTitle"], { "id": "id"; }, {}, never>;
281282
// (undocumented)
282-
static ɵfac: i0.ɵɵFactoryDeclaration<MatDialogTitle, [{ optional: true; }, null, null]>;
283+
static ɵfac: i0.ɵɵFactoryDeclaration<MatDialogTitle, never>;
283284
}
284285

285286
// @public

0 commit comments

Comments
 (0)