Skip to content

virtual-scroll: use detectChanges rather than markForCheck in CdkVirtualScrollViewport #12158

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.

Sign up for GitHub

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

Merged
merged 3 commits into from
Jul 12, 2018
Merged

virtual-scroll: use detectChanges rather than markForCheck in CdkVirtualScrollViewport #12158

Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
8438ba8
virtual-scroll: use `detectChanges` rather than `markForCheck` in
mmalerba Jul 11, 2018
38afb9e
fix lint and tests
mmalerba Jul 11, 2018
138e2be
address comments
mmalerba Jul 11, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
151 changes: 77 additions & 74 deletions src/cdk-experimental/scrolling/virtual-scroll-viewport.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -48,7 +48,7 @@ function rangesEqual(r1: ListRange, r2: ListRange): boolean {
encapsulation: ViewEncapsulation.None,
changeDetection: ChangeDetectionStrategy.OnPush,
})
export class CdkVirtualScrollViewport implements DoCheck, OnInit, OnDestroy {
export class CdkVirtualScrollViewport implements OnInit, OnDestroy {
/** Emits when the viewport is detached from a CdkVirtualForOf. */
private _detachedSubject = new Subject<void>();

Expand Down Expand Up @@ -102,37 +102,29 @@ export class CdkVirtualScrollViewport implements DoCheck, OnInit, OnDestroy {
/** Observable that emits when the viewport is destroyed. */
private _destroyed = new Subject<void>();

/** Whether there is a pending change detection cycle. */
private _checkPending = false;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

_isPendingChangeDetection or _needsChangeDetection?


/** A list of functions to run after the next change detection cycle. */
private _runAfterCheck: Function[] = [];

constructor(public elementRef: ElementRef, private _changeDetectorRef: ChangeDetectorRef,
private _ngZone: NgZone, private _sanitizer: DomSanitizer,
@Inject(VIRTUAL_SCROLL_STRATEGY) private _scrollStrategy: VirtualScrollStrategy) {}

ngOnInit() {
// It's still too early to measure the viewport at this point. Deferring with a promise allows
// the Viewport to be rendered with the correct size before we measure.
Promise.resolve().then(() => {
this._ngZone.runOutsideAngular(() => Promise.resolve().then(() => {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Expand comment to explain why you run outside the zone?

this._measureViewportSize();
this._scrollStrategy.attach(this);

this._ngZone.runOutsideAngular(() => {
fromEvent(this.elementRef.nativeElement, 'scroll')
// Sample the scroll stream at every animation frame. This way if there are multiple
// scroll events in the same frame we only need to recheck our layout once.
.pipe(sampleTime(0, animationFrameScheduler), takeUntil(this._destroyed))
.subscribe(() => this._scrollStrategy.onContentScrolled());
});
});
}

ngDoCheck() {
// In order to batch setting the scroll offset together with other DOM writes, we wait until a
// change detection cycle to actually apply it.
if (this._pendingScrollOffset != null) {
if (this.orientation === 'horizontal') {
this.elementRef.nativeElement.scrollLeft = this._pendingScrollOffset;
} else {
this.elementRef.nativeElement.scrollTop = this._pendingScrollOffset;
}
}
fromEvent(this.elementRef.nativeElement, 'scroll')
// Sample the scroll stream at every animation frame. This way if there are multiple
// scroll events in the same frame we only need to recheck our layout once.
.pipe(sampleTime(0, animationFrameScheduler), takeUntil(this._destroyed))
.subscribe(() => this._scrollStrategy.onContentScrolled());
}));
}

ngOnDestroy() {
Expand All @@ -151,16 +143,18 @@ export class CdkVirtualScrollViewport implements DoCheck, OnInit, OnDestroy {
if (this._forOf) {
throw Error('CdkVirtualScrollViewport is already attached.');
}
this._forOf = forOf;

// Subscribe to the data stream of the CdkVirtualForOf to keep track of when the data length
// changes.
this._forOf.dataStream.pipe(takeUntil(this._detachedSubject)).subscribe(data => {
const len = data.length;
if (len !== this._dataLength) {
this._dataLength = len;
this._scrollStrategy.onDataLengthChanged();
}
this._ngZone.runOutsideAngular(() => {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Add comment that explains why this is going out of the zone?

this._forOf = forOf;
this._forOf.dataStream.pipe(takeUntil(this._detachedSubject)).subscribe(data => {
const len = data.length;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should this be called something like newLength?

if (len !== this._dataLength) {
this._dataLength = len;
this._scrollStrategy.onDataLengthChanged();
}
});
});
}

Expand Down Expand Up @@ -190,40 +184,22 @@ export class CdkVirtualScrollViewport implements DoCheck, OnInit, OnDestroy {
return this._renderedRange;
}

// TODO(mmalebra): Consider calling `detectChanges()` directly rather than the methods below.

/**
* Sets the total size of all content (in pixels), including content that is not currently
* rendered.
*/
setTotalContentSize(size: number) {
if (this._totalContentSize !== size) {
// Re-enter the Angular zone so we can mark for change detection.
this._ngZone.run(() => {
this._totalContentSize = size;
this._changeDetectorRef.markForCheck();
});
this._totalContentSize = size;
this._markForCheck();
}
}

/** Sets the currently rendered range of indices. */
setRenderedRange(range: ListRange) {
if (!rangesEqual(this._renderedRange, range)) {
// Re-enter the Angular zone so we can mark for change detection.
this._ngZone.run(() => {
this._renderedRangeSubject.next(this._renderedRange = range);
this._changeDetectorRef.markForCheck();
});
// Queue this up in a `Promise.resolve()` so that if the user makes a series of calls
// like:
//
// viewport.setRenderedRange(...);
// viewport.setTotalContentSize(...);
// viewport.setRenderedContentOffset(...);
//
// The call to `onContentRendered` will happen after all of the updates have been applied.
this._ngZone.runOutsideAngular(() => this._ngZone.onStable.pipe(take(1)).subscribe(
() => Promise.resolve().then(() => this._scrollStrategy.onContentRendered())));
this._renderedRangeSubject.next(this._renderedRange = range);
this._markForCheck(() => this._scrollStrategy.onContentRendered());
}
}

Expand All @@ -250,25 +226,18 @@ export class CdkVirtualScrollViewport implements DoCheck, OnInit, OnDestroy {
this._renderedContentOffsetNeedsRewrite = true;
}
if (this._rawRenderedContentTransform != transform) {
// Re-enter the Angular zone so we can mark for change detection.
this._ngZone.run(() => {
// We know this value is safe because we parse `offset` with `Number()` before passing it
// into the string.
this._rawRenderedContentTransform = transform;
this._renderedContentTransform = this._sanitizer.bypassSecurityTrustStyle(transform);
this._changeDetectorRef.markForCheck();

// If the rendered content offset was specified as an offset to the end of the content,
// rewrite it as an offset to the start of the content.
this._ngZone.onStable.pipe(take(1)).subscribe(() => {
if (this._renderedContentOffsetNeedsRewrite) {
this._renderedContentOffset -= this.measureRenderedContentSize();
this._renderedContentOffsetNeedsRewrite = false;
this.setRenderedContentOffset(this._renderedContentOffset);
} else {
this._scrollStrategy.onRenderedOffsetChanged();
}
});
// We know this value is safe because we parse `offset` with `Number()` before passing it
// into the string.
this._rawRenderedContentTransform = transform;
this._renderedContentTransform = this._sanitizer.bypassSecurityTrustStyle(transform);
this._markForCheck(() => {
if (this._renderedContentOffsetNeedsRewrite) {
this._renderedContentOffset -= this.measureRenderedContentSize();
this._renderedContentOffsetNeedsRewrite = false;
this.setRenderedContentOffset(this._renderedContentOffset);
} else {
this._scrollStrategy.onRenderedOffsetChanged();
}
});
}
}
Expand All @@ -277,10 +246,8 @@ export class CdkVirtualScrollViewport implements DoCheck, OnInit, OnDestroy {
setScrollOffset(offset: number) {
// Rather than setting the offset immediately, we batch it up to be applied along with other DOM
// writes during the next change detection cycle.
this._ngZone.run(() => {
this._pendingScrollOffset = offset;
this._changeDetectorRef.markForCheck();
});
this._pendingScrollOffset = offset;
this._markForCheck();
}

/** Gets the current scroll offset of the viewport (in pixels). */
Expand Down Expand Up @@ -319,4 +286,40 @@ export class CdkVirtualScrollViewport implements DoCheck, OnInit, OnDestroy {
this._viewportSize = this.orientation === 'horizontal' ?
viewportEl.clientWidth : viewportEl.clientHeight;
}

/** Queue up change detection to run. */
private _markForCheck(runAfter?: Function) {
if (runAfter) {
this._runAfterCheck.push(runAfter);
}
if (!this._checkPending) {
this._checkPending = true;
this._ngZone.runOutsideAngular(() => Promise.resolve().then(() => {
if (this._ngZone.isStable) {
this._doCheck();
} else {
this._ngZone.onStable.pipe(take(1)).subscribe(() => this._doCheck());
}
}));
}
}

/** Run change detection. */
private _doCheck() {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Call this method something like _detectChangesForScroll?

this._checkPending = false;
this._ngZone.run(() => this._changeDetectorRef.detectChanges());
// In order to batch setting the scroll offset together with other DOM writes, we wait until a
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't quite follow what this comment means about waiting, since detectChanges is synchronous

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, but in the _markForCheck method above I'm batching them up using a Promise.resolve(). This way if we change multiple properties it waits until after they're all updated and calls detectChanges once

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should the comment live where the wait is scheduled?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I split the comment up to explain the batching part where it actually happens

// change detection cycle to actually apply it.
if (this._pendingScrollOffset != null) {
if (this.orientation === 'horizontal') {
this.elementRef.nativeElement.scrollLeft = this._pendingScrollOffset;
} else {
this.elementRef.nativeElement.scrollTop = this._pendingScrollOffset;
}
}
for(let fn of this._runAfterCheck) {
fn();
}
this._runAfterCheck = [];
}
}