WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
RESOLVED FIXED
92163
flexitems can overflow the flexbox due to rounding
https://bugs.webkit.org/show_bug.cgi?id=92163
Summary
flexitems can overflow the flexbox due to rounding
Tony Chang
Reported
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.
Attachments
proof of concept patch
(1.85 KB, patch)
2012-07-24 15:33 PDT
,
Emil A Eklund
no flags
Details
Formatted Diff
Diff
Patch
(7.84 KB, patch)
2012-07-25 16:45 PDT
,
Tony Chang
no flags
Details
Formatted Diff
Diff
Show Obsolete
(1)
View All
Add attachment
proposed patch, testcase, etc.
Emil A Eklund
Comment 1
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.
Emil A Eklund
Comment 2
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));
Tony Chang
Comment 3
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.
Emil A Eklund
Comment 4
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.
Tony Chang
Comment 5
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.
Emil A Eklund
Comment 6
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.
Emil A Eklund
Comment 7
2012-07-24 15:33:18 PDT
Created
attachment 154156
[details]
proof of concept patch
Tony Chang
Comment 8
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!
Emil A Eklund
Comment 9
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.
Tony Chang
Comment 10
2012-07-25 16:45:24 PDT
Created
attachment 154482
[details]
Patch
Levi Weintraub
Comment 11
2012-07-25 18:17:16 PDT
Comment on
attachment 154482
[details]
Patch LGTM
WebKit Review Bot
Comment 12
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
>
WebKit Review Bot
Comment 13
2012-07-25 19:25:13 PDT
All reviewed patches have been landed. Closing bug.
Brian Weinstein
Comment 14
2012-07-26 10:46:05 PDT
It looks like this patch caused a couple test failures:
http://build.webkit.org/results/Apple%20Lion%20Debug%20WK1%20(Tests)/r123756%20(1286)/results.html
http://build.webkit.org/results/Apple%20Lion%20Debug%20WK1%20(Tests)/r123756%20(1286)/css3/flexbox/flex-algorithm-min-max-pretty-diff.html
and
http://build.webkit.org/results/Apple%20Lion%20Debug%20WK1%20(Tests)/r123756%20(1286)/css3/flexbox/flex-rounding-pretty-diff.html
Tony Chang
Comment 15
2012-07-26 11:07:58 PDT
Tracking test failures in
bug 92352
.
Note
You need to
log in
before you can comment on or make changes to this bug.
Top of Page
Format For Printing
XML
Clone This Bug