Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(cdk/scrolling): fix virtual scrolling jankiness with run coalescing #28846

Merged
merged 2 commits into from
Apr 10, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
52 changes: 33 additions & 19 deletions src/cdk/scrolling/virtual-scroll-viewport.ts
Original file line number Diff line number Diff line change
Expand Up @@ -8,14 +8,17 @@

import {Directionality} from '@angular/cdk/bidi';
import {ListRange} from '@angular/cdk/collections';
import {Platform} from '@angular/cdk/platform';
import {
afterNextRender,
booleanAttribute,
ChangeDetectionStrategy,
ChangeDetectorRef,
Component,
ElementRef,
inject,
Inject,
Injector,
Input,
NgZone,
OnDestroy,
Expand All @@ -25,21 +28,20 @@ import {
ViewChild,
ViewEncapsulation,
} from '@angular/core';
import {Platform} from '@angular/cdk/platform';
import {
animationFrameScheduler,
asapScheduler,
Observable,
Subject,
Observer,
Subject,
Subscription,
} from 'rxjs';
import {auditTime, startWith, takeUntil} from 'rxjs/operators';
import {ScrollDispatcher} from './scroll-dispatcher';
import {CdkScrollable, ExtendedScrollToOptions} from './scrollable';
import {VIRTUAL_SCROLL_STRATEGY, VirtualScrollStrategy} from './virtual-scroll-strategy';
import {ViewportRuler} from './viewport-ruler';
import {CdkVirtualScrollRepeater} from './virtual-scroll-repeater';
import {VIRTUAL_SCROLL_STRATEGY, VirtualScrollStrategy} from './virtual-scroll-strategy';
import {CdkVirtualScrollable, VIRTUAL_SCROLLABLE} from './virtual-scrollable';

/** Checks if the given ranges are equal. */
Expand Down Expand Up @@ -173,6 +175,10 @@ export class CdkVirtualScrollViewport extends CdkVirtualScrollable implements On
/** Subscription to changes in the viewport size. */
private _viewportChanges = Subscription.EMPTY;

private _injector = inject(Injector);

private _isDestroyed = false;

constructor(
public override elementRef: ElementRef<HTMLElement>,
private _changeDetectorRef: ChangeDetectorRef,
Expand Down Expand Up @@ -250,6 +256,8 @@ export class CdkVirtualScrollViewport extends CdkVirtualScrollable implements On
this._detachedSubject.complete();
this._viewportChanges.unsubscribe();

this._isDestroyed = true;

super.ngOnDestroy();
}

Expand Down Expand Up @@ -498,23 +506,29 @@ export class CdkVirtualScrollViewport extends CdkVirtualScrollable implements On

/** Run change detection. */
private _doChangeDetection() {
this._isChangeDetectionPending = false;

// Apply the content transform. The transform can't be set via an Angular binding because
// bypassSecurityTrustStyle is banned in Google. However the value is safe, it's composed of
// string literals, a variable that can only be 'X' or 'Y', and user input that is run through
// the `Number` function first to coerce it to a numeric value.
this._contentWrapper.nativeElement.style.transform = this._renderedContentTransform;
// Apply changes to Angular bindings. Note: We must call `markForCheck` to run change detection
// from the root, since the repeated items are content projected in. Calling `detectChanges`
// instead does not properly check the projected content.
this.ngZone.run(() => this._changeDetectorRef.markForCheck());

const runAfterChangeDetection = this._runAfterChangeDetection;
this._runAfterChangeDetection = [];
for (const fn of runAfterChangeDetection) {
fn();
if (this._isDestroyed) {
return;
}

this.ngZone.run(() => {
this._changeDetectorRef.markForCheck();
afterNextRender(
() => {
this._isChangeDetectionPending = false;
// Apply the content transform. The transform can't be set via an Angular binding because
// bypassSecurityTrustStyle is banned in Google. However the value is safe, it's composed of
// string literals, a variable that can only be 'X' or 'Y', and user input that is run through
// the `Number` function first to coerce it to a numeric value.
this._contentWrapper.nativeElement.style.transform = this._renderedContentTransform;
const runAfterChangeDetection = this._runAfterChangeDetection;
this._runAfterChangeDetection = [];
for (const fn of runAfterChangeDetection) {
fn();
}
},
{injector: this._injector},
);
});
}

/** Calculates the `style.width` and `style.height` for the spacer element. */
Expand Down
2 changes: 1 addition & 1 deletion src/dev-app/main.ts
Original file line number Diff line number Diff line change
Expand Up @@ -53,6 +53,6 @@ bootstrapApplication(DevApp, {
{provide: Directionality, useClass: DevAppDirectionality},
cachedAppState.zoneless
? ɵprovideZonelessChangeDetection()
: provideZoneChangeDetection({eventCoalescing: true}),
: provideZoneChangeDetection({eventCoalescing: true, runCoalescing: true}),
],
});