Height of fixed height cell is not proper when cell's row is under row spanning cell.

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 236 matching lines @@
             if (inColSpan)
                 c.inColSpan = true;
         }
         m_cCol++;
         cSpan -= currentSpan;
         inColSpan = true;
     }
     cell->setCol(table()->effColToCol(col));
 }
 
-// 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)
+// Getting height of rows in current rowSpan cell, getting total height of rows and adjusting rowSpan cell height with border spacing.
+void RenderTableSection::getRowsHeightInRowSpan(RenderTableCell* cell, int& expectedTotalRowsHeight, int& totalRowsHeight, Vector<int>& rowsHeight)

[Julien - ping for review, 2013/07/03 21:08:22]

Really get a structure for the parameters you always pass around, that would
make the code a lot more readable.

That would also show you that naming this a getter is really a misnomer, it's
populating your variable.

[a.suchit, 2013/07/13 00:49:56]

Done.

 {
-    ASSERT(rowSpanCells.size());
-
-    // FIXME: For now, we handle the first rowspan cell in the table but this is wrong.
-    RenderTableCell* cell = rowSpanCells[0];
-
     unsigned rowSpan = cell->rowSpan();
     unsigned rowIndex = cell->rowIndex();
-    int initialPos = m_rowPos[rowIndex + rowSpan];
+    int rowSpanCellHeight = cell->logicalHeightForRowSizing();
 
-    int totalRowsHeight = 0;
-    int rowSpanCellHeight = cell->logicalHeightForRowSizing();
-    Vector<int> rowsHeight(rowSpan);
-
-    // Getting height of rows in current rowSpan cell, getting total height of rows and adjusting rowSpan cell height with border spacing.
+    totalRowsHeight = 0;
     for (unsigned row = 0; row < rowSpan; row++) {
         unsigned actualRow = row + rowIndex;
         rowsHeight[row] = m_rowPos[actualRow + 1] - m_rowPos[actualRow] - borderSpacingForRow(actualRow);
         totalRowsHeight += rowsHeight[row];
         rowSpanCellHeight -= borderSpacingForRow(actualRow);
     }
     rowSpanCellHeight += borderSpacingForRow(rowIndex + rowSpan - 1);
 
-    if (!totalRowsHeight || rowSpanCellHeight <= totalRowsHeight)
+    expectedTotalRowsHeight = rowSpanCellHeight;

[Julien - ping for review, 2013/07/03 21:08:22]

Why do we need |rowSpanCellHeight| then?

[a.suchit, 2013/07/13 00:49:56]

Changed

+}
+
+// If percent is more than 100 then whole extra height would be distribute in percent rows based on their percent ratios so that
+// all percent rows would get additional height from extra rowspanning height.
+// Else specified percent (totalPercent) of extra height would be distributed in percent rows and
+// remaining would be distributed in other rows.
+void RenderTableSection::distributeExtraRowSpanHeightToPrecentRows(RenderTableCell* cell, int totalPercent, int& extraRowSpanningHeight, Vector<int>& rowsHeight)

[Julien - ping for review, 2013/07/03 21:08:22]

|cell| is not modified (and is not expected to be) so it should be const
RenderTableCell*. This comment applies to the other methods.

+{
+    if (!extraRowSpanningHeight || !totalPercent)
         return;
 
-    // Recalculating the height of rows based on rowSpan cell height if rowSpan cell height is more than total height of rows.
-    int remainingHeight = rowSpanCellHeight;
+    unsigned rowSpan = cell->rowSpan();
+    unsigned rowIndex = cell->rowIndex();
+
+    if (100 < totalPercent) {

[Julien - ping for review, 2013/07/03 21:08:22]

You never answered about this and I am pretty much convinced this is wrong and
adds some unneeded complexity.

[a.suchit, 2013/07/13 00:49:56]

Removed

+        int changedPosBy = 0;
+        for (unsigned row = rowIndex; row < (rowIndex + rowSpan); row++) {
+            if (m_grid[row].logicalHeight.isPercent())
+                changedPosBy += extraRowSpanningHeight * m_grid[row].logicalHeight.percent() / totalPercent;
+            m_rowPos[row + 1] += changedPosBy;
+        }
+
+        // Remaining height added in the last row under rowSpan cell
+        m_rowPos[rowIndex + rowSpan] += extraRowSpanningHeight - changedPosBy;
+        extraRowSpanningHeight -= changedPosBy;
+    } else {
+        int changedPosBy = 0;
+        for (unsigned row = rowIndex; row < (rowIndex + rowSpan); row++) {
+            if (m_grid[row].logicalHeight.isPercent())
+                changedPosBy += extraRowSpanningHeight * m_grid[row].logicalHeight.percent() / 100;

[Julien - ping for review, 2013/07/03 21:08:22]

This code is backwards if you look at what we do in
RenderTableSection::distributeExtraLogicalHeightToPercentRows:

totalPercent = min(totalPercent, 100)

(ie we allow totalPercent to be above 100 and still keep the total ratio correct
which seems to match FF).

[a.suchit, 2013/07/13 00:49:56]

Done.

+            m_rowPos[row + 1] += changedPosBy;
+        }
+
+        extraRowSpanningHeight -= changedPosBy;
+    }
+}
+
+// If auto rows are present then whole extra height would be distribute in auto rows based on their height ratios so that
+// all auto rows would get its own share from extra height.
+// Else extra height would be distributed in other rows.

[Julien - ping for review, 2013/07/03 21:08:22]

This line is useless. As a whole, your comments are *what* comments and for the
second time, I am pointing out that *why* comments are much much more useful.

[a.suchit, 2013/07/13 00:49:56]

Done.

+void RenderTableSection::distributeExtraRowSpanHeightToAutoRows(RenderTableCell* cell, int totalAutoRowsHeight, int& extraRowSpanningHeight, Vector<int>& rowsHeight)
+{
+    if (!extraRowSpanningHeight || !totalAutoRowsHeight)
+        return;
+
+    unsigned rowSpan = cell->rowSpan();
+    unsigned rowIndex = cell->rowIndex();
+    int changedPosBy = 0;
 
     for (unsigned row = rowIndex; row < (rowIndex + rowSpan); row++) {
-        int rowHeight = (rowSpanCellHeight * rowsHeight[row - rowIndex]) / totalRowsHeight;
-        remainingHeight -= rowHeight;
-        m_rowPos[row + 1] = m_rowPos[row] + rowHeight + borderSpacingForRow(row);
+        if (m_grid[row].logicalHeight.isAuto())
+            changedPosBy += (extraRowSpanningHeight * rowsHeight[row - rowIndex]) / totalAutoRowsHeight;
+        m_rowPos[row + 1] += changedPosBy;
     }
     // Remaining height added in the last row under rowSpan cell

[Julien - ping for review, 2013/07/03 21:08:22]

This comment is also a *what* comment, I would rather see a *why* comment like:

// If we have at least an auto row(s), we need to ensure that we have
distributed all the extra space to the last one. Not doing so would yield to
fixed rows being wrongly increased.

Btw, that whole comment hints at a missing ASSERT here:
ASSERT(!extraRowSpanningHeight);

[a.suchit, 2013/07/13 00:49:56]

Done.

-    m_rowPos[rowIndex + rowSpan] += remainingHeight;
+    m_rowPos[rowIndex + rowSpan] += extraRowSpanningHeight - changedPosBy;
+
+    extraRowSpanningHeight -= changedPosBy;
+}
+// Whole extra height would be distribute in  remaining rows based on their height ratios so that

[Julien - ping for review, 2013/07/03 21:08:22]

Again, that's a what comment. Here you could explain:
1) that it's the last hope of spreading the extra logical space so we just do
it.
2) why it makes sense to use the current height ratio as our guide.

[a.suchit, 2013/07/13 00:49:56]

Done.

+// all rows would get its own share from extra height.
+void RenderTableSection::distributeExtraRowSpanHeightToRemainingRows(RenderTableCell* cell, int totalRemainingRowsHeight, int& extraRowSpanningHeight, Vector<int>& rowsHeight)
+{
+    if (!extraRowSpanningHeight || !totalRemainingRowsHeight)
+        return;
+
+    unsigned rowSpan = cell->rowSpan();
+    unsigned rowIndex = cell->rowIndex();
+    int changedPosBy = 0;
+
+    for (unsigned row = rowIndex; row < (rowIndex + rowSpan); row++) {
+        if (!m_grid[row].logicalHeight.isPercent())
+            changedPosBy += (extraRowSpanningHeight * rowsHeight[row - rowIndex]) / totalRemainingRowsHeight;
+        m_rowPos[row + 1] += changedPosBy;
+    }
+    // Remaining height added in the last row under rowSpan cell
+    m_rowPos[rowIndex + rowSpan] += extraRowSpanningHeight - changedPosBy;
+
+    extraRowSpanningHeight -= changedPosBy;
+}
+
+// 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 rowSpan = cell->rowSpan();
+
+    int expectedTotalRowsHeight = 0;
+    Vector<int> rowsHeight(rowSpan);
+    int totalRowsHeight = 0;
+
+    getRowsHeightInRowSpan(cell, expectedTotalRowsHeight, totalRowsHeight, rowsHeight);
+
+    if (!totalRowsHeight || expectedTotalRowsHeight <= totalRowsHeight)
+        return;
+
+    unsigned rowIndex = cell->rowIndex();
+    int totalPercent = 0;
+    int totalAutoRowsHeight = 0;
+    int totalRemainingRowsHeight = 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 -= rowsHeight[row - rowIndex];
+        } else if (m_grid[row].logicalHeight.isAuto()) {
+            totalAutoRowsHeight += rowsHeight[row - rowIndex];
+        }
+    }
+
+    int initialPos = m_rowPos[rowIndex + rowSpan];
+    int extraRowSpanningHeight = expectedTotalRowsHeight - totalRowsHeight;
+
+    distributeExtraRowSpanHeightToPrecentRows(cell, totalPercent, extraRowSpanningHeight, rowsHeight);
+    distributeExtraRowSpanHeightToAutoRows(cell, totalAutoRowsHeight, extraRowSpanningHeight, rowsHeight);
+    distributeExtraRowSpanHeightToRemainingRows(cell, totalRemainingRowsHeight, extraRowSpanningHeight, rowsHeight);
+
+    ASSERT(!extraRowSpanningHeight);
 
     // Getting total changed height in the table
-    unsigned changedHeight = changedHeight = m_rowPos[rowIndex + rowSpan] - initialPos;
+    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++)
             m_rowPos[row] += changedHeight;
     }
 }
 
@@ skipping 1235 matching lines @@
     info.addMember(logicalHeight, "logicalHeight");
 }
 
 void RenderTableSection::CellStruct::reportMemoryUsage(MemoryObjectInfo* memoryObjectInfo) const
 {
     MemoryClassInfo info(memoryObjectInfo, this, PlatformMemoryTypes::Rendering);
     info.addMember(cells, "cells");
 }
 
 } // namespace WebCore