Tables with specific merge cell configuration render with extra height to tr elements.

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 9798ab87a229cc917135d91d964a4b6988882107..410888d0ac4965b7b98ed406c2f3fba154f17b6c 100644
--- a/Source/core/rendering/RenderTableSection.cpp
+++ b/Source/core/rendering/RenderTableSection.cpp
@@ -485,57 +485,65 @@ static bool compareRowSpanCellsInHeightDistributionOrder(const RenderTableCell*
     return false;
 }
 
-bool RenderTableSection::isHeightNeededForRowHavingOnlySpanningCells(unsigned row)
+unsigned RenderTableSection::calcRowHeightHavingOnlySpanningCells(unsigned row, int& accumulatedPositionIncrease, unsigned effectedRowByExtraHeight, unsigned& extraHeightToPropagate, Vector<int>& rowsCountWithOnlySpanningCells)

[dsinclair, 2014/10/15 21:37:41]

s/rowsCountWithOnlySpanningCells/rowCountWithOnlySpanningCells

[dsinclair, 2014/10/15 21:37:42]

How does accumulatedPositionIncrease relate to extraHeightToPropagate?

[dsinclair, 2014/10/15 21:37:42]

What does effectedRowByExtraHeight mean? Does it mean the row was effected by
extra height? Or, is it the height that effected the row?

[suchit.agrawal, 2014/10/25 04:28:59]

There is no relation between these 2.

extraHeightToPropagate is the extra height applied till end of current spanning
cell in distributeRowSpanHeightToRows(). This extra height is the total
additional height calculated in all rows (all rows before current spanning cell)
of the table.

accumulatedPositionIncrease is the total additional height calculated in
previous rows of current spanning cell in
updateRowsHeightHavingOnlySpanningCells().

[dsinclair, 2014/11/05 17:03:20]

So, if I'm understanding correctly, accumulatedPositionIncrease is only related
to the current spanning cells and extraHeightToPropagate is related to the
entire table. Yes?

[a.suchit, 2014/12/03 13:09:13]

right.

 {
+    ASSERT(rowHasOnlySpanningCells(row));
+
     unsigned totalCols = m_grid[row].row.size();
 
     if (!totalCols)
-        return false;
+        return 0;
+
+    unsigned rowHeight = 0;
 
     for (unsigned col = 0; col < totalCols; col++) {
         const CellStruct& rowSpanCell = cellAt(row, col);
 
         if (rowSpanCell.cells.size()) {

[dsinclair, 2014/10/15 21:37:41]

if (!rowSpanCell.cells.size())
  continue;

[a.suchit, 2014/12/03 13:09:13]

Done.

             RenderTableCell* cell = rowSpanCell.cells[0];
-            const unsigned rowIndex = cell->rowIndex();
-            const unsigned rowSpan = cell->rowSpan();
-            int totalRowSpanCellHeight = 0;
 
-            for (unsigned row = 0; row < rowSpan; row++) {
-                unsigned actualRow = row + rowIndex;
-                totalRowSpanCellHeight += m_rowPos[actualRow + 1] - m_rowPos[actualRow];
-            }
-            totalRowSpanCellHeight -= borderSpacingForRow(rowIndex + rowSpan - 1);
+            if (cell->rowSpan() > 1) {

[dsinclair, 2014/10/15 21:37:42]

if (cell.rowSpan() <= 1)
  continue;

[a.suchit, 2014/12/03 13:09:13]

Done.

+                const unsigned rowIndex = cell->rowIndex();
+                const unsigned rowSpan = cell->rowSpan();

[dsinclair, 2014/10/15 21:37:42]

Can we rename these to cellRowIndex and cellRowSpan to make it obvious why they
are different from row above?

[a.suchit, 2014/12/03 13:09:13]

Done.

+
+                // As we are moving from top to bottom for calculating rows height of row with only spanning cells
+                // and all previous rows are already processed so we need to find the number of rows with only

[dsinclair, 2014/10/15 21:37:41]

This comment is difficult to process, does the below make sense?

// As we are going from the top of the table to the bottom to calculate the row
// heights for rows that only contain spanning cells and all previous rows are
// processed we only need to find the number of rows with spanning cells from
the
// current cell to the end of the current cells spanning height.

[a.suchit, 2014/12/03 13:09:13]

Done.

+                // spanning cells presents from current row to end of the spanning cell.
+                unsigned startRow = max(rowIndex, row);
+                unsigned endRow = rowIndex + rowSpan;

[dsinclair, 2014/10/15 21:37:42]

Does this mean the endRow of our current cell, or the row that comes after the
ending row of our cell?

[suchit.agrawal, 2014/10/25 04:28:59]

we can see it in 2 ways,
1. It is the row that comes after the ending row of our cell.
2. It is the end border of the ending row of our cell.

+                unsigned spanningCellsRowsCountHavingZeroHeight = 0;
+
+                if (startRow) {
+                    spanningCellsRowsCountHavingZeroHeight = rowsCountWithOnlySpanningCells[endRow - 1] - rowsCountWithOnlySpanningCells[startRow - 1];

[dsinclair, 2014/10/15 21:37:42]

Why is this startRow - 1 and not startRow?

[suchit.agrawal, 2014/10/25 04:28:59]

000112223334
        ^  ^
4-3=1. Means 1 row with only spanning cell. But actually it have 2 (fist row and
last row).
So 4-2=2

+                } else {
+                    spanningCellsRowsCountHavingZeroHeight = (rowsCountWithOnlySpanningCells[endRow - 1] - rowsCountWithOnlySpanningCells[startRow]) + rowsCountWithOnlySpanningCells[0];

[dsinclair, 2014/10/15 21:37:41]

If I'm reading this correctly we're doing:

spanning = count[endRow - 1] - count[0] + count[0];

Is that correct? We'd only get in here if startRow is 0. So we subtract and then
add the value for 0?

[suchit.agrawal, 2014/10/25 04:28:59]

yes, we can remove subtraction and addition.

[a.suchit, 2014/12/03 13:09:13]

Done.

+                }
 
-            if (totalRowSpanCellHeight < cell->logicalHeightForRowSizing())
-                return true;
-        }
-    }
+                int totalRowspanCellHeight = m_rowPos[endRow] - m_rowPos[rowIndex];

[dsinclair, 2014/10/15 21:37:41]

It's very confusing that we jump back and forth between startRow and rowIndex
for what we're starting with. Can we fixup these names so it's clearer what's
going on?

[suchit.agrawal, 2014/10/25 04:28:59]

I will change |startRow| to |startRowforSpanningCellCount|.

[a.suchit, 2014/12/03 13:09:13]

Done.

 
-    return false;
-}
+                totalRowspanCellHeight -= borderSpacingForRow(endRow - 1);
 
-unsigned RenderTableSection::calcRowHeightHavingOnlySpanningCells(unsigned row)
-{
-    ASSERT(rowHasOnlySpanningCells(row));
+                totalRowspanCellHeight += accumulatedPositionIncrease;
+                if (effectedRowByExtraHeight >= rowIndex && effectedRowByExtraHeight < endRow)
+                    totalRowspanCellHeight += extraHeightToPropagate;
 
-    unsigned totalCols = m_grid[row].row.size();
+                if (totalRowspanCellHeight < cell->logicalHeightForRowSizing()) {
+                    unsigned extraHeightRequired = rowSpanCell.cells[0]->logicalHeightForRowSizing() - totalRowspanCellHeight;
+                    int remainder = extraHeightRequired % spanningCellsRowsCountHavingZeroHeight;
 
-    if (!totalCols)
-        return 0;
+                    if (remainder)
+                        extraHeightRequired += spanningCellsRowsCountHavingZeroHeight - remainder;
 
-    unsigned rowHeight = 0;
-
-    for (unsigned col = 0; col < totalCols; col++) {
-        const CellStruct& rowSpanCell = cellAt(row, col);
-        if (rowSpanCell.cells.size() && rowSpanCell.cells[0]->rowSpan() > 1)
-            rowHeight = std::max(rowHeight, rowSpanCell.cells[0]->logicalHeightForRowSizing() / rowSpanCell.cells[0]->rowSpan());
+                    rowHeight = std::max(rowHeight, extraHeightRequired / spanningCellsRowsCountHavingZeroHeight);

[dsinclair, 2014/10/15 21:37:41]

Wont' this fail to set rowHeight in the case where it's less then
logicalHeightForRowSizing? So, isn't it now possible for us to return a 0
rowHeight?

[dsinclair, 2014/10/15 21:37:41]

Can we ever have a case where spanningCellsRowsCountHavingZeroHeight is 0?
Should we have an ASSERT to that effect?

[suchit.agrawal, 2014/10/25 04:28:59]

rowHeight would be less than logicalHeightForRowSizing because
logicalHeightForRowSizing is the total height of spanning rows.
It may return 0 or non-zero. It is depend on extraHeightRequired.

[suchit.agrawal, 2014/10/25 04:28:59]

spanningCellsRowsCountHavingZeroHeight would not be 0 because this fuction calls
only when current row have only spanning cells.

+                }
+            }
+        }
     }
 
     return rowHeight;
 }
 
-void RenderTableSection::updateRowsHeightHavingOnlySpanningCells(RenderTableCell* cell, struct SpanningRowsHeight& spanningRowsHeight)
+void RenderTableSection::updateRowsHeightHavingOnlySpanningCells(RenderTableCell* cell, struct SpanningRowsHeight& spanningRowsHeight, unsigned& extraHeightToPropagate, Vector<int>& rowsCountWithOnlySpanningCells)
 {
     ASSERT(spanningRowsHeight.rowHeight.size());
 
@@ -547,8 +555,8 @@ void RenderTableSection::updateRowsHeightHavingOnlySpanningCells(RenderTableCell
 
     for (unsigned row = 0; row < spanningRowsHeight.rowHeight.size(); row++) {
         unsigned actualRow = row + rowIndex;
-        if (!spanningRowsHeight.rowHeight[row] && rowHasOnlySpanningCells(actualRow) && isHeightNeededForRowHavingOnlySpanningCells(actualRow)) {
-            spanningRowsHeight.rowHeight[row] = calcRowHeightHavingOnlySpanningCells(actualRow);
+        if (!spanningRowsHeight.rowHeight[row] && rowHasOnlySpanningCells(actualRow)) {
+            spanningRowsHeight.rowHeight[row] = calcRowHeightHavingOnlySpanningCells(actualRow, accumulatedPositionIncrease, rowIndex + rowSpan, extraHeightToPropagate, rowsCountWithOnlySpanningCells);

[dsinclair, 2014/10/15 21:37:41]

We set effectedRowByExtraHeight to rowIndex + rowSpan. So, it's the next row,
after the spanning row that gets the extra height?

If I'm reading the above correctly, this row won't necessarily get the extra
height. It only changes if it's within the index + span of a given cell?

[suchit.agrawal, 2014/10/25 04:28:59]

In calcRowHeightHavingOnlySpanningCells(), We may go beyond (index + span)
because current row have many other spanning cells and those spanning cells may
start before current cell index and/or end after current cell (index + span).

             accumulatedPositionIncrease += spanningRowsHeight.rowHeight[row];
         }
         m_rowPos[actualRow + 1] += accumulatedPositionIncrease;
@@ -571,6 +579,16 @@ void RenderTableSection::distributeRowSpanHeightToRows(SpanningRenderTableCells&
     unsigned lastRowIndex = 0;
     unsigned lastRowSpan = 0;
 
+    Vector<int> rowsCountWithOnlySpanningCells;
+
+    // At this stage, Height of the rows are zero for the one containing only spanning cells.

[dsinclair, 2014/10/15 21:37:42]

This comment doesn't make sense to me, can you re-write it?

[suchit.agrawal, 2014/10/25 04:28:59]

We are collecting the information of the rows with only spanning cells because
these row heights are 0 and we may required to give some height to these rows.
So there is comment indicating that we are collecting information for all the
rows with only spanning cells because all these rows have 0 height.

+    int count = 0;
+    for (unsigned row = 0; row < m_grid.size(); row++) {
+        if (rowHasOnlySpanningCells(row))
+            count++;
+        rowsCountWithOnlySpanningCells.append(count);

[dsinclair, 2014/10/15 21:37:41]

I'm not sure I understand what this is storing. We have a vector of ints. The
count always increases, so if each row only contains spanning cells the vector
will contain: 1,2,3,4,5,6?

Or, if only a couple of them do we'll have something like: 1,1,3,3,5,5?

[suchit.agrawal, 2014/10/25 04:28:59]

yes, we are doing same here.

+    }
+
     for (unsigned i = 0; i < rowSpanCells.size(); i++) {
         RenderTableCell* cell = rowSpanCells[i];
 
@@ -608,7 +626,7 @@ void RenderTableSection::distributeRowSpanHeightToRows(SpanningRenderTableCells&
 
         // Here we are handling only row(s) who have only rowspanning cells and do not have any empty cell.
         if (spanningRowsHeight.isAnyRowWithOnlySpanningCells)
-            updateRowsHeightHavingOnlySpanningCells(cell, spanningRowsHeight);
+            updateRowsHeightHavingOnlySpanningCells(cell, spanningRowsHeight, extraHeightToPropagate, rowsCountWithOnlySpanningCells);
 
         // 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