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])
+            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);
     }
     // 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);
 
+        // 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;
 
         // 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 1147 matching lines @@
     else
         cellLocation.setX(table()->columnPositions()[effectiveColumn] + horizontalBorderSpacing);
 
     cell->setLogicalLocation(cellLocation);
 
     if (!RuntimeEnabledFeatures::repaintAfterLayoutEnabled())
         view()->addLayoutDelta(oldCellLocation - cell->location());
 }
 
 } // namespace WebCore