Skip to content

Commit ed490c4

Browse files
committed
perf(overlay): remove detached overlays from the DOM
Removes the detached overlays from the DOM, rather than keeping their host element. Previously we kept the elements in the DOM, because they don't interfere with user interactions and it made it easier to handle animations, however doing so can cause scroll jank, because the host still has dimensions and is being rendered by the browser. Fixes #12341.
1 parent e462f3d commit ed490c4

File tree

4 files changed

+84
-19
lines changed

4 files changed

+84
-19
lines changed

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

-4
Original file line numberDiff line numberDiff line change
@@ -51,15 +51,11 @@ describe('Overlay directives', () => {
5151
fixture.detectChanges();
5252

5353
expect(overlayContainerElement.textContent).toContain('Menu content');
54-
expect(getPaneElement().style.pointerEvents)
55-
.toBe('auto', 'Expected the overlay pane to enable pointerEvents when attached.');
5654

5755
fixture.componentInstance.isOpen = false;
5856
fixture.detectChanges();
5957

6058
expect(overlayContainerElement.textContent).toBe('');
61-
expect(getPaneElement().style.pointerEvents)
62-
.toBe('none', 'Expected the overlay pane to disable pointerEvents when detached.');
6359
});
6460

6561
it('should destroy the overlay when the directive is destroyed', () => {

src/cdk/overlay/overlay-ref.ts

+33-3
Original file line numberDiff line numberDiff line change
@@ -9,8 +9,8 @@
99
import {Direction, Directionality} from '@angular/cdk/bidi';
1010
import {ComponentPortal, Portal, PortalOutlet, TemplatePortal} from '@angular/cdk/portal';
1111
import {ComponentRef, EmbeddedViewRef, NgZone} from '@angular/core';
12-
import {Observable, Subject} from 'rxjs';
13-
import {take} from 'rxjs/operators';
12+
import {Observable, Subject, merge} from 'rxjs';
13+
import {take, takeUntil} from 'rxjs/operators';
1414
import {OverlayKeyboardDispatcher} from './keyboard/overlay-keyboard-dispatcher';
1515
import {OverlayConfig} from './overlay-config';
1616
import {coerceCssPixelValue, coerceArray} from '@angular/cdk/coercion';
@@ -31,6 +31,12 @@ export class OverlayRef implements PortalOutlet, OverlayReference {
3131
private _backdropClick: Subject<MouseEvent> = new Subject();
3232
private _attachments = new Subject<void>();
3333
private _detachments = new Subject<void>();
34+
35+
/**
36+
* Reference to the parent of the `_host` at the time it was detached. Used to restore
37+
* the `_host` to its original position in the DOM when it gets re-attached.
38+
*/
39+
private _previousHostParent: HTMLElement;
3440
private _keydownEventsObservable: Observable<KeyboardEvent> = Observable.create(observer => {
3541
const subscription = this._keydownEvents.subscribe(observer);
3642
this._keydownEventSubscriptions++;
@@ -99,6 +105,10 @@ export class OverlayRef implements PortalOutlet, OverlayReference {
99105
}
100106

101107
// Update the pane element with the given configuration.
108+
if (!this._host.parentElement && this._previousHostParent) {
109+
this._previousHostParent.appendChild(this._host);
110+
}
111+
102112
this._updateStackingOrder();
103113
this._updateElementSize();
104114
this._updateElementDirection();
@@ -176,6 +186,26 @@ export class OverlayRef implements PortalOutlet, OverlayReference {
176186
// Remove this overlay from keyboard dispatcher tracking.
177187
this._keyboardDispatcher.remove(this);
178188

189+
// Keeping the host element in DOM the can cause scroll jank, because it still gets rendered,
190+
// even though it's transparent and unclickable. We can't remove the host here immediately,
191+
// because the overlay pane's content might still be animating. This stream helps us avoid
192+
// interrupting the animation by waiting for the pane to become empty.
193+
const subscription = this._ngZone.onStable
194+
.asObservable()
195+
.pipe(takeUntil(merge(this._attachments, this._detachments)))
196+
.subscribe(() => {
197+
// Needs a couple of checks for the pane and host, because
198+
// they may have been removed by the time the zone stabilizes.
199+
if (!this._pane || !this._host || this._pane.children.length === 0) {
200+
if (this._host && this._host.parentElement) {
201+
this._previousHostParent = this._host.parentElement;
202+
this._previousHostParent.removeChild(this._host);
203+
}
204+
205+
subscription.unsubscribe();
206+
}
207+
});
208+
179209
return detachmentResult;
180210
}
181211

@@ -203,7 +233,7 @@ export class OverlayRef implements PortalOutlet, OverlayReference {
203233
this._host = null!;
204234
}
205235

206-
this._pane = null!;
236+
this._previousHostParent = this._pane = null!;
207237

208238
if (isAttached) {
209239
this._detachments.next();

src/cdk/overlay/overlay.spec.ts

+45-9
Original file line numberDiff line numberDiff line change
@@ -7,9 +7,10 @@ import {
77
ErrorHandler,
88
Injectable,
99
EventEmitter,
10+
NgZone,
1011
} from '@angular/core';
1112
import {Direction, Directionality} from '@angular/cdk/bidi';
12-
import {dispatchFakeEvent} from '@angular/cdk/testing';
13+
import {dispatchFakeEvent, MockNgZone} from '@angular/cdk/testing';
1314
import {
1415
ComponentPortal,
1516
PortalModule,
@@ -35,19 +36,26 @@ describe('Overlay', () => {
3536
let overlayContainer: OverlayContainer;
3637
let viewContainerFixture: ComponentFixture<TestComponentWithTemplatePortals>;
3738
let dir: Direction;
39+
let zone: MockNgZone;
3840

3941
beforeEach(async(() => {
4042
dir = 'ltr';
4143
TestBed.configureTestingModule({
4244
imports: [OverlayModule, PortalModule, OverlayTestModule],
43-
providers: [{
44-
provide: Directionality,
45-
useFactory: () => {
46-
const fakeDirectionality = {};
47-
Object.defineProperty(fakeDirectionality, 'value', {get: () => dir});
48-
return fakeDirectionality;
49-
}
50-
}],
45+
providers: [
46+
{
47+
provide: Directionality,
48+
useFactory: () => {
49+
const fakeDirectionality = {};
50+
Object.defineProperty(fakeDirectionality, 'value', {get: () => dir});
51+
return fakeDirectionality;
52+
}
53+
},
54+
{
55+
provide: NgZone,
56+
useFactory: () => zone = new MockNgZone()
57+
},
58+
],
5159
}).compileComponents();
5260
}));
5361

@@ -342,6 +350,33 @@ describe('Overlay', () => {
342350
expect(overlayRef.getDirection()).toBe('ltr');
343351
});
344352

353+
it('should add and remove the overlay host as the ref is being attached and detached', () => {
354+
const overlayRef = overlay.create();
355+
356+
overlayRef.attach(componentPortal);
357+
viewContainerFixture.detectChanges();
358+
359+
expect(overlayRef.hostElement.parentElement)
360+
.toBeTruthy('Expected host element to be in the DOM.');
361+
362+
overlayRef.detach();
363+
364+
expect(overlayRef.hostElement.parentElement)
365+
.toBeTruthy('Expected host element not to have been removed immediately.');
366+
367+
viewContainerFixture.detectChanges();
368+
zone.simulateZoneExit();
369+
370+
expect(overlayRef.hostElement.parentElement)
371+
.toBeFalsy('Expected host element to have been removed once the zone stabilizes.');
372+
373+
overlayRef.attach(componentPortal);
374+
viewContainerFixture.detectChanges();
375+
376+
expect(overlayRef.hostElement.parentElement)
377+
.toBeTruthy('Expected host element to be back in the DOM.');
378+
});
379+
345380
describe('positioning', () => {
346381
let config: OverlayConfig;
347382

@@ -354,6 +389,7 @@ describe('Overlay', () => {
354389

355390
overlay.create(config).attach(componentPortal);
356391
viewContainerFixture.detectChanges();
392+
zone.simulateZoneExit();
357393
tick();
358394

359395
expect(overlayContainerElement.querySelectorAll('.fake-positioned').length).toBe(1);

src/lib/tooltip/tooltip.spec.ts

+6-3
Original file line numberDiff line numberDiff line change
@@ -512,21 +512,24 @@ describe('MatTooltip', () => {
512512
it('should keep the overlay direction in sync with the trigger direction', fakeAsync(() => {
513513
dir.value = 'rtl';
514514
tooltipDirective.show();
515-
tick();
515+
tick(0);
516516
fixture.detectChanges();
517+
tick(500);
517518

518519
let tooltipWrapper =
519520
overlayContainerElement.querySelector('.cdk-overlay-connected-position-bounding-box')!;
520521
expect(tooltipWrapper.getAttribute('dir')).toBe('rtl', 'Expected tooltip to be in RTL.');
521522

522523
tooltipDirective.hide(0);
523-
tick();
524+
tick(0);
524525
fixture.detectChanges();
526+
tick(500);
525527

526528
dir.value = 'ltr';
527529
tooltipDirective.show();
528-
tick();
530+
tick(0);
529531
fixture.detectChanges();
532+
tick(500);
530533

531534
tooltipWrapper =
532535
overlayContainerElement.querySelector('.cdk-overlay-connected-position-bounding-box')!;

0 commit comments

Comments
 (0)