Row spanning cell content is flowing out of the cell border if this row spanning cell comes under t…

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 765b35e238ac819d6edba8188fcf0df6bf4c88cc..348a709d9f44af865ba6b2efd74f8d1351276993 100644
--- a/Source/core/rendering/RenderTableSection.cpp
+++ b/Source/core/rendering/RenderTableSection.cpp
@@ -369,12 +369,36 @@ void RenderTableSection::distributeExtraRowSpanHeightToRemainingRows(RenderTable
     extraRowSpanningHeight -= accumulatedPositionIncrease;
 }
 
+// 1. To save extra height distribution of all spanning cells those start and end at same row, We process only
+// heighest spanning cell and sort these type of cells in descending order based on their height.
+// 2. If inner most cell's extra height distributed first then it's parent spanning cells height can be set as
+// specified by user. So sorting these types of cells in ascending order based on their size.

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

The comment is OK but I think we could make this comment way clearer:

// To avoid unneeded extra height distributions, we apply the following sorting
algorithm:
// 1. We sort the cells by increasing starting row, then increasing last row (ie
the top-most, longest cells first)
// 2. For cells spanning the same rows, we sort by intrinsic size.

[a.suchit, 2013/07/31 10:26:13]

Done.

+static bool compareRowSpanCellsInHeightDistributionOrder(RenderTableCell* cell2, RenderTableCell* cell1)

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

const RenderTableCell*, const RenderTableCell*

[a.suchit, 2013/07/31 10:26:13]

Done.

+{

[a.suchit, 2013/07/31 10:26:13]

+----------+----------+----------+
|          |    B     |    C     |
|          +----------+----------+----------+
|          |          |          |     F    |
|          |          |    E     |----------+
|          |          |          |     G    |
|          |    D     |          +----------+
|    A     |          +----------+ E extra height
|          |          |    H     |
|          |          +----------+ D extra height
|          +----------+----------+----------+
|          |    I     |    J     |     K    |
|          +----------+----------+----------+
+----------+----------+----------+ A extra height
|    L     |    M     |    N     |
+----------+----------+----------+ just border space
|          |          +----------+
|          |          |    Q     |
|          |          |----------+
|          |          |    R     |
|          +    P     +----------+
|    O     |          |    S     |
|          +          +----------+
|          +          |    T     |
|          |          +----------+ P extra height
|          +----------+----------+
+----------+ O extra height

A, D, E, O, P list sorted and becomes E, D, A, O, P. Where P does not play any
role.

+    ASSERT(cell1);
+    ASSERT(cell2);

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

These ASSERTs are unneeded for 2 reasons:
- unless the std is wrong this won't happen (and it should be tested enough that
it's unlikely to happen)
- you would crash below anyway.

[a.suchit, 2013/07/31 10:26:13]

Done.

+
+    unsigned cellRowIndex1 = cell1->rowIndex();
+    unsigned cellRowSpan1 = cell1->rowSpan();
+    unsigned cellRowIndex2 = cell2->rowIndex();
+    unsigned cellRowSpan2 = cell2->rowSpan();
+
+    if (cellRowIndex1 == cellRowIndex2 && cellRowSpan1 == cellRowSpan2)
+        return (cell2->logicalHeightForRowSizing() > cell1->logicalHeightForRowSizing());

[a.suchit, 2013/07/31 10:26:13]

This condition covers below case. In below case, A, B, C are 4 rowspan cells and
Only B spanning cell will distribute it's extra height in rows and A, C does not
need to distribute their's extra height because B is highest and A, C's extra
height covers by A's extra height.

+---+---+---+---+
|   |   |   | D |
|   |   |   +---+
|   |   |   | E |
|   |   |   +---+---+
| A | B | C | F | H |
|   |   |   +---+---+
|   |   |   | G | I |
|   |   |   +---+---+
|   |   +---+
+---+   |
    +---+

A, B, C list sorted and becomes B, A, C and A, C will not play any role.

+
+    return (cellRowIndex2 >= cellRowIndex1 && (cellRowIndex2 + cellRowSpan2) <= (cellRowIndex1 + cellRowSpan1));

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

The comparator function passed to std::sort should be a *strict* comparator.
This means that this condition should be changed to be strict or else badness
will happen.

[a.suchit, 2013/07/31 10:26:13]

I did not get meaning of strict comparator and not able to find that what to
improve in this condition.

This condition covers below three cases. (A is 4 rowspan cell and B is 2 rowspan
cell.)

+---+---+       +---+---+---+   +---+---+
|   | C |       |   |   | C |   |   | C |
|   +---+       |   |   +---+   |   +---+---+
|   | D |       |   | B | D |   |   |   | D |
|   +---+---+   |   |   +---+   |   |   +---+
| A |   | E |   | A +---+       | A | B | E |
|   |   +---+   |   | E |       |   |   +---+
|   | B | F |   |   +---+       |   +---+
|   |   |---+   |   | F |       |   | F |
|   +---+       |   +---+       |   +---+
+---+           +---+           +---+

A, B list is sorted and becomes B, A.

Below 3 cases are same in different scenario. (B is 4 rowspan cell and A is 2
rowspan cell.)

+---+---+       +---+---+---+   +---+---+
| C |   |       |   |   | C |   | C |   |
+---+   |       | A |   +---+   +---+   +---+
| D |   |       |   |   | D |   |   |   | D |
+---+   +---+   |   |   +---+   |   |   +---+
| A | B | E |   +---+ B |       | A | B | E |
|   |   +---+   | E |   |       |   |   +---+
|   |   | F |   +---+   |       +---+   +
|   |   |---+   | F |   |       | F |   |
|   +---+       +---+   |       +---+   |
+---+               +---+           +---+

B, A list is sorted and becomes A, B.

+}
+
 // 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;
@@ -384,14 +408,12 @@ void RenderTableSection::distributeRowSpanHeightToRows(SpanningRenderTableCells&
 
         unsigned rowIndex = cell->rowIndex();
 
-        // 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;
-
         unsigned rowSpan = cell->rowSpan();
         int originalBeforePosition = m_rowPos[rowIndex + rowSpan];
 
+        if (rowIndex == lastRowIndex && rowSpan == lastRowSpan)

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

Nit: Do we need to compute |originalBeforePosition| above even if we don't use
it by bailing out here?

Also it would be nice to add a comment why it's correct to bail out here.

[a.suchit, 2013/07/31 10:26:13]

Done.

+            continue;
+
         if (extraHeightToPropagate) {
             for (unsigned row = lastRowIndex + lastRowSpan + 1; row <= rowIndex + rowSpan; row++)
                 m_rowPos[row] += extraHeightToPropagate;
@@ -411,6 +433,9 @@ void RenderTableSection::distributeRowSpanHeightToRows(SpanningRenderTableCells&
         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 < (rowIndex + rowSpan); row++) {
             if (m_grid[row].logicalHeight.isPercent()) {