From ccb49f36d6bad7167bf84885070e06af9d1425b6 Mon Sep 17 00:00:00 2001 From: Manuel Mtz-Almeida Date: Thu, 23 Mar 2017 23:01:43 +0100 Subject: [PATCH] fix(virtual-list): several issues --- .../test/virtual-scroll.spec.ts | 2 +- .../virtual-scroll/virtual-scroll.ts | 328 +++++++++--------- src/components/virtual-scroll/virtual-util.ts | 30 +- 3 files changed, 182 insertions(+), 178 deletions(-) diff --git a/src/components/virtual-scroll/test/virtual-scroll.spec.ts b/src/components/virtual-scroll/test/virtual-scroll.spec.ts index c1c20efb82e..2d7d3fa1a36 100644 --- a/src/components/virtual-scroll/test/virtual-scroll.spec.ts +++ b/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)}, diff --git a/src/components/virtual-scroll/virtual-scroll.ts b/src/components/virtual-scroll/virtual-scroll.ts index cbc1e193930..2697f8fa134 100644 --- a/src/components/virtual-scroll/virtual-scroll.ts +++ b/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 = 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(() => { - this._init = true; - if (isPresent(this._changes())) { - this.readUpdate(true); - - // wait for the content to be writable - var subscription = _ctrl.writeReady.subscribe(() => { - subscription.unsubscribe(); - this.writeUpdate(true); - }); - } + 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; + 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._cells, - this._hdrFn, - this._ftrFn, - this._data); + this._records, + this._cells, + this._hdrFn, + this._ftrFn, + 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; + 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); - } + if (needClean) { + // ******** DOM WRITE **************** + updateDimensions(this._plt, nodes, cells, data, true); + data.topCell = 0; + data.bottomCell = (cells.length - 1); + } - adjustRendered(cells, data); + 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); + 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(); - } + if (needClean) { + this._cd.detectChanges(); + } - this._plt.raf(() => { // 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 **************** - initReadNodes(this._plt, nodes, cells, data); - }); - this._dom.write(() => { - const ele = this._elementRef.nativeElement; - const recordsLength = records.length; - const renderer = this._renderer; + // ******** DOM READ **************** + this._dom.read(() => initReadNodes(this._plt, nodes, cells, data)); + this._dom.write(() => { // update the bound context for each node updateNodeContext(nodes, cells, data); // ******** DOM WRITE **************** - for (var i = 0; i < nodes.length; i++) { - (nodes[i].view).detectChanges(); - } + this._stepChangeDetection(); + // ******** 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); - - // ******** DOM WRITE **************** - writeToNodes(this._plt, nodes, cells, recordsLength); - - // ******** DOM WRITE **************** - this._setHeight( - estimateHeight(recordsLength, cells[cells.length - 1], this._vHeight, 0.25) - ); + // ******** DOM WRITE **************** + this.setElementClass('virtual-scroll', true); - this._content.imgsUpdate(); + // ******** DOM WRITE **************** + this.setElementClass('virtual-loading', false); + } + assert(this._queue === SCROLL_QUEUE_NO_CHANGES, 'queue value should be NO_CHANGES'); }); }); } @@ -579,90 +570,112 @@ export class VirtualScroll implements DoCheck, AfterContentInit, OnDestroy { /** * @hidden */ - scrollUpdate(ev: ScrollEvent) { - // there is a queue system so that we can - // spread out the work over multiple frames - const data = this._data; + private _stepDOMWrite() { const cells = this._cells; const nodes = this._nodes; + const recordsLength = this._records.length; - // set the scroll top from the scroll event - data.scrollTop = ev.scrollTop; - - if (this._queue === SCROLL_QUEUE_DOM_WRITE) { - // there are DOM writes we need to take care of in this frame + // ******** DOM WRITE **************** + writeToNodes(this._plt, nodes, cells, recordsLength); - this._dom.write(() => { - const recordsLength = this._records.length; + // ******** DOM WRITE **************** + this._setHeight( + estimateHeight(recordsLength, cells[cells.length - 1], this._vHeight, 0.25) + ); - // ******** DOM WRITE **************** - writeToNodes(this._plt, nodes, cells, recordsLength); + // we're done here, good work + this._queue = SCROLL_QUEUE_NO_CHANGES; + } - // ******** DOM WRITE **************** - this._setHeight( - estimateHeight(recordsLength, cells[cells.length - 1], this._vHeight, 0.25) - ); + /** + * @private + */ + private _stepChangeDetection() { + // we need to do some change detection in this frame + // we've got work painting do, let's throw it in the + // domWrite callback so everyone plays nice + // ******** DOM WRITE **************** + const nodes = this._nodes; + for (var i = 0; i < nodes.length; i++) { + if (nodes[i].hasChanges) { + (nodes[i].view).detectChanges(); + } + } - // we're done here, good work - this._queue = SCROLL_QUEUE_NO_CHANGES; - }); + // on the next frame we need write to the dom nodes manually + this._queue = SCROLL_QUEUE_DOM_WRITE; + } - } else if (this._queue === SCROLL_QUEUE_CHANGE_DETECTION) { - // we need to do some change detection in this frame + /** + * @private + */ + private _stepNoChanges() { + const data = this._data; - 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) { - (nodes[i].view).detectChanges(); - } - } + // 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; + } - // on the next frame we need write to the dom nodes manually - this._queue = SCROLL_QUEUE_DOM_WRITE; - }); + const cells = this._cells; + const nodes = this._nodes; + const records = this._records; - } 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); + // don't bother updating if the scrollTop hasn't changed much + this._lastCheck = data.scrollTop; - if (Math.abs(data.scrollDiff) > SCROLL_DIFFERENCE_MINIMUM) { - // 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); - if (data.scrollDiff > 0) { - // load data we may not have processed yet - var stopAtHeight = (data.scrollTop + data.renderHeight); + processRecords(stopAtHeight, records, cells, + this._hdrFn, this._ftrFn, data); + } - processRecords(stopAtHeight, this._records, cells, - this._hdrFn, this._ftrFn, data); - } + // ******** DOM READ **************** + updateDimensions(this._plt, nodes, cells, data, false); - // ******** DOM READ **************** - updateDimensions(this._plt, nodes, cells, data, false); + adjustRendered(cells, data); - 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 + ); - 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; - 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); + } + } - // 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); - - adjustRendered(cells, data); - - // ******** DOM READS ABOVE / DOM WRITES BELOW **************** + updateDimensions(this._plt, this._nodes, this._cells, this._data, false); + adjustRendered(this._cells, this._data); + // ******** DOM WRITE *************** this._dom.write(() => { - const recordsLength = this._records.length; - // update the bound context for each node - updateNodeContext(nodes, cells, data); - - // ******** DOM WRITE **************** - for (var i = 0; i < nodes.length; i++) { - (nodes[i].view).detectChanges(); - } - - // ******** DOM WRITE **************** - writeToNodes(this._plt, nodes, cells, recordsLength); + updateNodeContext(this._nodes, this._cells, this._data); + // ******** DOM WRITE *************** + this._stepChangeDetection(); // ******** DOM WRITE **************** - this._setHeight( - estimateHeight(recordsLength, cells[cells.length - 1], this._vHeight, 0.05) - ); - - this._queue = SCROLL_QUEUE_NO_CHANGES; + this._stepDOMWrite(); }); } @@ -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) { - throw 'virtualItem required within virtualScroll'; - } + assert(this._itmTmp, '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; } } diff --git a/src/components/virtual-scroll/virtual-util.ts b/src/components/virtual-scroll/virtual-util.ts index f1c20ae2b4a..abd912686f1 100644 --- a/src/components/virtual-scroll/virtual-util.ts +++ b/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 = { - row: 0, - width: 0, - height: 0, - top: 0, - left: 0, - tmpl: -1 - }; + previousCell = PREVIOUS_CELL; 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; } }