fix(virtual-scroll): move views that are already attached instead of inserting (#15348)

Author: kara

src/cdk/scrolling/virtual-for-of.ts

@@ -293,12 +293,10 @@ export class CdkVirtualForOf<T> 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<CdkVirtualForOfContext<T>>);
+        this._cacheView(this._detachView(adjustedPreviousIndex !));
       } else {  // Item moved.
         const view = this._viewContainerRef.get(adjustedPreviousIndex!) as
             EmbeddedViewRef<CdkVirtualForOfContext<T>>;
@@ -343,18 +341,9 @@ export class CdkVirtualForOf<T> implements CollectionViewer, DoCheck, OnDestroy
     }
   }
 
-  /** Get a view for a new item, either from the cache or by creating a new one. */
-  private _getViewForNewItem(): EmbeddedViewRef<CdkVirtualForOfContext<T>> {
-    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<CdkVirtualForOfContext<T>> {
+    return this._insertViewFromCache(index) || this._createEmbeddedViewAt(index);
   }
 
   /** Update the computed properties on the `CdkVirtualForOfContext`. */
@@ -364,4 +353,37 @@ export class CdkVirtualForOf<T> 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<CdkVirtualForOfContext<T>> {
+    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<CdkVirtualForOfContext<T>>|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<CdkVirtualForOfContext<T>> {
+    return this._viewContainerRef.detach(index) as
+        EmbeddedViewRef<CdkVirtualForOfContext<T>>;
+  }
 }

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<any>;
-  @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;