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 (#28846)

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

This fixes an issue with the virtual scroll when run coalescing is
turned on which caused the virtual scroll viewport to incorrectly
determine when items has been rendered.

Co-authored-by: Andrew Scott <atscott@google.com>

* fix(cdk/scrolling): Ensure we don't detect changes after destroy

---------

Co-authored-by: Andrew Scott <atscott@google.com>
  • Loading branch information
mmalerba and atscott committed Apr 10, 2024
1 parent 8340e62 commit d91d0d4
Show file tree
Hide file tree
Showing 2 changed files with 34 additions and 20 deletions.
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}),
],
});

0 comments on commit d91d0d4

Please sign in to comment.