Bug 78412 - CSS 2.1 failure: fixed-table-layout-006 fails
: CSS 2.1 failure: fixed-table-layout-006 fails
Status: RESOLVED FIXED
: WebKit
CSS
: 528+ (Nightly build)
: Unspecified Unspecified
: P2 Normal
Assigned To:
:
:
:
: 13339 18565 47141
  Show dependency treegraph
 
Reported: 2012-02-11 02:35 PST by
Modified: 2012-05-25 16:43 PST (History)


Attachments
Patch (117.99 KB, patch)
2012-02-11 05:24 PST, Robert Hogan
no flags Review Patch | Details | Formatted Diff | Diff
Patch (119.47 KB, patch)
2012-03-21 13:39 PST, Robert Hogan
jchaffraix: review+
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-02-11 02:35:18 PST
Need to include borders when calculating column width in fixed layout tables
------- Comment #1 From 2012-02-11 05:24:45 PST -------
Created an attachment (id=126630) [details]
Patch
------- Comment #2 From 2012-02-19 22:50:46 PST -------
(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.
------- Comment #3 From 2012-02-20 11:58:43 PST -------
(In reply to comment #2)
> (From update of attachment 126630 [details] [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 From 2012-02-24 14:18:59 PST -------
(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 (!!!).

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 From 2012-03-19 15:30:19 PST -------
(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.

> 
> 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 From 2012-03-21 09:53:57 PST -------
(In reply to comment #5)
> (In reply to comment #4)
> > (From update of attachment 126630 [details] [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 From 2012-03-21 13:39:22 PST -------
Created an attachment (id=133105) [details]
Patch
------- Comment #8 From 2012-03-21 13:42:02 PST -------
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 From 2012-03-21 14:42:31 PST -------
(From update of attachment 133105 [details])
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 From 2012-03-22 12:08:20 PST -------
Committed r111742: <http://trac.webkit.org/changeset/111742>
------- Comment #11 From 2012-03-22 19:57:57 PST -------
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 From 2012-03-22 20:26:18 PST -------
> 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 From 2012-03-22 20:27:17 PST -------
Cool, thanks Julien!
------- Comment #14 From 2012-03-23 12:18:56 PST -------
(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 From 2012-03-30 03:11:39 PST -------
Bug 13339 and bug 18565 are basically duplicates of this one.
------- Comment #16 From 2012-03-30 11:00:36 PST -------
*** Bug 13339 has been marked as a duplicate of this bug. ***
------- Comment #17 From 2012-03-30 11:00:55 PST -------
*** Bug 18565 has been marked as a duplicate of this bug. ***
------- Comment #18 From 2012-04-06 14:11:33 PST -------
Additional chromium mac/win baselines landed in http://trac.webkit.org/changeset/113492 .
------- Comment #19 From 2012-05-25 16:43:14 PST -------
Sigh, this change broke box-sizing: border-box for fixed table layout (actually counting them twice), see bug 87536.