Skip to content

Commit

Permalink
perf(scroll-dispatcher): avoid triggering change detection on scroll
Browse files Browse the repository at this point in the history
* 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.
  • Loading branch information
crisbeto committed Mar 20, 2017
1 parent 259ff5f commit 42e7cfb
Show file tree
Hide file tree
Showing 5 changed files with 67 additions and 21 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand Down
38 changes: 30 additions & 8 deletions src/lib/core/overlay/scroll/scroll-dispatcher.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -41,32 +41,54 @@ 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.
// The notification should be picked up by the service.
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', () => {
Expand Down
21 changes: 13 additions & 8 deletions src/lib/core/overlay/scroll/scroll-dispatcher.ts
Original file line number Diff line number Diff line change
@@ -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';
Expand All @@ -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<void> = new Subject<void>();

Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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
};
23 changes: 20 additions & 3 deletions src/lib/core/overlay/scroll/scrollable.ts
Original file line number Diff line number Diff line change
@@ -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';

Expand All @@ -13,22 +14,38 @@ import 'rxjs/add/observable/fromEvent';
selector: '[cdk-scrollable]'
})
export class Scrollable implements OnInit, OnDestroy {
private _elementScrolled: Subject<Event> = 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<any> {
return Observable.fromEvent(this._elementRef.nativeElement, 'scroll');
return this._elementScrolled.asObservable();
}

getElementRef(): ElementRef {
Expand Down
2 changes: 1 addition & 1 deletion src/lib/core/testing/dispatch-events.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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));
}

Expand Down

0 comments on commit 42e7cfb

Please sign in to comment.