Bug 16811 – Row size/position is wrongly calculated when table having overlapping rowspan cell and colspan cell

• Component: Tables
• Status: RESOLVED FIXED

[Steve Nicolai]

The attached fragment from the morningstar website recreates the problem with Safari 3.0.4 with webkit 523.12.2 on MacOS 10.4.11

The full webpage is:
http://profile.morningstar.com/Hewitt/Profile.asp?CountryID=USA&Symbol=0275100006

In the third row of the table in the fragment is a cell containing an image that has specified width and height 154x154 (actual is 152x152).  When I look at the table in the render tree in I see that the table height is 139.

Visually, the bottom of the image intrudes into the following table.

I would expect that the table height be large enough to display the image without affecting the following table.

[Steve Nicolai]

Created attachment 18359 [details]
Fragment to recreate the problem

[Steve Nicolai]

This bug still happens with the nightly build from Jan 9, build r29336.

[Mark Rowe (bdash)]

Confirmed with Safari 3.0.4 and TOT on Leopard.

[Steve Nicolai]

Created attachment 18711 [details]
reduced fragment

I reduced the fragment some more and did a little debugging on it.

The enclosed fragment has a cell with rowspan=2 in the second column.  The next row has colspan=2 in the first cell.  This sets CellStruct.inColSpan on that cell.  This causes RenderTableSection::calcRowHeight() takes the continue on line 338 of today's TOT never calculating the true height needed for the rowspan.

Camino 1.5.4 renders this reduced fragment properly.

[Steve Nicolai]

Created attachment 18728 [details]
A patch for the problem

This patch happens to produce the correct output and doesn't cause any new failures in the test suite when I run it.  I have doubts that it is the correct patch however.

[David Kilzer (:ddkilzer)]

Comment on attachment 18728 [details]
A patch for the problem

Thanks for attaching a patch!  In the future, please set the "review?" flag on patches you attach to bugs if you'd like them considered for committing.

I'm going to mark this as review- since it needs a ChangeLog and a test case.  (I did not review this for correctness however.)

See this page for more details about contributing code:

http://webkit.org/coding/contributing.html

[Pravin D]

Created attachment 150222 [details]
Patch

[Eric Seidel (no email)]

Comment on attachment 150222 [details]
Patch

I'm confused how the code previously was OK using a single cell?

[Robert Hogan]

Comment on attachment 150222 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=150222&action=review

> Source/WebCore/ChangeLog:10
> +        end in the row r-1, it is not included in the calculation. On the other hand if there is a cell with rowspan > 1 and ends in row r-1 then the height 
> +        this height is taken into calculation. In our case the r-1 has a cell which has 2 overlapping cells, one with colspan > 1 and other with rowspan > 1

typo: 'the height this height'.

> Source/WebCore/ChangeLog:11
> +        and rowspan ends in r-1. Only the primary cell(last cell added) was being considered for calculating the position of the row r-1.

If you make a clearer split in this explanation between (i) the statement of the way things currently work and why it's broken and (ii) the new, correct behaviour and why it works, it would make this log easier to grok.

> Source/WebCore/rendering/RenderTableSection.cpp:379
> +                // find out the baseline
> +                EVerticalAlign va = cell->style()->verticalAlign();
> +                if (va == BASELINE || va == TEXT_BOTTOM || va == TEXT_TOP || va == SUPER || va == SUB || va == LENGTH) {
> +                    LayoutUnit baselinePosition = cell->cellBaselinePosition();
> +                    if (baselinePosition > cell->borderBefore() + cell->paddingBefore()) {
> +                        m_grid[cellStartRow].baseline = max(m_grid[cellStartRow].baseline, baselinePosition - cell->intrinsicPaddingBefore());
> +                        baselineDescent = max(baselineDescent, m_rowPos[cellStartRow] + cellLogicalHeight - (baselinePosition - cell->intrinsicPaddingBefore()));
> +                    }

I don't think you need this in the new loop, do things break if you move it outside? I think you should experiment with moving this and other stuff out of the loop, I can't say for sure what else should go.

> LayoutTests/fast/table/last-cell-of-rowspan-overlapping-colspan-cell.html:20
> +<tr>
> +<td>
> +<img width="20" height="60" />
> +</td>
> +<td  rowspan="2" >
> +<img  width="20" height="120" style="border:1px solid green;"/>
> +</td>
> +</tr>
> +<tr>
> +<td  colspan="2">
> +<img src="" height="20"  />
> +</td>
> +</tr>
> +</table>

Need some indentation here to keep the test readable.

[Pravin D]

(In reply to comment #8)
> (From update of attachment 150222 [details])
> I'm confused how the code previously was OK using a single cell?

I don't think so the code was working properly previously. 

Below is the change set

http://trac.webkit.org/changeset/63994/trunk/WebCore/rendering/RenderTableSection.h  

CellStruct{
  -   RenderTableCell* cell; 
  +   Vector<RenderTableCell*, 1> cells;  
}

CellStruct was made to have more than one cell at a given position (row,col). 

The algo in calcRowLogicalHeight() to calculate the position of rows is written assuming there can be only one cell at position(row,col).

This issue is reported before the change set 63994 so... :)

[Pravin D]

Created attachment 151527 [details]
Patch

[Pravin D]

(In reply to comment #9)
> (From update of attachment 150222 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=150222&action=review
> 
> > Source/WebCore/ChangeLog:10
> > +        end in the row r-1, it is not included in the calculation. On the other hand if there is a cell with rowspan > 1 and ends in row r-1 then the height 
> > +        this height is taken into calculation. In our case the r-1 has a cell which has 2 overlapping cells, one with colspan > 1 and other with rowspan > 1
> 
> typo: 'the height this height'.
> 
> > Source/WebCore/ChangeLog:11
> > +        and rowspan ends in r-1. Only the primary cell(last cell added) was being considered for calculating the position of the row r-1.
> 
> If you make a clearer split in this explanation between (i) the statement of the way things currently work and why it's broken and (ii) the new, correct behaviour and why it works, it would make this log easier to grok.
> 
> > Source/WebCore/rendering/RenderTableSection.cpp:379
> > +                // find out the baseline
> > +                EVerticalAlign va = cell->style()->verticalAlign();
> > +                if (va == BASELINE || va == TEXT_BOTTOM || va == TEXT_TOP || va == SUPER || va == SUB || va == LENGTH) {
> > +                    LayoutUnit baselinePosition = cell->cellBaselinePosition();
> > +                    if (baselinePosition > cell->borderBefore() + cell->paddingBefore()) {
> > +                        m_grid[cellStartRow].baseline = max(m_grid[cellStartRow].baseline, baselinePosition - cell->intrinsicPaddingBefore());
> > +                        baselineDescent = max(baselineDescent, m_rowPos[cellStartRow] + cellLogicalHeight - (baselinePosition - cell->intrinsicPaddingBefore()));
> > +                    }
> 
> I don't think you need this in the new loop, do things break if you move it outside? I think you should experiment with moving this and other stuff out of the loop, I can't say for sure what else should go.
> 
I think this should be part of the loop. The baseline of a row is correctly determined if all the cells in the row(even rowSpan cells) participate. In case of rowSpan cells, they participate only when the last cell of the row is encountered. 
According to CSS2.1
http://www.w3.org/TR/CSS21/tables.html#height-layout

"The maximum distance between the top of the cell box and the baseline over all cells of the row that have 'vertical-align: baseline' is used to set the baseline of the row."
Thus if we leave out the rowSpan cell baseline we will have a similar issue(intead of height it would be baseline).

So as is the case with height we need to consider all the cells at a given position for calculating the baseline.

> > LayoutTests/fast/table/last-cell-of-rowspan-overlapping-colspan-cell.html:20
> > +<tr>
> > +<td>
> > +<img width="20" height="60" />
> > +</td>
> > +<td  rowspan="2" >
> > +<img  width="20" height="120" style="border:1px solid green;"/>
> > +</td>
> > +</tr>
> > +<tr>
> > +<td  colspan="2">
> > +<img src="" height="20"  />
> > +</td>
> > +</tr>
> > +</table>
> 
> Need some indentation here to keep the test readable.
> 
Done...

[Julien Chaffraix]

Comment on attachment 151527 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=151527&action=review

> Source/WebCore/rendering/RenderTableSection.cpp:380
> +                    }

I think it's fine to compute the baseline in the loop. We are taking the max so it shouldn't impact the result in most cases.

> LayoutTests/fast/table/last-cell-of-rowspan-overlapping-colspan-cell-expected.html:1
> +<html>

Unless you are testing quirksmode behavior, test case *should* include a doctype:

<!DOCTYPE html>

This is all the more true for a test involving an auto-table layout.

> LayoutTests/fast/table/last-cell-of-rowspan-overlapping-colspan-cell-expected.html:3
> +  <title> Ref Test for bug https://bugs.webkit.org/show_bug.cgi?id=16811 </title>

I prefer to dump the bug (including a link) when text is allowed in the change (like here). A title doesn't add much as you wouldn't see it in DRT output (on top of not being clickable).

> LayoutTests/fast/table/last-cell-of-rowspan-overlapping-colspan-cell-expected.html:6
> +  <p> The Height of the green rectangle should be same as the yellow rectangle and the green rectangle must be contained

it would be nice to add a description of 'what' you are testing. Example:

<p>This test checks that overlapping cells get sized properly.</p>

> LayoutTests/fast/table/last-cell-of-rowspan-overlapping-colspan-cell-expected.html:9
> +  <table  style="border:1px solid yellow;" cellspacing="0" cellpadding="0" align="center">

Couldn't we remove more styling from here? The align="center" for example seems like it could be removed. Arguably same for the other attributes.

> LayoutTests/fast/table/last-cell-of-rowspan-overlapping-colspan-cell-expected.html:19
> +      <td  >

Unneeded spaces (not repeated for the other cases)

[Pravin D]

Created attachment 152019 [details]
Patch

[Julien Chaffraix]

Comment on attachment 152019 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=152019&action=review

> LayoutTests/fast/table/last-cell-of-rowspan-overlapping-colspan-cell-expected.html:5
> +<head>
> +  <title> Ref Test for bug https://bugs.webkit.org/show_bug.cgi?id=16811 </title>
> +</head>

Stray code (not repeated for the other file).

> LayoutTests/fast/table/last-cell-of-rowspan-overlapping-colspan-cell-expected.html:23
> +      <img src="" height="20"/>

Is this src="" attribute necessary?

[Pravin D]

Created attachment 152069 [details]
Patch

[WebKit Review Bot]

Comment on attachment 152069 [details]
Patch

Clearing flags on attachment: 152069

Committed r122516: <http://trac.webkit.org/changeset/122516>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.