Table rows are incorrectly collapsed in case of hidden cells and rowspans.

Source/core/rendering/RenderTableSection.cpp

 /*
  * Copyright (C) 1997 Martin Jones (mjones@kde.org)
  *           (C) 1997 Torben Weis (weis@kde.org)
  *           (C) 1998 Waldo Bastian (bastian@kde.org)
  *           (C) 1999 Lars Knoll (knoll@kde.org)
  *           (C) 1999 Antti Koivisto (koivisto@kde.org)
  * Copyright (C) 2003, 2004, 2005, 2006, 2008, 2009, 2010 Apple Inc. All rights reserved.
  * Copyright (C) 2006 Alexey Proskuryakov (ap@nypop.com)
  *
  * This library is free software; you can redistribute it and/or
@@ skipping 270 matching lines @@
     const unsigned rowIndex = cell->rowIndex();
 
     spanningRowsHeight.spanningCellHeightIgnoringBorderSpacing = cell->logicalHeightForRowSizing();
 
     spanningRowsHeight.rowHeight.resize(rowSpan);
     spanningRowsHeight.totalRowsHeight = 0;
     for (unsigned row = 0; row < rowSpan; row++) {
         unsigned actualRow = row + rowIndex;
 
         spanningRowsHeight.rowHeight[row] = m_rowPos[actualRow + 1] - m_rowPos[actualRow] - borderSpacingForRow(actualRow);
-        if (!spanningRowsHeight.rowHeight[row])
-            spanningRowsHeight.rowWithOnlySpanningCells |= rowHasOnlySpanningCells(actualRow);
+        if (!spanningRowsHeight.rowWithOnlySpanningCells && !spanningRowsHeight.rowHeight[row])

[Julien - ping for review, 2014/02/06 20:30:45]

You misunderstood what I said: this code is equivalent to the previous one and I
was fine with using a logical OR. I don't see the value in this part of the
change.

If you name this variable rowWithOnlySpanningCells, it means that any row that
*doesn't* have spanning cells should turn it to false but here it's the
opposite. That makes wonder if it's not wrongly named.

[a.suchit, 2014/02/18 13:07:10]

We are checking all the rows of rowspanning cell. If any row is having only
spanning cells and empty cells are considered as non rowspanning cell then
|rowWithOnlySpanningCells| becomes true.

At least one spanning row is having only spanning cells than
|rowWithOnlySpanningCells| becomes true

+            spanningRowsHeight.rowWithOnlySpanningCells = rowHasOnlySpanningCells(actualRow);
 
         spanningRowsHeight.totalRowsHeight += spanningRowsHeight.rowHeight[row];
         spanningRowsHeight.spanningCellHeightIgnoringBorderSpacing -= borderSpacingForRow(actualRow);
     }
     // We don't span the following row so its border-spacing (if any) should be included.
     spanningRowsHeight.spanningCellHeightIgnoringBorderSpacing += borderSpacingForRow(rowIndex + rowSpan - 1);
 }
 
 void RenderTableSection::distributeExtraRowSpanHeightToPercentRows(RenderTableCell* cell, int totalPercent, int& extraRowSpanningHeight, Vector<int>& rowsHeight)
 {
@@ skipping 232 matching lines @@
                 m_rowPos[row] += extraHeightToPropagate;
         }
 
         lastRowIndex = rowIndex;
         lastRowSpan = rowSpan;
 
         struct SpanningRowsHeight spanningRowsHeight;
 
         populateSpanningRowsHeightFromCell(cell, spanningRowsHeight);
 
+        // if only spanning cells are present in spanning rows then apply height based on spanning cells.
+        // Here we are handling only row(s) who have only rowspanning cells and do not have any empty cell.
         if (spanningRowsHeight.rowWithOnlySpanningCells)
             updateRowsHeightHavingOnlySpanningCells(cell, spanningRowsHeight);
 
-        if (!spanningRowsHeight.totalRowsHeight || spanningRowsHeight.spanningCellHeightIgnoringBorderSpacing <= spanningRowsHeight.totalRowsHeight) {
+        // else if total height of spanning rows are 0 then apply rowspanning cells height here.
+        // Here we are handling only row(s) who have rowspanning cell(s) and at least one empty cell.

[Julien - ping for review, 2014/02/06 20:30:45]

I tried reading those comments several times but as I keep pointing out, they
are just mirroring the code and thus don't add value. What we need is 'why'
comments (why is it correct to do that?) or comments adding values to our code
('what' comments that go beyond just the code by giving easy to understand
examples or helping the reader follow the high level algorithm).

[a.suchit, 2014/02/18 13:07:10]

Done.

+        if (!spanningRowsHeight.totalRowsHeight) {
+            if (spanningRowsHeight.spanningCellHeightIgnoringBorderSpacing)
+                m_rowPos[rowIndex + rowSpan] += spanningRowsHeight.spanningCellHeightIgnoringBorderSpacing + borderSpacingForRow(rowIndex + rowSpan - 1);

[Julien - ping for review, 2014/02/06 20:30:45]

This should be spanningCellEndIndex for consistency.

[a.suchit, 2014/02/18 13:07:10]

Done.

+
             extraHeightToPropagate = m_rowPos[rowIndex + rowSpan] - originalBeforePosition;
             continue;

[Julien - ping for review, 2014/02/06 20:30:45]

My biggest issue with this code is that it will wrongly handle the case where
you only have spanning cells maybe with empty cells and some set row's height.

Also it is allocating the extra space to the last row in a rowspan
(m_rowPos[rowIndex + rowSpan]).

         }
 
+        if (spanningRowsHeight.spanningCellHeightIgnoringBorderSpacing <= spanningRowsHeight.totalRowsHeight) {
+            extraHeightToPropagate = m_rowPos[rowIndex + rowSpan] - originalBeforePosition;

[Julien - ping for review, 2014/02/06 20:30:45]

How does this ensures that we allocate all of
spanningCellHeightIgnoringBorderSpacing?

[a.suchit, 2014/02/18 13:07:10]

Spanning rows total height is already more then the spanning cell height with
ignoring border space.
So If spanning cell already have enough height which is equal or more than
specified by user for this spanning cell then we are leaving spanning cell with
same height.

+            continue;
+        }
+
+        // else distribute extra height in percent rows, auto rows and fixed rows respectively.
+        // Below we are handling only row(s) who have at least one visible cell without rowspan value.
         int totalPercent = 0;
         int totalAutoRowsHeight = 0;
         int totalRemainingRowsHeight = spanningRowsHeight.totalRowsHeight;
 
         // FIXME: Inner spanning cell height should not change if it have fixed height when it's parent spanning cell
         // is distributing it's extra height in rows.
 
         // Calculate total percentage, total auto rows height and total rows height except percent rows.
         for (unsigned row = rowIndex; row < spanningCellEndIndex; row++) {
             if (m_grid[row].logicalHeight.isPercent()) {
@@ skipping 1167 matching lines @@
     else
         cellLocation.setX(table()->columnPositions()[effectiveColumn] + horizontalBorderSpacing);
 
     cell->setLogicalLocation(cellLocation);
 
     if (!RuntimeEnabledFeatures::repaintAfterLayoutEnabled())
         view()->addLayoutDelta(oldCellLocation - cell->location());
 }
 
 } // namespace WebCore