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

Source/core/rendering/RenderTableSection.cpp

Index: Source/core/rendering/RenderTableSection.cpp
diff --git a/Source/core/rendering/RenderTableSection.cpp b/Source/core/rendering/RenderTableSection.cpp
index 252991e60cf6acc34738d8b7d07fb9f1342bc761..81f3c8f6261359f9f6824ce8975df781c317677d 100644
--- a/Source/core/rendering/RenderTableSection.cpp
+++ b/Source/core/rendering/RenderTableSection.cpp
@@ -288,8 +288,8 @@ void RenderTableSection::populateSpanningRowsHeightFromCell(RenderTableCell* cel
         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])
+            spanningRowsHeight.rowWithOnlySpanningCells = rowHasOnlySpanningCells(actualRow);

[Julien - ping for review, 2014/02/26 22:29:31]

As mentioned previously, this change should be removed as it doesn't add
anything.

The fact that you *had* to explain what it is supposed to mean (and what you
said just mirrored the code) underlines that this variable is badly named.

Note that booleans should have a verb, which is not the case anyway so we could
kill 2 birds with one stone.

[a.suchit, 2014/03/03 12:28:31]

change variable name |rowWithOnlySpanningCells| to
|isAnyRowWithOnlySpanningCells| which explain every thing here.

 
         spanningRowsHeight.totalRowsHeight += spanningRowsHeight.rowHeight[row];
         spanningRowsHeight.spanningCellHeightIgnoringBorderSpacing -= borderSpacingForRow(actualRow);
@@ -542,14 +542,33 @@ void RenderTableSection::distributeRowSpanHeightToRows(SpanningRenderTableCells&
 
         populateSpanningRowsHeightFromCell(cell, spanningRowsHeight);
 
+        // 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) {
+        // Here we are handling only row(s) who have rowspanning cell(s) and at least one empty cell.
+        // We are not considered the row as rowWithOnlySpanningCells when row contains spanningcells and
+        // at least one empty cell. So These type of rows does not get any height. And when any spanning cell
+        // present with only such type of rows than spanning cell height becomes 0. In this senario, text
+        // present in spanning cells will overlap with other cells because it don't have any height.
+        // Spanning cell should have at least the height of the text present in spanning cell. So in place of
+        // distributing this height in all the spanning rows, we can give this height to one of the row of
+        // spanning cell because finally it have to look as one cell.
+        // So here, we are giving text'height of spanning cell to the last row of spanning cell.

[Julien - ping for review, 2014/02/26 22:29:31]

First and this is a recurring issue, I have a hard time understanding what you
mean and that's only because I can't parse your English. My advice would be to
stick to very simple sentences, splitting complex ones into more simple. Also be
mindful of the conjugation and word used.

// This code handle row(s) that have rowspanning cell(s) and at least one empty
cell.
// Such rows are not handled below and end up having a height of 0. That would
mean
// content overlapping if one of their cells has any content. To avoid the
problem, we
// add all the remaining spanning cells' height to the last spanned row.
// This means that we could grow a row past its 'height' or break percentage
spreading
// however this is better than overlapping content. FIXME: Is there a better
algorithm?

[a.suchit, 2014/03/03 12:28:31]

Done.

+        if (!spanningRowsHeight.totalRowsHeight) {
+            if (spanningRowsHeight.spanningCellHeightIgnoringBorderSpacing)
+                m_rowPos[spanningCellEndIndex] += spanningRowsHeight.spanningCellHeightIgnoringBorderSpacing + borderSpacingForRow(spanningCellEndIndex - 1);
+
+            extraHeightToPropagate = m_rowPos[spanningCellEndIndex] - originalBeforePosition;
+            continue;
+        }
+
+        if (spanningRowsHeight.spanningCellHeightIgnoringBorderSpacing <= spanningRowsHeight.totalRowsHeight) {
             extraHeightToPropagate = m_rowPos[rowIndex + rowSpan] - originalBeforePosition;
             continue;
         }
 
+        // 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;