Skip to content

Commit

Permalink
perf(overlay): remove detached overlays from the DOM
Browse files Browse the repository at this point in the history
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.
  • Loading branch information
crisbeto committed Jul 28, 2018
1 parent e462f3d commit ed490c4
Show file tree
Hide file tree
Showing 4 changed files with 84 additions and 19 deletions.
4 changes: 0 additions & 4 deletions src/cdk/overlay/overlay-directives.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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', () => {
Expand Down
36 changes: 33 additions & 3 deletions src/cdk/overlay/overlay-ref.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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';
Expand All @@ -31,6 +31,12 @@ export class OverlayRef implements PortalOutlet, OverlayReference {
private _backdropClick: Subject<MouseEvent> = new Subject();
private _attachments = new Subject<void>();
private _detachments = new Subject<void>();

/**
* 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<KeyboardEvent> = Observable.create(observer => {
const subscription = this._keydownEvents.subscribe(observer);
this._keydownEventSubscriptions++;
Expand Down Expand Up @@ -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();
Expand Down Expand Up @@ -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;
}

Expand Down Expand Up @@ -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();
Expand Down
54 changes: 45 additions & 9 deletions src/cdk/overlay/overlay.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand All @@ -35,19 +36,26 @@ describe('Overlay', () => {
let overlayContainer: OverlayContainer;
let viewContainerFixture: ComponentFixture<TestComponentWithTemplatePortals>;
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();
}));

Expand Down Expand Up @@ -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;

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

overlay.create(config).attach(componentPortal);
viewContainerFixture.detectChanges();
zone.simulateZoneExit();
tick();

expect(overlayContainerElement.querySelectorAll('.fake-positioned').length).toBe(1);
Expand Down
9 changes: 6 additions & 3 deletions src/lib/tooltip/tooltip.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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')!;
Expand Down

0 comments on commit ed490c4

Please sign in to comment.