From 71886f84efa0fb3c1e8ac66e13807dfd17fa2848 Mon Sep 17 00:00:00 2001 From: Kristiyan Kostadinov Date: Wed, 1 Aug 2018 16:15:23 +0200 Subject: [PATCH] perf(overlay): remove detached overlays from the DOM (#12414) 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. --- src/cdk/overlay/overlay-directives.spec.ts | 4 -- src/cdk/overlay/overlay-ref.ts | 36 +++++++++++++-- src/cdk/overlay/overlay.spec.ts | 54 ++++++++++++++++++---- src/lib/tooltip/tooltip.spec.ts | 9 ++-- 4 files changed, 84 insertions(+), 19 deletions(-) diff --git a/src/cdk/overlay/overlay-directives.spec.ts b/src/cdk/overlay/overlay-directives.spec.ts index 875a0a16ecc2..685052565e40 100644 --- a/src/cdk/overlay/overlay-directives.spec.ts +++ b/src/cdk/overlay/overlay-directives.spec.ts @@ -51,15 +51,11 @@ describe('Overlay directives', () => { fixture.detectChanges(); expect(overlayContainerElement.textContent).toContain('Menu content'); - expect(getPaneElement().style.pointerEvents) - .toBe('auto', 'Expected the overlay pane to enable pointerEvents when attached.'); fixture.componentInstance.isOpen = false; fixture.detectChanges(); expect(overlayContainerElement.textContent).toBe(''); - expect(getPaneElement().style.pointerEvents) - .toBe('none', 'Expected the overlay pane to disable pointerEvents when detached.'); }); it('should destroy the overlay when the directive is destroyed', () => { diff --git a/src/cdk/overlay/overlay-ref.ts b/src/cdk/overlay/overlay-ref.ts index 3ff749565ed8..e0bfeceb593d 100644 --- a/src/cdk/overlay/overlay-ref.ts +++ b/src/cdk/overlay/overlay-ref.ts @@ -9,8 +9,8 @@ import {Direction, Directionality} from '@angular/cdk/bidi'; import {ComponentPortal, Portal, PortalOutlet, TemplatePortal} from '@angular/cdk/portal'; import {ComponentRef, EmbeddedViewRef, NgZone} from '@angular/core'; -import {Observable, Subject} from 'rxjs'; -import {take} from 'rxjs/operators'; +import {Observable, Subject, merge} from 'rxjs'; +import {take, takeUntil} from 'rxjs/operators'; import {OverlayKeyboardDispatcher} from './keyboard/overlay-keyboard-dispatcher'; import {OverlayConfig} from './overlay-config'; import {coerceCssPixelValue, coerceArray} from '@angular/cdk/coercion'; @@ -31,6 +31,12 @@ export class OverlayRef implements PortalOutlet, OverlayReference { private _backdropClick: Subject = new Subject(); private _attachments = new Subject(); private _detachments = new Subject(); + + /** + * Reference to the parent of the `_host` at the time it was detached. Used to restore + * the `_host` to its original position in the DOM when it gets re-attached. + */ + private _previousHostParent: HTMLElement; private _keydownEventsObservable: Observable = Observable.create(observer => { const subscription = this._keydownEvents.subscribe(observer); this._keydownEventSubscriptions++; @@ -99,6 +105,10 @@ export class OverlayRef implements PortalOutlet, OverlayReference { } // Update the pane element with the given configuration. + if (!this._host.parentElement && this._previousHostParent) { + this._previousHostParent.appendChild(this._host); + } + this._updateStackingOrder(); this._updateElementSize(); this._updateElementDirection(); @@ -176,6 +186,26 @@ export class OverlayRef implements PortalOutlet, OverlayReference { // Remove this overlay from keyboard dispatcher tracking. this._keyboardDispatcher.remove(this); + // Keeping the host element in DOM the can cause scroll jank, because it still gets rendered, + // even though it's transparent and unclickable. We can't remove the host here immediately, + // because the overlay pane's content might still be animating. This stream helps us avoid + // interrupting the animation by waiting for the pane to become empty. + const subscription = this._ngZone.onStable + .asObservable() + .pipe(takeUntil(merge(this._attachments, this._detachments))) + .subscribe(() => { + // Needs a couple of checks for the pane and host, because + // they may have been removed by the time the zone stabilizes. + if (!this._pane || !this._host || this._pane.children.length === 0) { + if (this._host && this._host.parentElement) { + this._previousHostParent = this._host.parentElement; + this._previousHostParent.removeChild(this._host); + } + + subscription.unsubscribe(); + } + }); + return detachmentResult; } @@ -203,7 +233,7 @@ export class OverlayRef implements PortalOutlet, OverlayReference { this._host = null!; } - this._pane = null!; + this._previousHostParent = this._pane = null!; if (isAttached) { this._detachments.next(); diff --git a/src/cdk/overlay/overlay.spec.ts b/src/cdk/overlay/overlay.spec.ts index bde5039aaf3e..05f944422ee1 100644 --- a/src/cdk/overlay/overlay.spec.ts +++ b/src/cdk/overlay/overlay.spec.ts @@ -7,9 +7,10 @@ import { ErrorHandler, Injectable, EventEmitter, + NgZone, } from '@angular/core'; import {Direction, Directionality} from '@angular/cdk/bidi'; -import {dispatchFakeEvent} from '@angular/cdk/testing'; +import {dispatchFakeEvent, MockNgZone} from '@angular/cdk/testing'; import { ComponentPortal, PortalModule, @@ -35,19 +36,26 @@ describe('Overlay', () => { let overlayContainer: OverlayContainer; let viewContainerFixture: ComponentFixture; let dir: Direction; + let zone: MockNgZone; beforeEach(async(() => { dir = 'ltr'; TestBed.configureTestingModule({ imports: [OverlayModule, PortalModule, OverlayTestModule], - providers: [{ - provide: Directionality, - useFactory: () => { - const fakeDirectionality = {}; - Object.defineProperty(fakeDirectionality, 'value', {get: () => dir}); - return fakeDirectionality; - } - }], + providers: [ + { + provide: Directionality, + useFactory: () => { + const fakeDirectionality = {}; + Object.defineProperty(fakeDirectionality, 'value', {get: () => dir}); + return fakeDirectionality; + } + }, + { + provide: NgZone, + useFactory: () => zone = new MockNgZone() + }, + ], }).compileComponents(); })); @@ -342,6 +350,33 @@ describe('Overlay', () => { expect(overlayRef.getDirection()).toBe('ltr'); }); + it('should add and remove the overlay host as the ref is being attached and detached', () => { + const overlayRef = overlay.create(); + + overlayRef.attach(componentPortal); + viewContainerFixture.detectChanges(); + + expect(overlayRef.hostElement.parentElement) + .toBeTruthy('Expected host element to be in the DOM.'); + + overlayRef.detach(); + + expect(overlayRef.hostElement.parentElement) + .toBeTruthy('Expected host element not to have been removed immediately.'); + + viewContainerFixture.detectChanges(); + zone.simulateZoneExit(); + + expect(overlayRef.hostElement.parentElement) + .toBeFalsy('Expected host element to have been removed once the zone stabilizes.'); + + overlayRef.attach(componentPortal); + viewContainerFixture.detectChanges(); + + expect(overlayRef.hostElement.parentElement) + .toBeTruthy('Expected host element to be back in the DOM.'); + }); + describe('positioning', () => { let config: OverlayConfig; @@ -354,6 +389,7 @@ describe('Overlay', () => { overlay.create(config).attach(componentPortal); viewContainerFixture.detectChanges(); + zone.simulateZoneExit(); tick(); expect(overlayContainerElement.querySelectorAll('.fake-positioned').length).toBe(1); diff --git a/src/lib/tooltip/tooltip.spec.ts b/src/lib/tooltip/tooltip.spec.ts index 64f7cb3c6dd1..25bea8c35488 100644 --- a/src/lib/tooltip/tooltip.spec.ts +++ b/src/lib/tooltip/tooltip.spec.ts @@ -512,21 +512,24 @@ describe('MatTooltip', () => { it('should keep the overlay direction in sync with the trigger direction', fakeAsync(() => { dir.value = 'rtl'; tooltipDirective.show(); - tick(); + tick(0); fixture.detectChanges(); + tick(500); let tooltipWrapper = overlayContainerElement.querySelector('.cdk-overlay-connected-position-bounding-box')!; expect(tooltipWrapper.getAttribute('dir')).toBe('rtl', 'Expected tooltip to be in RTL.'); tooltipDirective.hide(0); - tick(); + tick(0); fixture.detectChanges(); + tick(500); dir.value = 'ltr'; tooltipDirective.show(); - tick(); + tick(0); fixture.detectChanges(); + tick(500); tooltipWrapper = overlayContainerElement.querySelector('.cdk-overlay-connected-position-bounding-box')!;