Bug 77028 – Specified width CSS tables should not include border and padding as part of that width.

• Status: RESOLVED
• Resolution: FIXED
• Component: Tables

[Max Vujovic]

Created attachment 123973 [details]
Reproduction

In the attached reproduction, the div with display: table (a CSS table) should be 400px wide, which is 50% of its 800px wide parent div. 

In FF and Opera, the CSS table is 400px wide. In WebKit, it is only 200px wide because the CSS table's 200px padding is included in the CSS table's percent width.

[Max Vujovic]

Created attachment 123974 [details]
Screenshot

Adding a screenshot with renderings from WebKit on top, Opera in the middle, and FF on bottom.

[Arpita Bahuguna]

Created attachment 171193 [details]
Patch

[Arpita Bahuguna]

Created attachment 171381 [details]
Patch

[Julien Chaffraix]

Comment on attachment 171381 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=171381&action=review

> Source/WebCore/rendering/RenderTable.cpp:321
> +    if (isCSSTable && (styleLogicalWidth.isFixed() || styleLogicalWidth.isPercent()) && styleLogicalWidth.isPositive()) {

Shouldn't *any* specified width be handled in this way? This will fix the issue for percent but other values still have the issue (e.g. viewport relative length).

We should fix all cases at once by using styleLogicalWidth.isSpecified() instead of just patching our way around.

> LayoutTests/fast/table/css-table-width-with-border-padding.html:43
> +    </div>

This should probably be a check-layout.js (the upside is that it would run faster).

> LayoutTests/fast/table/css-table-width-with-border-padding.html:46
> +</html>

It would be nice to have some checks against box-sizing values as part of this too. That makes sure we don't break it (which I don't think you do).

[Arpita Bahuguna]

Created attachment 172220 [details]
Patch

[Arpita Bahuguna]

(In reply to comment #4)
> (From update of attachment 171381 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=171381&action=review
> 
Thanks for the review Julien. Have uploaded another patch with the changes.

> > Source/WebCore/rendering/RenderTable.cpp:321
> > +    if (isCSSTable && (styleLogicalWidth.isFixed() || styleLogicalWidth.isPercent()) && styleLogicalWidth.isPositive()) {
> 
> Shouldn't *any* specified width be handled in this way? This will fix the issue for percent but other values still have the issue (e.g. viewport relative length).
> 
> We should fix all cases at once by using styleLogicalWidth.isSpecified() instead of just patching our way around.
> 
Yes, the check should indeed be for all specified values. Have made the change.

> > LayoutTests/fast/table/css-table-width-with-border-padding.html:43
> > +    </div>
> 
> This should probably be a check-layout.js (the upside is that it would run faster).
> 
Done.

> > LayoutTests/fast/table/css-table-width-with-border-padding.html:46
> > +</html>
> 
> It would be nice to have some checks against box-sizing values as part of this too. That makes sure we don't break it (which I don't think you do).
Done.

[Arpita Bahuguna]

Created attachment 172538 [details]
Patch

[Arpita Bahuguna]

Patch with modified layout test for verifying viewport relative widths as well.

[Arpita Bahuguna]

Created attachment 172544 [details]
Patch

[WebKit Review Bot]

Comment on attachment 172544 [details]
Patch

Attachment 172544 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/14728860

New failing tests:
fast/table/css-table-width-with-border-padding.html

[Build Bot]

Comment on attachment 172544 [details]
Patch

Attachment 172544 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/14744471

New failing tests:
fast/table/css-table-width-with-border-padding.html

[Arpita Bahuguna]

Created attachment 172747 [details]
Patch

[Arpita Bahuguna]

Comment on attachment 172747 [details]
Patch

Changed the layout test using check-layout.js to a ref test since verifying the behavior with viewport relative width made the test platform specific.

[Arpita Bahuguna]

Created attachment 173029 [details]
Patch

[Arpita Bahuguna]

Comment on attachment 173029 [details]
Patch

DRT does force set the width of our viewport to 800 across all platforms. Thus reverting the layout test to use check-layout.js again.

[Julien Chaffraix]

Comment on attachment 173029 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=173029&action=review

I don't need to see the updated patch.

> Source/WebCore/ChangeLog:12
> +        This was already handled for fixed width CSS tables but not for percent
> +        width tables.

The ChangeLog seems stale as you handle any specified length. Also please rename the bug to cover the extended use case (which includes updating the tests).

> LayoutTests/fast/table/css-table-width-with-border-padding.html:86
> +    <!-- For box-sizing: padding-box (not supported) -->

You should probably mention that the results as checking in are wrong: because we don't support box-sizing: padding-box, the results are 100px longer.

You could also check in the expected result. I don't feel strongly about the 2 options (which are equivalent). Filing a bug or commenting on the box-sizing: padding-box change is important in both cases.

[Arpita Bahuguna]

Updating bug title.

[Arpita Bahuguna]

Created attachment 173225 [details]
Patch

[WebKit Review Bot]

Comment on attachment 173225 [details]
Patch

Clearing flags on attachment: 173225

Committed r134265: <http://trac.webkit.org/changeset/134265>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Nir Eitan]

Hi, I still encounter the exact same issue. Using Safari 6.0.5 (8536.30.1). Maybe something broke this behavior again?