WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
RESOLVED FIXED
84167
CSS 2.1 failure: inline-table-001 fails
https://bugs.webkit.org/show_bug.cgi?id=84167
Summary
CSS 2.1 failure: inline-table-001 fails
Robert Hogan
Reported
2012-04-17 10:58:12 PDT
RenderTable needs its own lastLineBoxBaseline() to handle the baseline of text in cells in the first row.
Attachments
Patch
(375.80 KB, patch)
2012-04-23 11:44 PDT
,
Robert Hogan
no flags
Details
Formatted Diff
Diff
Patch
(311.75 KB, patch)
2012-05-01 13:58 PDT
,
Robert Hogan
no flags
Details
Formatted Diff
Diff
Archive of layout-test-results from ec2-cr-linux-03
(6.19 MB, application/zip)
2012-05-01 14:53 PDT
,
WebKit Review Bot
no flags
Details
Patch
(365.54 KB, patch)
2012-05-01 16:51 PDT
,
Robert Hogan
no flags
Details
Formatted Diff
Diff
Patch
(365.42 KB, patch)
2012-05-08 12:42 PDT
,
Robert Hogan
no flags
Details
Formatted Diff
Diff
Archive of layout-test-results from ec2-cr-linux-01
(2.34 MB, application/zip)
2012-05-08 14:22 PDT
,
WebKit Review Bot
no flags
Details
Patch
(383.68 KB, patch)
2012-05-09 12:08 PDT
,
Robert Hogan
no flags
Details
Formatted Diff
Diff
Archive of layout-test-results from ec2-cr-linux-04
(5.88 MB, application/zip)
2012-05-09 13:50 PDT
,
WebKit Review Bot
no flags
Details
Patch
(383.75 KB, patch)
2012-05-09 14:54 PDT
,
Robert Hogan
no flags
Details
Formatted Diff
Diff
Show Obsolete
(5)
View All
Add attachment
proposed patch, testcase, etc.
Robert Hogan
Comment 1
2012-04-23 11:44:15 PDT
Created
attachment 138394
[details]
Patch
Julien Chaffraix
Comment 2
2012-04-24 15:26:45 PDT
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 3
2012-04-30 12:59:12 PDT
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
Comment 4
2012-05-01 13:58:54 PDT
Created
attachment 139670
[details]
Patch
WebKit Review Bot
Comment 5
2012-05-01 14:53:50 PDT
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
Comment 6
2012-05-01 14:53:56 PDT
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
Comment 7
2012-05-01 16:51:05 PDT
Created
attachment 139711
[details]
Patch
Robert Hogan
Comment 8
2012-05-02 10:55:40 PDT
(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 9
2012-05-03 12:32:51 PDT
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
Comment 10
2012-05-08 12:41:21 PDT
(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
Comment 11
2012-05-08 12:42:55 PDT
Created
attachment 140767
[details]
Patch
WebKit Review Bot
Comment 12
2012-05-08 14:22:25 PDT
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
Comment 13
2012-05-08 14:22:32 PDT
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
Comment 14
2012-05-08 14:29:55 PDT
(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)
Comment 15
2012-05-08 14:34:17 PDT
The flaky test dashboard could tell us. Presumably we should skip that accessibility test.
Julien Chaffraix
Comment 16
2012-05-08 15:29:01 PDT
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 17
2012-05-09 11:12:26 PDT
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 18
2012-05-09 11:49:18 PDT
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
Comment 19
2012-05-09 12:08:06 PDT
Created
attachment 140993
[details]
Patch
WebKit Review Bot
Comment 20
2012-05-09 13:50:38 PDT
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
Comment 21
2012-05-09 13:50:53 PDT
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
Comment 22
2012-05-09 14:54:03 PDT
Created
attachment 141019
[details]
Patch
Robert Hogan
Comment 23
2012-05-16 14:06:05 PDT
Committed
r117339
: <
http://trac.webkit.org/changeset/117339
>
Julien Chaffraix
Comment 24
2012-05-16 17:21:33 PDT
Added new baselines for Chromium due to some tests not being in text_expectations.txt in
http://trac.webkit.org/changeset/117367
.
Julien Chaffraix
Comment 25
2012-06-14 16:17:53 PDT
***
Bug 27037
has been marked as a duplicate of this bug. ***
Jessie Berlin
Comment 26
2012-08-30 17:19:19 PDT
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
Comment 27
2012-08-30 17:22:06 PDT
(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
.
Note
You need to
log in
before you can comment on or make changes to this bug.
Top of Page
Format For Printing
XML
Clone This Bug