Bug 84167 - CSS 2.1 failure: inline-table-001 fails
: CSS 2.1 failure: inline-table-001 fails
Status: RESOLVED FIXED
: WebKit
CSS
: 528+ (Nightly build)
: Unspecified Unspecified
: P2 Normal
Assigned To:
:
:
:
: 85405
  Show dependency treegraph
 
Reported: 2012-04-17 10:58 PST by
Modified: 2012-08-30 17:22 PST (History)


Attachments
Patch (375.80 KB, patch)
2012-04-23 11:44 PST, Robert Hogan
no flags Review Patch | Details | Formatted Diff | Diff
Patch (311.75 KB, patch)
2012-05-01 13:58 PST, Robert Hogan
no flags Review Patch | Details | Formatted Diff | Diff
Archive of layout-test-results from ec2-cr-linux-03 (6.19 MB, application/zip)
2012-05-01 14:53 PST, WebKit Review Bot
no flags Details
Patch (365.54 KB, patch)
2012-05-01 16:51 PST, Robert Hogan
no flags Review Patch | Details | Formatted Diff | Diff
Patch (365.42 KB, patch)
2012-05-08 12:42 PST, Robert Hogan
no flags Review Patch | Details | Formatted Diff | Diff
Archive of layout-test-results from ec2-cr-linux-01 (2.34 MB, application/zip)
2012-05-08 14:22 PST, WebKit Review Bot
no flags Details
Patch (383.68 KB, patch)
2012-05-09 12:08 PST, Robert Hogan
no flags Review Patch | Details | Formatted Diff | Diff
Archive of layout-test-results from ec2-cr-linux-04 (5.88 MB, application/zip)
2012-05-09 13:50 PST, WebKit Review Bot
no flags Details
Patch (383.75 KB, patch)
2012-05-09 14:54 PST, Robert Hogan
no flags Review Patch | Details | Formatted Diff | Diff


Note

You need to log in before you can comment on or make changes to this bug.


Description From 2012-04-17 10:58:12 PST
RenderTable needs its own lastLineBoxBaseline() to handle the baseline of text in cells in the first row.
------- Comment #1 From 2012-04-23 11:44:15 PST -------
Created an attachment (id=138394) [details]
Patch
------- Comment #2 From 2012-04-24 15:26:45 PST -------
(From update of attachment 138394 [details])
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.
------- Comment #3 From 2012-04-30 12:59:12 PST -------
(From update of attachment 138394 [details])
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.
------- Comment #4 From 2012-05-01 13:58:54 PST -------
Created an attachment (id=139670) [details]
Patch
------- Comment #5 From 2012-05-01 14:53:50 PST -------
(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
tables/mozilla/bugs/bug2479-2.html
------- Comment #6 From 2012-05-01 14:53:56 PST -------
Created an attachment (id=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
------- Comment #7 From 2012-05-01 16:51:05 PST -------
Created an attachment (id=139711) [details]
Patch
------- Comment #8 From 2012-05-02 10:55:40 PST -------
(In reply to comment #5)
> (From update of attachment 139670 [details] [details])
> Attachment 139670 [details] [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.
------- Comment #9 From 2012-05-03 12:32:51 PST -------
(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)?

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?
------- Comment #10 From 2012-05-08 12:41:21 PST -------
(In reply to comment #9)
> (From update of attachment 139711 [details] [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.
------- Comment #11 From 2012-05-08 12:42:55 PST -------
Created an attachment (id=140767) [details]
Patch
------- Comment #12 From 2012-05-08 14:22:25 PST -------
(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
------- Comment #13 From 2012-05-08 14:22:32 PST -------
Created an attachment (id=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
------- Comment #14 From 2012-05-08 14:29:55 PST -------
(In reply to comment #12)
> (From update of attachment 140767 [details] [details])
> Attachment 140767 [details] [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.
------- Comment #15 From 2012-05-08 14:34:17 PST -------
The flaky test dashboard could tell us.  Presumably we should skip that accessibility test.
------- Comment #16 From 2012-05-08 15:29:01 PST -------
(From update of attachment 140767 [details])
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).
------- Comment #17 From 2012-05-09 11:12:26 PST -------
(From update of attachment 140767 [details])
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.
------- Comment #18 From 2012-05-09 11:49:18 PST -------
(From update of attachment 140767 [details])
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.
------- Comment #19 From 2012-05-09 12:08:06 PST -------
Created an attachment (id=140993) [details]
Patch
------- Comment #20 From 2012-05-09 13:50:38 PST -------
(From update of attachment 140993 [details])
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
------- Comment #21 From 2012-05-09 13:50:53 PST -------
Created an attachment (id=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
------- Comment #22 From 2012-05-09 14:54:03 PST -------
Created an attachment (id=141019) [details]
Patch
------- Comment #23 From 2012-05-16 14:06:05 PST -------
Committed r117339: <http://trac.webkit.org/changeset/117339>
------- Comment #24 From 2012-05-16 17:21:33 PST -------
Added new baselines for Chromium due to some tests not being in text_expectations.txt in http://trac.webkit.org/changeset/117367.
------- Comment #25 From 2012-06-14 16:17:53 PST -------
*** Bug 27037 has been marked as a duplicate of this bug. ***
------- Comment #26 From 2012-08-30 17:19:19 PST -------
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.
------- Comment #27 From 2012-08-30 17:22:06 PST -------
(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.