Bug 88741 - Width not properly recalculated with -webkit-box-sizing: border-box
Summary: Width not properly recalculated with -webkit-box-sizing: border-box
Status: RESOLVED CONFIGURATION CHANGED
Alias: None
Product: WebKit
Classification: Unclassified
Component: CSS (show other bugs)
Version: 528+ (Nightly build)
Hardware: Mac OS X 10.7
: P2 Normal
Assignee: Nobody
URL: http://jsfiddle.net/TrevorBurnham/gCAsj/
Keywords:
Depends on:
Blocks:
 
Reported: 2012-06-10 14:48 PDT by Trevor Burnham
Modified: 2022-07-13 10:06 PDT (History)
7 users (show)

See Also:


Attachments
Proposed patch (4.89 KB, patch)
2012-06-21 13:44 PDT, Kulanthaivel Palanichamy
no flags Details | Formatted Diff | Diff
Proposed patch (9.43 KB, patch)
2012-06-26 15:27 PDT, Kulanthaivel Palanichamy
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Trevor Burnham 2012-06-10 14:48:45 PDT
When an element in border-box mode with fixed width has a change in padding-left (e.g. due to a hover state), its child will not expand to fill the space.  Here's a test case: http://jsfiddle.net/TrevorBurnham/gCAsj/ (Note that in Firefox 11, this example works as expected, complete with smooth transition.) The same bug occurs with padding-right: http://jsfiddle.net/TrevorBurnham/fh2qD/ (Again, this works fine in Firefox.) Changes in padding-top and padding-bottom appear to work properly.
Comment 1 Kulanthaivel Palanichamy 2012-06-21 13:44:28 PDT
Created attachment 148879 [details]
Proposed patch

Ideally we should have been able to detect content box size change in RenderBlock::recomputeLogicalWidth(), but 
unfortunately we never keep the old style in RenderObject to compare old border and padding values.
One another possible fix could be compare the old and new styles in RenderBox::styleDidChange() for
border and padding change when box-sizing is border-box, set setChildNeedsLayout(true),
then force relayout on children in RenderBlock::layoutBlockChildren() if normalChildNeedsLayout() is true.
Comment 2 Kulanthaivel Palanichamy 2012-06-26 15:27:27 PDT
Created attachment 149616 [details]
Proposed patch

Not convinced by my previous patch.
Comment 3 Julien Chaffraix 2012-07-09 19:21:36 PDT
Comment on attachment 149616 [details]
Proposed patch

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

> LayoutTests/fast/box-sizing/box-sizing-dynamic-padding-with-relative-width-child.html:18
> +        setTimeout('test2();', 100);

I don't understand the need for your 100ms delays. They shouldn't be needed to get proper results after a layout.

Moreover, they make the test take at least 500ms which is a very long time for a test case.

> Source/WebCore/rendering/RenderBlock.cpp:1381
> +    return oldWidth != logicalWidth() || oldColumnWidth != desiredColumnWidth() || contentBoxDirty();

I feel like the real fix should be here: the logic should be able to handle box-sizing change properly. There will be several ways of handling that (in order of preference):
* introduce a wrapper around LayoutUnit that is aware of the length's type (border vs content) we are manipulating to avoid mistakes (it will require some work but is a good long term change as we could catch dumb mistakes)
* standardize on having logicalWidth / logicalHeight always be the border-box / content-box. In this case, rename logicalWidth to match this decision.
* invalidate our width in styleDidChange to make sure we return 'true' here.

> Source/WebCore/rendering/RenderObject.h:1003
> +        ADD_BOOLEAN_BITFIELD(contentBoxDirty, ContentBoxDirty);

As discussed, I really feel this is a pretty weak use of our last remaining bit. Changing box-sizing is fairly uncommon and I would rather keep this bit for something more useful.
Comment 4 Brent Fulgham 2022-07-13 10:06:33 PDT
Safari, Chrome, and Firefox all agree on rendering for this test case. I don't believe there is any remaining compatibility issue.