Skip to content

Commit aabdc3b

Browse files
committed
perf: remove persistent global scroll listener
* 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 630dfad commit aabdc3b

File tree

3 files changed

+30
-15
lines changed

3 files changed

+30
-15
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, () => {});
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
@@ -97,7 +97,7 @@ export class ScrollDispatcher {
9797
subscription.add(() => {
9898
this._scrolledCount--;
9999

100-
if (this._globalSubscription && !this.scrollableReferences.size && !this._scrolledCount) {
100+
if (this._globalSubscription && !this._scrolledCount) {
101101
this._globalSubscription.unsubscribe();
102102
this._globalSubscription = null;
103103
}

src/cdk/scrolling/viewport-ruler.ts

Lines changed: 9 additions & 14 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,23 +31,20 @@ 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 _invalidateCacheSubscriptions: Subscription[];
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._invalidateCacheSubscriptions = [
45-
scrollDispatcher.scrolled(0, () => this._cacheViewportGeometry()),
46-
this.change().subscribe(() => this._cacheViewportGeometry())
47-
];
42+
// Update the cached viewport size on changes.
43+
this._invalidateCache = this.change().subscribe(() => this._cacheViewportGeometry());
4844
}
4945

5046
ngOnDestroy() {
51-
this._invalidateCacheSubscriptions.forEach(subscription => subscription.unsubscribe());
47+
this._invalidateCache.unsubscribe();
5248
}
5349

5450
/** Gets a ClientRect for the viewport's bounds. */
@@ -125,15 +121,14 @@ export class ViewportRuler implements OnDestroy {
125121
/** @docs-private */
126122
export function VIEWPORT_RULER_PROVIDER_FACTORY(parentRuler: ViewportRuler,
127123
platform: Platform,
128-
ngZone: NgZone,
129-
scrollDispatcher: ScrollDispatcher) {
130-
return parentRuler || new ViewportRuler(platform, ngZone, scrollDispatcher);
124+
ngZone: NgZone) {
125+
return parentRuler || new ViewportRuler(platform, ngZone);
131126
}
132127

133128
/** @docs-private */
134129
export const VIEWPORT_RULER_PROVIDER = {
135130
// If there is already a ViewportRuler available, use that. Otherwise, provide a new one.
136131
provide: ViewportRuler,
137-
deps: [[new Optional(), new SkipSelf(), ViewportRuler], Platform, NgZone, ScrollDispatcher],
132+
deps: [[new Optional(), new SkipSelf(), ViewportRuler], Platform, NgZone],
138133
useFactory: VIEWPORT_RULER_PROVIDER_FACTORY
139134
};

0 commit comments

Comments
 (0)