Bug 85652 – CSS 2.1 failure: table-height-algorithm-012 fails

• Component: CSS
• Status: RESOLVED FIXED

[Robert Hogan]

A spanning cell with Vertical-align set to 'baseline' aligns the cell's content baseline (which is the bottom of the first line of text or in-flow content) with the baseline of the first of the rows it spans.

[Robert Hogan]

Created attachment 142763 [details]
Patch

[Julien Chaffraix]

Comment on attachment 142763 [details]
Patch

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

> Source/WebCore/rendering/RenderTableSection.cpp:633
> +                        intrinsicPaddingBefore = getBaseline(cell->rowIndex()) - (b - oldIntrinsicPaddingBefore);

For the record, I wonder if this is correct.

The baseline on a rowspan is 0 except for the last row per calcRowLogicalHeight's computation. This is causing other bugs with rowspans but for now, it looks like you should be using the last row in the row span. This should work in the children flexing case above as we do the computation for each row in a row span (this is a HUGE FIXME btw).

[Julien Chaffraix]

Comment on attachment 142763 [details]
Patch

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

By the way, while touching this code, let's use better names (|b| is really uncool).

>> Source/WebCore/rendering/RenderTableSection.cpp:633
>> +                        intrinsicPaddingBefore = getBaseline(cell->rowIndex()) - (b - oldIntrinsicPaddingBefore);
> 
> For the record, I wonder if this is correct.
> 
> The baseline on a rowspan is 0 except for the last row per calcRowLogicalHeight's computation. This is causing other bugs with rowspans but for now, it looks like you should be using the last row in the row span. This should work in the children flexing case above as we do the computation for each row in a row span (this is a HUGE FIXME btw).

After thinking it over, the patch is correct when the baseline is not determined by rowspans. I still think the patch could make us regress some combination of rowspan and baseline alignment (I wish I had a concrete example but I don't have time to look into that) due to calcRowLogicalHeight putting the extra space to the last row in a rowspan.

AFAICT our testing is lacking for vertical-align + rowspan. If you added more tests for that (or pointed out some testing I missed), it would help to convince me.

[Robert Hogan]

Created attachment 145135 [details]
Patch

[Robert Hogan]

Comment on attachment 145135 [details]
Patch

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

> LayoutTests/ChangeLog:33
> +        * platform/chromium-linux-x86/fast/css/vertical-align-baseline-rowspan-004-expected.png: Added.
> +        * platform/chromium-linux-x86/fast/css/vertical-align-baseline-rowspan-004-expected.txt: Added.
> +        * platform/chromium-linux-x86/fast/css/vertical-align-baseline-rowspan-005-expected.png: Added.
> +        * platform/chromium-linux-x86/fast/css/vertical-align-baseline-rowspan-005-expected.txt: Added.
> +        * platform/chromium-linux-x86/fast/css/vertical-align-baseline-rowspan-006-expected.png: Added.
> +        * platform/chromium-linux-x86/fast/css/vertical-align-baseline-rowspan-006-expected.txt: Added.

I've kept png results here to ease review - I plan to add ref tests to the patch for landing.

[Julien Chaffraix]

Comment on attachment 145135 [details]
Patch

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

> Source/WebCore/rendering/RenderTableSection.cpp:633
>                      LayoutUnit b = cell->cellBaselinePosition();

As mentioned, let's rename |b| to something understandable (baselinePosition or cellBaselinePosition)

> LayoutTests/ChangeLog:18
> +        * fast/css/vertical-align-baseline-rowspan-001.htm: Added.
> +        * fast/css/vertical-align-baseline-rowspan-002.htm: Added.
> +        * fast/css/vertical-align-baseline-rowspan-003.htm: Added.
> +            These are a few variations on table-height-algorithm-012.htm
> +        * fast/css/vertical-align-baseline-rowspan-004.htm: Added.
> +        * fast/css/vertical-align-baseline-rowspan-005.htm: Added.
> +        * fast/css/vertical-align-baseline-rowspan-006.htm: Added.
> +            These last 3 test the calculation of the baseline RenderTableSection::calcRowLogicalHeight,
> +            where we now always use the first row in the span. Without this the 'Filler Text' in the middle
> +            cell aligns beyond the top of the table.

Do you intent to contribute those tests back to the CSS WG?

Also all your example have the cells that don't belong to the first one in the rowspan empty (they end up being 2 pixels due to the default border spacing). Would it be possible to give them some real size (maybe also some color) as this would cover more the issue I had in mind?

>> LayoutTests/ChangeLog:33
>> +        * platform/chromium-linux-x86/fast/css/vertical-align-baseline-rowspan-006-expected.txt: Added.
> 
> I've kept png results here to ease review - I plan to add ref tests to the patch for landing.

Thanks for pointing this out.

Aren't some of the baselines a bit off? (like for 006, I would have expected the middle text's baseline to be aligned with the other cells' text baselines)

[Robert Hogan]

Created attachment 145868 [details]
Patch

[Julien Chaffraix]

Comment on attachment 145868 [details]
Patch

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

r=me, with some comment and don't forget to turn the tests into ref-tests.

> LayoutTests/css2.1/20110323/table-height-algorithm-012.htm:25
> +        <p>Test passes if the bottom of "Filler Text" below is aligned.</p>

I would put it this way:

This test passes if the "Filler Text" are baseline aligned. (baseline has a signification outside CSS)

The use of the singular is kind of weird in the sentence and bottom is not very precise. (not repeat for the other tests)

> LayoutTests/fast/css/vertical-align-baseline-rowspan-002.htm:41
> +                <td></td>

It's technically missing a cell here but it's probably good to see that we properly baseline align.

> LayoutTests/fast/css/vertical-align-baseline-rowspan-004.htm:24
> +            #spanning
> +            {
> +                vertical-align: baseline;
> +            }

This is unneeded as td has the same property.

> LayoutTests/fast/css/vertical-align-baseline-rowspan-005.htm:33
> +<!--    The first cell in the row-span is the only one that has a baseline - the others are vertical-align:top.
> +        Observing its baseline leads it to align the top of its text to the top of the text of the other two cells.  -->

This could be dumped in the output as it is a valuable explanation (ojan is a fan of descriptions in tests and I tend to agree (not repeated for other description comments).

> LayoutTests/fast/css/vertical-align-baseline-rowspan-005.htm:34
> +        <p>Test passes if the top of "Filler Text" below is aligned.</p>

This test passes if the cap-height of the "Filler Text" are aligned? (not repeated for the "top" tests)

[Robert Hogan]

Landed: http://trac.webkit.org/changeset/120616

webkit-patch didn't update the bug for some reason.