Summary: | Can avoid a second layout of flex-items in some cases | ||||||
---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Ojan Vafai <ojan> | ||||
Component: | Layout and Rendering | Assignee: | Nobody <webkit-unassigned> | ||||
Status: | NEW --- | ||||||
Severity: | Normal | CC: | cgarcia, eric, hyatt, jchaffraix, tony | ||||
Priority: | P2 | ||||||
Version: | 528+ (Nightly build) | ||||||
Hardware: | Unspecified | ||||||
OS: | Unspecified | ||||||
Bug Depends on: | |||||||
Bug Blocks: | 62048 | ||||||
Attachments: |
|
Description
Ojan Vafai
2012-05-30 15:57:38 PDT
Created attachment 174091 [details]
Patch
I'm getting a failure in 3 tests that I'm not sure why.
css3/flexbox/inline-flex-crash.html
css3/flexbox/inline-flex-crash2.html
The output is different in both cases, but it doesn't crash.
css3/flexbox/auto-margins.html
The output is correct, but the first time it's called it fails to get the margin values, although the inspector shows the right ones and simply reloading the page fixes the test.
All other css3 flexbox tests pass.
Comment on attachment 174091 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=174091&action=review > Source/WebCore/rendering/RenderFlexibleBox.cpp:1090 > + LayoutUnit childCurrentSize = isColumnFlow() ? child->logicalHeight() : child->logicalWidth(); You want mainAxisExtentForChild(). You don't want the child's logical height since it can have an orthogonal writing mode to the flexbox. Comment on attachment 174091 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=174091&action=review This is actually a separate optimization than bug 87905. That bug is about when we *do* change the height, there are many cases where we don't need to do a layout (i.e. if there are no descendants or css properties that depend on the height). This is still a good optimization though. It just needs it's own bug. > Source/WebCore/rendering/RenderFlexibleBox.cpp:1094 > + if (child->size().isZero() || childPreferredSize != childCurrentSize) { > + child->setChildNeedsLayout(true, MarkOnlyThis); > + child->layoutIfNeeded(); > + } I don't think we need the isZero check. Instead, we should always call child->layoutIfNeeded() here (i.e. move it out of the if-statement), but only setChildNeedsLayout if childPreferredSize != childCurrentSize. Thank you guys for the review! I've just opened a new bug with a patch addressing your comments, see: https://bugs.webkit.org/show_bug.cgi?id=102352 css3/flexbox/inline-flex-crash.html and css3/flexbox/inline-flex-crash2.html don't fail anymore with the new patch. |