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

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, 2013 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 467 matching lines @@
     if (cellIsFullyIncludedInOtherCell(cell1, cell2))
         return true;
     // Sorting lower row index first because first we need to apply the extra height of spanning cell which
     // comes first in the table so lower rows's position would increment in sequence.
     if (!cellIsFullyIncludedInOtherCell(cell2, cell1))
         return (cell1->rowIndex() < cell2->rowIndex());
 
     return false;
 }
 
-bool RenderTableSection::isHeightNeededForRowHavingOnlySpanningCells(unsigned row)
-{
-    unsigned totalCols = m_grid[row].row.size();
-
-    if (!totalCols)
-        return false;
-
-    for (unsigned col = 0; col < totalCols; col++) {
-        const CellStruct& rowSpanCell = cellAt(row, col);
-
-        if (rowSpanCell.cells.size()) {
-            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 (totalRowSpanCellHeight < cell->logicalHeightForRowSizing())
-                return true;
-        }
-    }
-
-    return false;
-}
-
-unsigned RenderTableSection::calcRowHeightHavingOnlySpanningCells(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 0;
 
     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());
+
+        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];
+
+            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.

+                }
+
+                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.

+
+                totalRowspanCellHeight -= borderSpacingForRow(endRow - 1);
+
+                totalRowspanCellHeight += accumulatedPositionIncrease;
+                if (effectedRowByExtraHeight >= rowIndex && effectedRowByExtraHeight < endRow)
+                    totalRowspanCellHeight += extraHeightToPropagate;
+
+                if (totalRowspanCellHeight < cell->logicalHeightForRowSizing()) {
+                    unsigned extraHeightRequired = rowSpanCell.cells[0]->logicalHeightForRowSizing() - totalRowspanCellHeight;
+                    int remainder = extraHeightRequired % spanningCellsRowsCountHavingZeroHeight;
+
+                    if (remainder)
+                        extraHeightRequired += spanningCellsRowsCountHavingZeroHeight - remainder;
+
+                    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());
 
     int accumulatedPositionIncrease = 0;
     const unsigned rowSpan = cell->rowSpan();
     const unsigned rowIndex = cell->rowIndex();
 
     ASSERT_UNUSED(rowSpan, rowSpan == spanningRowsHeight.rowHeight.size());
 
     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;
     }
 
     spanningRowsHeight.totalRowsHeight += 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());
 
     // 'rowSpanCells' list is already sorted based on the cells rowIndex in ascending order
     // Arrange row spanning cell in the order in which we need to process first.
     std::sort(rowSpanCells.begin(), rowSpanCells.end(), compareRowSpanCellsInHeightDistributionOrder);
 
     unsigned extraHeightToPropagate = 0;
     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];
 
         unsigned rowIndex = cell->rowIndex();
 
         unsigned rowSpan = cell->rowSpan();
 
         unsigned spanningCellEndIndex = rowIndex + rowSpan;
         unsigned lastSpanningCellEndIndex = lastRowIndex + lastRowSpan;
 
@@ skipping 17 matching lines @@
 
         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.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
         // 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?
         if (!spanningRowsHeight.totalRowsHeight) {
             if (spanningRowsHeight.spanningCellHeightIgnoringBorderSpacing)
@@ skipping 988 matching lines @@
     // FIXME: The table's direction should determine our row's direction, not the section's (see bug 96691).
     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);
 }
 
 } // namespace blink