From 2470a4249e9b64af9654589841a504effb0eb8b7 Mon Sep 17 00:00:00 2001 From: Kara Date: Mon, 4 Mar 2019 11:20:07 -0800 Subject: [PATCH] fix(virtual-scroll): move views that are already attached instead of inserting (#15348) --- src/cdk/scrolling/virtual-for-of.ts | 54 +++++++++++++------ .../scrolling/virtual-scroll-viewport.spec.ts | 48 +++++++++-------- 2 files changed, 65 insertions(+), 37 deletions(-) diff --git a/src/cdk/scrolling/virtual-for-of.ts b/src/cdk/scrolling/virtual-for-of.ts index cfefe041d19..4cad5509d5a 100644 --- a/src/cdk/scrolling/virtual-for-of.ts +++ b/src/cdk/scrolling/virtual-for-of.ts @@ -293,12 +293,10 @@ export class CdkVirtualForOf implements CollectionViewer, DoCheck, OnDestroy adjustedPreviousIndex: number | null, currentIndex: number | null) => { if (record.previousIndex == null) { // Item added. - const view = this._getViewForNewItem(); - this._viewContainerRef.insert(view, currentIndex!); + const view = this._insertViewForNewItem(currentIndex!); view.context.$implicit = record.item; } else if (currentIndex == null) { // Item removed. - this._cacheView(this._viewContainerRef.detach(adjustedPreviousIndex!) as - EmbeddedViewRef>); + this._cacheView(this._detachView(adjustedPreviousIndex !)); } else { // Item moved. const view = this._viewContainerRef.get(adjustedPreviousIndex!) as EmbeddedViewRef>; @@ -343,18 +341,9 @@ export class CdkVirtualForOf implements CollectionViewer, DoCheck, OnDestroy } } - /** Get a view for a new item, either from the cache or by creating a new one. */ - private _getViewForNewItem(): EmbeddedViewRef> { - return this._templateCache.pop() || this._viewContainerRef.createEmbeddedView(this._template, { - $implicit: null!, - cdkVirtualForOf: this._cdkVirtualForOf, - index: -1, - count: -1, - first: false, - last: false, - odd: false, - even: false - }); + /** Inserts a view for a new item, either from the cache or by creating a new one. */ + private _insertViewForNewItem(index: number): EmbeddedViewRef> { + return this._insertViewFromCache(index) || this._createEmbeddedViewAt(index); } /** Update the computed properties on the `CdkVirtualForOfContext`. */ @@ -364,4 +353,37 @@ export class CdkVirtualForOf implements CollectionViewer, DoCheck, OnDestroy context.even = context.index % 2 === 0; context.odd = !context.even; } + + /** Creates a new embedded view and moves it to the given index */ + private _createEmbeddedViewAt(index: number): EmbeddedViewRef> { + const view = this._viewContainerRef.createEmbeddedView(this._template, { + $implicit: null!, + cdkVirtualForOf: this._cdkVirtualForOf, + index: -1, + count: -1, + first: false, + last: false, + odd: false, + even: false + }); + if (index < this._viewContainerRef.length) { + this._viewContainerRef.move(view, index); + } + return view; + } + + /** Inserts a recycled view from the cache at the given index. */ + private _insertViewFromCache(index: number): EmbeddedViewRef>|null { + const cachedView = this._templateCache.pop(); + if (cachedView) { + this._viewContainerRef.insert(cachedView, index); + } + return cachedView || null; + } + + /** Detaches the embedded view at the given index. */ + private _detachView(index: number): EmbeddedViewRef> { + return this._viewContainerRef.detach(index) as + EmbeddedViewRef>; + } } diff --git a/src/cdk/scrolling/virtual-scroll-viewport.spec.ts b/src/cdk/scrolling/virtual-scroll-viewport.spec.ts index 747acf48ab1..207e9d187a2 100644 --- a/src/cdk/scrolling/virtual-scroll-viewport.spec.ts +++ b/src/cdk/scrolling/virtual-scroll-viewport.spec.ts @@ -12,7 +12,6 @@ import { NgZone, TrackByFunction, ViewChild, - ViewContainerRef, ViewEncapsulation } from '@angular/core'; import {async, ComponentFixture, fakeAsync, flush, inject, TestBed} from '@angular/core/testing'; @@ -488,49 +487,51 @@ describe('CdkVirtualScrollViewport', () => { it('should trackBy value by default', fakeAsync(() => { testComponent.items = []; - spyOn(testComponent.virtualForViewContainer, 'detach').and.callThrough(); + spyOn(testComponent.virtualForOf, '_detachView').and.callThrough(); finishInit(fixture); testComponent.items = [0]; fixture.detectChanges(); flush(); - expect(testComponent.virtualForViewContainer.detach).not.toHaveBeenCalled(); + expect(testComponent.virtualForOf._detachView).not.toHaveBeenCalled(); testComponent.items = [1]; fixture.detectChanges(); flush(); - expect(testComponent.virtualForViewContainer.detach).toHaveBeenCalled(); + expect(testComponent.virtualForOf._detachView).toHaveBeenCalled(); })); it('should trackBy index when specified', fakeAsync(() => { testComponent.trackBy = i => i; testComponent.items = []; - spyOn(testComponent.virtualForViewContainer, 'detach').and.callThrough(); + spyOn(testComponent.virtualForOf, '_detachView').and.callThrough(); finishInit(fixture); testComponent.items = [0]; fixture.detectChanges(); flush(); - expect(testComponent.virtualForViewContainer.detach).not.toHaveBeenCalled(); + expect(testComponent.virtualForOf._detachView).not.toHaveBeenCalled(); testComponent.items = [1]; fixture.detectChanges(); flush(); - expect(testComponent.virtualForViewContainer.detach).not.toHaveBeenCalled(); + expect(testComponent.virtualForOf._detachView).not.toHaveBeenCalled(); })); it('should recycle views when template cache is large enough to accommodate', fakeAsync(() => { testComponent.trackBy = i => i; - const spy = - spyOn(testComponent.virtualForViewContainer, 'createEmbeddedView').and.callThrough(); + const spy = spyOn(testComponent.virtualForOf, '_createEmbeddedViewAt') + .and.callThrough(); + finishInit(fixture); // Should create views for the initial rendered items. - expect(testComponent.virtualForViewContainer.createEmbeddedView).toHaveBeenCalledTimes(4); + expect(testComponent.virtualForOf._createEmbeddedViewAt) + .toHaveBeenCalledTimes(4); spy.calls.reset(); triggerScroll(viewport, 10); @@ -540,7 +541,8 @@ describe('CdkVirtualScrollViewport', () => { // 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 // item is now partially on the screen at the bottom and so a new view is needed. - expect(testComponent.virtualForViewContainer.createEmbeddedView).toHaveBeenCalledTimes(1); + expect(testComponent.virtualForOf._createEmbeddedViewAt) + .toHaveBeenCalledTimes(1); spy.calls.reset(); const maxOffset = @@ -553,18 +555,21 @@ describe('CdkVirtualScrollViewport', () => { // As we scroll through the rest of the items, no new views should be created, our existing 5 // can just be recycled as appropriate. - expect(testComponent.virtualForViewContainer.createEmbeddedView).not.toHaveBeenCalled(); + expect(testComponent.virtualForOf._createEmbeddedViewAt) + .not.toHaveBeenCalled(); })); it('should not recycle views when template cache is full', fakeAsync(() => { testComponent.trackBy = i => i; testComponent.templateCacheSize = 0; - const spy = - spyOn(testComponent.virtualForViewContainer, 'createEmbeddedView').and.callThrough(); - finishInit(fixture); + const spy = spyOn(testComponent.virtualForOf, '_createEmbeddedViewAt') + .and.callThrough(); + + finishInit(fixture); // Should create views for the initial rendered items. - expect(testComponent.virtualForViewContainer.createEmbeddedView).toHaveBeenCalledTimes(4); + expect(testComponent.virtualForOf._createEmbeddedViewAt) + .toHaveBeenCalledTimes(4); spy.calls.reset(); triggerScroll(viewport, 10); @@ -574,7 +579,8 @@ describe('CdkVirtualScrollViewport', () => { // 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 // item is now partially on the screen at the bottom and so a new view is needed. - expect(testComponent.virtualForViewContainer.createEmbeddedView).toHaveBeenCalledTimes(1); + expect(testComponent.virtualForOf._createEmbeddedViewAt) + .toHaveBeenCalledTimes(1); spy.calls.reset(); const maxOffset = @@ -587,7 +593,8 @@ describe('CdkVirtualScrollViewport', () => { // Since our template cache size is 0, as we scroll through the rest of the items, we need to // create a new view for each one. - expect(testComponent.virtualForViewContainer.createEmbeddedView).toHaveBeenCalledTimes(5); + expect(testComponent.virtualForOf._createEmbeddedViewAt) + .toHaveBeenCalledTimes(5); })); it('should render up to maxBufferPx when buffer dips below minBufferPx', fakeAsync(() => { @@ -830,8 +837,8 @@ function triggerScroll(viewport: CdkVirtualScrollViewport, offset?: number) { }) class FixedSizeVirtualScroll { @ViewChild(CdkVirtualScrollViewport) viewport: CdkVirtualScrollViewport; - @ViewChild(CdkVirtualForOf) virtualForOf: CdkVirtualForOf; - @ViewChild(CdkVirtualForOf, {read: ViewContainerRef}) virtualForViewContainer: ViewContainerRef; + // Casting virtualForOf as any so we can spy on private methods + @ViewChild(CdkVirtualForOf) virtualForOf: any; @Input() orientation = 'vertical'; @Input() viewportSize = 200; @@ -882,7 +889,6 @@ class FixedSizeVirtualScroll { }) class FixedSizeVirtualScrollWithRtlDirection { @ViewChild(CdkVirtualScrollViewport) viewport: CdkVirtualScrollViewport; - @ViewChild(CdkVirtualForOf, {read: ViewContainerRef}) virtualForViewContainer: ViewContainerRef; @Input() orientation = 'vertical'; @Input() viewportSize = 200;