Bug 78412 - CSS 2.1 failure: fixed-table-layout-006 fails
: CSS 2.1 failure: fixed-table-layout-006 fails
Status: RESOLVED FIXED
Product: WebKit
Classification: Unclassified
Component: CSS
: 528+ (Nightly build)
: Unspecified Unspecified
: P2 Normal
Assigned To: Robert Hogan
:
Depends on:
Blocks: 47141 13339 18565
  Show dependency treegraph
 
Reported: 2012-02-11 02:35 PST by Robert Hogan
Modified: 2012-05-25 16:43 PDT (History)
10 users (show)

See Also:


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

Note You need to log in before you can comment on or make changes to this bug.
Description Robert Hogan 2012-02-11 02:35:18 PST
Need to include borders when calculating column width in fixed layout tables
Comment 1 Robert Hogan 2012-02-11 05:24:45 PST
Created attachment 126630 [details]
Patch
Comment 2 Julien Chaffraix 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.
Comment 3 Robert Hogan 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">

<!-- 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 4 Julien Chaffraix 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
Comment 5 Robert Hogan 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?
Comment 6 Julien Chaffraix 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.
Comment 7 Robert Hogan 2012-03-21 13:39:22 PDT
Created attachment 133105 [details]
Patch
Comment 8 WebKit Review Bot 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.
Comment 9 Julien Chaffraix 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 :(
Comment 10 Robert Hogan 2012-03-22 12:08:20 PDT
Committed r111742: <http://trac.webkit.org/changeset/111742>
Comment 11 Emil A Eklund 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?
Comment 12 Julien Chaffraix 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.
Comment 13 Emil A Eklund 2012-03-22 20:27:17 PDT
Cool, thanks Julien!
Comment 14 Robert Hogan 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!
Comment 15 Marek Stepien 2012-03-30 03:11:39 PDT
Bug 13339 and bug 18565 are basically duplicates of this one.
Comment 16 Robert Hogan 2012-03-30 11:00:36 PDT
*** Bug 13339 has been marked as a duplicate of this bug. ***
Comment 17 Robert Hogan 2012-03-30 11:00:55 PDT
*** Bug 18565 has been marked as a duplicate of this bug. ***
Comment 18 Tony Chang 2012-04-06 14:11:33 PDT
Additional chromium mac/win baselines landed in http://trac.webkit.org/changeset/113492 .
Comment 19 Julien Chaffraix 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.