Fixing column widths in tables.

chrome/browser/resources/shared/js/cr/ui/table/table_list.js

Index: chrome/browser/resources/shared/js/cr/ui/table/table_list.js
diff --git a/chrome/browser/resources/shared/js/cr/ui/table/table_list.js b/chrome/browser/resources/shared/js/cr/ui/table/table_list.js
index e9865982e2900324ccf58fcd166df4029d75a34a..df47f50d60acd23fb1213f4e9c067830262085c2 100644
--- a/chrome/browser/resources/shared/js/cr/ui/table/table_list.js
+++ b/chrome/browser/resources/shared/js/cr/ui/table/table_list.js
@@ -33,32 +33,122 @@ cr.define('cr.ui.table', function() {
     },
 
     /**
-     * Resizes columns.
+     * Resizes columns. Called when column width changed.
      */
     resize: function() {
-      var cm = this.table_.columnModel;
-
-      var cells = this.querySelectorAll('.table-row-cell');
-      if (cells.length % cm.size != 0) {
-        this.redraw();
-        return;
+      if (this.updateScrollbars_()) {

[arv (Not doing code reviews), 2012/11/30 19:05:52]

No {} for single line ifs

[SeRya, 2012/12/01 20:14:37]

Done.

+        List.prototype.redraw.call(this);

[arv (Not doing code reviews), 2012/11/30 19:05:52]

Why not this.redraw()?

[SeRya, 2012/12/01 20:14:37]

this.redraw updated scrollbars and calls List.prototype.redraw. There is no need
to update scrollbars one again.

       }
-      var rowsCount = cells.length / cm.size;
+      this.resizeCells_();
+    },
+
+    /**
+     * Updates width of cells.
+     */
+    resizeCells_: function() {
+      var cm = this.table_.columnModel;
+      for (var row = this.firstElementChild; row;
+           row = row.nextElementSibling) {
+        if (row.tagName != 'LI')
+          continue;
 
-      for (var row = 0; row < rowsCount; row++) {
         for (var i = 0; i < cm.size; i++) {
-          cells[row * cm.size + i].style.width = cm.getWidth(i) + '%';
+          row.children[i].style.width = cm.getWidth(i) + 'px';
         }
+        row.style.width = cm.totalWidth + 'px';
       }
+      this.afterFiller_.style.width = cm.totalWidth + 'px';
     },
 
     /**
      * Redraws the viewport.
      */
     redraw: function() {
-      if (!this.table_.columnModel)
+      if (this.batchCount_ != 0)
         return;
+      this.updateScrollbars_();
+
       List.prototype.redraw.call(this);
+      this.resizeCells_();
+    },
+
+    /**
+     * Returns the height of after filler in the list.
+     * @param {number} lastIndex The index of item past the last in viewport.
+     * @return {number} The height of after filler.
+     * @override
+     */
+    getAfterFillerHeight: function(lastIndex) {
+      // If the list is empty set height to 1 to show horizontal
+      // scroll bar.
+      return lastIndex == 0 ? 1 :
+          cr.ui.List.prototype.getAfterFillerHeight.call(this, lastIndex);
+    },
+
+    /**
+     * Shows or hides vertical and horizontal scroll bars in the list.
+     * @return {boolean} True if horizontal scroll bar changed.
+     */
+    updateScrollbars_: function() {

[arv (Not doing code reviews), 2012/11/30 19:05:52]

How many reflows does this cause? It is really hard to follow. It seems like it
might do 4 reflows. Can we do this in a single reflow?

[SeRya, 2012/12/01 20:14:37]

In worse case it could be 4 reflows indeed:
1. if cm.totalWidth > offsetWidth && style.overflowX != 'scroll'
2. Item height is not fixed and heights are not cached (areAllItemsVisible_
cause reflow in ths case).
3. !this.areAllItemsVisible_(height) && style.overflowY == 'scroll'

We could cache (offsetWidth - clientWidth) assuming it doesn't change. The same
with height. So there is a room for some optimization. But actually I don't
think it worth additional complication?

+      var cm = this.table.columnModel;
+      var s = this.style;

[arv (Not doing code reviews), 2012/11/30 19:05:52]

s/s/style/

[arv (Not doing code reviews), 2012/11/30 19:05:52]

Maybe this should be the computed style?

[SeRya, 2012/12/01 20:14:37]

Done.

[SeRya, 2012/12/01 20:14:37]

this.style.overflow should always have value to reduce complexity. So it's
correct.

+      if (!cm || cm.size == 0) {
+        if (s.overflow != 'hidden') {
+          s.overflow = 'hidden';
+          return true;
+        } else {
+          return false;
+        }
+      }
+
+      var height = this.offsetHeight;
+      var changed = false;
+      if (cm.totalWidth > this.offsetWidth) {
+        if (s.overflowX != 'scroll') {
+          s.overflowX = 'scroll';

[arv (Not doing code reviews), 2012/11/30 19:05:52]

Can this code needs to be reordered to do all the  querying of the layout before
you start mutating the style?

[SeRya, 2012/12/01 20:14:37]

I've done it for offsetWidth. But it is important to query clientWidth/Height
after appropriate style set (in order to measure it with scrollbars). I don't
expect performance impart since additional reflow happen only style changed that
happen relatively rare.

+        }
+        // Once we sure there will be horizontal
+        // scrollbar calculate with this height.
+        height = this.clientHeight;
+      }
+      if (this.areAllItemsVisible_(height)) {
+        if (cm.totalWidth <= this.offsetWidth && s.overflowX != 'hidden') {
+          s.overflowX = 'hidden';
+        }
+        changed = this.showVerticalScrollBar_(false);
+      } else {
+        changed = this.showVerticalScrollBar_(true);
+        var x = cm.totalWidth <= this.clientWidth ? 'hidden' : 'scroll';
+        if (s.overflowX != x) {
+          s.overflowX = x;
+        }
+      }
+      return changed;
+    },
+
+    /**
+     * SHows or hides vertical scroll bar.

[arv (Not doing code reviews), 2012/11/30 19:05:52]

typo

[SeRya, 2012/12/01 20:14:37]

Done.

+     * @return {boolean} True if visibility changed.
+     */
+    showVerticalScrollBar_: function(on) {

[arv (Not doing code reviews), 2012/11/30 19:05:52]

rename on to something more descriptive

[SeRya, 2012/12/01 20:14:37]

It looks descriptive for me. If you see something more descriptive please write
it here.

+      var s = this.style;

[arv (Not doing code reviews), 2012/11/30 19:05:52]

computed style?

[SeRya, 2012/12/01 20:14:37]

this.style.overflow expected to have own value. So this.style is correct.

+      if (on && s.overflowY == 'scroll')
+        return false;
+      if (!on && s.overflowY == 'hidden')
+        return false;
+      s.overflowY = on ? 'scroll' : 'hidden';
+      return true;
+    },
+
+    /**
+     * @param {number} visibleHeight Height in pixels.
+     * @return {boolean} True if all rows could be accomodiated in
+     *                   visibleHeight pixels.
+     */
+    areAllItemsVisible_: function(visibleHeight) {
+      if (!this.dataModel || this.dataModel.length == 0)
+        return true;
+      return this.getItemTop(this.dataModel.length) <= visibleHeight;
     },
 
     /**
@@ -80,7 +170,7 @@ cr.define('cr.ui.table', function() {
 
       for (var i = 0; i < cm.size; i++) {
         var cell = table.ownerDocument.createElement('div');
-        cell.style.width = cm.getWidth(i) + '%';
+        cell.style.width = cm.getWidth(i) + 'px';
         cell.className = 'table-row-cell';
         if (cm.isEndAlign(i))
           cell.style.textAlign = 'end';
@@ -89,6 +179,7 @@ cr.define('cr.ui.table', function() {
 
         listItem.appendChild(cell);
       }
+      listItem.style.width = cm.totalWidth + 'px';
 
       return listItem;
     },