Bug 78412

Summary:

CSS 2.1 failure: fixed-table-layout-006 fails

Product:

WebKit

Reporter:

Robert Hogan <robert>

Component:

CSS

Assignee:

Robert Hogan <robert>

Status:

RESOLVED FIXED

Severity:

Normal

CC:

eae, eric, hyatt, jchaffraix, marcoos+bwo, nicholas, robert, Tam, tony, webkit.review.bot

Priority:

Version:

528+ (Nightly build)

Hardware:

Unspecified

OS:

Unspecified

Bug Depends on:

Bug Blocks:

47141, 13339, 18565

Attachments:

Description	Flags
Patch	none
Patch	jchaffraix: review+

Robert Hogan

Reported 2012-02-11 02:35:18 PST

Need to include borders when calculating column width in fixed layout tables

Attachments
Patch (117.99 KB, patch) 2012-02-11 05:24 PST, Robert Hogan	no flags	Details Formatted Diff Diff
Patch (119.47 KB, patch) 2012-03-21 13:39 PDT, Robert Hogan	jchaffraix: review+	Details Formatted Diff Diff
Show Obsolete (1) View All Add attachment proposed patch, testcase, etc.

Robert Hogan

Comment 1 2012-02-11 05:24:45 PST

Created attachment 126630 [details] Patch

Julien Chaffraix

Comment 2 2012-02-19 22:50:46 PST

Comment on attachment 126630 [details] Patch Looking at the tables/mozilla/bugs/bug2509-expected.png change, it looks like we are regressing with this change: FF includes some padding inside the bottom right cell, which we do before your patch but don't after. I would need to think more to know what is expected here but I would like to know your opinion on that.

Robert Hogan

Comment 3 2012-02-20 11:58:43 PST

(In reply to comment #2) > (From update of attachment 126630 [details]) > Looking at the tables/mozilla/bugs/bug2509-expected.png change, it looks like we are regressing with this change: FF includes some padding inside the bottom right cell, which we do before your patch but don't after. I would need to think more to know what is expected here but I would like to know your opinion on that. If you add : TD {border: solid green 1px; font-family: times new roman; font-size: 16px; } so that FF uses the same font as Chromium's default both render with the same loss of 'padding' in the last cell. <html><head><style> TABLE {table-layout:fixed; border: solid red 1px; width: 400} TD {border: solid green 1px; font-family: times new roman; font-size: 16px; } TD.one {border: solid blue 1px; width: 300} TD.two {border: solid blue 1px;} </style></head><body style="font: Helvetica 10px">  <TABLE><TR> <TD class="one">What's New on WebDeveloper.com</TD> <TD class="two">umm</TD> </TR><TR> <TD>blah blah blah blah blah blah blah blah blah blah blah blah</TD> <TD>blah blah blah blah blah blah blah blah blah blah blah blah</TD> </TR></TABLE> </body></html>

Julien Chaffraix

Comment 4 2012-02-24 14:18:59 PST

Comment on attachment 126630 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=126630&action=review > Source/WebCore/rendering/FixedTableLayout.cpp:160 > + w.setValue(w.value() + cell->borderAndPaddingLogicalWidth()); I don't think this is the right change. On some code path, you may end up accounting for borders and paddings twice (!!!). Doing some archaeology, it looks like you should just revert https://bugs.webkit.org/show_bug.cgi?id=8126 that mentioned that borders and paddings should not go into column's width (if I am reading your CL correctly). This doesn't match CSS 3 anymore: http://www.w3.org/TR/2002/WD-css3-border-20021107/#the-border-shorthands http://www.w3.org/TR/css3-box/#the-padding

Robert Hogan

Comment 5 2012-03-19 15:30:19 PDT

(In reply to comment #4) > (From update of attachment 126630 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=126630&action=review > > > Source/WebCore/rendering/FixedTableLayout.cpp:160 > > + w.setValue(w.value() + cell->borderAndPaddingLogicalWidth()); > > I don't think this is the right change. On some code path, you may end up accounting for borders and paddings twice (!!!). I think it is the right change. If the width of the cell is specified the effective column width needs to be the width of the cell plus the borders and padding. > > Doing some archaeology, it looks like you should just revert https://bugs.webkit.org/show_bug.cgi?id=8126 that mentioned that borders and paddings should not go into column's width (if I am reading your CL correctly). This doesn't match CSS 3 anymore: > http://www.w3.org/TR/2002/WD-css3-border-20021107/#the-border-shorthands > http://www.w3.org/TR/css3-box/#the-padding Bug 8126 is for cases where the <col> width has been specified - that needs to be considered inclusive of border and padding so it's correct to remove them from the returned width. It's also correct to add the border and padding back in (with my patch) when considering the effective column width in a fixed layout table. Some more tests have been added to the suite for section 17.5.2.1 and this patch now passes all of them: http://test.csswg.org/harness/test/CSS21_DEV/section/17.5.2.1/ There are 68 of them in total. If you agree with my comments above should I add them to this patch or separately?

Julien Chaffraix

Comment 6 2012-03-21 09:53:57 PDT

(In reply to comment #5) > (In reply to comment #4) > > (From update of attachment 126630 [details] [details]) > > View in context: https://bugs.webkit.org/attachment.cgi?id=126630&action=review > > > > > Source/WebCore/rendering/FixedTableLayout.cpp:160 > > > + w.setValue(w.value() + cell->borderAndPaddingLogicalWidth()); > > > > I don't think this is the right change. On some code path, you may end up accounting for borders and paddings twice (!!!). > > I think it is the right change. If the width of the cell is specified the effective column width needs to be the width of the cell plus the borders and padding. I still think it is possible to count the borders and paddings twice if we set box-sizing to 'border-box' or 'padding-box'. I don't think this should prevent you from moving though as I am not sure this is handled properly at the moment. > Some more tests have been added to the suite for section 17.5.2.1 and this patch now passes all of them: > > http://test.csswg.org/harness/test/CSS21_DEV/section/17.5.2.1/ > > There are 68 of them in total. If you agree with my comments above should I add them to this patch or separately? Please add the 68 test cases in a follow-up bug.

Robert Hogan

Comment 7 2012-03-21 13:39:22 PDT

Created attachment 133105 [details] Patch

WebKit Review Bot

Comment 8 2012-03-21 13:42:02 PDT

Attachment 133105 [details] did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/css2..." exit_code: 1 LayoutTests/platform/gtk/test_expectations.txt:78: More specific entry on line 78 overrides line 12 http/tests/security/cross-origin-xsl-redirect-BLOCKED.html [test/expectations] [5] LayoutTests/platform/gtk/test_expectations.txt:107: Path does not exist. css3/flexbox/inline-flexbox.html [test/expectations] [5] Total errors found: 2 in 21 files If any of these errors are false positives, please file a bug against check-webkit-style.

Julien Chaffraix

Comment 9 2012-03-21 14:42:31 PDT

Comment on attachment 133105 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=133105&action=review r=me > LayoutTests/css2.1/20110323/fixed-table-layout-013-expected.html:15 > + width: 98px; As commented on IRC, it's neat to have our own -expected.html file but confusing as it doesn't belong to the official test suite :(

Robert Hogan

Comment 10 2012-03-22 12:08:20 PDT

Committed r111742: <http://trac.webkit.org/changeset/111742>

Emil A Eklund

Comment 11 2012-03-22 19:57:57 PDT

A bunch of table tests fails after this change, specifically the following: tables/mozilla/bugs/bug34176.html tables/mozilla/bugs/bug2509.html tables/mozilla/bugs/bug2123.html tables/mozilla_expected_failures/bugs/bug7243.html tables/mozilla_expected_failures/bugs/bug59252.html Is this just a case of them needed to be rebaselined or is it an indication of a real problem?

Julien Chaffraix

Comment 12 2012-03-22 20:26:18 PDT

> Is this just a case of them needed to be rebaselined or is it an indication of a real problem? All the tests you mentioned were rebaselined as part of the change, I would bet those are for different Chromium platforms as test_expectations.txt wasn't touched (my mistake, I should have picked that during the review). You can rebaseline them, if they match the baselines included in the original change.

Emil A Eklund

Comment 13 2012-03-22 20:27:17 PDT

Cool, thanks Julien!

Robert Hogan

Comment 14 2012-03-23 12:18:56 PDT

(In reply to comment #12) > > Is this just a case of them needed to be rebaselined or is it an indication of a real problem? > > All the tests you mentioned were rebaselined as part of the change, I would bet those are for different Chromium platforms as test_expectations.txt wasn't touched It was: http://trac.webkit.org/changeset/111742/trunk/LayoutTests/platform/chromium/test_expectations.txt Turns out I put MAC & WIN to fail the rebaselined tests but rebaselined some shared results, so their failures were not exactly as specified in the expectations file. > (my mistake, I should have picked that during the review). You can rebaseline them, if they match the baselines included in the original change. Thanks for sorting this out while I was offline!

Marek Stepien

Comment 15 2012-03-30 03:11:39 PDT

Bug 13339 and bug 18565 are basically duplicates of this one.

Robert Hogan

Comment 16 2012-03-30 11:00:36 PDT

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

Robert Hogan

Comment 17 2012-03-30 11:00:55 PDT

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

Tony Chang

Comment 18 2012-04-06 14:11:33 PDT

Additional chromium mac/win baselines landed in http://trac.webkit.org/changeset/113492 .

Julien Chaffraix

Comment 19 2012-05-25 16:43:14 PDT

Sigh, this change broke box-sizing: border-box for fixed table layout (actually counting them twice), see bug 87536.

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