Bug 92163

Summary: flexitems can overflow the flexbox due to rounding
Product: WebKit Reporter: Tony Chang <tony>
Component: Layout and RenderingAssignee: Tony Chang <tony>
Status: RESOLVED FIXED    
Severity: Normal CC: bweinstein, eae, eric, leviw, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
URL: http://plexode.com/eval3/#ht=%3Cdiv%20style%3D%22padding%3A%2010px%3B%20background-color%3A%20black%22%3E%0A%0A%3Cdiv%20style%3D%22display%3A-webkit-flex%3B%20width%3A%20101px%3B%20background-color%3A%20grey%22%3E%0A%20%20%3Cdiv%20id%3Da%20style%3D%22-webkit-flex%3A%201%3B%20background-color%3A%20blue%3B%20height%3A%2030px%22%3Ehello%3C%2Fdiv%3E%0A%20%20%3Cdiv%20id%3Db%20style%3D%22-webkit-flex%3A%201%3B%20background-color%3A%20yellow%3B%20-webkit-align-self%3Aflex-start%22%3Eworld%3C%2Fdiv%3E%0A%3C%2Fdiv%3E%0A%0A%3C%2Fdiv%3E%0A&jt=getComputedStyle(document.getElementById('a')).width%20%2B%20%22%2C%22%20%2B%20getComputedStyle(document.getElementById('b')).width%0A
Bug Depends on: 92352    
Bug Blocks: 62048    
Attachments:
Description Flags
proof of concept patch
none
Patch none

Description Tony Chang 2012-07-24 14:35:39 PDT
Flexbox items don't seem to be getting subpixel positions when rounding.  See the URL for an example.  It's a flexbox that is 101 pixels wide and has 2 children, each should be the same size (50.5px each).  Instead, they are both 51px.

For reference, in RenderFlexibleBox.cpp, we use setOverrideLogicalContent{Width,Height}() to set the width/height of children after we have computed the desired value.  We use setLocation() to position each child.  This happens in RenderFlexibleBox::layoutAndPlaceChildren.
Comment 1 Emil A Eklund 2012-07-24 15:12:09 PDT
It would be interesting to know where the rounding happens as both values seems to be rounded independently of each other.

Looking into it quickly it seems like the values are rounded before being passed into the layoutAndPlaceChildren method in the childSizes vector.
Comment 2 Emil A Eklund 2012-07-24 15:14:37 PDT
The problem might be the following code in RenderFlexibleBox::resolveFlexibleLengths:

            if (availableFreeSpace > 0 && totalFlexGrow > 0 && flexSign == PositiveFlexibility && isfinite(totalFlexGrow))
                childSize += static_cast<int>(lroundf(availableFreeSpace * child->style()->flexGrow() / totalFlexGrow));
            else if (availableFreeSpace < 0 && totalWeightedFlexShrink > 0 && flexSign == NegativeFlexibility && isfinite(totalWeightedFlexShrink))
                childSize += static_cast<int>(lroundf(availableFreeSpace * child->style()->flexShrink() * preferredChildSize / totalWeightedFlexShrink));
Comment 3 Tony Chang 2012-07-24 15:18:29 PDT
(In reply to comment #2)
> The problem might be the following code in RenderFlexibleBox::resolveFlexibleLengths:
> 
>             if (availableFreeSpace > 0 && totalFlexGrow > 0 && flexSign == PositiveFlexibility && isfinite(totalFlexGrow))
>                 childSize += static_cast<int>(lroundf(availableFreeSpace * child->style()->flexGrow() / totalFlexGrow));
>             else if (availableFreeSpace < 0 && totalWeightedFlexShrink > 0 && flexSign == NegativeFlexibility && isfinite(totalWeightedFlexShrink))
>                 childSize += static_cast<int>(lroundf(availableFreeSpace * child->style()->flexShrink() * preferredChildSize / totalWeightedFlexShrink));

That seems likely.  I'll see if I can track down the history of the lroundf usage.
Comment 4 Emil A Eklund 2012-07-24 15:20:11 PDT
turns out it is not quite that easy. Getting rid of that rounding makes both boxes return 50.5 and prevents the yellow box from being painted outside the container but it does cause the blue and yellow boxes to overlap by one pixel.
Comment 5 Tony Chang 2012-07-24 15:28:39 PDT
The lroundf is from the initial commit.  It doesn't seem necessary now, but we'll have to do something more to prevent the overlapping content.
Comment 6 Emil A Eklund 2012-07-24 15:31:09 PDT
Changing childLocation on line 1022 from IntPoint to LayoutPoint, in addition to getting rid of the rounding, seems to fix the issue. Might be other edge cases though.
Comment 7 Emil A Eklund 2012-07-24 15:33:18 PDT
Created attachment 154156 [details]
proof of concept patch
Comment 8 Tony Chang 2012-07-24 15:35:28 PDT
(In reply to comment #6)
> Changing childLocation on line 1022 from IntPoint to LayoutPoint, in addition to getting rid of the rounding, seems to fix the issue. Might be other edge cases though.

Magical!  When I make both those changes, all the flexbox tests pass for me.  I'll add a layout test for this and upload as a new patch.  Thanks for the help!
Comment 9 Emil A Eklund 2012-07-24 15:37:05 PDT
(In reply to comment #8)
> (In reply to comment #6)
> > Changing childLocation on line 1022 from IntPoint to LayoutPoint, in addition to getting rid of the rounding, seems to fix the issue. Might be other edge cases though.
> 
> Magical!  When I make both those changes, all the flexbox tests pass for me.  I'll add a layout test for this and upload as a new patch.  Thanks for the help!

Nice! Glad I was able to help.
Comment 10 Tony Chang 2012-07-25 16:45:24 PDT
Created attachment 154482 [details]
Patch
Comment 11 Levi Weintraub 2012-07-25 18:17:16 PDT
Comment on attachment 154482 [details]
Patch

LGTM
Comment 12 WebKit Review Bot 2012-07-25 19:25:10 PDT
Comment on attachment 154482 [details]
Patch

Clearing flags on attachment: 154482

Committed r123696: <http://trac.webkit.org/changeset/123696>
Comment 13 WebKit Review Bot 2012-07-25 19:25:13 PDT
All reviewed patches have been landed.  Closing bug.
Comment 15 Tony Chang 2012-07-26 11:07:58 PDT
Tracking test failures in bug 92352.