Skip to content

Commit d6698e1

Browse files
crisbetoandrewseguin
authored andcommitted
perf: remove persistent global scroll listener (#7560)
* Removes the global scroll listener from the viewport ruler, which was being used to invalidate the scroll position cache. Caching the scroll position is pointless since it'll be invalidated very frequently. Since most Material components depend on the `ViewportRuler` one way or another, it meant that there was a good chance that the scroll listener would stay around forever. * Fixes an issue that caused the scroll dispatcher not to remove its global scroll listener, if there are any scrollables on the page, even though nothing was subscribed to it. Fixes #6882.
1 parent 5cffd7c commit d6698e1

File tree

3 files changed

+29
-13
lines changed

3 files changed

+29
-13
lines changed

src/cdk/scrolling/scroll-dispatcher.spec.ts

Lines changed: 20 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -143,6 +143,26 @@ describe('Scroll Dispatcher', () => {
143143
'Expected global listeners to have been removed after the subscription has stopped.');
144144
});
145145

146+
it('should remove global listeners on unsubscribe, despite any other live scrollables', () => {
147+
const fixture = TestBed.createComponent(NestedScrollingComponent);
148+
fixture.detectChanges();
149+
150+
expect(scroll._globalSubscription).toBeNull('Expected no global listeners on init.');
151+
expect(scroll.scrollableReferences.size).toBe(4, 'Expected multiple scrollables');
152+
153+
const subscription = scroll.scrolled(0).subscribe(() => {});
154+
155+
expect(scroll._globalSubscription).toBeTruthy(
156+
'Expected global listeners after a subscription has been added.');
157+
158+
subscription.unsubscribe();
159+
160+
expect(scroll._globalSubscription).toBeNull(
161+
'Expected global listeners to have been removed after the subscription has stopped.');
162+
expect(scroll.scrollableReferences.size)
163+
.toBe(4, 'Expected scrollable count to stay the same');
164+
});
165+
146166
});
147167
});
148168

src/cdk/scrolling/scroll-dispatcher.ts

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -89,7 +89,7 @@ export class ScrollDispatcher {
8989
subscription.unsubscribe();
9090
this._scrolledCount--;
9191

92-
if (this._globalSubscription && !this.scrollableReferences.size && !this._scrolledCount) {
92+
if (this._globalSubscription && !this._scrolledCount) {
9393
this._globalSubscription.unsubscribe();
9494
this._globalSubscription = null;
9595
}

src/cdk/scrolling/viewport-ruler.ts

Lines changed: 8 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -8,7 +8,6 @@
88

99
import {Injectable, Optional, SkipSelf, NgZone, OnDestroy} from '@angular/core';
1010
import {Platform} from '@angular/cdk/platform';
11-
import {ScrollDispatcher} from './scroll-dispatcher';
1211
import {Observable} from 'rxjs/Observable';
1312
import {fromEvent} from 'rxjs/observable/fromEvent';
1413
import {merge} from 'rxjs/observable/merge';
@@ -32,21 +31,19 @@ export class ViewportRuler implements OnDestroy {
3231
/** Stream of viewport change events. */
3332
private _change: Observable<Event>;
3433

35-
/** Subscriptions to streams that invalidate the cached viewport dimensions. */
36-
private _invalidateCacheSubscription = Subscription.EMPTY;
34+
/** Subscription to streams that invalidate the cached viewport dimensions. */
35+
private _invalidateCache: Subscription;
3736

38-
constructor(platform: Platform, ngZone: NgZone, scrollDispatcher: ScrollDispatcher) {
37+
constructor(platform: Platform, ngZone: NgZone) {
3938
this._change = platform.isBrowser ? ngZone.runOutsideAngular(() => {
4039
return merge<Event>(fromEvent(window, 'resize'), fromEvent(window, 'orientationchange'));
4140
}) : observableOf();
4241

43-
// Subscribe to scroll and resize events and update the document rectangle on changes.
44-
this._invalidateCacheSubscription = merge(scrollDispatcher.scrolled(0), this.change())
45-
.subscribe(() => this._cacheViewportGeometry());
42+
this._invalidateCache = this.change().subscribe(() => this._cacheViewportGeometry());
4643
}
4744

4845
ngOnDestroy() {
49-
this._invalidateCacheSubscription.unsubscribe();
46+
this._invalidateCache.unsubscribe();
5047
}
5148

5249
/** Gets a ClientRect for the viewport's bounds. */
@@ -123,15 +120,14 @@ export class ViewportRuler implements OnDestroy {
123120
/** @docs-private */
124121
export function VIEWPORT_RULER_PROVIDER_FACTORY(parentRuler: ViewportRuler,
125122
platform: Platform,
126-
ngZone: NgZone,
127-
scrollDispatcher: ScrollDispatcher) {
128-
return parentRuler || new ViewportRuler(platform, ngZone, scrollDispatcher);
123+
ngZone: NgZone) {
124+
return parentRuler || new ViewportRuler(platform, ngZone);
129125
}
130126

131127
/** @docs-private */
132128
export const VIEWPORT_RULER_PROVIDER = {
133129
// If there is already a ViewportRuler available, use that. Otherwise, provide a new one.
134130
provide: ViewportRuler,
135-
deps: [[new Optional(), new SkipSelf(), ViewportRuler], Platform, NgZone, ScrollDispatcher],
131+
deps: [[new Optional(), new SkipSelf(), ViewportRuler], Platform, NgZone],
136132
useFactory: VIEWPORT_RULER_PROVIDER_FACTORY
137133
};

0 commit comments

Comments
 (0)