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

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

* fix: add more static queries for cdk virtual scroll

* refactor(button): use [attr.disabled] host binding in MatButton

With ivy, MatAnchor will inherit the host bindings of MatButton. The
`[disabled]` binding would then be an error since HTMLAnchorElement does
not have a `disabled` property. To work around this, we can simply use
the `disabled` attribute instead, which will always be reflected back
into the property when it exists.

Also add a missing `fixture.detectChanges()` to a test.

Author: kara

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

@@ -89,7 +89,7 @@ function finishInit(fixture: ComponentFixture<any>) {
   encapsulation: ViewEncapsulation.None,
 })
 class AutoSizeVirtualScroll {
-  @ViewChild(CdkVirtualScrollViewport) viewport: CdkVirtualScrollViewport;
+  @ViewChild(CdkVirtualScrollViewport, {static: true}) viewport: CdkVirtualScrollViewport;
 
   @Input() orientation = 'vertical';
   @Input() viewportSize = 200;

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

@@ -287,12 +287,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>>;
@@ -337,18 +335,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`. */
@@ -358,4 +347,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 {ComponentFixture, fakeAsync, flush, inject, TestBed} from '@angular/core/testing';
@@ -486,49 +485,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);
@@ -538,7 +539,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 =
@@ -551,18 +553,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);
@@ -572,7 +577,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 =
@@ -585,7 +591,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(() => {
@@ -828,8 +835,8 @@ function triggerScroll(viewport: CdkVirtualScrollViewport, offset?: number) {
 })
 class FixedSizeVirtualScroll {
   @ViewChild(CdkVirtualScrollViewport, {static: true}) viewport: CdkVirtualScrollViewport;
-  @ViewChild(CdkVirtualForOf, {static: true}) virtualForOf: CdkVirtualForOf<any>;
-  @ViewChild(CdkVirtualForOf, {read: ViewContainerRef}) virtualForViewContainer: ViewContainerRef;
+  // Casting virtualForOf as any so we can spy on private methods
+  @ViewChild(CdkVirtualForOf, {static: true}) virtualForOf: any;
 
   @Input() orientation = 'vertical';
   @Input() viewportSize = 200;
@@ -880,7 +887,6 @@ class FixedSizeVirtualScroll {
 })
 class FixedSizeVirtualScrollWithRtlDirection {
   @ViewChild(CdkVirtualScrollViewport, {static: true}) viewport: CdkVirtualScrollViewport;
-  @ViewChild(CdkVirtualForOf, {read: ViewContainerRef}) virtualForViewContainer: ViewContainerRef;
 
   @Input() orientation = 'vertical';
   @Input() viewportSize = 200;

src/lib/button/button.spec.ts

@@ -43,8 +43,9 @@ describe('MatButton', () => {
 
   it('should expose the ripple instance', () => {
     const fixture = TestBed.createComponent(TestApp);
-    const button = fixture.debugElement.query(By.css('button')).componentInstance as MatButton;
+    fixture.detectChanges();
 
+    const button = fixture.debugElement.query(By.directive(MatButton)).componentInstance;
     expect(button.ripple).toBeTruthy();
   });
 

src/lib/button/button.ts

@@ -70,7 +70,7 @@ export const _MatButtonMixinBase:
              button[mat-flat-button]`,
   exportAs: 'matButton',
   host: {
-    '[disabled]': 'disabled || null',
+    '[attr.disabled]': 'disabled || null',
     '[class._mat-animation-noopable]': '_animationMode === "NoopAnimations"',
   },
   templateUrl: 'button.html',