[webkit-reviews] review granted: [Bug 89819] Unexpected element sizes when mixing inline-table with box-sizing : [Attachment 149436] Proposed patch

[bugzilla-daemon at webkit.org]

Julien Chaffraix <jchaffraix at webkit.org> has granted Kulanthaivel Palanichamy
<kulanthaivel at codeaurora.org>'s request for review:
Bug 89819: Unexpected element sizes when mixing inline-table with box-sizing
https://bugs.webkit.org/show_bug.cgi?id=89819

Attachment 149436: Proposed patch
https://bugs.webkit.org/attachment.cgi?id=149436&action=review

------- Additional Comments from Julien Chaffraix <jchaffraix at webkit.org>
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.