From 442f7396a438b8ffa3471a2996d6da26cd9bab53 Mon Sep 17 00:00:00 2001 From: Kristiyan Kostadinov Date: Tue, 8 Apr 2025 10:13:15 +0200 Subject: [PATCH] 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 https://github.com/angular/components/issues/30586#issuecomment-2706211575. 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. --- src/cdk/scrolling/scroll-dispatcher.spec.ts | 18 ++++++++++++- src/cdk/scrolling/scroll-dispatcher.ts | 30 ++++++++++++++++++--- 2 files changed, 44 insertions(+), 4 deletions(-) diff --git a/src/cdk/scrolling/scroll-dispatcher.spec.ts b/src/cdk/scrolling/scroll-dispatcher.spec.ts index 530547bccd51..97c989652569 100644 --- a/src/cdk/scrolling/scroll-dispatcher.spec.ts +++ b/src/cdk/scrolling/scroll-dispatcher.spec.ts @@ -9,6 +9,7 @@ import { import {Component, ViewChild, ElementRef} from '@angular/core'; import {CdkScrollable, ScrollDispatcher, ScrollingModule} from './public-api'; import {dispatchFakeEvent} from '../testing/private'; +import {filter} from 'rxjs/operators'; describe('ScrollDispatcher', () => { beforeEach(waitForAsync(() => { @@ -106,7 +107,10 @@ describe('ScrollDispatcher', () => { it('should not register the same scrollable twice', () => { const scrollable = fixture.componentInstance.scrollable; const scrollSpy = jasmine.createSpy('scroll spy'); - const scrollSubscription = scroll.scrolled(0).subscribe(scrollSpy); + const scrollSubscription = scroll + .scrolled(0) + .pipe(filter(target => target === scrollable)) + .subscribe(scrollSpy); expect(scroll.scrollContainers.has(scrollable)).toBe(true); @@ -119,6 +123,18 @@ describe('ScrollDispatcher', () => { expect(scrollSpy).not.toHaveBeenCalled(); scrollSubscription.unsubscribe(); }); + + it('should register a capturing scroll event on the document', () => { + const spy = spyOn(document, 'addEventListener').and.callThrough(); + const subscription = scroll.scrolled(0).subscribe(); + + expect(spy).toHaveBeenCalledWith( + 'scroll', + jasmine.any(Function), + jasmine.objectContaining({capture: true}), + ); + subscription.unsubscribe(); + }); }); describe('Nested scrollables', () => { diff --git a/src/cdk/scrolling/scroll-dispatcher.ts b/src/cdk/scrolling/scroll-dispatcher.ts index 912cb3f79e52..e09d771c30c1 100644 --- a/src/cdk/scrolling/scroll-dispatcher.ts +++ b/src/cdk/scrolling/scroll-dispatcher.ts @@ -8,7 +8,15 @@ import {coerceElement} from '../coercion'; import {Platform} from '../platform'; -import {ElementRef, Injectable, NgZone, OnDestroy, RendererFactory2, inject} from '@angular/core'; +import { + DOCUMENT, + ElementRef, + Injectable, + NgZone, + OnDestroy, + RendererFactory2, + inject, +} from '@angular/core'; import {of as observableOf, Subject, Subscription, Observable, Observer} from 'rxjs'; import {auditTime, filter} from 'rxjs/operators'; import type {CdkScrollable} from './scrollable'; @@ -25,7 +33,9 @@ export class ScrollDispatcher implements OnDestroy { private _ngZone = inject(NgZone); private _platform = inject(Platform); private _renderer = inject(RendererFactory2).createRenderer(null, null); + private _document = inject(DOCUMENT); private _cleanupGlobalListener: (() => void) | undefined; + private _lastScrollFromDocument = false; constructor(...args: unknown[]); constructor() {} @@ -87,7 +97,15 @@ export class ScrollDispatcher implements OnDestroy { return new Observable((observer: Observer) => { if (!this._cleanupGlobalListener) { this._cleanupGlobalListener = this._ngZone.runOutsideAngular(() => - this._renderer.listen('document', 'scroll', () => this._scrolled.next()), + this._renderer.listen( + 'document', + 'scroll', + (event: Event) => { + this._lastScrollFromDocument = event.target === this._document; + this._scrolled.next(); + }, + {capture: true}, + ), ); } @@ -105,6 +123,7 @@ export class ScrollDispatcher implements OnDestroy { this._scrolledCount--; if (!this._scrolledCount) { + this._lastScrollFromDocument = false; this._cleanupGlobalListener?.(); this._cleanupGlobalListener = undefined; } @@ -132,7 +151,12 @@ export class ScrollDispatcher implements OnDestroy { const ancestors = this.getAncestorScrollContainers(elementOrElementRef); return this.scrolled(auditTimeInMs).pipe( - filter(target => !target || ancestors.indexOf(target) > -1), + filter(target => { + // The document is using capturing for its `scroll` event which means that we'll usually + // get two events here. This is what we want in most cases, but for the ancestor scrolling + // we actually want to know the exact ancestor that was scrolled. + return target ? ancestors.indexOf(target) > -1 : this._lastScrollFromDocument; + }), ); }