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

src/cdk-experimental/scrolling/virtual-scroll-viewport.spec.ts

@@ -29,6 +29,7 @@ describe('CdkVirtualScrollViewport', () => {
       viewport.setRenderedContentOffset(
           'arbitrary string as offset' as any, 'arbitrary string as to' as any);
       fixture.detectChanges();
+      flush();
 
       expect((viewport._renderedContentTransform as any).changingThisBreaksApplicationSecurity)
           .toBe('translateY(NaNpx)');
@@ -58,11 +59,13 @@ describe('CdkVirtualScrollViewport', () => {
     it('should update viewport size', fakeAsync(() => {
       testComponent.viewportSize = 300;
       fixture.detectChanges();
+      flush();
       viewport.checkViewportSize();
       expect(viewport.getViewportSize()).toBe(300);
 
       testComponent.viewportSize = 500;
       fixture.detectChanges();
+      flush();
       viewport.checkViewportSize();
       expect(viewport.getViewportSize()).toBe(500);
     }));
@@ -78,6 +81,7 @@ describe('CdkVirtualScrollViewport', () => {
       finishInit(fixture);
       triggerScroll(viewport, testComponent.itemSize + 5);
       fixture.detectChanges();
+      flush();
 
       expect(viewport.getOffsetToRenderedContentStart()).toBe(testComponent.itemSize,
           'should have 50px offset since first 50px item is not rendered');
@@ -87,6 +91,7 @@ describe('CdkVirtualScrollViewport', () => {
       finishInit(fixture);
       triggerScroll(viewport, testComponent.itemSize + 5);
       fixture.detectChanges();
+      flush();
 
       expect(viewport.measureScrollOffset()).toBe(testComponent.itemSize + 5);
     }));
@@ -110,6 +115,7 @@ describe('CdkVirtualScrollViewport', () => {
       finishInit(fixture);
       viewport.setTotalContentSize(10000);
       fixture.detectChanges();
+      flush();
 
       expect(viewport.elementRef.nativeElement.scrollHeight).toBe(10000);
     }));
@@ -118,6 +124,7 @@ describe('CdkVirtualScrollViewport', () => {
       finishInit(fixture);
       viewport.setRenderedRange({start: 2, end: 3});
       fixture.detectChanges();
+      flush();
 
       const items = fixture.elementRef.nativeElement.querySelectorAll('.item');
       expect(items.length).toBe(1, 'Expected 1 item to be rendered');
@@ -128,6 +135,7 @@ describe('CdkVirtualScrollViewport', () => {
       finishInit(fixture);
       viewport.setRenderedContentOffset(10, 'to-start');
       fixture.detectChanges();
+      flush();
 
       expect(viewport.getOffsetToRenderedContentStart()).toBe(10);
     }));
@@ -140,6 +148,7 @@ describe('CdkVirtualScrollViewport', () => {
 
       viewport.setRenderedContentOffset(contentSize + 10, 'to-end');
       fixture.detectChanges();
+      flush();
 
       expect(viewport.getOffsetToRenderedContentStart()).toBe(10);
     }));
@@ -148,8 +157,11 @@ describe('CdkVirtualScrollViewport', () => {
       finishInit(fixture);
       viewport.setScrollOffset(testComponent.itemSize * 2);
       fixture.detectChanges();
+      flush();
+
       triggerScroll(viewport);
       fixture.detectChanges();
+      flush();
 
       expect(viewport.elementRef.nativeElement.scrollTop).toBe(testComponent.itemSize * 2);
       expect(viewport.getRenderedRange()).toEqual({start: 2, end: 6});
@@ -163,6 +175,7 @@ describe('CdkVirtualScrollViewport', () => {
       for (let offset = 0; offset <= maxOffset; offset += 10) {
         triggerScroll(viewport, offset);
         fixture.detectChanges();
+        flush();
 
         const expectedRange = {
           start: Math.floor(offset / testComponent.itemSize),
@@ -188,6 +201,7 @@ describe('CdkVirtualScrollViewport', () => {
       for (let offset = maxOffset; offset >= 0; offset -= 10) {
         triggerScroll(viewport, offset);
         fixture.detectChanges();
+        flush();
 
         const expectedRange = {
           start: Math.floor(offset / testComponent.itemSize),
@@ -220,6 +234,7 @@ describe('CdkVirtualScrollViewport', () => {
           finishInit(fixture);
           triggerScroll(viewport, testComponent.itemSize * 2);
           fixture.detectChanges();
+          flush();
 
           expect(viewport.getRenderedRange()).toEqual({start: 1, end: 7},
               'should render 6 50px items to fill 200px space, plus one buffer element at the' +
@@ -231,6 +246,7 @@ describe('CdkVirtualScrollViewport', () => {
       finishInit(fixture);
       triggerScroll(viewport, testComponent.itemSize * 6);
       fixture.detectChanges();
+      flush();
 
       expect(viewport.getRenderedRange()).toEqual({start: 5, end: 10},
           'should render the last 5 50px items to fill 200px space, plus one buffer element at' +
@@ -241,12 +257,14 @@ describe('CdkVirtualScrollViewport', () => {
       finishInit(fixture);
       triggerScroll(viewport, testComponent.itemSize * 2);
       fixture.detectChanges();
+      flush();
 
       expect(viewport.getRenderedRange())
           .toEqual({start: 2, end: 6}, 'should render 4 50px items to fill 200px space');
 
       testComponent.itemSize *= 2;
       fixture.detectChanges();
+      flush();
 
       expect(viewport.getRenderedRange())
           .toEqual({start: 1, end: 3}, 'should render 2 100px items to fill 200px space');
@@ -256,12 +274,14 @@ describe('CdkVirtualScrollViewport', () => {
       finishInit(fixture);
       triggerScroll(viewport, testComponent.itemSize * 2);
       fixture.detectChanges();
+      flush();
 
       expect(viewport.getRenderedRange())
           .toEqual({start: 2, end: 6}, 'should render 4 50px items to fill 200px space');
 
       testComponent.bufferSize = 1;
       fixture.detectChanges();
+      flush();
 
       expect(viewport.getRenderedRange())
           .toEqual({start: 1, end: 7}, 'should expand to 1 buffer element on each side');
@@ -271,14 +291,18 @@ describe('CdkVirtualScrollViewport', () => {
       finishInit(fixture);
       triggerScroll(viewport, testComponent.itemSize * 6);
       fixture.detectChanges();
+      flush();
 
       expect(viewport.getOffsetToRenderedContentStart())
           .toBe(testComponent.itemSize * 6, 'should be scrolled to bottom of 10 item list');
 
       testComponent.items = Array(5).fill(0);
       fixture.detectChanges();
+      flush();
+
       triggerScroll(viewport);
       fixture.detectChanges();
+      flush();
 
       expect(viewport.getOffsetToRenderedContentStart())
           .toBe(testComponent.itemSize, 'should be scrolled to bottom of 5 item list');
@@ -293,6 +317,7 @@ describe('CdkVirtualScrollViewport', () => {
       for (let offset = 0; offset <= maxOffset; offset += 10) {
         triggerScroll(viewport, offset);
         fixture.detectChanges();
+        flush();
 
         const expectedRange = {
           start: Math.floor(offset / testComponent.itemSize),
@@ -319,6 +344,7 @@ describe('CdkVirtualScrollViewport', () => {
       for (let offset = maxOffset; offset >= 0; offset -= 10) {
         triggerScroll(viewport, offset);
         fixture.detectChanges();
+        flush();
 
         const expectedRange = {
           start: Math.floor(offset / testComponent.itemSize),
@@ -346,6 +372,7 @@ describe('CdkVirtualScrollViewport', () => {
 
       data.next([1, 2, 3]);
       fixture.detectChanges();
+      flush();
 
       expect(viewport.getRenderedRange())
           .toEqual({start: 0, end: 3}, 'newly emitted items should be rendered');
@@ -355,7 +382,6 @@ describe('CdkVirtualScrollViewport', () => {
       const data = new Subject<number[]>();
       testComponent.items = new ArrayDataSource(data) as any;
       finishInit(fixture);
-      flush();
 
       expect(viewport.getRenderedRange())
           .toEqual({start: 0, end: 0}, 'no items should be rendered');
@@ -375,11 +401,13 @@ describe('CdkVirtualScrollViewport', () => {
 
       testComponent.items = [0];
       fixture.detectChanges();
+      flush();
 
       expect(testComponent.virtualForViewContainer.detach).not.toHaveBeenCalled();
 
       testComponent.items = [1];
       fixture.detectChanges();
+      flush();
 
       expect(testComponent.virtualForViewContainer.detach).toHaveBeenCalled();
     }));
@@ -392,11 +420,13 @@ describe('CdkVirtualScrollViewport', () => {
 
       testComponent.items = [0];
       fixture.detectChanges();
+      flush();
 
       expect(testComponent.virtualForViewContainer.detach).not.toHaveBeenCalled();
 
       testComponent.items = [1];
       fixture.detectChanges();
+      flush();
 
       expect(testComponent.virtualForViewContainer.detach).not.toHaveBeenCalled();
     }));
@@ -413,6 +443,7 @@ describe('CdkVirtualScrollViewport', () => {
       spy.calls.reset();
       triggerScroll(viewport, 10);
       fixture.detectChanges();
+      flush();
 
       // As we first start to scroll we need to create one more item. This is because the first item
       // is still partially on screen and therefore can't be removed yet. At the same time a new
@@ -425,6 +456,7 @@ describe('CdkVirtualScrollViewport', () => {
       for (let offset = 10; offset <= maxOffset; offset += 10) {
         triggerScroll(viewport, offset);
         fixture.detectChanges();
+        flush();
       }
 
       // As we scroll through the rest of the items, no new views should be created, our existing 5
@@ -445,6 +477,7 @@ describe('CdkVirtualScrollViewport', () => {
       spy.calls.reset();
       triggerScroll(viewport, 10);
       fixture.detectChanges();
+      flush();
 
       // As we first start to scroll we need to create one more item. This is because the first item
       // is still partially on screen and therefore can't be removed yet. At the same time a new
@@ -457,6 +490,7 @@ describe('CdkVirtualScrollViewport', () => {
       for (let offset = 10; offset <= maxOffset; offset += 10) {
         triggerScroll(viewport, offset);
         fixture.detectChanges();
+        flush();
       }
 
       // Since our template cache size is 0, as we scroll through the rest of the items, we need to
@@ -514,6 +548,7 @@ function finishInit(fixture: ComponentFixture<any>) {
 
   // On the second cycle we render the items.
   fixture.detectChanges();
+  flush();
 }
 
 /** Trigger a scroll event on the viewport (optionally setting a new scroll offset). */

src/cdk-experimental/scrolling/virtual-scroll-viewport.ts

@@ -11,7 +11,6 @@ import {
   ChangeDetectionStrategy,
   ChangeDetectorRef,
   Component,
-  DoCheck,
   ElementRef,
   Inject,
   Input,
@@ -48,7 +47,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>();
 
@@ -102,37 +101,33 @@ 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 _isChangeDetectionPending = false;
+
+  /** A list of functions to run after the next change detection cycle. */
+  private _runAfterChangeDetection: 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(() => {
+    // the Viewport to be rendered with the correct size before we measure. We run this outside the
+    // zone to avoid causing more change detection cycles. We handle the change detection loop
+    // ourselves instead.
+    this._ngZone.runOutsideAngular(() => Promise.resolve().then(() => {
       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());
-      });
-    });
-  }
+      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;
-      }
-    }
+      this._markChangeDetectionNeeded();
+    }));
   }
 
   ngOnDestroy() {
@@ -151,16 +146,19 @@ 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();
-      }
+    // changes. Run outside the zone to avoid triggering change detection, since we're managing the
+    // change detection loop ourselves.
+    this._ngZone.runOutsideAngular(() => {
+      this._forOf = forOf;
+      this._forOf.dataStream.pipe(takeUntil(this._detachedSubject)).subscribe(data => {
+        const newLength = data.length;
+        if (newLength !== this._dataLength) {
+          this._dataLength = newLength;
+          this._scrollStrategy.onDataLengthChanged();
+        }
+      });
     });
   }
 
@@ -190,40 +188,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._markChangeDetectionNeeded();
     }
   }
 
   /** 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._markChangeDetectionNeeded(() => this._scrollStrategy.onContentRendered());
     }
   }
 
@@ -250,25 +230,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._markChangeDetectionNeeded(() => {
+        if (this._renderedContentOffsetNeedsRewrite) {
+          this._renderedContentOffset -= this.measureRenderedContentSize();
+          this._renderedContentOffsetNeedsRewrite = false;
+          this.setRenderedContentOffset(this._renderedContentOffset);
+        } else {
+          this._scrollStrategy.onRenderedOffsetChanged();
+        }
       });
     }
   }
@@ -277,10 +250,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._markChangeDetectionNeeded();
   }
 
   /** Gets the current scroll offset of the viewport (in pixels). */
@@ -319,4 +290,45 @@ export class CdkVirtualScrollViewport implements DoCheck, OnInit, OnDestroy {
     this._viewportSize = this.orientation === 'horizontal' ?
         viewportEl.clientWidth : viewportEl.clientHeight;
   }
+
+  /** Queue up change detection to run. */
+  private _markChangeDetectionNeeded(runAfter?: Function) {
+    if (runAfter) {
+      this._runAfterChangeDetection.push(runAfter);
+    }
+
+    // Use a Promise to batch together calls to `_doChangeDetection`. This way if we set a bunch of
+    // properties sequentially we only have to run `_doChangeDetection` once at the end.
+    if (!this._isChangeDetectionPending) {
+      this._isChangeDetectionPending = true;
+      this._ngZone.runOutsideAngular(() => Promise.resolve().then(() => {
+        if (this._ngZone.isStable) {
+           this._doChangeDetection();
+        } else {
+          this._ngZone.onStable.pipe(take(1)).subscribe(() => this._doChangeDetection());
+        }
+      }));
+    }
+  }
+
+  /** Run change detection. */
+  private _doChangeDetection() {
+    this._isChangeDetectionPending = false;
+
+    // Apply changes to Angular bindings.
+    this._ngZone.run(() => this._changeDetectorRef.detectChanges());
+    // Apply the pending scroll offset separately, since it can't be set up as an Angular binding.
+    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._runAfterChangeDetection) {
+      fn();
+    }
+    this._runAfterChangeDetection = [];
+  }
 }