Bug 106571 – REGRESSION(r120616): Cell's logical height wrongly computed with vertical-align: baseline and rowspan

• Component: Tables
• Status: RESOLVED FIXED

[Julien Chaffraix]

Created attachment 182144 [details]
Reduction

See URL and attached test case.

It looks like we don't properly grow the first line to account for the baseline, thus leading the content of the first row to overflow into the second row. The attached test case shows that a bit better than the original test case as the image is bigger.

[Robert Hogan]

Created attachment 182811 [details]
Patch

[Julien Chaffraix]

Comment on attachment 182811 [details]
Patch

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

> Source/WebCore/rendering/RenderTableSection.cpp:326
> -                        m_grid[cellStartRow].baseline = max(m_grid[cellStartRow].baseline, baselinePosition - cell->intrinsicPaddingBefore());
> -                        baselineDescent = max(baselineDescent, m_rowPos[cellStartRow] + cellLogicalHeight - (baselinePosition - cell->intrinsicPaddingBefore()));
> +                        m_grid[cellStartRow].baseline = max(m_grid[cellStartRow].baseline, baselinePosition);

Note that this is how the baseline is computed in RenderTableSection::layoutRows which would have been nice to mention somewhere (e.g in the ChangeLog).

Also you are not removing cell->intrinsicPaddingBefore() from m_grid[cellStartRow].baseline which I would expect to change most baselines. Do you know why we don't see a lot of failing test cases because of that?

> Source/WebCore/rendering/RenderTableSection.cpp:335
> +                // Increase row height if the baseline requires it. This is always the first row in the case of a rowspan.
> +                if (m_grid[cellStartRow].baseline)
> +                    m_rowPos[cellStartRow + 1] = max<int>(m_rowPos[cellStartRow + 1], m_rowPos[cellStartRow] + m_grid[cellStartRow].baseline + baselineDescent);

Why do we need to move this code inside the loop? It seems bad as we now will recompute our position for each cells in each rows instead of doing it for each rows.

> LayoutTests/ChangeLog:11
> +        * fast/css/vertical-align-baseline-rowspan-010-expected.html: Added.
> +        * fast/css/vertical-align-baseline-rowspan-010.html: Added.
> +        * fast/css/vertical-align-baseline-rowspan-011-expected.html: Added.
> +        * fast/css/vertical-align-baseline-rowspan-011.html: Added.

I usually advise people to pick up meaningful names for new tests.

[Robert Hogan]

Comment on attachment 182811 [details]
Patch

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

>> Source/WebCore/rendering/RenderTableSection.cpp:326
>> +                        m_grid[cellStartRow].baseline = max(m_grid[cellStartRow].baseline, baselinePosition);
> 
> Note that this is how the baseline is computed in RenderTableSection::layoutRows which would have been nice to mention somewhere (e.g in the ChangeLog).
> 
> Also you are not removing cell->intrinsicPaddingBefore() from m_grid[cellStartRow].baseline which I would expect to change most baselines. Do you know why we don't see a lot of failing test cases because of that?

Lack of test coverage for the rowspan case I suspect, and cases where the border and padding would materially affect the baseline across a row. It doesn't make sense to me that we would consider a baseline less than the real one in this calculation so I don't think it should ever have been there.

>> Source/WebCore/rendering/RenderTableSection.cpp:335
>> +                    m_rowPos[cellStartRow + 1] = max<int>(m_rowPos[cellStartRow + 1], m_rowPos[cellStartRow] + m_grid[cellStartRow].baseline + baselineDescent);
> 
> Why do we need to move this code inside the loop? It seems bad as we now will recompute our position for each cells in each rows instead of doing it for each rows.

This is at the root of the regression observed in your test case. With a rowspan, this function will go to the last row in the span and then calculate the baseline. It the new baseline should grow the row it sits on (i.e. the first row) then that's the one we need to grow. In order to do that, we need to move this statement to where it can spot cellStartRow growing due to a new baseline exceeding the old one.

[Robert Hogan]

Comment on attachment 182811 [details]
Patch

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

>>> Source/WebCore/rendering/RenderTableSection.cpp:335
>>> +                    m_rowPos[cellStartRow + 1] = max<int>(m_rowPos[cellStartRow + 1], m_rowPos[cellStartRow] + m_grid[cellStartRow].baseline + baselineDescent);
>> 
>> Why do we need to move this code inside the loop? It seems bad as we now will recompute our position for each cells in each rows instead of doing it for each rows.
> 
> This is at the root of the regression observed in your test case. With a rowspan, this function will go to the last row in the span and then calculate the baseline. It the new baseline should grow the row it sits on (i.e. the first row) then that's the one we need to grow. In order to do that, we need to move this statement to where it can spot cellStartRow growing due to a new baseline exceeding the old one.

I guess I could limit it to cells that have a rowspan though, and keep the check per row below - yes, that would be less expensive.

[Robert Hogan]

Comment on attachment 182811 [details]
Patch

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

>> LayoutTests/ChangeLog:11
>> +        * fast/css/vertical-align-baseline-rowspan-011.html: Added.
> 
> I usually advise people to pick up meaningful names for new tests.

I thought it best to put them together with the other ones covering the behaviour of baseline and vertical align with a rowspan.

[Robert Hogan]

Created attachment 183808 [details]
Patch

[Julien Chaffraix]

Comment on attachment 183808 [details]
Patch

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

> Source/WebCore/rendering/RenderTableSection.cpp:331
> +                        if (cell->rowSpan() == 1)
> +                            baselineDescent = max(baselineDescent, cellLogicalHeight - (baselinePosition - cell->intrinsicPaddingBefore()));
> +                        m_rowPos[cellStartRow + 1] = max<int>(m_rowPos[cellStartRow + 1], m_rowPos[cellStartRow] + m_grid[cellStartRow].baseline + baselineDescent);

Is this not open to race conditions as baselineDescent could increase *after* you compute your next row's position? Adding some testing about that would be a good addition regardless.

The old code was doing the sizing after walking over all the cells in a row which prevents that (on top of being more efficient).

[Julien Chaffraix]

Comment on attachment 183808 [details]
Patch

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

r=me, if you can make the test not ref-tests, you get extra bonus points but I will let you see if my suggestions work / make any sense.

>> Source/WebCore/rendering/RenderTableSection.cpp:331
>> +                        m_rowPos[cellStartRow + 1] = max<int>(m_rowPos[cellStartRow + 1], m_rowPos[cellStartRow] + m_grid[cellStartRow].baseline + baselineDescent);
> 
> Is this not open to race conditions as baselineDescent could increase *after* you compute your next row's position? Adding some testing about that would be a good addition regardless.
> 
> The old code was doing the sizing after walking over all the cells in a row which prevents that (on top of being more efficient).

Walking through the code over, this is my misunderstanding. There is no race condition as we keep track of the maximums and update them properly.

> LayoutTests/ChangeLog:11
> +        * fast/css/vertical-align-baseline-rowspan-011.html: Added.

I agree it's better to group them (especially since they are not in fast/table :() even if I still don't like the naming.

> LayoutTests/fast/css/vertical-align-baseline-rowspan-010.html:25
> +             <td>XXX</td><td>XXX</td><td rowspan="2"><img></td><td>XXX</td><td>XXX</td>
> +        </tr>
> +        <tr>
> +             <td>XXX</td><td>XXX</td><td>XXX</td><td>XXX</td>
> +        </tr>

Not totally sure about this one but you could check that the first row (the first td XXX) matches the height of the image as this is required for baseline alignment.

> LayoutTests/fast/css/vertical-align-baseline-rowspan-011.html:21
> +             <td>XXX</td><td>XXX</td><td rowspan="2"><img src="resources/greenbox-100px.png"></td><td>XXX</td><td>XXX</td>

This could be converted to a layout-only test easily: you want the first row to match the size of the rowspan (the second is effectively 0 sized).

[Robert Hogan]

(In reply to comment #8)
> (From update of attachment 183808 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=183808&action=review
> 
> r=me, if you can make the test not ref-tests, you get extra bonus points but I will let you see if my suggestions work / make any sense.

I'm more comfortable with ref-tests for these two as the reference results can check for the baseline of the text as well as the height of the row.

[WebKit Review Bot]

Comment on attachment 183808 [details]
Patch

Clearing flags on attachment: 183808

Committed r140907: <http://trac.webkit.org/changeset/140907>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.