Don't add artifical 1 pixel width to empty tables

Source/core/layout/TableLayoutAlgorithmAuto.cpp

 /*
  * Copyright (C) 2002 Lars Knoll (knoll@kde.org)
  *           (C) 2002 Dirk Mueller (mueller@kde.org)
  * Copyright (C) 2003, 2006, 2008, 2010 Apple Inc. All rights reserved.
  *
  * This library is free software; you can redistribute it and/or
  * modify it under the terms of the GNU Library General Public
  * License as published by the Free Software Foundation; either
  * version 2 of the License.
  *
  * This library is distributed in the hope that it will be useful,
  * but WITHOUT ANY WARRANTY; without even the implied warranty of
  * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
  * Library General Public License for more details.
  *
  * You should have received a copy of the GNU Library General Public License
  * along with this library; see the file COPYING.LIB.  If not, write to
  * the Free Software Foundation, Inc., 51 Franklin Street, Fifth Floor,
  * Boston, MA 02110-1301, USA.
  */
 
 #include "config.h"
 #include "core/layout/TableLayoutAlgorithmAuto.h"
 
 #include "core/layout/LayoutTable.h"
 #include "core/layout/LayoutTableCell.h"
 #include "core/layout/LayoutTableCol.h"
 #include "core/layout/LayoutTableSection.h"
+#include "core/layout/LayoutText.h"
 #include "core/layout/TextAutosizer.h"
 
 namespace blink {
 
 TableLayoutAlgorithmAuto::TableLayoutAlgorithmAuto(LayoutTable* table)
     : TableLayoutAlgorithm(table)
     , m_hasPercent(false)
     , m_effectiveLogicalWidthDirty(true)
 {
 }
 
 TableLayoutAlgorithmAuto::~TableLayoutAlgorithmAuto()
 {
 }
 
+static bool isEmptyCell(LayoutObject* object)

[mstensho (USE GERRIT), 2015/05/18 08:45:38]

I don't get this.

<p>Should be 50% lime and 50% cyan.</p>
<table cellspacing="0" cellpadding="0" style="width:100%; height:50px;">
    <tr>
        <td style="background:lime;"></td>
        <td style="background:cyan;"><div></div></td>
    </tr>
</table>

With your changes, the two columns end up in different "spread categories", so
that all extra space is given to the second one that "has content". No other
browser does this. I think the "has content" distinction should go away
completely.

[rhogan, 2015/05/19 19:45:42]

The second one should definitely be treated as empty. In fact, I realized while
working through your review that we should be allocating initial width to
columns that have only empty cells. Doing this allows us to match FF on
empty-cells.html and is just saner in general. I think the reason it wasn't done
before is that columns with empty cells were sometimes shorthand for columns
that had no cells of their own and just fleshed out a colspan so it wasn't safe,
without recognizing that's what they were, to go ahead
and allocate width.

+{
+    for (LayoutObject* curr = object; curr; curr = curr->nextSibling()) {
+        if (curr->isText() && toLayoutText(curr)->isAllCollapsibleWhitespace())
+            continue;
+        if (curr->slowFirstChild() && isEmptyCell(curr->slowFirstChild()))
+            continue;
+        return false;
+    }
+    return true;
+}
+
 void TableLayoutAlgorithmAuto::recalcColumn(unsigned effCol)
 {
     Layout& columnLayout = m_layoutStruct[effCol];
 
     LayoutTableCell* fixedContributor = 0;
     LayoutTableCell* maxContributor = 0;
 
     for (LayoutObject* child = m_table->children()->firstChild(); child; child = child->nextSibling()) {
         if (child->isLayoutTableCol()) {
             // LayoutTableCols don't have the concept of preferred logical width, but we need to clear their dirty bits
             // so that if we call setPreferredWidthsDirty(true) on a col or one of its descendants, we'll mark it's
             // ancestors as dirty.
             toLayoutTableCol(child)->clearPreferredLogicalWidthsDirtyBits();
         } else if (child->isTableSection()) {
             LayoutTableSection* section = toLayoutTableSection(child);
             unsigned numRows = section->numRows();
             for (unsigned i = 0; i < numRows; i++) {
                 LayoutTableSection::CellStruct current = section->cellAt(i, effCol);
                 LayoutTableCell* cell = current.primaryCell();
 
                 if (current.inColSpan || !cell)
                     continue;
 
-                bool cellHasContent = cell->children()->firstChild() || cell->style()->hasBorder() || cell->style()->hasPadding() || cell->style()->hasBackground();
+                bool cellHasContent = cell->style()->hasBorder() || cell->style()->hasPadding() || !isEmptyCell(cell->firstChild());

[mstensho (USE GERRIT), 2015/05/18 08:45:38]

This certainly looks like a step in the right direction, but, as I wrote above,
I think the "has content" thing should go away (maybe except for using it to set
columnLayout.emptyCellsOnly; not sure how that one works). Why even bother to
check for border of padding. If such things are present, min logical width
should already be larger than 0, so there should be no need to force it to 1.

[rhogan, 2015/05/19 19:45:42]

Good point - and in your example above (which I've added as a test) both should
be treated as empty cells. So I've adapted the check to look for minimum
preferred width and failing that, a div element with non-empty children.

                 if (cellHasContent)
                     columnLayout.emptyCellsOnly = false;
 
                 // A cell originates in this column. Ensure we have
                 // a min/max width of at least 1px for this column now.
                 columnLayout.minLogicalWidth = std::max<int>(columnLayout.minLogicalWidth, cellHasContent ? 1 : 0);
-                columnLayout.maxLogicalWidth = std::max<int>(columnLayout.maxLogicalWidth, 1);
 
                 if (cell->colSpan() == 1) {
                     columnLayout.minLogicalWidth = std::max<int>(cell->minPreferredLogicalWidth(), columnLayout.minLogicalWidth);
                     if (cell->maxPreferredLogicalWidth() > columnLayout.maxLogicalWidth) {
                         columnLayout.maxLogicalWidth = cell->maxPreferredLogicalWidth();
                         maxContributor = cell;
                     }
 
                     // All browsers implement a size limit on the cell's max width.
                     // Our limit is based on KHTML's representation that used 16 bits widths.
@@ skipping 281 matching lines @@
                 // can't satify this condition, treat as variable
                 cellLogicalWidth = Length();
             } else {
                 maxLogicalWidth = std::max(maxLogicalWidth, static_cast<int>(std::max(spanMaxLogicalWidth, cellMaxLogicalWidth) * 100  / cellLogicalWidth.percent()));
 
                 // all non percent columns in the span get percent values to sum up correctly.
                 float percentMissing = cellLogicalWidth.percent() - totalPercent;
                 int totalWidth = 0;
                 for (unsigned pos = effCol; pos < lastCol; ++pos) {
                     if (!m_layoutStruct[pos].effectiveLogicalWidth.hasPercent())
-                        totalWidth += m_layoutStruct[pos].effectiveMaxLogicalWidth;
+                        totalWidth += m_layoutStruct[pos].clampedEffectiveMaxLogicalWidth();
                 }
 
                 for (unsigned pos = effCol; pos < lastCol && totalWidth > 0; ++pos) {
                     if (!m_layoutStruct[pos].effectiveLogicalWidth.hasPercent()) {
                         float percent = percentMissing * static_cast<float>(m_layoutStruct[pos].effectiveMaxLogicalWidth) / totalWidth;
-                        totalWidth -= m_layoutStruct[pos].effectiveMaxLogicalWidth;
+                        totalWidth -= m_layoutStruct[pos].clampedEffectiveMaxLogicalWidth();
                         percentMissing -= percent;
                         if (percent > 0)
                             m_layoutStruct[pos].effectiveLogicalWidth.setValue(Percent, percent);
                         else
                             m_layoutStruct[pos].effectiveLogicalWidth = Length();
                     }
                 }
             }
         }
 
@@ skipping 138 matching lines @@
         m_layoutStruct[i].computedLogicalWidth = cellLogicalWidth;
         available -= cellLogicalWidth;
         Length& logicalWidth = m_layoutStruct[i].effectiveLogicalWidth;
         switch (logicalWidth.type()) {
         case Percent:
             havePercent = true;
             totalPercent += logicalWidth.percent();
             break;
         case Fixed:
             numFixed++;
-            totalFixed += m_layoutStruct[i].effectiveMaxLogicalWidth;
+            totalFixed += m_layoutStruct[i].clampedEffectiveMaxLogicalWidth();
             // fall through
             break;
         case Auto:
             if (m_layoutStruct[i].emptyCellsOnly) {
                 numAutoEmptyCellsOnly++;
             } else {
                 numAuto++;
-                totalAuto += m_layoutStruct[i].effectiveMaxLogicalWidth;
+                totalAuto += m_layoutStruct[i].clampedEffectiveMaxLogicalWidth();
                 allocAuto += cellLogicalWidth;
             }
             break;
         default:
             break;
         }
     }
 
     // allocate width to percent cols
     if (available > 0 && havePercent) {
@@ skipping 46 matching lines @@
 
     if (available > 0 && m_hasPercent && totalPercent < 100)
         distributeWidthToColumns<float, Percent, AllCells, ExtraWidth, StartToEnd>(available, totalPercent);
 
     if (available > 0 && nEffCols > numAutoEmptyCellsOnly) {
         unsigned total = nEffCols - numAutoEmptyCellsOnly;
         // Starting from the last cell is for compatibility with FF/IE - it isn't specified anywhere.
         distributeWidthToColumns<unsigned, Auto, NonEmptyCells, LeftoverWidth, EndToStart>(available, total);
     }
 
+    // spread over the empty cells
+    if (available > 0) {

[mstensho (USE GERRIT), 2015/05/20 09:34:22]

Why not remove this code block entirely? You now spread over empty cells before
distributing excess space, so I don't see how we would ever get here.

+        unsigned total = numAutoEmptyCellsOnly;
+        distributeWidthToColumns<unsigned, Auto, EmptyCells, LeftoverWidth, EndToStart>(available, total);

[mstensho (USE GERRIT), 2015/05/18 08:45:39]

Is it possible to over-allocate here? If so, don't you need to add a
corresponding shrink step?

If it's not possible to over-allocate, you should ASSERT on that after having
distributed.

[rhogan, 2015/05/19 19:45:42]

No, the shrink step is keyed to Length type only - it's agnostic to empty cells.

+    }
+
     // If we have overallocated, reduce every cell according to the difference between desired width and minwidth
     // this seems to produce to the pixel exact results with IE. Wonder is some of this also holds for width distributing.
     // This is basically the reverse of how we grew the cells.
     if (available < 0)
         shrinkColumnWidth(Auto, available);
     if (available < 0)
         shrinkColumnWidth(Fixed, available);
     if (available < 0)
         shrinkColumnWidth(Percent, available);
 
     int pos = 0;
     for (size_t i = 0; i < nEffCols; ++i) {
         m_table->setColumnPosition(i, pos);
         pos += m_layoutStruct[i].computedLogicalWidth + m_table->hBorderSpacing();
     }
     m_table->setColumnPosition(m_table->columnPositions().size() - 1, pos);
 }
 
 template<typename Total, LengthType lengthType, CellsToProcess cellsToProcess, DistributionMode distributionMode, DistributionDirection distributionDirection>
 void TableLayoutAlgorithmAuto::distributeWidthToColumns(int& available, Total total)
 {
     // TODO(alancutter): Make this work correctly for calc lengths.
     int nEffCols = static_cast<int>(m_table->numEffCols());
     bool startToEnd = distributionDirection == StartToEnd;
     for (int i = startToEnd ? 0 : nEffCols - 1; startToEnd ? i < nEffCols : i > -1; startToEnd ? ++i : --i) {
         const Length& logicalWidth = m_layoutStruct[i].effectiveLogicalWidth;
         if (cellsToProcess == NonEmptyCells && logicalWidth.isAuto() && m_layoutStruct[i].emptyCellsOnly)
             continue;
+        if (cellsToProcess == EmptyCells && logicalWidth.isAuto() && !m_layoutStruct[i].emptyCellsOnly)
+            continue;
         if (distributionMode != LeftoverWidth && logicalWidth.type() != lengthType)
             continue;
 
         float factor = 1;
         if (distributionMode != LeftoverWidth) {
             if (lengthType == Percent)
                 factor = logicalWidth.percent();
             else if (lengthType == Auto || lengthType == Fixed)
-                factor = m_layoutStruct[i].effectiveMaxLogicalWidth;
+                factor = m_layoutStruct[i].clampedEffectiveMaxLogicalWidth();
         }
 
         int newWidth = available * factor / total;
         int cellLogicalWidth = (distributionMode == InitialWidth) ? max<int>(m_layoutStruct[i].computedLogicalWidth, newWidth) : newWidth;
         available -= cellLogicalWidth;
         total -= factor;
         m_layoutStruct[i].computedLogicalWidth = (distributionMode == InitialWidth) ? cellLogicalWidth : m_layoutStruct[i].computedLogicalWidth + cellLogicalWidth;
 
         // If we have run out of width to allocate we're done.
         // Also, any overallocation will be unwound later so no point in building up cells to unwind, just bail now.
@@ skipping 21 matching lines @@
             int reduce = available * minMaxDiff / logicalWidthBeyondMin;
             m_layoutStruct[i].computedLogicalWidth += reduce;
             available -= reduce;
             logicalWidthBeyondMin -= minMaxDiff;
             if (available >= 0)
                 break;
         }
     }
 }
 }