From 42e7cfb6379b9f3fe48e7b07334937e1ab99292d Mon Sep 17 00:00:00 2001 From: crisbeto Date: Mon, 20 Mar 2017 21:40:28 +0100 Subject: [PATCH] perf(scroll-dispatcher): avoid triggering change detection on scroll * Avoids triggering change detection when listening to global scroll events in the `ScrollDispatcher`. * Avoids triggering change detection when scrolling inside of `Scrollable` instances. * Switches a `ScrollDispatcher` test to use spies, instead of toggling booleans. --- .../connected-position-strategy.spec.ts | 4 +- .../overlay/scroll/scroll-dispatcher.spec.ts | 38 +++++++++++++++---- .../core/overlay/scroll/scroll-dispatcher.ts | 21 ++++++---- src/lib/core/overlay/scroll/scrollable.ts | 23 +++++++++-- src/lib/core/testing/dispatch-events.ts | 2 +- 5 files changed, 67 insertions(+), 21 deletions(-) diff --git a/src/lib/core/overlay/position/connected-position-strategy.spec.ts b/src/lib/core/overlay/position/connected-position-strategy.spec.ts index 2606d163fbb6..fb0dcb66bb73 100644 --- a/src/lib/core/overlay/position/connected-position-strategy.spec.ts +++ b/src/lib/core/overlay/position/connected-position-strategy.spec.ts @@ -534,7 +534,9 @@ describe('ConnectedPositionStrategy', () => { fakeElementRef, {originX: 'start', originY: 'bottom'}, {overlayX: 'start', overlayY: 'top'}); - strategy.withScrollableContainers([new Scrollable(new FakeElementRef(scrollable), null)]); + + strategy.withScrollableContainers([ + new Scrollable(new FakeElementRef(scrollable), null, null, null)]); positionChangeHandler = jasmine.createSpy('positionChangeHandler'); onPositionChangeSubscription = strategy.onPositionChange.subscribe(positionChangeHandler); diff --git a/src/lib/core/overlay/scroll/scroll-dispatcher.spec.ts b/src/lib/core/overlay/scroll/scroll-dispatcher.spec.ts index 1cd3a1b6af40..8f8282e58117 100644 --- a/src/lib/core/overlay/scroll/scroll-dispatcher.spec.ts +++ b/src/lib/core/overlay/scroll/scroll-dispatcher.spec.ts @@ -41,15 +41,15 @@ describe('Scroll Dispatcher', () => { it('should notify through the directive and service that a scroll event occurred', fakeAsync(() => { - let hasDirectiveScrollNotified = false; // Listen for notifications from scroll directive - let scrollable = fixture.componentInstance.scrollable; - scrollable.elementScrolled().subscribe(() => { hasDirectiveScrollNotified = true; }); + const scrollable = fixture.componentInstance.scrollable; + const directiveSpy = jasmine.createSpy('directive scroll callback'); + scrollable.elementScrolled().subscribe(directiveSpy); // Listen for notifications from scroll service with a throttle of 100ms const throttleTime = 100; - let hasServiceScrollNotified = false; - scroll.scrolled(throttleTime, () => { hasServiceScrollNotified = true; }); + const serviceSpy = jasmine.createSpy('service scroll callback'); + scroll.scrolled(throttleTime, serviceSpy); // Emit a scroll event from the scrolling element in our component. // This event should be picked up by the scrollable directive and notify. @@ -57,16 +57,38 @@ describe('Scroll Dispatcher', () => { dispatchFakeEvent(fixture.componentInstance.scrollingElement.nativeElement, 'scroll'); // The scrollable directive should have notified the service immediately. - expect(hasDirectiveScrollNotified).toBe(true); + expect(directiveSpy).toHaveBeenCalled(); // Verify that the throttle is used, the service should wait for the throttle time until // sending the notification. - expect(hasServiceScrollNotified).toBe(false); + expect(serviceSpy).not.toHaveBeenCalled(); // After the throttle time, the notification should be sent. tick(throttleTime); - expect(hasServiceScrollNotified).toBe(true); + expect(serviceSpy).toHaveBeenCalled(); })); + + it('should not execute the global events in the Angular zone', () => { + const spy = jasmine.createSpy('zone unstable callback'); + const subscription = fixture.ngZone.onUnstable.subscribe(spy); + + scroll.scrolled(0, () => {}); + dispatchFakeEvent(document, 'scroll'); + dispatchFakeEvent(window, 'resize'); + + expect(spy).not.toHaveBeenCalled(); + subscription.unsubscribe(); + }); + + it('should not execute the scrollable events in the Angular zone', () => { + const spy = jasmine.createSpy('zone unstable callback'); + const subscription = fixture.ngZone.onUnstable.subscribe(spy); + + dispatchFakeEvent(fixture.componentInstance.scrollingElement.nativeElement, 'scroll'); + + expect(spy).not.toHaveBeenCalled(); + subscription.unsubscribe(); + }); }); describe('Nested scrollables', () => { diff --git a/src/lib/core/overlay/scroll/scroll-dispatcher.ts b/src/lib/core/overlay/scroll/scroll-dispatcher.ts index 8542aac102ac..abd8ea55410e 100644 --- a/src/lib/core/overlay/scroll/scroll-dispatcher.ts +++ b/src/lib/core/overlay/scroll/scroll-dispatcher.ts @@ -1,4 +1,4 @@ -import {Injectable, ElementRef, Optional, SkipSelf} from '@angular/core'; +import {Injectable, ElementRef, Optional, SkipSelf, NgZone} from '@angular/core'; import {Scrollable} from './scrollable'; import {Subject} from 'rxjs/Subject'; import {Observable} from 'rxjs/Observable'; @@ -17,6 +17,8 @@ export const DEFAULT_SCROLL_TIME = 20; */ @Injectable() export class ScrollDispatcher { + constructor(private _ngZone: NgZone) { } + /** Subject for notifying that a registered scrollable reference element has been scrolled. */ _scrolled: Subject = new Subject(); @@ -69,10 +71,12 @@ export class ScrollDispatcher { this._scrolledCount++; if (!this._globalSubscription) { - this._globalSubscription = Observable.merge( - Observable.fromEvent(window.document, 'scroll'), - Observable.fromEvent(window, 'resize') - ).subscribe(() => this._notify()); + this._globalSubscription = this._ngZone.runOutsideAngular(() => { + return Observable.merge( + Observable.fromEvent(window.document, 'scroll'), + Observable.fromEvent(window, 'resize') + ).subscribe(() => this._notify()); + }); } // Note that we need to do the subscribing from here, in order to be able to remove @@ -118,13 +122,14 @@ export class ScrollDispatcher { } } -export function SCROLL_DISPATCHER_PROVIDER_FACTORY(parentDispatcher: ScrollDispatcher) { - return parentDispatcher || new ScrollDispatcher(); +export function SCROLL_DISPATCHER_PROVIDER_FACTORY(parentDispatcher: ScrollDispatcher, + ngZone: NgZone) { + return parentDispatcher || new ScrollDispatcher(ngZone); } export const SCROLL_DISPATCHER_PROVIDER = { // If there is already a ScrollDispatcher available, use that. Otherwise, provide a new one. provide: ScrollDispatcher, - deps: [[new Optional(), new SkipSelf(), ScrollDispatcher]], + deps: [[new Optional(), new SkipSelf(), ScrollDispatcher], NgZone], useFactory: SCROLL_DISPATCHER_PROVIDER_FACTORY }; diff --git a/src/lib/core/overlay/scroll/scrollable.ts b/src/lib/core/overlay/scroll/scrollable.ts index 3e5f2bb03616..7d24560d5f87 100644 --- a/src/lib/core/overlay/scroll/scrollable.ts +++ b/src/lib/core/overlay/scroll/scrollable.ts @@ -1,5 +1,6 @@ -import {Directive, ElementRef, OnInit, OnDestroy} from '@angular/core'; +import {Directive, ElementRef, OnInit, OnDestroy, NgZone, Renderer} from '@angular/core'; import {Observable} from 'rxjs/Observable'; +import {Subject} from 'rxjs/Subject'; import {ScrollDispatcher} from './scroll-dispatcher'; import 'rxjs/add/observable/fromEvent'; @@ -13,22 +14,38 @@ import 'rxjs/add/observable/fromEvent'; selector: '[cdk-scrollable]' }) export class Scrollable implements OnInit, OnDestroy { + private _elementScrolled: Subject = new Subject(); + private _scrollListener: Function; + constructor(private _elementRef: ElementRef, - private _scroll: ScrollDispatcher) {} + private _scroll: ScrollDispatcher, + private _ngZone: NgZone, + private _renderer: Renderer) {} ngOnInit() { + this._scrollListener = this._ngZone.runOutsideAngular(() => { + return this._renderer.listen(this.getElementRef().nativeElement, 'scroll', (event: Event) => { + this._elementScrolled.next(event); + }); + }); + this._scroll.register(this); } ngOnDestroy() { this._scroll.deregister(this); + + if (this._scrollListener) { + this._scrollListener(); + this._scrollListener = null; + } } /** * Returns observable that emits when a scroll event is fired on the host element. */ elementScrolled(): Observable { - return Observable.fromEvent(this._elementRef.nativeElement, 'scroll'); + return this._elementScrolled.asObservable(); } getElementRef(): ElementRef { diff --git a/src/lib/core/testing/dispatch-events.ts b/src/lib/core/testing/dispatch-events.ts index 6e57db2a66b1..7e137b967043 100644 --- a/src/lib/core/testing/dispatch-events.ts +++ b/src/lib/core/testing/dispatch-events.ts @@ -5,7 +5,7 @@ import { } from './event-objects'; /** Shorthand to dispatch a fake event on a specified node. */ -export function dispatchFakeEvent(node: Node, type: string) { +export function dispatchFakeEvent(node: Node | Window, type: string) { node.dispatchEvent(createFakeEvent(type)); }