fix(virtual-list): several issues

src/components/virtual-scroll/test/virtual-scroll.spec.ts

@@ -306,7 +306,7 @@ describe('VirtualScroll', () => {
   describe('initReadNodes', () => {
 
     it('should get all the row heights w/ 30% width rows', () => {
-      let firstTop = 3;
+      let firstTop = 13;
       nodes = [
         {cell: 0, tmpl: TEMPLATE_HEADER, view: getView(data.viewWidth, HEIGHT_HEADER, firstTop, 0)},
         {cell: 1, tmpl: TEMPLATE_ITEM, view: getView(90, HEIGHT_ITEM, HEIGHT_HEADER + firstTop, 0)},

src/components/virtual-scroll/virtual-scroll.ts

@@ -1,10 +1,10 @@
-import { AfterContentInit, ChangeDetectorRef, ContentChild, Directive, DoCheck, ElementRef, Input, IterableDiffers, NgZone, OnDestroy, Renderer, TrackByFn } from '@angular/core';
+import { AfterContentInit, ChangeDetectorRef, ContentChild, Directive, DoCheck, ElementRef, Input, DefaultIterableDiffer, IterableDiffers, NgZone, OnDestroy, Renderer, TrackByFn } from '@angular/core';
 
 import { adjustRendered, calcDimensions, estimateHeight, initReadNodes, processRecords, populateNodeData, updateDimensions, updateNodeContext, writeToNodes } from './virtual-util';
 import { Config } from '../../config/config';
 import { Content, ScrollEvent } from '../content/content';
 import { DomController } from '../../platform/dom-controller';
-import { isBlank, isFunction, isPresent } from '../../util/util';
+import { isBlank, isFunction, isPresent, assert } from '../../util/util';
 import { Platform } from '../../platform/platform';
 import { ViewController } from '../../navigation/view-controller';
 import { VirtualCell, VirtualData, VirtualNode } from './virtual-util';
@@ -215,12 +215,13 @@ import { VirtualHeader } from './virtual-header';
   selector: '[virtualScroll]'
 })
 export class VirtualScroll implements DoCheck, AfterContentInit, OnDestroy {
-  _differ: any;
+
+  _differ: DefaultIterableDiffer;
   _scrollSub: any;
   _scrollEndSub: any;
   _resizeSub: any;
   _init: boolean = false;
-  _lastEle: boolean;
+  _lastEle: boolean = false;
   _hdrFn: Function;
   _ftrFn: Function;
   _records: any[] = [];
@@ -231,7 +232,7 @@ export class VirtualScroll implements DoCheck, AfterContentInit, OnDestroy {
   _data: VirtualData = {
     scrollTop: 0,
   };
-  _queue: number;
+  _queue: number = SCROLL_QUEUE_NO_CHANGES;
   _recordSize: number = 0;
 
   @ContentChild(VirtualItem) _itmTmp: VirtualItem;
@@ -249,7 +250,7 @@ export class VirtualScroll implements DoCheck, AfterContentInit, OnDestroy {
   set virtualScroll(val: any) {
     this._records = val;
     if (isBlank(this._differ) && isPresent(val)) {
-      this._differ = this._iterableDiffers.find(val).create(this._cd, this.virtualTrackBy);
+      this._differ = <DefaultIterableDiffer>this._iterableDiffers.find(val).create(this._cd, this.virtualTrackBy);
     }
   }
 
@@ -388,18 +389,16 @@ export class VirtualScroll implements DoCheck, AfterContentInit, OnDestroy {
     this.setElementClass('virtual-loading', true);
 
     // wait for the content to be rendered and has readable dimensions
-    _ctrl.readReady.subscribe(() => {
+    const readSub = _ctrl.readReady.subscribe(() => {
+      readSub.unsubscribe();
+      this.readUpdate(true);
+    });
+
+    // wait for the content to be writable
+    const writeSub = _ctrl.writeReady.subscribe(() => {
+      writeSub.unsubscribe();
       this._init = true;
-      if (isPresent(this._changes())) {
+      this.writeUpdate(true);
-        this.readUpdate(true);
-
-        // wait for the content to be writable
-        var subscription = _ctrl.writeReady.subscribe(() => {
-          subscription.unsubscribe();
-          this.writeUpdate(true);
-        });
-      }
-
       this._listeners();
     });
   }
@@ -421,9 +420,9 @@ export class VirtualScroll implements DoCheck, AfterContentInit, OnDestroy {
 
     let needClean = false;
     if (changes) {
-      changes.forEachOperation((item: any, _: number, cindex: number) => {
+      changes.forEachOperation((item, _, cindex) => {
         if (item.previousIndex != null || (cindex < this._recordSize)) {
-            needClean = true;
+          needClean = true;
         }
       });
     } else {
@@ -457,11 +456,11 @@ export class VirtualScroll implements DoCheck, AfterContentInit, OnDestroy {
     data.scrollDiff = SCROLL_DIFFERENCE_MINIMUM + 1;
 
     processRecords(stopAtHeight,
-                   this._records,
+      this._records,
-                   this._cells,
+      this._cells,
-                   this._hdrFn,
+      this._hdrFn,
-                   this._ftrFn,
+      this._ftrFn,
-                   this._data);
+      this._data);
 
     // ******** DOM WRITE ****************
     this.renderVirtual(needClean);
@@ -475,7 +474,7 @@ export class VirtualScroll implements DoCheck, AfterContentInit, OnDestroy {
       this.bufferRatio);
   }
 
-  private _changes() {
+  private _changes(): DefaultIterableDiffer {
     if (isPresent(this._records) && isPresent(this._differ)) {
       return this._differ.diff(this._records);
     }
@@ -487,77 +486,69 @@ export class VirtualScroll implements DoCheck, AfterContentInit, OnDestroy {
    * DOM WRITE
    */
   renderVirtual(needClean: boolean) {
-    const nodes = this._nodes;
-    const cells = this._cells;
-    const data = this._data;
-    const records = this._records;
-
-    if (needClean) {
-      // ******** DOM WRITE ****************
-      updateDimensions(this._plt, nodes, cells, data, true);
-      data.topCell = 0;
-      data.bottomCell = (cells.length - 1);
-    }
-
-    adjustRendered(cells, data);
-
-    populateNodeData(data.topCell, data.bottomCell,
-                  data.viewWidth, true,
-                  cells, records, nodes,
-                  this._itmTmp.viewContainer,
-                  this._itmTmp.templateRef,
-                  this._hdrTmp && this._hdrTmp.templateRef,
-                  this._ftrTmp && this._ftrTmp.templateRef, needClean);
-
-    if (needClean) {
-      this._cd.detectChanges();
-    }
-
     this._plt.raf(() => {
+      const nodes = this._nodes;
+      const cells = this._cells;
+      const data = this._data;
+      const records = this._records;
+
+      if (needClean) {
+        // ******** DOM WRITE ****************
+        updateDimensions(this._plt, nodes, cells, data, true);
+        data.topCell = 0;
+        data.bottomCell = (cells.length - 1);
+      }
+
+      adjustRendered(cells, data);
+
+      populateNodeData(
+        data.topCell, data.bottomCell,
+        data.viewWidth, true,
+        cells, records, nodes,
+        this._itmTmp.viewContainer,
+        this._itmTmp.templateRef,
+        this._hdrTmp && this._hdrTmp.templateRef,
+        this._ftrTmp && this._ftrTmp.templateRef, needClean
+      );
+
+      if (needClean) {
+        this._cd.detectChanges();
+      }
+
       // at this point, this fn was called from within another
       // requestAnimationFrame, so the next dom reads/writes within the next frame
       // wait a frame before trying to read and calculate the dimensions
-      this._dom.read(() => {
+
-        // ******** DOM READ ****************
+      // ******** DOM READ ****************
-        initReadNodes(this._plt, nodes, cells, data);
+      this._dom.read(() => initReadNodes(this._plt, nodes, cells, data));
-      });
 
       this._dom.write(() => {
-        const ele = this._elementRef.nativeElement;
-        const recordsLength = records.length;
-        const renderer = this._renderer;
-
         // update the bound context for each node
         updateNodeContext(nodes, cells, data);
 
         // ******** DOM WRITE ****************
-        for (var i = 0; i < nodes.length; i++) {
+        this._stepChangeDetection();
-          (<any>nodes[i].view).detectChanges();
+        // ******** DOM WRITE ****************
-        }
+        this._stepDOMWrite();
+        // ******** DOM WRITE ****************
+        this._content.imgsUpdate();
 
+        // First time load
         if (!this._lastEle) {
           // add an element at the end so :last-child css doesn't get messed up
           // ******** DOM WRITE ****************
-          var lastEle: HTMLElement = renderer.createElement(ele, 'div');
+          var ele = this._elementRef.nativeElement;
+          var lastEle: HTMLElement = this._renderer.createElement(ele, 'div');
           lastEle.className = 'virtual-last';
           this._lastEle = true;
+
+          // ******** DOM WRITE ****************
+          this.setElementClass('virtual-scroll', true);
+
+          // ******** DOM WRITE ****************
+          this.setElementClass('virtual-loading', false);
         }
-
+        assert(this._queue === SCROLL_QUEUE_NO_CHANGES, 'queue value should be NO_CHANGES');
-        // ******** DOM WRITE ****************
-        this.setElementClass('virtual-scroll', true);
-
-        // ******** DOM WRITE ****************
-        this.setElementClass('virtual-loading', false);
-
-        // ******** DOM WRITE ****************
-        writeToNodes(this._plt, nodes, cells, recordsLength);
-
-        // ******** DOM WRITE ****************
-        this._setHeight(
-          estimateHeight(recordsLength, cells[cells.length - 1], this._vHeight, 0.25)
-        );
-
-        this._content.imgsUpdate();
       });
     });
   }
@@ -579,90 +570,112 @@ export class VirtualScroll implements DoCheck, AfterContentInit, OnDestroy {
   /**
    * @hidden
    */
-  scrollUpdate(ev: ScrollEvent) {
+  private _stepDOMWrite() {
-    // there is a queue system so that we can
-    // spread out the work over multiple frames
-    const data = this._data;
     const cells = this._cells;
     const nodes = this._nodes;
+    const recordsLength = this._records.length;
 
-    // set the scroll top from the scroll event
+    // ******** DOM WRITE ****************
-    data.scrollTop = ev.scrollTop;
+    writeToNodes(this._plt, nodes, cells, recordsLength);
 
-    if (this._queue === SCROLL_QUEUE_DOM_WRITE) {
+    // ******** DOM WRITE ****************
-      // there are DOM writes we need to take care of in this frame
+    this._setHeight(
+      estimateHeight(recordsLength, cells[cells.length - 1], this._vHeight, 0.25)
+    );
 
-      this._dom.write(() => {
+    // we're done here, good work
-        const recordsLength = this._records.length;
+    this._queue = SCROLL_QUEUE_NO_CHANGES;
+  }
 
-        // ******** DOM WRITE ****************
+  /**
-        writeToNodes(this._plt, nodes, cells, recordsLength);
+   * @private
-
+   */
-        // ******** DOM WRITE ****************
+  private _stepChangeDetection() {
-        this._setHeight(
+    // we need to do some change detection in this frame
-          estimateHeight(recordsLength, cells[cells.length - 1], this._vHeight, 0.25)
+    // we've got work painting do, let's throw it in the
-        );
+    // domWrite callback so everyone plays nice
-
+    // ******** DOM WRITE ****************
-        // we're done here, good work
+    const nodes = this._nodes;
-        this._queue = SCROLL_QUEUE_NO_CHANGES;
+    for (var i = 0; i < nodes.length; i++) {
-      });
+      if (nodes[i].hasChanges) {
-
+        (<any>nodes[i].view).detectChanges();
-    } else if (this._queue === SCROLL_QUEUE_CHANGE_DETECTION) {
-      // we need to do some change detection in this frame
-
-      this._dom.write(() => {
-        // we've got work painting do, let's throw it in the
-        // domWrite callback so everyone plays nice
-        // ******** DOM WRITE ****************
-        for (var i = 0; i < nodes.length; i++) {
-          if (nodes[i].hasChanges) {
-            (<any>nodes[i].view).detectChanges();
-          }
-        }
-
-        // on the next frame we need write to the dom nodes manually
-        this._queue = SCROLL_QUEUE_DOM_WRITE;
-      });
-
-    } else {
-      // no dom writes or change detection to take care of
-      // let's see if we've scroll far enough to require another check
-      data.scrollDiff = (data.scrollTop - this._lastCheck);
-
-      if (Math.abs(data.scrollDiff) > SCROLL_DIFFERENCE_MINIMUM) {
-        // don't bother updating if the scrollTop hasn't changed much
-        this._lastCheck = data.scrollTop;
-
-        if (data.scrollDiff > 0) {
-          // load data we may not have processed yet
-          var stopAtHeight = (data.scrollTop + data.renderHeight);
-
-          processRecords(stopAtHeight, this._records, cells,
-                         this._hdrFn, this._ftrFn, data);
-        }
-
-        // ******** DOM READ ****************
-        updateDimensions(this._plt, nodes, cells, data, false);
-
-        adjustRendered(cells, data);
-
-        var hasChanges = populateNodeData(data.topCell, data.bottomCell,
-                           data.viewWidth, data.scrollDiff > 0,
-                           cells, this._records, nodes,
-                           this._itmTmp.viewContainer,
-                           this._itmTmp.templateRef,
-                           this._hdrTmp && this._hdrTmp.templateRef,
-                           this._ftrTmp && this._ftrTmp.templateRef, false);
-
-        if (hasChanges) {
-          // queue making updates in the next frame
-          this._queue = SCROLL_QUEUE_CHANGE_DETECTION;
-
-          // update the bound context for each node
-          updateNodeContext(nodes, cells, data);
-        }
       }
+    }
 
+    // on the next frame we need write to the dom nodes manually
+    this._queue = SCROLL_QUEUE_DOM_WRITE;
+  }
+
+  /**
+   * @private
+   */
+  private _stepNoChanges() {
+    const data = this._data;
+
+    // let's see if we've scroll far enough to require another check
+    const diff = data.scrollDiff = (data.scrollTop - this._lastCheck);
+    if (Math.abs(diff) < SCROLL_DIFFERENCE_MINIMUM) {
+      return;
+    }
+
+    const cells = this._cells;
+    const nodes = this._nodes;
+    const records = this._records;
+
+    // don't bother updating if the scrollTop hasn't changed much
+    this._lastCheck = data.scrollTop;
+
+    if (diff > 0) {
+      // load data we may not have processed yet
+      var stopAtHeight = (data.scrollTop + data.renderHeight);
+
+      processRecords(stopAtHeight, records, cells,
+                      this._hdrFn, this._ftrFn, data);
+    }
+
+    // ******** DOM READ ****************
+    updateDimensions(this._plt, nodes, cells, data, false);
+
+    adjustRendered(cells, data);
+
+    var hasChanges = populateNodeData(
+      data.topCell, data.bottomCell,
+      data.viewWidth, diff > 0,
+      cells, records, nodes,
+      this._itmTmp.viewContainer,
+      this._itmTmp.templateRef,
+      this._hdrTmp && this._hdrTmp.templateRef,
+      this._ftrTmp && this._ftrTmp.templateRef, false
+    );
+
+    if (hasChanges) {
+      // queue making updates in the next frame
+      this._queue = SCROLL_QUEUE_CHANGE_DETECTION;
+
+      // update the bound context for each node
+      updateNodeContext(nodes, cells, data);
+    }
+  }
+
+  /**
+   * @private
+   */
+  scrollUpdate(ev: ScrollEvent) {
+    // set the scroll top from the scroll event
+    this._data.scrollTop = ev.scrollTop;
+
+    // there is a queue system so that we can
+    // spread out the work over multiple frames
+    const queue = this._queue;
+    if (queue === SCROLL_QUEUE_NO_CHANGES) {
+      // no dom writes or change detection to take care of
+      this._stepNoChanges();
+    } else if (queue === SCROLL_QUEUE_CHANGE_DETECTION) {
+      this._dom.write(() => this._stepChangeDetection());
+    } else {
+      assert(queue === SCROLL_QUEUE_DOM_WRITE, 'queue value unexpected');
+      // there are DOM writes we need to take care of in this frame
+      this._dom.write(() => this._stepDOMWrite());
     }
   }
 
@@ -671,37 +684,19 @@ export class VirtualScroll implements DoCheck, AfterContentInit, OnDestroy {
    * DOM WRITE
    */
   scrollEnd(ev: ScrollEvent) {
-    const nodes = this._nodes;
-    const cells = this._cells;
-    const data = this._data;
-
     // ******** DOM READ ****************
-    updateDimensions(this._plt, nodes, cells, data, false);
+    updateDimensions(this._plt, this._nodes, this._cells, this._data, false);
-
+    adjustRendered(this._cells, this._data);
-    adjustRendered(cells, data);
-
-    // ******** DOM READS ABOVE / DOM WRITES BELOW ****************
 
+    // ******** DOM WRITE ***************
     this._dom.write(() => {
-      const recordsLength = this._records.length;
-
       // update the bound context for each node
-      updateNodeContext(nodes, cells, data);
+      updateNodeContext(this._nodes, this._cells, this._data);
 
+      // ******** DOM WRITE ***************
+      this._stepChangeDetection();
       // ******** DOM WRITE ****************
-      for (var i = 0; i < nodes.length; i++) {
+      this._stepDOMWrite();
-        (<any>nodes[i].view).detectChanges();
-      }
-
-      // ******** DOM WRITE ****************
-      writeToNodes(this._plt, nodes, cells, recordsLength);
-
-      // ******** DOM WRITE ****************
-      this._setHeight(
-        estimateHeight(recordsLength, cells[cells.length - 1], this._vHeight, 0.05)
-      );
-
-      this._queue = SCROLL_QUEUE_NO_CHANGES;
     });
   }
 
@@ -709,6 +704,7 @@ export class VirtualScroll implements DoCheck, AfterContentInit, OnDestroy {
    * NO DOM
    */
   private _listeners() {
+    assert(!this._scrollSub, '_listeners was already called');
     if (!this._scrollSub) {
       this._resizeSub = this._plt.resize.subscribe(this.resize.bind(this));
       this._scrollSub = this._content.ionScroll.subscribe(this.scrollUpdate.bind(this));
@@ -734,9 +730,7 @@ export class VirtualScroll implements DoCheck, AfterContentInit, OnDestroy {
    * @hidden
    */
   ngAfterContentInit() {
-    if (!this._itmTmp) {
+    assert(this._itmTmp, 'virtualItem required within virtualScroll');
-      throw 'virtualItem required within virtualScroll';
-    }
 
     if (!this.approxItemHeight) {
       this.approxItemHeight = '40px';
@@ -755,7 +749,7 @@ export class VirtualScroll implements DoCheck, AfterContentInit, OnDestroy {
     this._resizeSub && this._resizeSub.unsubscribe();
     this._scrollSub && this._scrollSub.unsubscribe();
     this._scrollEndSub && this._scrollEndSub.unsubscribe();
-    this._scrollEndSub = this._scrollSub = null;
+    this._resizeSub = this._scrollEndSub = this._scrollSub = null;
     this._hdrFn = this._ftrFn = this._records = this._cells = this._nodes = this._data = null;
   }
 }

src/components/virtual-scroll/virtual-util.ts

@@ -2,6 +2,14 @@ import { ViewContainerRef, TemplateRef, EmbeddedViewRef } from '@angular/core';
 
 import { Platform } from '../../platform/platform';
 
+const PREVIOUS_CELL = {
+      row: 0,
+      width: 0,
+      height: 0,
+      top: 0,
+      left: 0,
+      tmpl: -1
+    };
 /**
  * NO DOM
  */
@@ -25,14 +33,7 @@ export function processRecords(stopAtHeight: number,
 
   } else {
     // no cells have been created yet
-    previousCell = {
+    previousCell = PREVIOUS_CELL;
-      row: 0,
-      width: 0,
-      height: 0,
-      top: 0,
-      left: 0,
-      tmpl: -1
-    };
     startRecordIndex = 0;
   }
 
@@ -317,9 +318,17 @@ export function updateDimensions(plt: Platform, nodes: VirtualNode[], cells: Vir
   data.bottomViewCell = 0;
 
   // completely realign position to ensure they're all accurately placed
-  for (var i = 1; i < totalCells; i++) {
+  cell = cells[0];
+  previousCell = {
+    row: 0,
+    width: 0,
+    height: 0,
+    top: cell.top,
+    left: 0,
+    tmpl: -1
+  };
+  for (var i = 0; i < totalCells; i++) {
     cell = cells[i];
-    previousCell = cells[i - 1];
 
     if (previousCell.left + previousCell.width + cell.width > data.viewWidth) {
       // new row
@@ -341,6 +350,7 @@ export function updateDimensions(plt: Platform, nodes: VirtualNode[], cells: Vir
     } else if (cell.top < viewableBottom && i > data.bottomViewCell) {
       data.bottomViewCell = i;
     }
+    previousCell = cell;
   }
 
 }