Skip to content

Commit

Permalink
fix(cdk/scrolling): fix virtual scrolling jankiness with run coalesci…
Browse files Browse the repository at this point in the history
…ng (#28968)

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

This reverts commit ee9abb8.

* fix(cdk/scrolling): Move setting transform outside `afterNextRender`

(cherry picked from commit 32d2683)
  • Loading branch information
mmalerba committed Apr 30, 2024
1 parent 1d7f2be commit c8b62a1
Show file tree
Hide file tree
Showing 2 changed files with 39 additions and 20 deletions.
57 changes: 38 additions & 19 deletions src/cdk/scrolling/virtual-scroll-viewport.ts
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,34 @@ 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(() => {
// 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._changeDetectorRef.markForCheck();

// 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;

afterNextRender(
() => {
this._isChangeDetectionPending = false;
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
Expand Up @@ -53,6 +53,6 @@ bootstrapApplication(DevApp, {
{provide: Directionality, useClass: DevAppDirectionality},
cachedAppState.zoneless
? provideExperimentalZonelessChangeDetection()
: provideZoneChangeDetection({eventCoalescing: true}),
: provideZoneChangeDetection({eventCoalescing: true, runCoalescing: true}),
],
});

0 comments on commit c8b62a1

Please sign in to comment.