Description
Tony Chang
2012-07-24 14:35:39 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. 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)); (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. 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. 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. 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. Created attachment 154156 [details]
proof of concept patch
(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! (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. Created attachment 154482 [details]
Patch
Comment on attachment 154482 [details]
Patch
LGTM
Comment on attachment 154482 [details] Patch Clearing flags on attachment: 154482 Committed r123696: <http://trac.webkit.org/changeset/123696> All reviewed patches have been landed. Closing bug. 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 Tracking test failures in bug 92352. |