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: | P2 | ||||||||
Version: | 528+ (Nightly build) | ||||||||
Hardware: | Unspecified | ||||||||
OS: | Unspecified | ||||||||
Bug Depends on: | |||||||||
Bug Blocks: | 47141, 13339, 18565 | ||||||||
Attachments: |
|
Description
Robert Hogan
2012-02-11 02:35:18 PST
Created attachment 126630 [details]
Patch
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.
(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"> <!-- this table should derive the width of the second column from what's left over from the table width after subtracting the first column width --> <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> 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 (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? (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. Created attachment 133105 [details]
Patch
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.
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 :( Committed r111742: <http://trac.webkit.org/changeset/111742> 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? > 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.
Cool, thanks Julien! (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! *** Bug 13339 has been marked as a duplicate of this bug. *** *** Bug 18565 has been marked as a duplicate of this bug. *** Additional chromium mac/win baselines landed in http://trac.webkit.org/changeset/113492 . Sigh, this change broke box-sizing: border-box for fixed table layout (actually counting them twice), see bug 87536. |