fix(virtual-scroll): ensure listeners added after init read

Author: adamdbradley

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

@@ -208,7 +208,7 @@ export class VirtualScroll implements DoCheck, AfterContentInit, OnDestroy {
 
   /**
    * @input {array} The data that builds the templates within the virtual scroll.
-   * This is the same data that you'd pass to `ngFor`. It's important to note
+   * This is the same data that you'd pass to `*ngFor`. It's important to note
    * that when this data has changed, then the entire virtual scroll is reset,
    * which is an expensive operation and should be avoided if possible.
    */
@@ -225,12 +225,12 @@ export class VirtualScroll implements DoCheck, AfterContentInit, OnDestroy {
    * should get created when initially rendered. The number is a
    * multiplier against the viewable area's height. For example, if it
    * takes `20` cells to fill up the height of the viewable area, then
-   * with a buffer ratio of `2` it will create `40` cells that are
+   * with a buffer ratio of `3` it will create `60` cells that are
    * available for reuse while scrolling. For better performance, it's
    * better to have more cells than what are required to fill the
-   * viewable area. Default is `2`.
+   * viewable area. Default is `3`.
    */
-  @Input() bufferRatio: number = 2;
+  @Input() bufferRatio: number = 3;
 
   /**
    * @input {string} The approximate width of each item template's cell.
@@ -239,20 +239,22 @@ export class VirtualScroll implements DoCheck, AfterContentInit, OnDestroy {
    * the scrollable area. This value can use either `px` or `%` units.
    * Note that the actual rendered size of each cell comes from the
    * app's CSS, whereas this approximation is used to help calculate
-   * initial dimensions. Default is `100%`.
+   * initial dimensions before the item has been rendered. Default is
+   * `100%`.
    */
   @Input() approxItemWidth: string = '100%';
 
   /**
-   * @input {string} Default is `40px`. It is important to provide this
+   * @input {string} It is important to provide this
    * if virtual item height will be significantly larger than the default
    * The approximate height of each virtual item template's cell.
    * This dimension is used to help determine how many cells should
    * be created when initialized, and to help calculate the height of
    * the scrollable area. This height value can only use `px` units.
    * Note that the actual rendered size of each cell comes from the
    * app's CSS, whereas this approximation is used to help calculate
-   * initial dimensions.
+   * initial dimensions before the item has been rendered. Default is
+   * `40px`.
    */
   @Input() approxItemHeight: string;
 
@@ -274,7 +276,7 @@ export class VirtualScroll implements DoCheck, AfterContentInit, OnDestroy {
    * the scrollable area. This height value can only use `px` units.
    * Note that the actual rendered size of each cell comes from the
    * app's CSS, whereas this approximation is used to help calculate
-   * initial dimensions. Default is `40px`.
+   * initial dimensions before the item has been rendered. Default is `40px`.
    */
   @Input() approxHeaderHeight: string = '40px';
 
@@ -285,7 +287,7 @@ export class VirtualScroll implements DoCheck, AfterContentInit, OnDestroy {
    * the scrollable area. This value can use either `px` or `%` units.
    * Note that the actual rendered size of each cell comes from the
    * app's CSS, whereas this approximation is used to help calculate
-   * initial dimensions. Default is `100%`.
+   * initial dimensions before the item has been rendered. Default is `100%`.
    */
   @Input() approxFooterWidth: string = '100%';
 
@@ -296,7 +298,7 @@ export class VirtualScroll implements DoCheck, AfterContentInit, OnDestroy {
    * the scrollable area. This height value can only use `px` units.
    * Note that the actual rendered size of each cell comes from the
    * app's CSS, whereas this approximation is used to help calculate
-   * initial dimensions. Default is `40px`.
+   * initial dimensions before the item has been rendered. Default is `40px`.
    */
   @Input() approxFooterHeight: string = '40px';
 
@@ -345,7 +347,7 @@ export class VirtualScroll implements DoCheck, AfterContentInit, OnDestroy {
     private _content: Content,
     private _platform: Platform,
     @Optional() private _ctrl: ViewController,
-    config: Config,
+    private _config: Config,
     private _dom: DomController) {
 
     // hide the virtual scroll element with opacity so we don't
@@ -355,6 +357,8 @@ export class VirtualScroll implements DoCheck, AfterContentInit, OnDestroy {
 
     // wait for the content to be rendered and has readable dimensions
     _content.readReady.subscribe(() => {
+      this._init = true;
+
       if (this._hasChanges()) {
         this.readUpdate();
 
@@ -363,15 +367,24 @@ export class VirtualScroll implements DoCheck, AfterContentInit, OnDestroy {
           subscription.unsubscribe();
           this.writeUpdate();
         });
-
-        if (!this._scrollSub) {
-          // listen for scroll events
-          this.addScrollListener(config.getBoolean('virtualScrollEventAssist'));
-        }
       }
+
+      this._listeners();
     });
   }
 
+  /**
+   * @private
+   */
+  ngDoCheck() {
+    if (this._init && this._hasChanges()) {
+      // only continue if we've already initialized
+      // and if there actually are changes
+      this.readUpdate();
+      this.writeUpdate();
+    }
+  }
+
   readUpdate() {
     console.debug(`virtual-scroll, readUpdate`);
 
@@ -406,37 +419,6 @@ export class VirtualScroll implements DoCheck, AfterContentInit, OnDestroy {
     return (isPresent(this._records) && isPresent(this._differ) && isPresent(this._differ.diff(this._records)));
   }
 
-  /**
-   * @private
-   */
-  ngDoCheck() {
-    if (this._init && this._hasChanges()) {
-      // only continue if we've already initialized
-      // and if there actually are changes
-      this.readUpdate();
-      this.writeUpdate();
-    }
-  }
-
-  /**
-   * @private
-   */
-  ngAfterContentInit() {
-    if (!this._init) {
-
-      if (!this._itmTmp) {
-        throw 'virtualItem required within virtualScroll';
-      }
-
-      this._init = true;
-
-      if (!this.approxItemHeight) {
-        this.approxItemHeight = '40px';
-        console.warn('Virtual Scroll: Please provide an "approxItemHeight" input to ensure proper virtual scroll rendering');
-      }
-    }
-  }
-
   /**
    * @private
    * DOM WRITE
@@ -502,7 +484,7 @@ export class VirtualScroll implements DoCheck, AfterContentInit, OnDestroy {
       writeToNodes(nodes, cells, recordsLength);
 
       // ******** DOM WRITE ****************
-      this.setVirtualHeight(
+      this._setHeight(
         estimateHeight(recordsLength, cells[cells.length - 1], this._vHeight, 0.25)
       );
 
@@ -525,13 +507,14 @@ export class VirtualScroll implements DoCheck, AfterContentInit, OnDestroy {
     data.scrollTop = ev.scrollTop;
 
     if (this._queue === ScrollQueue.RequiresDomWrite) {
+      // there are DOM writes we need to take care of in this frame
 
       this._dom.write(() => {
         // ******** DOM WRITE ****************
         writeToNodes(nodes, cells, this._records.length);
 
         // ******** DOM WRITE ****************
-        this.setVirtualHeight(
+        this._setHeight(
           estimateHeight(this._records.length, cells[cells.length - 1], this._vHeight, 0.25)
         );
 
@@ -540,6 +523,7 @@ export class VirtualScroll implements DoCheck, AfterContentInit, OnDestroy {
       });
 
     } else if (this._queue === ScrollQueue.RequiresChangeDetection) {
+      // 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
@@ -556,7 +540,8 @@ export class VirtualScroll implements DoCheck, AfterContentInit, OnDestroy {
       });
 
     } 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) {
@@ -625,7 +610,7 @@ export class VirtualScroll implements DoCheck, AfterContentInit, OnDestroy {
       writeToNodes(nodes, cells, this._records.length);
 
       // ******** DOM WRITE ****************
-      this.setVirtualHeight(
+      this._setHeight(
         estimateHeight(this._records.length, cells[cells.length - 1], this._vHeight, 0.05)
       );
 
@@ -634,10 +619,9 @@ export class VirtualScroll implements DoCheck, AfterContentInit, OnDestroy {
   }
 
   /**
-   * @private
    * DOM WRITE
    */
-  setVirtualHeight(newVirtualHeight: number) {
+  private _setHeight(newVirtualHeight: number) {
     if (newVirtualHeight !== this._vHeight) {
       // ******** DOM WRITE ****************
       this._renderer.setElementStyle(this._elementRef.nativeElement, 'height', newVirtualHeight > 0 ? newVirtualHeight + 'px' : '');
@@ -647,31 +631,42 @@ export class VirtualScroll implements DoCheck, AfterContentInit, OnDestroy {
     }
   }
 
+  private _listeners() {
+    if (!this._scrollSub) {
+      if (this._config.getBoolean('virtualScrollEventAssist')) {
+        // use JS scrolling for iOS UIWebView
+        // goal is to completely remove this when iOS
+        // fully supports scroll events
+        // listen to JS scroll events
+        this._content.enableJsScroll();
+      }
+
+      this._scrollSub = this._content.ionScroll.subscribe((ev: ScrollEvent) => {
+        this.scrollUpdate(ev);
+      });
+
+      this._scrollEndSub = this._content.ionScrollEnd.subscribe((ev: ScrollEvent) => {
+        this.scrollEnd(ev);
+      });
+    }
+  }
+
   /**
    * @private
-   * NO DOM
    */
-  addScrollListener(eventAssist: boolean) {
-    if (eventAssist) {
-      // use JS scrolling for iOS UIWebView
-      // goal is to completely remove this when iOS
-      // fully supports scroll events
-      // listen to JS scroll events
-      this._content.enableJsScroll();
+  ngAfterContentInit() {
+    if (!this._itmTmp) {
+      throw 'virtualItem required within virtualScroll';
     }
 
-    this._scrollSub = this._content.ionScroll.subscribe((ev: ScrollEvent) => {
-      this.scrollUpdate(ev);
-    });
-
-    this._scrollEndSub = this._content.ionScrollEnd.subscribe((ev: ScrollEvent) => {
-      this.scrollEnd(ev);
-    });
+    if (!this.approxItemHeight) {
+      this.approxItemHeight = '40px';
+      console.warn('Virtual Scroll: Please provide an "approxItemHeight" input to ensure proper virtual scroll rendering');
+    }
   }
 
   /**
    * @private
-   * NO DOM
    */
   ngOnDestroy() {
     this._scrollSub && this._scrollSub.unsubscribe();
@@ -680,7 +675,7 @@ export class VirtualScroll implements DoCheck, AfterContentInit, OnDestroy {
 
 }
 
-const SCROLL_DIFFERENCE_MINIMUM = 20;
+const SCROLL_DIFFERENCE_MINIMUM = 40;
 
 export const enum ScrollQueue {
   NoChanges,