Bug 84167 – CSS 2.1 failure: inline-table-001 fails

• Component: CSS
• Status: RESOLVED FIXED

[Robert Hogan]

RenderTable needs its own lastLineBoxBaseline() to handle the baseline of text in cells in the first row.

[Robert Hogan]

Created attachment 138394 [details]
Patch

[Julien Chaffraix]

Comment on attachment 138394 [details]
Patch

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

> Source/WebCore/rendering/RenderTable.cpp:1221
> +static LayoutUnit getLineBoxBaseline(const RenderTable* table, bool firstLineBox)

Please no magical boolean. It makes the code less readable to read, especially since firstLineBox == false means that you return the *last* line box baseline which is not really obvious if you are looking at the caller.

> Source/WebCore/rendering/RenderTable.cpp:1238
> +    // The 'first' linebox baseline in a table in the absence of any text in the first section
> +    // is the top of the table.
> +    if (firstLineBox)
> +        return topNonEmptySection->logicalTop();

Shouldn't we query somehow the first row of your section instead of returning the section's "baseline" here?

Here is the CSS 2.1 part that prompted this question: "The baseline of an ’inline-table’ is the baseline of the first row of the table." (which is also what RenderTableSection::firstLineBoxBaseline does btw).

> Source/WebCore/rendering/RenderTable.h:241
> +    virtual LayoutUnit lastLineBoxBaseline() const;

OVERRIDE please.

> LayoutTests/ChangeLog:64
> +        * platform/chromium-linux/fast/inline-block/001-expected.png:
> +            Progression, the boxes now line up in the same way as FF and Opera.

I am wondering if the text in capital letters at the beginning of the page shouldn't be removed as we now matches other browsers. Also it may be the best time to do that as we require all ports to rebaseline both the text and the image.

[Robert Hogan]

Comment on attachment 138394 [details]
Patch

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

>> Source/WebCore/rendering/RenderTable.cpp:1238
>> +        return topNonEmptySection->logicalTop();
> 
> Shouldn't we query somehow the first row of your section instead of returning the section's "baseline" here?
> 
> Here is the CSS 2.1 part that prompted this question: "The baseline of an ’inline-table’ is the baseline of the first row of the table." (which is also what RenderTableSection::firstLineBoxBaseline does btw).

We do check that, a couple of lines up. However if it returns nothing or -1 it means there's no text there so the top of the section is used instead.

[Robert Hogan]

Created attachment 139670 [details]
Patch

[WebKit Review Bot]

Comment on attachment 139670 [details]
Patch

Attachment 139670 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/12587654

New failing tests:
fast/invalid/residual-style.html
tables/mozilla/bugs/bug2479-2.html

[WebKit Review Bot]

Created attachment 139688 [details]
Archive of layout-test-results from ec2-cr-linux-03

The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: ec2-cr-linux-03  Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'>  Platform: Linux-2.6.35-28-virtual-x86_64-with-Ubuntu-10.10-maverick

[Robert Hogan]

Created attachment 139711 [details]
Patch

[Robert Hogan]

(In reply to comment #5)
> (From update of attachment 139670 [details])
> Attachment 139670 [details] did not pass chromium-ews (chromium-xvfb):
> Output: http://queues.webkit.org/results/12587654
> 
> New failing tests:
> fast/invalid/residual-style.html

It turns out the change in results to this test in my original patch was a regression not a progression. The most recent patch leaves the results for the test unaltered.

> tables/mozilla/bugs/bug2479-2.html

Forgot to rebaseline this - it merge conflicted when I updated my tree.

[Julien Chaffraix]

Comment on attachment 139711 [details]
Patch

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

> Source/WebCore/rendering/RenderTableSection.cpp:958
> +        if (cell && cell->contentLogicalHeight())

What would happen if you have some visual overflow here? Some outline (not counted against visual overflow nor height)?

For the records, auto table layout has a different view on empty cells:

RenderTableSection::CellStruct current = section->cellAt(i, effCol);
RenderTableCell* cell = current.primaryCell();

bool cellHasContent = cell && !current.inColSpan && (cell->firstChild() || cell->style()->hasBorder() || cell->style()->hasPadding());

(I don't know if this definition makes more sense, just sayin')

> LayoutTests/ChangeLog:81
> +            Text rebaselines.

How about the other platforms? If you land the change as-is, you will be breaking most ports due to different baselines unfortunately.

> LayoutTests/fast/css/empty-cell-baseline-expected.html:33
> +                background-color:green;

I am kind of dubious of this expected results: it is more or less the exact equivalent of the test (except for that line). It really looks like we don't see the cell color or we would happily fail. Can't we just make the same output with a different method (like divs that are not tables) or just getting rid of the cell altogether?

> LayoutTests/fast/css/empty-cell-baseline.html:1
> +<!DOCTYPE html PUBLIC "-//W3C//DTD HTML 4.01//EN" "http://www.w3.org/TR/html4/strict.dtd">

HTML5 DOCTYPE here?

> LayoutTests/fast/css/empty-cell-baseline.html:38
> +        <p style="font: 1em Ahem;">Test</p>

Could the test indicate what it expects in the output instead of test instead of a <meta> tag? The CSS test suite cases do that but not yours.

It should be possible to use a readable font (instead of Ahem) as I would expect the same fall-back mechanism between the reference and the test.

> LayoutTests/fast/css/empty-cell-baseline.html:41
> +        <div id="table">
> +            <div id="row">
> +                <div id="cell"></div>

I guess using table, tr, td wouldn't work here as those were not CSS tables but HTML tables?

[Robert Hogan]

(In reply to comment #9)
> (From update of attachment 139711 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=139711&action=review
> 
> > Source/WebCore/rendering/RenderTableSection.cpp:958
> > +        if (cell && cell->contentLogicalHeight())
> 
> What would happen if you have some visual overflow here? Some outline (not counted against visual overflow nor height)?

Per our discussion on IRC I've left this as is - outlines have no impact and visual overflow isn't possible on a cell with no content.

> > LayoutTests/ChangeLog:81
> > +            Text rebaselines.
> 
> How about the other platforms? If you land the change as-is, you will be breaking most ports due to different baselines unfortunately.

I tend to suppress the rebaselines at landing - rebasing a patch with changes to the expectations files is no fun at all.

> 
> > LayoutTests/fast/css/empty-cell-baseline-expected.html:33
> > +                background-color:green;
> 

Fixed this reference result to be more concise.

> 
> > LayoutTests/fast/css/empty-cell-baseline.html:1
> > +<!DOCTYPE html PUBLIC "-//W3C//DTD HTML 4.01//EN" "http://www.w3.org/TR/html4/strict.dtd">
> 
> HTML5 DOCTYPE here?

I've stuck with this - as it's the one used in the css tests.

> 
> > LayoutTests/fast/css/empty-cell-baseline.html:38
> > +        <p style="font: 1em Ahem;">Test</p>
> 
> Could the test indicate what it expects in the output instead of test instead of a <meta> tag? The CSS test suite cases do that but not yours.

I've added an expeectation to the body of the test. 

> 
> It should be possible to use a readable font (instead of Ahem) as I would expect the same fall-back mechanism between the reference and the test.

But I need to stick with the ahem font and as I do not get consistent positioning without it.

> 
> > LayoutTests/fast/css/empty-cell-baseline.html:41
> > +        <div id="table">
> > +            <div id="row">
> > +                <div id="cell"></div>
> 
> I guess using table, tr, td wouldn't work here as those were not CSS tables but HTML tables?

This is the style used in the css suites - it's a copy/paste artifact more than anything else. I've left it as is.

[Robert Hogan]

Created attachment 140767 [details]
Patch

[WebKit Review Bot]

Comment on attachment 140767 [details]
Patch

Attachment 140767 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/12648396

New failing tests:
accessibility/aria-disabled.html

[WebKit Review Bot]

Created attachment 140784 [details]
Archive of layout-test-results from ec2-cr-linux-01

The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: ec2-cr-linux-01  Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'>  Platform: Linux-2.6.35-28-virtual-x86_64-with-Ubuntu-10.10-maverick

[Robert Hogan]

(In reply to comment #12)
> (From update of attachment 140767 [details])
> Attachment 140767 [details] did not pass chromium-ews (chromium-xvfb):
> Output: http://queues.webkit.org/results/12648396
> 
> New failing tests:
> accessibility/aria-disabled.html

This is a false positive - can't reproduce it here.

[Eric Seidel (no email)]

The flaky test dashboard could tell us.  Presumably we should skip that accessibility test.

[Julien Chaffraix]

Comment on attachment 140767 [details]
Patch

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

> Source/WebCore/rendering/RenderTable.cpp:1234
> +    // The 'first' linebox baseline in a table in the absence of any text in the first section

Adding up lines above would make the comment stand out more.

> Source/WebCore/rendering/RenderTable.cpp:1238
> +    // The 'last' linebox baseline in a table is the baseline of text in the first

Ditto.

> LayoutTests/ChangeLog:82
> +

The ChangeLog doesn't mention fast/inline-block/001.html.

> LayoutTests/fast/inline-block/001.html:-5
> -THIS TEST'S RESULTS ARE NOT CORRECT, SINCE INLINE TABLES ARE NOT
> -BASELINE-ALIGNING PROPERLY, AND WHEN THEY DO, THE FIRST ROW WILL BE
> -USED.

If we don't expect this test to ever pass, I wonder if we just shouldn't remove it (assuming it is correctly super-seeded by the new CSS 2.1 tests). As it is now, the output is confusing as the test says "This text should be on the same line." (and it's not).

Looking again at the test itself, it doesn't strike me as obvious why we don't expect it to pass as I would expect the inline-table to share the same baseline (all their structure being equal).

[Robert Hogan]

Comment on attachment 140767 [details]
Patch

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

> LayoutTests/ChangeLog:67
> +        * platform/chromium-linux/fast/inline-block/001-expected.png:
> +            Progression, the boxes now line up in the same way as FF and Opera.

Here it is! :)

>> LayoutTests/fast/inline-block/001.html:-5
>> -USED.
> 
> If we don't expect this test to ever pass, I wonder if we just shouldn't remove it (assuming it is correctly super-seeded by the new CSS 2.1 tests). As it is now, the output is confusing as the test says "This text should be on the same line." (and it's not).
> 
> Looking again at the test itself, it doesn't strike me as obvious why we don't expect it to pass as I would expect the inline-table to share the same baseline (all their structure being equal).

I think the answer to this is in the spec (very bottom of http://www.w3.org/TR/CSS21/visudet.html#leading):

"The baseline of an 'inline-table' is the baseline of the first row of the table.

The baseline of an 'inline-block' is the baseline of its last line box in the normal flow, unless it has either no in-flow line boxes or if its 'overflow' property has a computed value other than 'visible', in which case the baseline is the bottom margin edge."

The result is that for an adjacent inline-block and inline-table,  the first row of the inline table will align roughly with the last line of the inline block. This much makes sense to me - though I'm not entirely sure why the first line/row of the table and the last line of the block do no line up exactly. I don't think that's a bug (FF and Opera render it the same) - just a gap in my understanding.

[Julien Chaffraix]

Comment on attachment 140767 [details]
Patch

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

r+ assuming you update the confusing test, add the missing expectations and get a green EWS before landing :)

Please consider making the space changes too.

>>> LayoutTests/fast/inline-block/001.html:-5
>>> -USED.
>> 
>> If we don't expect this test to ever pass, I wonder if we just shouldn't remove it (assuming it is correctly super-seeded by the new CSS 2.1 tests). As it is now, the output is confusing as the test says "This text should be on the same line." (and it's not).
>> 
>> Looking again at the test itself, it doesn't strike me as obvious why we don't expect it to pass as I would expect the inline-table to share the same baseline (all their structure being equal).
> 
> I think the answer to this is in the spec (very bottom of http://www.w3.org/TR/CSS21/visudet.html#leading):
> 
> "The baseline of an 'inline-table' is the baseline of the first row of the table.
> 
> The baseline of an 'inline-block' is the baseline of its last line box in the normal flow, unless it has either no in-flow line boxes or if its 'overflow' property has a computed value other than 'visible', in which case the baseline is the bottom margin edge."
> 
> The result is that for an adjacent inline-block and inline-table,  the first row of the inline table will align roughly with the last line of the inline block. This much makes sense to me - though I'm not entirely sure why the first line/row of the table and the last line of the block do no line up exactly. I don't think that's a bug (FF and Opera render it the same) - just a gap in my understanding.

As discussed, the explanation makes sense. The output is confusing so it would need to be updated to explain that the author's assumptions are incorrect.

[Robert Hogan]

Created attachment 140993 [details]
Patch

[WebKit Review Bot]

Comment on attachment 140993 [details]
Patch

Attachment 140993 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/12654703

New failing tests:
fast/css/empty-cell-baseline.html

[WebKit Review Bot]

Created attachment 141010 [details]
Archive of layout-test-results from ec2-cr-linux-04

The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: ec2-cr-linux-04  Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'>  Platform: Linux-2.6.35-28-virtual-x86_64-with-Ubuntu-10.10-maverick

[Robert Hogan]

Created attachment 141019 [details]
Patch

[Robert Hogan]

Committed r117339: <http://trac.webkit.org/changeset/117339>

[Julien Chaffraix]

Added new baselines for Chromium due to some tests not being in text_expectations.txt in http://trac.webkit.org/changeset/117367.

[Julien Chaffraix]

*** Bug 27037 has been marked as a duplicate of this bug. ***

[Jessie Berlin]

These fast/css-generated-content/nested-tables-with-before-after-content-crash.html has started passing on Lion:

http://build.webkit.org/results/Apple%20Lion%20Debug%20WK1%20(Tests)/r127191%20(2391)/results.html

I am not sure exactly when it started passing, because the revision it started passing when expected to be failing at was when Dirk Pranke made the mac-lion TestExpectations file start working (http://trac.webkit.org/changeset/127190), but I am going to remove the failing expectation.

[Jessie Berlin]

(In reply to comment #26)
> These fast/css-generated-content/nested-tables-with-before-after-content-crash.html has started passing on Lion:
> 
> http://build.webkit.org/results/Apple%20Lion%20Debug%20WK1%20(Tests)/r127191%20(2391)/results.html
> 
> I am not sure exactly when it started passing, because the revision it started passing when expected to be failing at was when Dirk Pranke made the mac-lion TestExpectations file start working (http://trac.webkit.org/changeset/127190), but I am going to remove the failing expectation.

Removed in http://trac.webkit.org/changeset/127216.