From fae1529e64f8e535fe60bbd6d4c5b0b8d517f877 Mon Sep 17 00:00:00 2001 From: crisbeto Date: Thu, 23 Feb 2017 21:44:25 +0100 Subject: [PATCH 1/4] perf(scroll-dispatcher): lazily subscribe to global events Switches the `ScrollDispatcher` to only subscribe to `scroll` and `resize` events if there any registered callbacks. Also unsubscribes once there are no more callbacks. Fixes #3237. --- .../core/overlay/scroll/scroll-dispatcher.ts | 22 ++++++++++++++----- 1 file changed, 16 insertions(+), 6 deletions(-) diff --git a/src/lib/core/overlay/scroll/scroll-dispatcher.ts b/src/lib/core/overlay/scroll/scroll-dispatcher.ts index 334ebabfa37e..6edb0e982507 100644 --- a/src/lib/core/overlay/scroll/scroll-dispatcher.ts +++ b/src/lib/core/overlay/scroll/scroll-dispatcher.ts @@ -4,6 +4,7 @@ import {Subject} from 'rxjs/Subject'; import {Observable} from 'rxjs/Observable'; import {Subscription} from 'rxjs/Subscription'; import 'rxjs/add/observable/fromEvent'; +import 'rxjs/add/observable/merge'; import 'rxjs/add/operator/auditTime'; @@ -19,18 +20,15 @@ export class ScrollDispatcher { /** Subject for notifying that a registered scrollable reference element has been scrolled. */ _scrolled: Subject = new Subject(); + /** Keeps track of the global `scroll` and `resize` subscriptions. */ + private _globalSubscription: Subscription; + /** * Map of all the scrollable references that are registered with the service and their * scroll event subscriptions. */ scrollableReferences: Map = new Map(); - constructor() { - // By default, notify a scroll event when the document is scrolled or the window is resized. - Observable.fromEvent(window.document, 'scroll').subscribe(() => this._notify()); - Observable.fromEvent(window, 'resize').subscribe(() => this._notify()); - } - /** * Registers a Scrollable with the service and listens for its scrolled events. When the * scrollable is scrolled, the service emits the event in its scrolled observable. @@ -39,6 +37,13 @@ export class ScrollDispatcher { register(scrollable: Scrollable): void { const scrollSubscription = scrollable.elementScrolled().subscribe(() => this._notify()); this.scrollableReferences.set(scrollable, scrollSubscription); + + if (!this._globalSubscription) { + this._globalSubscription = Observable.merge( + Observable.fromEvent(window.document, 'scroll'), + Observable.fromEvent(window, 'resize') + ).subscribe(() => this._notify()); + } } /** @@ -49,6 +54,11 @@ export class ScrollDispatcher { if (this.scrollableReferences.has(scrollable)) { this.scrollableReferences.get(scrollable).unsubscribe(); this.scrollableReferences.delete(scrollable); + + if (!this.scrollableReferences.size && this._globalSubscription) { + this._globalSubscription.unsubscribe(); + this._globalSubscription = null; + } } } From f7959bd9a075ab9686cde23558156564b75e7a68 Mon Sep 17 00:00:00 2001 From: crisbeto Date: Mon, 27 Feb 2017 22:35:46 +0100 Subject: [PATCH 2/4] refactor: account for the `scrolled` subscriptions when adding the global listeners --- .../core/overlay/position/viewport-ruler.ts | 2 +- .../overlay/scroll/scroll-dispatcher.spec.ts | 40 +++++++++++- .../core/overlay/scroll/scroll-dispatcher.ts | 64 ++++++++++++------- src/lib/tooltip/tooltip.ts | 2 +- 4 files changed, 83 insertions(+), 25 deletions(-) diff --git a/src/lib/core/overlay/position/viewport-ruler.ts b/src/lib/core/overlay/position/viewport-ruler.ts index d39271f75411..c87be208a021 100644 --- a/src/lib/core/overlay/position/viewport-ruler.ts +++ b/src/lib/core/overlay/position/viewport-ruler.ts @@ -17,7 +17,7 @@ export class ViewportRuler { this._cacheViewportGeometry(); // Subscribe to scroll and resize events and update the document rectangle on changes. - scrollDispatcher.scrolled().subscribe(() => this._cacheViewportGeometry()); + scrollDispatcher.scrolled(0, () => this._cacheViewportGeometry()); } /** Gets a ClientRect for the viewport's bounds. */ diff --git a/src/lib/core/overlay/scroll/scroll-dispatcher.spec.ts b/src/lib/core/overlay/scroll/scroll-dispatcher.spec.ts index 77342af30f20..2ffb8eb4b87a 100644 --- a/src/lib/core/overlay/scroll/scroll-dispatcher.spec.ts +++ b/src/lib/core/overlay/scroll/scroll-dispatcher.spec.ts @@ -48,7 +48,7 @@ describe('Scroll Dispatcher', () => { // Listen for notifications from scroll service with a throttle of 100ms const throttleTime = 100; let hasServiceScrollNotified = false; - scroll.scrolled(throttleTime).subscribe(() => { hasServiceScrollNotified = true; }); + scroll.scrolled(throttleTime, () => { hasServiceScrollNotified = true; }); // Emit a scroll event from the scrolling element in our component. // This event should be picked up by the scrollable directive and notify. @@ -90,6 +90,44 @@ describe('Scroll Dispatcher', () => { expect(scrollableElementIds).toEqual(['scrollable-1', 'scrollable-1a']); }); }); + + describe('lazy subscription', () => { + let scroll: ScrollDispatcher; + + beforeEach(inject([ScrollDispatcher], (s: ScrollDispatcher) => { + scroll = s; + })); + + it('should lazily add global listeners as Scrollable instances are added and removed', () => { + expect(scroll._globalSubscription).toBeNull('Expected no global listeners on init.'); + + let fixture = TestBed.createComponent(ScrollingComponent); + fixture.detectChanges(); + + expect(scroll._globalSubscription).toBeTruthy( + 'Expected global listeners after a scrollable is added.'); + + fixture.destroy(); + + expect(scroll._globalSubscription).toBeNull( + 'Expected global listeners to be removed after scrollable is destroyed.'); + }); + + it('should lazily add global listeners as service subscriptions are added and removed', () => { + expect(scroll._globalSubscription).toBeNull('Expected no global listeners on init.'); + + const subscription = scroll.scrolled(0, () => {}); + + expect(scroll._globalSubscription).toBeTruthy( + 'Expected global listeners after a subscription has been added.'); + + subscription.unsubscribe(); + + expect(scroll._globalSubscription).toBeNull( + 'Expected global listeners to have been removed after the subscription has stopped.'); + }); + + }); }); diff --git a/src/lib/core/overlay/scroll/scroll-dispatcher.ts b/src/lib/core/overlay/scroll/scroll-dispatcher.ts index 6edb0e982507..4ce316e7a146 100644 --- a/src/lib/core/overlay/scroll/scroll-dispatcher.ts +++ b/src/lib/core/overlay/scroll/scroll-dispatcher.ts @@ -21,7 +21,10 @@ export class ScrollDispatcher { _scrolled: Subject = new Subject(); /** Keeps track of the global `scroll` and `resize` subscriptions. */ - private _globalSubscription: Subscription; + _globalSubscription: Subscription = null; + + /** Keeps track of the amount of subscriptions to `scrolled`. Used for cleaning up afterwards. */ + private _scrolledCount = 0; /** * Map of all the scrollable references that are registered with the service and their @@ -36,14 +39,9 @@ export class ScrollDispatcher { */ register(scrollable: Scrollable): void { const scrollSubscription = scrollable.elementScrolled().subscribe(() => this._notify()); - this.scrollableReferences.set(scrollable, scrollSubscription); - if (!this._globalSubscription) { - this._globalSubscription = Observable.merge( - Observable.fromEvent(window.document, 'scroll'), - Observable.fromEvent(window, 'resize') - ).subscribe(() => this._notify()); - } + this.scrollableReferences.set(scrollable, scrollSubscription); + this._addGlobalSubscription(); } /** @@ -54,27 +52,31 @@ export class ScrollDispatcher { if (this.scrollableReferences.has(scrollable)) { this.scrollableReferences.get(scrollable).unsubscribe(); this.scrollableReferences.delete(scrollable); - - if (!this.scrollableReferences.size && this._globalSubscription) { - this._globalSubscription.unsubscribe(); - this._globalSubscription = null; - } + this._removeGlobalSubscription(); } } /** - * Returns an observable that emits an event whenever any of the registered Scrollable + * Subscribes to an observable that emits an event whenever any of the registered Scrollable * references (or window, document, or body) fire a scrolled event. Can provide a time in ms * to override the default "throttle" time. */ - scrolled(auditTimeInMs: number = DEFAULT_SCROLL_TIME): Observable { - // In the case of a 0ms delay, return the observable without auditTime since it does add - // a perceptible delay in processing overhead. - if (auditTimeInMs == 0) { - return this._scrolled.asObservable(); - } - - return this._scrolled.asObservable().auditTime(auditTimeInMs); + scrolled(auditTimeInMs: number = DEFAULT_SCROLL_TIME, callback: () => any): Subscription { + // In the case of a 0ms delay, use an observable without auditTime + // since it does add a perceptible delay in processing overhead. + let observable = auditTimeInMs > 0 ? + this._scrolled.asObservable().auditTime(auditTimeInMs) : + this._scrolled.asObservable(); + + this._scrolledCount++; + this._addGlobalSubscription(); + + // Note that we need to do the subscribing from here, in order to be able to remove + // the global event listeners once there are no more subscriptions. + return observable.subscribe(callback).add(() => { + this._scrolledCount--; + this._removeGlobalSubscription(); + }); } /** Returns all registered Scrollables that contain the provided element. */ @@ -106,6 +108,24 @@ export class ScrollDispatcher { _notify() { this._scrolled.next(); } + + /** Sets up the global event listeners, if they're not active already. */ + private _addGlobalSubscription(): void { + if (!this._globalSubscription) { + this._globalSubscription = Observable.merge( + Observable.fromEvent(window.document, 'scroll'), + Observable.fromEvent(window, 'resize') + ).subscribe(() => this._notify()); + } + } + + /** Removes the global event listeners, if there are no more subscriptions listening to them. */ + private _removeGlobalSubscription(): void { + if (this._globalSubscription && !this.scrollableReferences.size && !this._scrolledCount) { + this._globalSubscription.unsubscribe(); + this._globalSubscription = null; + } + } } export function SCROLL_DISPATCHER_PROVIDER_FACTORY(parentDispatcher: ScrollDispatcher) { diff --git a/src/lib/tooltip/tooltip.ts b/src/lib/tooltip/tooltip.ts index fc621826beaf..e146c4540655 100644 --- a/src/lib/tooltip/tooltip.ts +++ b/src/lib/tooltip/tooltip.ts @@ -150,7 +150,7 @@ export class MdTooltip implements OnInit, OnDestroy { ngOnInit() { // When a scroll on the page occurs, update the position in case this tooltip needs // to be repositioned. - this.scrollSubscription = this._scrollDispatcher.scrolled(SCROLL_THROTTLE_MS).subscribe(() => { + this.scrollSubscription = this._scrollDispatcher.scrolled(SCROLL_THROTTLE_MS, () => { if (this._overlayRef) { this._overlayRef.updatePosition(); } From be043631e656b7a00e79e296c56832804fe2ee88 Mon Sep 17 00:00:00 2001 From: crisbeto Date: Mon, 27 Feb 2017 22:36:59 +0100 Subject: [PATCH 3/4] fix: wrong value being passed in --- src/lib/core/overlay/position/viewport-ruler.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/lib/core/overlay/position/viewport-ruler.ts b/src/lib/core/overlay/position/viewport-ruler.ts index c87be208a021..2df3fe3ed5b0 100644 --- a/src/lib/core/overlay/position/viewport-ruler.ts +++ b/src/lib/core/overlay/position/viewport-ruler.ts @@ -17,7 +17,7 @@ export class ViewportRuler { this._cacheViewportGeometry(); // Subscribe to scroll and resize events and update the document rectangle on changes. - scrollDispatcher.scrolled(0, () => this._cacheViewportGeometry()); + scrollDispatcher.scrolled(null, () => this._cacheViewportGeometry()); } /** Gets a ClientRect for the viewport's bounds. */ From fc105e36802aac5c928f9ffee2752032bc06c217 Mon Sep 17 00:00:00 2001 From: crisbeto Date: Thu, 2 Mar 2017 21:03:37 +0100 Subject: [PATCH 4/4] fix: don't add global listener for scrollables --- .../overlay/scroll/scroll-dispatcher.spec.ts | 15 -------- .../core/overlay/scroll/scroll-dispatcher.ts | 34 +++++++------------ 2 files changed, 12 insertions(+), 37 deletions(-) diff --git a/src/lib/core/overlay/scroll/scroll-dispatcher.spec.ts b/src/lib/core/overlay/scroll/scroll-dispatcher.spec.ts index 2ffb8eb4b87a..8b53de57666f 100644 --- a/src/lib/core/overlay/scroll/scroll-dispatcher.spec.ts +++ b/src/lib/core/overlay/scroll/scroll-dispatcher.spec.ts @@ -98,21 +98,6 @@ describe('Scroll Dispatcher', () => { scroll = s; })); - it('should lazily add global listeners as Scrollable instances are added and removed', () => { - expect(scroll._globalSubscription).toBeNull('Expected no global listeners on init.'); - - let fixture = TestBed.createComponent(ScrollingComponent); - fixture.detectChanges(); - - expect(scroll._globalSubscription).toBeTruthy( - 'Expected global listeners after a scrollable is added.'); - - fixture.destroy(); - - expect(scroll._globalSubscription).toBeNull( - 'Expected global listeners to be removed after scrollable is destroyed.'); - }); - it('should lazily add global listeners as service subscriptions are added and removed', () => { expect(scroll._globalSubscription).toBeNull('Expected no global listeners on init.'); diff --git a/src/lib/core/overlay/scroll/scroll-dispatcher.ts b/src/lib/core/overlay/scroll/scroll-dispatcher.ts index 4ce316e7a146..8542aac102ac 100644 --- a/src/lib/core/overlay/scroll/scroll-dispatcher.ts +++ b/src/lib/core/overlay/scroll/scroll-dispatcher.ts @@ -41,7 +41,6 @@ export class ScrollDispatcher { const scrollSubscription = scrollable.elementScrolled().subscribe(() => this._notify()); this.scrollableReferences.set(scrollable, scrollSubscription); - this._addGlobalSubscription(); } /** @@ -52,7 +51,6 @@ export class ScrollDispatcher { if (this.scrollableReferences.has(scrollable)) { this.scrollableReferences.get(scrollable).unsubscribe(); this.scrollableReferences.delete(scrollable); - this._removeGlobalSubscription(); } } @@ -69,13 +67,23 @@ export class ScrollDispatcher { this._scrolled.asObservable(); this._scrolledCount++; - this._addGlobalSubscription(); + + if (!this._globalSubscription) { + this._globalSubscription = 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 // the global event listeners once there are no more subscriptions. return observable.subscribe(callback).add(() => { this._scrolledCount--; - this._removeGlobalSubscription(); + + if (this._globalSubscription && !this.scrollableReferences.size && !this._scrolledCount) { + this._globalSubscription.unsubscribe(); + this._globalSubscription = null; + } }); } @@ -108,24 +116,6 @@ export class ScrollDispatcher { _notify() { this._scrolled.next(); } - - /** Sets up the global event listeners, if they're not active already. */ - private _addGlobalSubscription(): void { - if (!this._globalSubscription) { - this._globalSubscription = Observable.merge( - Observable.fromEvent(window.document, 'scroll'), - Observable.fromEvent(window, 'resize') - ).subscribe(() => this._notify()); - } - } - - /** Removes the global event listeners, if there are no more subscriptions listening to them. */ - private _removeGlobalSubscription(): void { - if (this._globalSubscription && !this.scrollableReferences.size && !this._scrolledCount) { - this._globalSubscription.unsubscribe(); - this._globalSubscription = null; - } - } } export function SCROLL_DISPATCHER_PROVIDER_FACTORY(parentDispatcher: ScrollDispatcher) {