Skip to content

Commit e5c93fc

Browse files
committed
fix(overlay): only clear duplicate containers from different platform
Currently we clear all overlay containers when we create a new one as a way to avoid duplicate content coming in from the server. Our current approach is a little too aggressive, because it can pick up containers from different apps. These changes add an extra attribute to the container so that we can determine which platform it's coming from. Fixes #16851.
1 parent c07ec04 commit e5c93fc

File tree

5 files changed

+89
-16
lines changed

5 files changed

+89
-16
lines changed

src/cdk/overlay/fullscreen-overlay-container.ts

Lines changed: 9 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -9,6 +9,7 @@
99
import {Injectable, Inject, OnDestroy} from '@angular/core';
1010
import {OverlayContainer} from './overlay-container';
1111
import {DOCUMENT} from '@angular/common';
12+
import {Platform} from '@angular/cdk/platform';
1213

1314

1415
/**
@@ -23,8 +24,14 @@ export class FullscreenOverlayContainer extends OverlayContainer implements OnDe
2324
private _fullScreenEventName: string | undefined;
2425
private _fullScreenListener: () => void;
2526

26-
constructor(@Inject(DOCUMENT) _document: any) {
27-
super(_document);
27+
constructor(
28+
@Inject(DOCUMENT) _document: any,
29+
/**
30+
* @deprecated `platform` parameter to become required.
31+
* @breaking-change 10.0.0
32+
*/
33+
platform?: Platform) {
34+
super(_document, platform);
2835
}
2936

3037
ngOnDestroy() {

src/cdk/overlay/overlay-container.spec.ts

Lines changed: 29 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -53,9 +53,10 @@ describe('OverlayContainer', () => {
5353
.toBe(false, 'Expected the overlay container not to have class "commander-shepard"');
5454
});
5555

56-
it('should ensure that there is only one overlay container on the page', () => {
56+
it('should remove overlay containers from the server when on the browser', () => {
5757
const extraContainer = document.createElement('div');
5858
extraContainer.classList.add('cdk-overlay-container');
59+
extraContainer.setAttribute('platform', 'server');
5960
document.body.appendChild(extraContainer);
6061

6162
overlayContainer.getContainerElement();
@@ -65,6 +66,33 @@ describe('OverlayContainer', () => {
6566
extraContainer.parentNode.removeChild(extraContainer);
6667
}
6768
});
69+
70+
it('should remove overlay containers from other unit tests', () => {
71+
const extraContainer = document.createElement('div');
72+
extraContainer.classList.add('cdk-overlay-container');
73+
extraContainer.setAttribute('platform', 'test');
74+
document.body.appendChild(extraContainer);
75+
76+
overlayContainer.getContainerElement();
77+
expect(document.querySelectorAll('.cdk-overlay-container').length).toBe(1);
78+
79+
if (extraContainer.parentNode) {
80+
extraContainer.parentNode.removeChild(extraContainer);
81+
}
82+
});
83+
84+
it('should not remove extra containers that were created on the browser', () => {
85+
const extraContainer = document.createElement('div');
86+
extraContainer.classList.add('cdk-overlay-container');
87+
document.body.appendChild(extraContainer);
88+
89+
overlayContainer.getContainerElement();
90+
91+
expect(document.querySelectorAll('.cdk-overlay-container').length).toBe(2);
92+
93+
extraContainer.parentNode!.removeChild(extraContainer);
94+
});
95+
6896
});
6997

7098
/** Test-bed component that contains a TempatePortal and an ElementRef. */

src/cdk/overlay/overlay-container.ts

Lines changed: 46 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -15,21 +15,36 @@ import {
1515
Optional,
1616
SkipSelf,
1717
} from '@angular/core';
18+
import {Platform} from '@angular/cdk/platform';
1819

20+
/**
21+
* Whether we're in a testing environment.
22+
* TODO(crisbeto): remove this once we have an overlay testing module.
23+
*/
24+
const isTestEnvironment: boolean = typeof window !== 'undefined' && !!window &&
25+
!!((window as any).__karma__ || (window as any).jasmine);
1926

2027
/** Container inside which all overlays will render. */
2128
@Injectable({providedIn: 'root'})
2229
export class OverlayContainer implements OnDestroy {
2330
protected _containerElement: HTMLElement;
2431
protected _document: Document;
2532

26-
constructor(@Inject(DOCUMENT) document: any) {
33+
constructor(
34+
@Inject(DOCUMENT) document: any,
35+
/**
36+
* @deprecated `platform` parameter to become required.
37+
* @breaking-change 10.0.0
38+
*/
39+
protected _platform?: Platform) {
2740
this._document = document;
2841
}
2942

3043
ngOnDestroy() {
31-
if (this._containerElement && this._containerElement.parentNode) {
32-
this._containerElement.parentNode.removeChild(this._containerElement);
44+
const container = this._containerElement;
45+
46+
if (container && container.parentNode) {
47+
container.parentNode.removeChild(container);
3348
}
3449
}
3550

@@ -52,16 +67,40 @@ export class OverlayContainer implements OnDestroy {
5267
* with the 'cdk-overlay-container' class on the document body.
5368
*/
5469
protected _createContainer(): void {
70+
// @breaking-change 10.0.0 Remove null check for `_platform`.
71+
const isBrowser = this._platform ? this._platform.isBrowser : typeof window !== 'undefined';
5572
const containerClass = 'cdk-overlay-container';
56-
const previousContainers = this._document.getElementsByClassName(containerClass);
5773

58-
// Remove any old containers. This can happen when transitioning from the server to the client.
59-
for (let i = 0; i < previousContainers.length; i++) {
60-
previousContainers[i].parentNode!.removeChild(previousContainers[i]);
74+
if (isBrowser || isTestEnvironment) {
75+
const oppositePlatformContainers =
76+
this._document.querySelectorAll(`.${containerClass}[platform="server"], ` +
77+
`.${containerClass}[platform="test"]`);
78+
79+
// Remove any old containers from the opposite platform.
80+
// This can happen when transitioning from the server to the client.
81+
for (let i = 0; i < oppositePlatformContainers.length; i++) {
82+
oppositePlatformContainers[i].parentNode!.removeChild(oppositePlatformContainers[i]);
83+
}
6184
}
6285

6386
const container = this._document.createElement('div');
6487
container.classList.add(containerClass);
88+
89+
// A long time ago we kept adding new overlay containers whenever a new app was instantiated,
90+
// but at some point we added logic which clears the duplicate ones in order to avoid leaks.
91+
// The new logic was a little too aggressive since it was breaking some legitimate use cases.
92+
// To mitigate the problem we made it so that only containers from a different platform are
93+
// cleared, but the side-effect was that people started depending on the overly-aggressive
94+
// logic to clean up their tests for them. Until we can introduce an overlay-specific testing
95+
// module which does the cleanup, we try to detect that we're in a test environment and we
96+
// always clear the container. See #17006.
97+
// TODO(crisbeto): remove the test environment check once we have an overlay testing module.
98+
if (isTestEnvironment) {
99+
container.setAttribute('platform', 'test');
100+
} else if (!isBrowser) {
101+
container.setAttribute('platform', 'server');
102+
}
103+
65104
this._document.body.appendChild(container);
66105
this._containerElement = container;
67106
}

src/material/select/testing/shared.spec.ts

Lines changed: 0 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -226,13 +226,11 @@ function getActiveElementId() {
226226
<mat-option *ngFor="let state of states" [value]="state.code">{{ state.name }}</mat-option>
227227
</mat-select>
228228
</mat-form-field>
229-
230229
<mat-form-field>
231230
<mat-select multiple id="multiple-selection">
232231
<mat-option *ngFor="let state of states" [value]="state.code">{{ state.name }}</mat-option>
233232
</mat-select>
234233
</mat-form-field>
235-
236234
<mat-form-field>
237235
<mat-select id="grouped">
238236
<mat-optgroup *ngFor="let group of stateGroups" [label]="group.name">
@@ -242,7 +240,6 @@ function getActiveElementId() {
242240
</mat-optgroup>
243241
</mat-select>
244242
</mat-form-field>
245-
246243
<mat-form-field>
247244
<mat-select [formControl]="formControl" id="with-form-control">
248245
<mat-option *ngFor="let state of states" [value]="state.code">{{ state.name }}</mat-option>
@@ -283,4 +280,3 @@ class SelectHarnessTest {
283280
}
284281
];
285282
}
286-

tools/public_api_guard/cdk/overlay.d.ts

Lines changed: 5 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -152,7 +152,8 @@ export declare type FlexibleConnectedPositionStrategyOrigin = ElementRef | HTMLE
152152
};
153153

154154
export declare class FullscreenOverlayContainer extends OverlayContainer implements OnDestroy {
155-
constructor(_document: any);
155+
constructor(_document: any,
156+
platform?: Platform);
156157
protected _createContainer(): void;
157158
getFullscreenElement(): Element;
158159
ngOnDestroy(): void;
@@ -224,7 +225,9 @@ export interface OverlayConnectionPosition {
224225
export declare class OverlayContainer implements OnDestroy {
225226
protected _containerElement: HTMLElement;
226227
protected _document: Document;
227-
constructor(document: any);
228+
protected _platform?: Platform | undefined;
229+
constructor(document: any,
230+
_platform?: Platform | undefined);
228231
protected _createContainer(): void;
229232
getContainerElement(): HTMLElement;
230233
ngOnDestroy(): void;

0 commit comments

Comments
 (0)