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

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 351 matching lines @@
             }
         }
         m_rowPos[row + 1] += accumulatedPositionIncrease;
     }
 
     ASSERT(!remainder);
 
     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;
 
     for (unsigned i = 0; i < rowSpanCells.size(); i++) {
         RenderTableCell* cell = rowSpanCells[i];
 
         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;
         }
 
         lastRowIndex = rowIndex;
         lastRowSpan = rowSpan;
 
         struct SpanningRowsHeight spanningRowsHeight;
 
         populateSpanningRowsHeightFromCell(cell, spanningRowsHeight);
 
         if (!spanningRowsHeight.totalRowsHeight || spanningRowsHeight.spanningCellHeightIgnoringBorderSpacing <= spanningRowsHeight.totalRowsHeight)
             continue;
 
         int totalPercent = 0;
         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()) {
                 totalPercent += m_grid[row].logicalHeight.percent();
                 totalRemainingRowsHeight -= spanningRowsHeight.rowHeight[row - rowIndex];
             } else if (m_grid[row].logicalHeight.isAuto()) {
                 totalAutoRowsHeight += spanningRowsHeight.rowHeight[row - rowIndex];
             }
         }
 
@@ skipping 1228 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