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 528 matching lines @@
         lastRowIndex = rowIndex;
         lastRowSpan = rowSpan;
 
         struct SpanningRowsHeight spanningRowsHeight;
 
         populateSpanningRowsHeightFromCell(cell, spanningRowsHeight);
 
         if (spanningRowsHeight.rowWithOnlySpanningCells)
             updateRowsHeightHavingOnlySpanningCells(cell, spanningRowsHeight);
 
-        if (!spanningRowsHeight.totalRowsHeight || spanningRowsHeight.spanningCellHeightIgnoringBorderSpacing <= spanningRowsHeight.totalRowsHeight) {
+        // Row would be valid or we can say, row would be visible only when at least one cell should start from that row
+        // and cell have rowspan=1.

[Julien - ping for review, 2013/11/21 02:46:23]

I have a hard time putting this line in context.

Based on your following example, it seems that you should just say that "we only
take into account non-empty rows, which are rows with at least a visible
(display != none) cell.

[suchit.agrawal, 2013/11/22 11:49:41]

Thanks, I am tring to explain same thing here.

+        // But there is some exception like, cell with property display:none, rowspan=1 and this is only cell present
+        // in a row than that row will not visible.
+        // Just check an example here :
+        // +---+---+                                           +---+---+
+        // | A | B |    If D cell set as 'display:none' then   | A | B |   D row height does not matter here.
+        // +---+---+                                           +---+---+
+        // |   | D |                                           | C | E |
+        // + C +---+                                           +---+---+
+        // |   | E |
+        // +---+---+
+        // If D and E both cells set as 'display:none' then here is matter for D or/and E cells height becuase C cell should
+        // display.

[Julien - ping for review, 2013/11/21 02:46:23]

I don't think I understand how this relates to the algorithm. If a cell has
'display: none' then it doesn't have a renderer so there is no height to spread
anyway.

[suchit.agrawal, 2013/11/22 11:49:41]

Here you have explained my 1 above and 2 below lines in small sentence.
D and E are not have any height to spread in C. So C height remains 0.
So here we are applying C logical height in last row of rowspan cell. 

+        // Our Algo works only when, (1.)at least one non-rowspan cell presents with rowspan cell OR (2.)all cells in the row
+        // are rowspan cells.
+        // So in this case, D and E cells are invisible so D and E rows are 0 and D and E rows are not based on any of above
+        // 2 conditions. So C rowspan cell is getting 0 height.

[Julien - ping for review, 2013/11/21 02:46:23]

Could you explain what the rowspan cell is? Is this the empty row that is
spanned?

+        // So C cell height assigned to last row (E row).
+        // FIXME : If combination of rowspan cells are present here then in some scenarios outer cells may get more height than
+        // expected.

[Julien - ping for review, 2013/11/21 02:46:23]

FIXMEs are great but they are better when they are _actionable_. Here I have no
clue what a scenario where we would be wrong is.

[suchit.agrawal, 2013/11/22 11:49:41]

x---x---x---x---x
| A | B | C | D |  row1
x---x---x---x---x
|   |   | G |      row2
|   | F X---x
|   |   |   |      row3
| E x---x H x
|   |   |   |      row4
|   | I x---x
|   |   | J |      row5
x---x---x---x

I was thinking for like this scenarios if G and J are hidden (display:none).
Here, rowspan cell's logical height goes in the last row of rowspan cell.
Assume
F, H and I heights are 10 each.
H should be part of row3 and row4 but in our case it would be fully part of only
row4.
F should be part of row2 and row3 but in our case it would be part of only row3.
I should be part of row4 and row5 but in our case it would be part of only row4.
Then display will come like below :-
x---x---x---x---x
| A | B | C | D |  row1
x---x---x---x---x                  row2 height would be 0.
|   |   |
|   | F X          row3
|   |   |
| E x---x---x
|   |   |   |
|   | I x H x      row4
|   |   |   |
x---x---x---x                      row5 height would be 0.

But it should look like :-

x---x---x---x---x
| A | B | C | D |  row1
x---x---x---x---x                  row2 height would be 0.
|   |   |   |
|   | F |   |      row3
|   |   |   | 
| E x---x H x
|   |   |   |
|   | I |   |      row4
|   |   |   |
x---x---x---x                      row5 height would be 0.

+        if (!spanningRowsHeight.totalRowsHeight) {
+            if (spanningRowsHeight.spanningCellHeightIgnoringBorderSpacing)
+                m_rowPos[rowIndex + rowSpan] += spanningRowsHeight.spanningCellHeightIgnoringBorderSpacing + borderSpacingForRow(rowIndex + rowSpan - 1);
+
             extraHeightToPropagate = m_rowPos[rowIndex + rowSpan] - originalBeforePosition;
             continue;
         }
+
+        if (spanningRowsHeight.spanningCellHeightIgnoringBorderSpacing <= spanningRowsHeight.totalRowsHeight) {
+            extraHeightToPropagate = m_rowPos[rowIndex + rowSpan] - originalBeforePosition;
+            continue;
+        }
 
         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++) {
@@ skipping 1249 matching lines @@
     if (!style()->isLeftToRightDirection())
         cellLocation.setX(table()->columnPositions()[table()->numEffCols()] - table()->columnPositions()[table()->colToEffCol(cell->col() + cell->colSpan())] + horizontalBorderSpacing);
     else
         cellLocation.setX(table()->columnPositions()[effectiveColumn] + horizontalBorderSpacing);
 
     cell->setLogicalLocation(cellLocation);
     view()->addLayoutDelta(oldCellLocation - cell->location());
 }
 
 } // namespace WebCore