diff --git a/src/cdk-experimental/scrolling/virtual-scroll-viewport.spec.ts b/src/cdk-experimental/scrolling/virtual-scroll-viewport.spec.ts index a1a8211534ba..fec1f848e69f 100644 --- a/src/cdk-experimental/scrolling/virtual-scroll-viewport.spec.ts +++ b/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(); 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) { // On the second cycle we render the items. fixture.detectChanges(); + flush(); } /** Trigger a scroll event on the viewport (optionally setting a new scroll offset). */ diff --git a/src/cdk-experimental/scrolling/virtual-scroll-viewport.ts b/src/cdk-experimental/scrolling/virtual-scroll-viewport.ts index e720075e3dec..d76611dcbd20 100644 --- a/src/cdk-experimental/scrolling/virtual-scroll-viewport.ts +++ b/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(); @@ -102,37 +101,33 @@ export class CdkVirtualScrollViewport implements DoCheck, OnInit, OnDestroy { /** Observable that emits when the viewport is destroyed. */ private _destroyed = new Subject(); + /** 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 = []; + } }