Bug 89819 – Unexpected element sizes when mixing inline-table with box-sizing

• Status: RESOLVED
• Resolution: FIXED
• Component: CSS

[James Holderness]

Created attachment 149183 [details]
test case

Steps to reproduce:
1. Create a document with four paragraph elements
2. Set the width and height of each of them to 100px.
3. Add a 5px border to each of them.
4. Set the box-sizing on the first two to border-box and the second two to content-box.
5. Set the display style alternately to inline-table and border-box, so you have a mix of every combination of box-sizing and display style.

I would have expected the two paragraphs with border-box box-sizing to have a size of 100x100 and the two with context-box box-sizing to have a size of 110x110. This is the behaviour I see on Firefox (13.0.1).

However on Safari 5.1.7 (webkit version 534.57.2) the inline-table paragraph with content-box box-sizing has a size of 100x100 (as if inline tables are always assumed to use border-box box-sizing).

On Chrome 19.0.1084.56 (webkit version 536.5) the inline-table paragraph with border-box box-sizing has a size of 110x110 (as if inline tables are always assumed to use content-box box-sizing).

Webkit nightly (537.1+) behaves the same as Chrome.

Neither of the two webkit implementations seem correct to me. Is this a half fixed, half regressed bug? Or is this particular combination of styles open to interpretation and thus all implementations are correct?

I don't believe this to be platform or OS specific (I've seen this on Windows, OSX and iOS).

[Kulanthaivel Palanichamy]

Created attachment 149436 [details]
Proposed patch

[Julien Chaffraix]

Comment on attachment 149436 [details]
Proposed patch

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

> LayoutTests/fast/box-sizing/css-table-with-box-sizing-expected.txt:8
> +PASS successfullyParsed is true
> +
> +TEST COMPLETE

The order is wrong, this should be at the end... That's because we dump them before your code execute: you can either make your test window.jsTestIsAsync = true (bad way) or just moving your <script> at the end of your HTML and dumping everything directly without using a 'load' event.

> LayoutTests/fast/box-sizing/css-table-with-box-sizing-expected.txt:29
> +content-box 
> +120x120
> +css-table
> +content-box

Nit: If we could remove those from the output, it would be nice (it's just a matter of wrapping your paragraphs and removing them before dumping the output).

> LayoutTests/fast/box-sizing/css-table-with-box-sizing.html:8
> +    if (window.testRunner)
> +        window.testRunner.waitUntilDone();

No need for waitUntilDone as we dump after the 'load' handler.

> LayoutTests/fast/box-sizing/css-table-with-box-sizing.html:35
> +      -webkit-box-sizing:border-box;

-webkit-box-sizing is mapped to box-sizing, I would rather *not* have the prefixed (in this case legacy) value take over. Let's just remove those lines.

> Source/WebCore/rendering/RenderTable.cpp:277
> +        if (style()->boxSizing() == CONTENT_BOX)
> +            borders = borderStart() + borderEnd() + (collapseBorders() ? ZERO_LAYOUT_UNIT : paddingStart() + paddingEnd());

This really makes me wonder if we shouldn't just override the padding functions to just check for collapsing borders and return ZERO_LAYOUT_UNIT. Here we could just remove this code and just using computeBorderBoxLogicalWidth.

> Source/WebCore/rendering/RenderTable.cpp:402
> +        if ((node() && node()->hasTagName(tableTag)) || style()->boxSizing() == BORDER_BOX)

As discussed, this would need a FIXME. We cannot apply box-sizing: content-box on <table> which other browsers allows.

[Kulanthaivel Palanichamy]

Created attachment 149569 [details]
Proposed patch

Changes based on review comments.

[WebKit Review Bot]

Comment on attachment 149569 [details]
Proposed patch

Rejecting attachment 149569 [details] from commit-queue.

Failed to run "['/mnt/git/webkit-commit-queue/Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '-..." exit_code: 1

ERROR: /mnt/git/webkit-commit-queue/LayoutTests/ChangeLog neither lists a valid reviewer nor contains the string "Unreviewed" or "Rubber stamp" (case insensitive).

Full output: http://queues.webkit.org/results/13094593

[Kulanthaivel Palanichamy]

Created attachment 149599 [details]
Proposed patch

Changed 'Reviewed By'

[Julien Chaffraix]

Comment on attachment 149599 [details]
Proposed patch

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

> LayoutTests/fast/box-sizing/css-table-with-box-sizing.html:53
> +	startTest();

Note that this function is unneeded.

> Source/WebCore/rendering/RenderTable.cpp:402
> +        // FIXME: We cannot apply box-sizing: content-box on <table> which other browsers allows.

typo: other browsers allow (no 's')

[Kulanthaivel Palanichamy]

Created attachment 149619 [details]
Proposed patch

[Julien Chaffraix]

Comment on attachment 149619 [details]
Proposed patch

Terrific thanks!

[WebKit Review Bot]

Comment on attachment 149619 [details]
Proposed patch

Clearing flags on attachment: 149619

Committed r121309: <http://trac.webkit.org/changeset/121309>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.