cr/ui/list.js: Support rows with variable heights.

chrome/browser/resources/shared/js/cr/ui/list.js

Index: chrome/browser/resources/shared/js/cr/ui/list.js
diff --git a/chrome/browser/resources/shared/js/cr/ui/list.js b/chrome/browser/resources/shared/js/cr/ui/list.js
index af323dcd9a0ad9d7020d54e9b6804458d1ecc09e..bf86aa1e201ac809339fc572248ba63a0936ab68 100644
--- a/chrome/browser/resources/shared/js/cr/ui/list.js
+++ b/chrome/browser/resources/shared/js/cr/ui/list.js
@@ -121,6 +121,7 @@ cr.define('cr.ui', function() {
      * can be expanded to show more detail. It is explicitly set by client code
      * when the height of the lead item is changed with {@code set
      * leadItemHeight}, and presumed equal to {@code itemHeight_} otherwise.
+     * This value will be ignored when {@code fixedHeight_} is false.
      * @type {number}
      * @private
      */
@@ -135,6 +136,26 @@ cr.define('cr.ui', function() {
     autoExpands_: false,
 
     /**
+     * Cached list items.
+     * @type {Array<cr.ui.ListItem>}
+     * @private
+     */
+    cachedItems_: [],

[arv (Not doing code reviews), 2011/11/21 20:38:01]

These two arrays needs to go on the instance of they will be shared between all
instances.

[yoshiki, 2011/11/28 11:03:31]

Done.

+    /**
+     * Cached sized of list items.
+     * @type {Array<cr.ui.ListItem>}
+     * @private
+     */
+    cachedItemSizes_: [],
+
+    /**
+     * Whether or not the list view has a blank space below the last row.
+     * @type {boolean}
+     * @private
+     */
+    remainSpace_: true,
+
+    /**
      * Function used to create grid items.
      * @type {function(): !ListItem}
      * @private
@@ -177,6 +198,8 @@ cr.define('cr.ui', function() {
               this.boundHandleDataModelPermuted_);
           this.dataModel_.removeEventListener('change',
                                               this.boundHandleDataModelChange_);
+          this.dataModel_.removeEventListener('splice',
+                                              this.boundHandleDataModelChange_);
         }
 
         this.dataModel_ = dataModel;
@@ -192,6 +215,8 @@ cr.define('cr.ui', function() {
               this.boundHandleDataModelPermuted_);
           this.dataModel_.addEventListener('change',
                                            this.boundHandleDataModelChange_);
+          this.dataModel_.addEventListener('splice',
+                                           this.boundHandleDataModelChange_);
         }
 
         this.redraw();
@@ -249,6 +274,20 @@ cr.define('cr.ui', function() {
     },
 
     /**
+     * Whether or not the rows on list have various heights.
+     * @type {boolean}
+     */
+    get fixedHeight() {
+      return this.fixedHeight;

[arv (Not doing code reviews), 2011/11/21 20:38:01]

iloop

I assume you meant this.fixedHeight_

[yoshiki, 2011/11/28 11:03:31]

Done.

+    },
+    set fixedHeight(fixedHeight) {
+      if (this.fixedHeight_ == fixedHeight)
+        return;
+      this.fixedHeight_ = fixedHeight;
+      this.redraw();
+    },
+
+    /**
      * Convenience alias for selectionModel.selectedItem
      * @type {cr.ui.ListItem}
      */
@@ -275,11 +314,11 @@ cr.define('cr.ui', function() {
      * @type {number}
      */
     get leadItemHeight() {
-      return this.leadItemHeight_ || this.getItemHeight_();
+      return this.leadItemHeight_ || this.getDefaultItemHeight_();
     },
     set leadItemHeight(height) {
       if (height) {
-        var size = this.getItemSize_();
+        var size = this.getDefaultItemSize_();
         this.leadItemHeight_ = Math.max(0, height + size.marginVertical);
       } else {
         this.leadItemHeight_ = 0;
@@ -363,27 +402,36 @@ cr.define('cr.ui', function() {
     },
 
     /**
-     * @return {number} The height of an item, measuring it if necessary.
+     * @return {number} The height of default item, measuring it if necessary.
      * @private
      */
-    getItemHeight_: function() {
-      return this.getItemSize_().height;
+    getDefaultItemHeight_: function() {
+      return this.getDefaultItemSize_().height;
+    },
+
+    getItemHeightByIndex_: function(index) {

[arv (Not doing code reviews), 2011/11/21 20:38:01]

missing jsdoc

[yoshiki, 2011/11/28 11:03:31]

Done.

+      if (this.cachedItemSizes_[index])
+        return this.cachedItemSizes_[index].height;
+
+      var item = this.getListItemByIndex(index);
+      return item ? this.getItemSize_(item).height :
+                    this.getDefaultItemHeight_();
     },
 
     /**
-     * @return {number} The width of an item, measuring it if necessary.
+     * @return {number} The width of default item, measuring it if necessary.
      * @private
      */
-    getItemWidth_: function() {
-      return this.getItemSize_().width;
+    getDefaultItemWidth_: function() {
+      return this.getDefaultItemSize_().width;
     },
 
     /**
      * @return {{height: number, width: number}} The height and width
-     *     of an item, measuring it if necessary.
+     *     of default item, measuring it if necessary.
      * @private
      */
-    getItemSize_: function() {
+    getDefaultItemSize_: function() {
       if (!this.measured_ || !this.measured_.height) {
         this.measured_ = measureItem(this);
       }
@@ -391,6 +439,22 @@ cr.define('cr.ui', function() {
     },
 
     /**
+     * @return {{height: number, width: number}} The height and width
+     *     of an item, measuring it if necessary.
+     * @private
+     */
+    getItemSize_: function(item) {
+      if (this.cachedItemSizes_[item.listIndex])
+        return this.cachedItemSizes_[item.listIndex];
+
+      var size = measureItem(this, item);
+      if (!isNaN(size.height) && !isNaN(size.weight))
+        this.cachedItemSizes_[item.listIndex] = size;
+
+      return size;
+    },
+
+    /**
      * Callback for the double click event.
      * @param {Event} e The mouse event object.
      * @private
@@ -602,7 +666,8 @@ cr.define('cr.ui', function() {
     },
 
     handleDataModelChange_: function(e) {
-      if (e.index >= this.firstIndex_ && e.index < this.lastIndex_) {
+      if (e.index >= this.firstIndex_ &&
+          (e.index < this.lastIndex_ || this.remainSpace_)) {

[arv (Not doing code reviews), 2011/11/21 20:38:01]

rename to remainingSpace_?

[yoshiki, 2011/11/28 11:03:31]

Done.

         if (this.cachedItems_[e.index])
           delete this.cachedItems_[e.index];
         this.redraw();
@@ -611,11 +676,23 @@ cr.define('cr.ui', function() {
 
     /**
      * @param {number} index The index of the item.
-     * @return {number} The top position of the item inside the list, not taking
-     *     into account lead item. May vary in the case of multiple columns.
+     * @return {number} The top position of the item inside the list.
      */
     getItemTop: function(index) {
-      return index * this.getItemHeight_();
+      if (this.fixedHeight_) {
+        var itemHeight = this.getDefaultItemHeight_();
+        var top = index * itemHeight;
+        if (this.selectionModel.leadIndex > -1 &&
+            this.selectionModel.leadIndex < index) {
+          top += this.leadItemHeight - itemHeight;
+        }
+        return top;
+      } else {
+        var top = 0;
+        for (var i = 0; i < index; i++)

[arv (Not doing code reviews), 2011/11/21 20:38:01]

Only skip {} for if/else

[yoshiki, 2011/11/28 11:03:31]

Done.

+          top += this.getItemHeightByIndex_(i);
+        return top;
+      }
     },
 
     /**
@@ -645,32 +722,41 @@ cr.define('cr.ui', function() {
       if (!dataModel || index < 0 || index >= dataModel.length)
         return false;
 
-      var itemHeight = this.getItemHeight_();
+      var itemHeight = this.getItemHeightByIndex_(index);
       var scrollTop = this.scrollTop;
       var top = this.getItemTop(index);
-      var leadIndex = this.selectionModel.leadIndex;
-
-      // Adjust for the lead item if it is above the given index.
-      if (leadIndex > -1 && leadIndex < index)
-        top += this.leadItemHeight - itemHeight;
-      else if (leadIndex == index)
-        itemHeight = this.leadItemHeight;
-
-      if (top < scrollTop) {
-        this.scrollTop = top;
-        return true;
-      } else {
-        var clientHeight = this.clientHeight;
-        var cs = getComputedStyle(this);
-        var paddingY = parseInt(cs.paddingTop, 10) +
-                       parseInt(cs.paddingBottom, 10);
+      var clientHeight = this.clientHeight;
 
-        if (top + itemHeight > scrollTop + clientHeight - paddingY) {
-          this.scrollTop = top + itemHeight - clientHeight + paddingY;
+      var self = this;
+      // Function to adjust the tops of viewport and row.
+      var scrollToAdjustTopFunc = function() {

[arv (Not doing code reviews), 2011/11/21 20:38:01]

function scrollToAdjustTop() {

[yoshiki, 2011/11/28 11:03:31]

Done.

+          self.scrollTop = top;
           return true;
-        }
+      };
+      // Function to adjust the bottoms of viewport and row.
+      var scrollToAdjustBottomFunc = function () {

[arv (Not doing code reviews), 2011/11/21 20:38:01]

function scrollToAdjustBottom() {

}

[yoshiki, 2011/11/28 11:03:31]

Done.

+          var cs = getComputedStyle(self);
+          var paddingY = parseInt(cs.paddingTop, 10) +
+                         parseInt(cs.paddingBottom, 10);
+
+          if (top + itemHeight > scrollTop + clientHeight - paddingY) {
+            self.scrollTop = top + itemHeight - clientHeight + paddingY;
+            return true;
+          }
+          return false;
+      };
+
+      if (itemHeight <= clientHeight) {
+        if (top < scrollTop)
+          return scrollToAdjustTopFunc(index);
+        else ((scrollTop + clientHeight) < (top + itemHeight))

[arv (Not doing code reviews), 2011/11/21 20:38:01]

Syntax error

[yoshiki, 2011/11/28 11:03:31]

Done.

+          return scrollToAdjustBottomFunc(index);
+      } else {
+        if (scrollTop < top)
+          return scrollToAdjustTopFunc(index);
+        else if ((top + itemHeight) < (scrollTop + clientHeight))

[arv (Not doing code reviews), 2011/11/21 20:38:01]

no else after return

[arv (Not doing code reviews), 2011/11/21 20:38:01]

Too many parentheses

if (top + itemHeight < scrollTop + clientHeight)

[yoshiki, 2011/11/28 11:03:31]

Done.

[yoshiki, 2011/11/28 11:03:31]

Done.

+          return scrollToAdjustBottomFunc(index);
       }
-
       return false;
     },
 
@@ -756,14 +842,8 @@ cr.define('cr.ui', function() {
      * @private
      */
     getHeightsForIndex_: function(index) {
-      var itemHeight = this.getItemHeight_();
+      var itemHeight = this.getItemHeightByIndex_(index);
       var top = this.getItemTop(index);
-      if (this.selectionModel.leadIndex > -1 &&
-          this.selectionModel.leadIndex < index) {
-        top += this.leadItemHeight - itemHeight;
-      } else if (this.selectionModel.leadIndex == index) {
-        itemHeight = this.leadItemHeight;
-      }
       return {top: top, height: itemHeight};
     },
 
@@ -772,27 +852,61 @@ cr.define('cr.ui', function() {
      * in pixels from the top) within the list. In the case of multiple columns,
      * returns the first index in the row.
      * @param {number} offset The y offset in pixels to get the index of.
-     * @return {number} The index of the list item.
+     * @return {number} The index of the list item. Returns the list size if
+     *     given offset exceeds the height of list.
      * @private
      */
     getIndexForListOffset_: function(offset) {
-      var itemHeight = this.getItemHeight_();
-      var leadIndex = this.selectionModel.leadIndex;
-      var leadItemHeight = this.leadItemHeight;
-      if (leadIndex < 0 || leadItemHeight == itemHeight) {
-        // Simple case: no lead item or lead item height is not different.
+      var itemHeight = this.getDefaultItemHeight_();
+      if (itemHeight == 0)

[arv (Not doing code reviews), 2011/11/21 20:38:01]

if (!itemHeight)

[yoshiki, 2011/11/28 11:03:31]

Done.

+          return this.dataModel.length;

[arv (Not doing code reviews), 2011/11/21 20:38:01]

fix indentation

[yoshiki, 2011/11/28 11:03:31]

Done.

+
+      if (this.fixedHeight_) {
+        var leadIndex = this.selectionModel.leadIndex;
+        var leadItemHeight = this.leadItemHeight;
+        if (leadIndex < 0 || leadItemHeight == itemHeight) {
+          // Simple case: no lead item or lead item height is not different.
+          return this.getFirstItemInRow(Math.floor(offset / itemHeight));
+        }
+        var leadTop = this.getItemTop(leadIndex);
+        // If the given offset is above the lead item, it's also simple.
+        if (offset < leadTop)
+          return this.getFirstItemInRow(Math.floor(offset / itemHeight));
+        // If the lead item contains the given offset, we just return its index.
+        if (offset < leadTop + leadItemHeight)

[arv (Not doing code reviews), 2011/11/21 20:38:01]

please add some newlines somewhere... this could use some grouping

[yoshiki, 2011/11/28 11:03:31]

Done.

+          return this.getFirstItemInRow(this.getItemRow(leadIndex));
+        // The given offset must be below the lead item. Adjust and recalculate.
+        offset -= leadItemHeight - itemHeight;
         return this.getFirstItemInRow(Math.floor(offset / itemHeight));
+      } else {
+        // If offset exceeds the height of list.
+        var lastHeight =  0;
+        if (this.dataModel.length) {
+          var h = this.getHeightsForIndex_(this.dataModel.length - 1);
+          lastHeight = h.top + h.height;
+        }
+        if (lastHeight < offset)
+          return this.dataModel.length;
+
+        // Estimates index.
+        var estimatedIndex = Math.min(Math.floor(offset / itemHeight),
+                                       this.dataModel.length - 1);

[arv (Not doing code reviews), 2011/11/21 20:38:01]

fix indentation

[yoshiki, 2011/11/28 11:03:31]

Done.

+        var isIncrementing = this.getItemTop(estimatedIndex) < offset;
+
+        // Searchs the correct index.
+        do {
+          var heights = this.getHeightsForIndex_(estimatedIndex);
+          var top = heights.top;
+          var height = heights.height;
+
+          if (top <= offset && offset <= (top + height))
+            break;
+
+          isIncrementing ? ++estimatedIndex: --estimatedIndex;
+        } while (0 < estimatedIndex && estimatedIndex < this.dataModel.length)
+
+        return estimatedIndex;
       }
-      var leadTop = this.getItemTop(leadIndex);
-      // If the given offset is above the lead item, it's also simple.
-      if (offset < leadTop)
-        return this.getFirstItemInRow(Math.floor(offset / itemHeight));
-      // If the lead item contains the given offset, we just return its index.
-      if (offset < leadTop + leadItemHeight)
-        return this.getFirstItemInRow(this.getItemRow(leadIndex));
-      // The given offset must be below the lead item. Adjust and recalculate.
-      offset -= leadItemHeight - itemHeight;
-      return this.getFirstItemInRow(Math.floor(offset / itemHeight));
     },
 
     /**
@@ -809,26 +923,25 @@ cr.define('cr.ui', function() {
     },
 
     /**
-     * Calculates the number of items fitting in viewport given the index of
-     * first item and heights.
-     * @param {number} itemHeight The height of the item.
-     * @param {number} firstIndex Index of the first item in viewport.
+     * Calculates the number of items fitting in the given viewport.
      * @param {number} scrollTop The scroll top position.
-     * @return {number} The number of items in view port.
-     */
-    getItemsInViewPort: function(itemHeight, firstIndex, scrollTop) {
-      // This is a bit tricky. We take the minimum of the available items to
-      // show and the number we want to show, so as not to go off the end of the
-      // list. For the number we want to show, we take the maximum of the number
-      // that would fit without a differently-sized lead item, and with one. We
-      // do this so that if the size of the lead item changes without a scroll
-      // event to trigger redrawing the list, we won't end up with empty space.
-      var clientHeight = this.clientHeight;
-      return this.autoExpands_ ? this.dataModel.length : Math.min(
-          this.dataModel.length - firstIndex,
-          Math.max(
-              Math.ceil(clientHeight / itemHeight) + 1,
-              this.countItemsInRange_(firstIndex, scrollTop + clientHeight)));
+     * @param {number} clientHeight The height of viewport.
+     * @return {{first: number, length: number, last: number}} The index of
+     *     first item in view port, The number of items, The item past the last.
+     */
+    getItemsInViewPort: function(scrollTop, clientHeight) {
+      if (this.autoExpands_) {
+        return {first: 0,

[arv (Not doing code reviews), 2011/11/21 20:38:01]

return {
  first: ...
};

[yoshiki, 2011/11/28 11:03:31]

Done.

+                length: this.dataModel.length,
+                last: this.dataModel.length};
+      } else {
+        var firstIndex = this.getIndexForListOffset_(scrollTop);
+        var lastIndex = this.getIndexForListOffset_(scrollTop + clientHeight);
+
+        return {first: firstIndex,
+                length: lastIndex - firstIndex + 1,
+                last: lastIndex + 1};
+      }
     },
 
     /**
@@ -849,6 +962,7 @@ cr.define('cr.ui', function() {
         listItem.listIndex = y;
         this.appendChild(listItem);
         newCachedItems[y] = listItem;
+        this.cachedItemSizes_[y] = measureItem(this, listItem);

[arv (Not doing code reviews), 2011/11/21 20:38:01]

This is bad. it will regress performance since it will make a reflow for every
iteration.

To solve this you need to add all the elements first before any measuring is
done.

[yoshiki, 2011/11/28 11:03:31]

Done.

       }
     },
 
@@ -858,8 +972,27 @@ cr.define('cr.ui', function() {
      * @param {number} itemHeight The height of the item.
      * @return {number} The height of after filler.
      */
-    getAfterFillerHeight: function(lastIndex, itemHeight) {
-      return (this.dataModel.length - lastIndex) * itemHeight;
+    getAfterFillerHeight: function(lastIndex) {
+      if (this.fixedHeight_) {
+        var itemHeight = this.getDefaultItemHeight_();
+        var afterFillerHeight =
+          (this.dataModel.length - lastIndex) * itemHeight;
+
+        var sm = this.selectionModel;
+        var leadIndex = sm.leadIndex;
+        if (leadIndex >= lastIndex)
+          afterFillerHeight += this.leadItemHeight - itemHeight;
+
+        return afterFillerHeight;
+      } else {
+        var height = 0;
+        for (var i = lastIndex; i < this.dataModel.length; i++) {
+          var item = this.getListItemByIndex(i);
+          if (item)
+            height += this.getItemSize_(item).height;
+        }
+        return height;
+      }
     },
 
     /**
@@ -871,49 +1004,55 @@ cr.define('cr.ui', function() {
 
       var dataModel = this.dataModel;
       if (!dataModel) {
+        this.cachedItems_ = {};
+        this.firstIndex_ = 0;
+        this.lastIndex_ = 0;
+        this.remainSpace_ = true;
         this.textContent = '';
         return;
       }
 
-      var scrollTop = this.scrollTop;
-      var clientHeight = this.clientHeight;
-
-      var itemHeight = this.getItemHeight_();
-
       // We cache the list items since creating the DOM nodes is the most
       // expensive part of redrawing.
       var cachedItems = this.cachedItems_ || {};
       var newCachedItems = {};
 
-      var desiredScrollHeight = this.getHeightsForIndex_(dataModel.length).top;
-
       var autoExpands = this.autoExpands_;
-      var firstIndex = autoExpands ? 0 : this.getIndexForListOffset_(scrollTop);
-      var itemsInViewPort = this.getItemsInViewPort(itemHeight, firstIndex,
-          scrollTop);
-      var lastIndex = firstIndex + itemsInViewPort;
+      var scrollTop = this.scrollTop;
+      var clientHeight = this.clientHeight;
+
+      var lastItemHeights = this.getHeightsForIndex_(dataModel.length - 1);
+      var desiredScrollHeight = lastItemHeights.top + lastItemHeights.height;
+
+      var itemsInViewPort = this.getItemsInViewPort(scrollTop, clientHeight);
+      // Draws the hidden rows just above/below the viewport to prevent
+      // flashing in scroll.
+      var firstIndex = Math.max(0, itemsInViewPort.first - 1);
+      var lastIndex = Math.min(itemsInViewPort.last + 1, dataModel.length);
 
+      var beforeFillerHeight =
+          this.autoExpands ? 0 : this.getItemTop(firstIndex);
+      var afterFillerHeight =
+          this.autoExpands ? 0 : this.getAfterFillerHeight(lastIndex);
+
+      // Clear list and Adds elements on list.
       this.textContent = '';
 
-      this.beforeFiller_.style.height =
-          this.getHeightsForIndex_(firstIndex).top + 'px';
+      this.beforeFiller_.style.height = beforeFillerHeight + 'px';
       this.appendChild(this.beforeFiller_);
 
-      var sm = this.selectionModel;
-      var leadIndex = sm.leadIndex;
-
       this.addItems(firstIndex, lastIndex, cachedItems, newCachedItems);

[arv (Not doing code reviews), 2011/11/21 20:38:01]

Make sure there are no measuring done in here.

Basically, you need to ensure that you do not interleave measuring with mutating
the dom (appendChild, element.style.foo assignment etc). You should be able to
see this in the inspector.

[yoshiki, 2011/11/28 11:03:31]

I added the function ensureAllItemSizesInCache() and call it in advance before
redrawing. I checked that the dom is not mutated after that before addItems().

On 2011/11/21 20:38:01, arv wrote:

 
-      var afterFillerHeight = this.getAfterFillerHeight(lastIndex, itemHeight);
-      if (leadIndex >= lastIndex)
-        afterFillerHeight += this.leadItemHeight - itemHeight;
       this.afterFiller_.style.height = afterFillerHeight + 'px';
       this.appendChild(this.afterFiller_);
 
+      var sm = this.selectionModel;
+      var leadIndex = sm.leadIndex;
+
       // We don't set the lead or selected properties until after adding all
       // items, in case they force relayout in response to these events.
       var listItem = null;
-      if (newCachedItems[leadIndex])
+      if (leadIndex != -1 && newCachedItems[leadIndex])
         newCachedItems[leadIndex].lead = true;
       for (var y = firstIndex; y < lastIndex; y++) {
         if (sm.getIndexSelected(y))
@@ -927,6 +1066,7 @@ cr.define('cr.ui', function() {
       this.firstIndex_ = firstIndex;
       this.lastIndex_ = lastIndex;
 
+      this.remainSpace_ = itemsInViewPort.last > dataModel.length;
       this.cachedItems_ = newCachedItems;
 
       // Measure again in case the item height has changed due to a page zoom.
@@ -936,7 +1076,7 @@ cr.define('cr.ui', function() {
       // a reflow (which made the redraw speed 3 times slower on my system).
       // By using a timeout the measuring will happen later when there is no
       // need for a reflow.
-      if (listItem) {
+      if (listItem && this.fixedHeight_) {
         var list = this;
         window.setTimeout(function() {
           if (listItem.parentNode == list) {
@@ -958,7 +1098,8 @@ cr.define('cr.ui', function() {
      * @param {number} index The row index to redraw.
      */
     redrawItem: function(index) {
-      if (index >= this.firstIndex_ && index < this.lastIndex_) {
+      if (index >= this.firstIndex_ &&
+          (index < this.lastIndex_ || this.remainSpace_)) {
         delete this.cachedItems_[index];
         this.redraw();
       }