Skip to content

Commit 442f739

Browse files
committed
fix(cdk/scrolling): use capturing for scroll event
Historically we have asked users to indicate their scrollable regions with `CdkScrollable` so that we don't over-subscribe to scroll events, however the downside is that it's very easy to forget to add it or to misspell the name. This becomes an even larger issue with standalone, because users also have to import `CdkScrollable`. As a result, we've had lots of issue reports related to scrolling in the past, the most recent being #30586 (comment). These changes switch to using a root capturing event on the `body` instead which will notify us of all scroll events automatically and will remove the need for users to set `cdkScrollable` in most cases.
1 parent b1cc050 commit 442f739

File tree

2 files changed

+44
-4
lines changed

2 files changed

+44
-4
lines changed

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

Lines changed: 17 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -9,6 +9,7 @@ import {
99
import {Component, ViewChild, ElementRef} from '@angular/core';
1010
import {CdkScrollable, ScrollDispatcher, ScrollingModule} from './public-api';
1111
import {dispatchFakeEvent} from '../testing/private';
12+
import {filter} from 'rxjs/operators';
1213

1314
describe('ScrollDispatcher', () => {
1415
beforeEach(waitForAsync(() => {
@@ -106,7 +107,10 @@ describe('ScrollDispatcher', () => {
106107
it('should not register the same scrollable twice', () => {
107108
const scrollable = fixture.componentInstance.scrollable;
108109
const scrollSpy = jasmine.createSpy('scroll spy');
109-
const scrollSubscription = scroll.scrolled(0).subscribe(scrollSpy);
110+
const scrollSubscription = scroll
111+
.scrolled(0)
112+
.pipe(filter(target => target === scrollable))
113+
.subscribe(scrollSpy);
110114

111115
expect(scroll.scrollContainers.has(scrollable)).toBe(true);
112116

@@ -119,6 +123,18 @@ describe('ScrollDispatcher', () => {
119123
expect(scrollSpy).not.toHaveBeenCalled();
120124
scrollSubscription.unsubscribe();
121125
});
126+
127+
it('should register a capturing scroll event on the document', () => {
128+
const spy = spyOn(document, 'addEventListener').and.callThrough();
129+
const subscription = scroll.scrolled(0).subscribe();
130+
131+
expect(spy).toHaveBeenCalledWith(
132+
'scroll',
133+
jasmine.any(Function),
134+
jasmine.objectContaining({capture: true}),
135+
);
136+
subscription.unsubscribe();
137+
});
122138
});
123139

124140
describe('Nested scrollables', () => {

src/cdk/scrolling/scroll-dispatcher.ts

Lines changed: 27 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -8,7 +8,15 @@
88

99
import {coerceElement} from '../coercion';
1010
import {Platform} from '../platform';
11-
import {ElementRef, Injectable, NgZone, OnDestroy, RendererFactory2, inject} from '@angular/core';
11+
import {
12+
DOCUMENT,
13+
ElementRef,
14+
Injectable,
15+
NgZone,
16+
OnDestroy,
17+
RendererFactory2,
18+
inject,
19+
} from '@angular/core';
1220
import {of as observableOf, Subject, Subscription, Observable, Observer} from 'rxjs';
1321
import {auditTime, filter} from 'rxjs/operators';
1422
import type {CdkScrollable} from './scrollable';
@@ -25,7 +33,9 @@ export class ScrollDispatcher implements OnDestroy {
2533
private _ngZone = inject(NgZone);
2634
private _platform = inject(Platform);
2735
private _renderer = inject(RendererFactory2).createRenderer(null, null);
36+
private _document = inject(DOCUMENT);
2837
private _cleanupGlobalListener: (() => void) | undefined;
38+
private _lastScrollFromDocument = false;
2939

3040
constructor(...args: unknown[]);
3141
constructor() {}
@@ -87,7 +97,15 @@ export class ScrollDispatcher implements OnDestroy {
8797
return new Observable((observer: Observer<CdkScrollable | void>) => {
8898
if (!this._cleanupGlobalListener) {
8999
this._cleanupGlobalListener = this._ngZone.runOutsideAngular(() =>
90-
this._renderer.listen('document', 'scroll', () => this._scrolled.next()),
100+
this._renderer.listen(
101+
'document',
102+
'scroll',
103+
(event: Event) => {
104+
this._lastScrollFromDocument = event.target === this._document;
105+
this._scrolled.next();
106+
},
107+
{capture: true},
108+
),
91109
);
92110
}
93111

@@ -105,6 +123,7 @@ export class ScrollDispatcher implements OnDestroy {
105123
this._scrolledCount--;
106124

107125
if (!this._scrolledCount) {
126+
this._lastScrollFromDocument = false;
108127
this._cleanupGlobalListener?.();
109128
this._cleanupGlobalListener = undefined;
110129
}
@@ -132,7 +151,12 @@ export class ScrollDispatcher implements OnDestroy {
132151
const ancestors = this.getAncestorScrollContainers(elementOrElementRef);
133152

134153
return this.scrolled(auditTimeInMs).pipe(
135-
filter(target => !target || ancestors.indexOf(target) > -1),
154+
filter(target => {
155+
// The document is using capturing for its `scroll` event which means that we'll usually
156+
// get two events here. This is what we want in most cases, but for the ancestor scrolling
157+
// we actually want to know the exact ancestor that was scrolled.
158+
return target ? ancestors.indexOf(target) > -1 : this._lastScrollFromDocument;
159+
}),
136160
);
137161
}
138162

0 commit comments

Comments
 (0)