Spanning logical height is not added properly in all spanning rows.

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 357 matching lines @@
 
     extraRowSpanningHeight -= accumulatedPositionIncrease;
 }
 
 // Distribute rowSpan cell height in rows those comes in rowSpan cell based on the ratio of row's height if
 // 1. RowSpan cell height is greater then the total height of rows in rowSpan cell
 void RenderTableSection::distributeRowSpanHeightToRows(SpanningRenderTableCells& rowSpanCells)
 {
     ASSERT(rowSpanCells.size());
 
-    // FIXME: For now, we handle the first rowspan cell in the table but this is wrong.
-    RenderTableCell* cell = rowSpanCells[0];
+    unsigned changedHeight = 0;

[Julien - ping for review, 2013/07/23 00:13:43]

Trying to find better naming for this one:
- extraHeightToPropagate
- heightToPropagate
- heightDifference
- heightDelta

(changedHeight sounds like the new height where it's actually a difference)

Slight preference for the first name but feel free to find a better one.

[a.suchit, 2013/07/23 12:19:56]

Done.

+    unsigned lastRowIndex = 0;
+    unsigned lastRowSpan = 0;
 
-    unsigned rowSpan = cell->rowSpan();
+    for (unsigned i = 0; i < rowSpanCells.size(); i++) {
+        RenderTableCell* cell = rowSpanCells[i];
 
-    struct SpanningRowsHeight spanningRowsHeight;
+        unsigned rowIndex = cell->rowIndex();
 
-    populateSpanningRowsHeightFromCell(cell, spanningRowsHeight);
+        // FIXME: For now, we are handling only rowspan cells those are not overlapping with other
+        // rowspan cells but this is wrong.
+        if (rowIndex < lastRowIndex + lastRowSpan)
+            continue;
 
-    if (!spanningRowsHeight.totalRowsHeight || spanningRowsHeight.spanningCellHeightIgnoringBorderSpacing <= spanningRowsHeight.totalRowsHeight)
-        return;
+        unsigned rowSpan = cell->rowSpan();
+        int initialPos = m_rowPos[rowIndex + rowSpan];

[Julien - ping for review, 2013/07/23 00:13:43]

Let's not abbreviate variables: |initialPosition|

Maybe better: originalBeforePosition

[a.suchit, 2013/07/23 12:19:56]

Done.

 
-    unsigned rowIndex = cell->rowIndex();
-    int totalPercent = 0;
-    int totalAutoRowsHeight = 0;
-    int totalRemainingRowsHeight = spanningRowsHeight.totalRowsHeight;
+        if (changedHeight) {
+            for (unsigned row = lastRowIndex + lastRowSpan + 1; row <= rowIndex + rowSpan; row++)
+                m_rowPos[row] += changedHeight;
+        }
 
-    // Calculate total percentage, total auto rows height and total rows height except percent rows.
-    for (unsigned row = rowIndex; row < (rowIndex + rowSpan); row++) {
-        if (m_grid[row].logicalHeight.isPercent()) {
-            totalPercent += m_grid[row].logicalHeight.percent();
-            totalRemainingRowsHeight -= spanningRowsHeight.rowHeight[row - rowIndex];
-        } else if (m_grid[row].logicalHeight.isAuto()) {
-            totalAutoRowsHeight += spanningRowsHeight.rowHeight[row - rowIndex];
+        lastRowIndex = rowIndex;
+        lastRowSpan = rowSpan;
+
+        struct SpanningRowsHeight spanningRowsHeight;
+
+        populateSpanningRowsHeightFromCell(cell, spanningRowsHeight);
+
+        if (spanningRowsHeight.totalRowsHeight && spanningRowsHeight.spanningCellHeightIgnoringBorderSpacing > spanningRowsHeight.totalRowsHeight) {
+
+            int totalPercent = 0;
+            int totalAutoRowsHeight = 0;
+            int totalRemainingRowsHeight = spanningRowsHeight.totalRowsHeight;
+
+            // Calculate total percentage, total auto rows height and total rows height except percent rows.
+            for (unsigned row = rowIndex; row < (rowIndex + rowSpan); row++) {
+                if (m_grid[row].logicalHeight.isPercent()) {
+                    totalPercent += m_grid[row].logicalHeight.percent();
+                    totalRemainingRowsHeight -= spanningRowsHeight.rowHeight[row - rowIndex];
+                } else if (m_grid[row].logicalHeight.isAuto()) {
+                    totalAutoRowsHeight += spanningRowsHeight.rowHeight[row - rowIndex];
+                }
+            }
+
+            int extraRowSpanningHeight = spanningRowsHeight.spanningCellHeightIgnoringBorderSpacing - spanningRowsHeight.totalRowsHeight;
+
+            distributeExtraRowSpanHeightToPercentRows(cell, totalPercent, extraRowSpanningHeight, spanningRowsHeight.rowHeight);
+            distributeExtraRowSpanHeightToAutoRows(cell, totalAutoRowsHeight, extraRowSpanningHeight, spanningRowsHeight.rowHeight);
+            distributeExtraRowSpanHeightToRemainingRows(cell, totalRemainingRowsHeight, extraRowSpanningHeight, spanningRowsHeight.rowHeight);
+
+            ASSERT(!extraRowSpanningHeight);
+
+            // Getting total changed height in the table
+            changedHeight = m_rowPos[rowIndex + rowSpan] - initialPos;
         }
     }
 
-    int initialPos = m_rowPos[rowIndex + rowSpan];
-    int extraRowSpanningHeight = spanningRowsHeight.spanningCellHeightIgnoringBorderSpacing - spanningRowsHeight.totalRowsHeight;
-
-    distributeExtraRowSpanHeightToPercentRows(cell, totalPercent, extraRowSpanningHeight, spanningRowsHeight.rowHeight);
-    distributeExtraRowSpanHeightToAutoRows(cell, totalAutoRowsHeight, extraRowSpanningHeight, spanningRowsHeight.rowHeight);
-    distributeExtraRowSpanHeightToRemainingRows(cell, totalRemainingRowsHeight, extraRowSpanningHeight, spanningRowsHeight.rowHeight);
-
-    ASSERT(!extraRowSpanningHeight);
-
-    // Getting total changed height in the table
-    unsigned changedHeight = m_rowPos[rowIndex + rowSpan] - initialPos;
-
     if (changedHeight) {
-        unsigned totalRows = m_grid.size();
-
         // Apply changed height by rowSpan cells to rows present at the end of the table
-        for (unsigned row = rowIndex + rowSpan + 1; row <= totalRows; row++)
+        for (unsigned row = lastRowIndex + lastRowSpan + 1; row <= m_grid.size(); row++)
             m_rowPos[row] += changedHeight;
     }
 }
 
 // Find out the baseline of the cell
 // If the cell's baseline is more then the row's baseline then the cell's baseline become the row's baseline
 // and if the row's baseline goes out of the row's boundries then adjust row height accordingly.
 void RenderTableSection::updateBaselineForCell(RenderTableCell* cell, unsigned row, LayoutUnit& baselineDescent)
 {
     if (!cell->isBaselineAligned())
@@ skipping 1203 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