From 61ecfd2de6bd7f4be0672871cd1ccec81c34162f Mon Sep 17 00:00:00 2001
From: crisbeto <crisbeto@abv.bg>
Date: Tue, 28 May 2019 23:57:24 +0200
Subject: [PATCH] fix(scrolling): virtual scroll throw off if directive injects
 ViewContainerRef

Fixes the virtual scrolling being thrown off and rendering the items in a wrong sequence, if there's a directive on the items that injects `ViewContainerRef`. The issue is due to the way we insert new views in a particular index. Rather than inserting the item at the index directly, we insert it and then move it into place. It looks like this behavior, coupled with Angular adding an extra comment node if something injects the `ViewContainerRef` causes the insertion order to be messed up.

Fixes #16130.
---
 src/cdk/scrolling/virtual-for-of.ts           | 28 +++----
 .../scrolling/virtual-scroll-viewport.spec.ts | 73 ++++++++++++++++++-
 2 files changed, 86 insertions(+), 15 deletions(-)

diff --git a/src/cdk/scrolling/virtual-for-of.ts b/src/cdk/scrolling/virtual-for-of.ts
index 862a4890fe43..d438c2524670 100644
--- a/src/cdk/scrolling/virtual-for-of.ts
+++ b/src/cdk/scrolling/virtual-for-of.ts
@@ -357,20 +357,20 @@ export class CdkVirtualForOf<T> implements CollectionViewer, DoCheck, OnDestroy
 
   /** 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;
+    // Note that it's important that we insert the item directly at the proper index,
+    // rather than inserting it and the moving it in place, because if there's a directive
+    // on the same node that injects the `ViewContainerRef`, Angular will insert another
+    // comment node which can throw off the move when it's being repeated for all items.
+    return this._viewContainerRef.createEmbeddedView(this._template, {
+      $implicit: null!,
+      cdkVirtualForOf: this._cdkVirtualForOf,
+      index: -1,
+      count: -1,
+      first: false,
+      last: false,
+      odd: false,
+      even: false
+    }, index);
   }
 
   /** Inserts a recycled view from the cache at the given index. */
diff --git a/src/cdk/scrolling/virtual-scroll-viewport.spec.ts b/src/cdk/scrolling/virtual-scroll-viewport.spec.ts
index d9cb33e76eb8..c77ba1f82994 100644
--- a/src/cdk/scrolling/virtual-scroll-viewport.spec.ts
+++ b/src/cdk/scrolling/virtual-scroll-viewport.spec.ts
@@ -12,7 +12,9 @@ import {
   NgZone,
   TrackByFunction,
   ViewChild,
-  ViewEncapsulation
+  ViewEncapsulation,
+  Directive,
+  ViewContainerRef
 } from '@angular/core';
 import {async, ComponentFixture, fakeAsync, flush, inject, TestBed} from '@angular/core/testing';
 import {animationFrameScheduler, Subject} from 'rxjs';
@@ -797,6 +799,36 @@ describe('CdkVirtualScrollViewport', () => {
           'Error: cdk-virtual-scroll-viewport requires the "itemSize" property to be set.');
     }));
   });
+
+  describe('with item that injects ViewContainerRef', () => {
+    let fixture: ComponentFixture<VirtualScrollWithItemInjectingViewContainer>;
+    let testComponent: VirtualScrollWithItemInjectingViewContainer;
+    let viewport: CdkVirtualScrollViewport;
+
+    beforeEach(async(() => {
+      TestBed.configureTestingModule({
+        imports: [ScrollingModule],
+        declarations: [VirtualScrollWithItemInjectingViewContainer, InjectsViewContainer],
+      }).compileComponents();
+    }));
+
+    beforeEach(() => {
+      fixture = TestBed.createComponent(VirtualScrollWithItemInjectingViewContainer);
+      testComponent = fixture.componentInstance;
+      viewport = testComponent.viewport;
+    });
+
+    it('should render the values in the correct sequence when an item is ' +
+      'injecting ViewContainerRef', fakeAsync(() => {
+        finishInit(fixture);
+
+        const contentWrapper =
+            viewport.elementRef.nativeElement.querySelector('.cdk-virtual-scroll-content-wrapper')!;
+
+        expect(Array.from(contentWrapper.children).map(child => child.textContent!.trim()))
+          .toEqual(['0', '1', '2', '3', '4', '5', '6', '7']);
+      }));
+  });
 });
 
 
@@ -938,3 +970,42 @@ class FixedSizeVirtualScrollWithRtlDirection {
 class VirtualScrollWithNoStrategy {
   items = [];
 }
+
+@Directive({
+  selector: '[injects-view-container]'
+})
+class InjectsViewContainer {
+  constructor(public viewContainerRef: ViewContainerRef) {
+  }
+}
+
+@Component({
+  template: `
+    <cdk-virtual-scroll-viewport itemSize="50">
+      <div injects-view-container class="item" *cdkVirtualFor="let item of items">{{item}}</div>
+    </cdk-virtual-scroll-viewport>
+  `,
+  styles: [`
+    .cdk-virtual-scroll-content-wrapper {
+      display: flex;
+      flex-direction: column;
+    }
+
+    .cdk-virtual-scroll-viewport {
+      width: 200px;
+      height: 200px;
+    }
+
+    .item {
+      width: 100%;
+      height: 50px;
+    }
+  `],
+  encapsulation: ViewEncapsulation.None
+})
+class VirtualScrollWithItemInjectingViewContainer {
+  @ViewChild(CdkVirtualScrollViewport, {static: true}) viewport: CdkVirtualScrollViewport;
+  itemSize = 50;
+  items = Array(20000).fill(0).map((_, i) => i);
+}
+