Bug 87905

Summary: Can avoid a second layout of flex-items in some cases
Product: WebKit Reporter: Ojan Vafai <ojan>
Component: Layout and RenderingAssignee: Nobody <webkit-unassigned>
Status: NEW    
Severity: Normal CC: ahmad.saleem792, cgarcia, eric, hyatt, jchaffraix, sgill26, tony, zalan
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on:    
Bug Blocks: 62048    
Attachments:
Description Flags
Patch tony: review-

Ojan Vafai
Reported 2012-05-30 15:57:38 PDT
Whenever we're extending the logicalHeight of a flex-item that we've already laid out (e.g. due to flexing or flex-align:stretch), we only need to layout after modifying the height if the contents of the item depend on it's height (e.g. contains % height elements and is a containing block, has a background-color, etc) because we already. The tricky bit here is maintaining the function of when we need to do a relayout. In theory, in some of these cases we could take this a step further and avoid doing a full relayout, e.g. we don't actually need to relayout just to extend the background color further up/down; for percentage height decendants, we already keep a list of them and only need to relayout those items, etc.
Attachments
Patch (1.92 KB, patch)
2012-11-14 00:13 PST, Carlos Garcia Campos 		tony: review-
DetailsFormatted DiffDiff
View All Add attachment proposed patch, testcase, etc.
Carlos Garcia Campos
Comment 1 2012-11-14 00:13:38 PST
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.
Tony Chang
Comment 2 2012-11-14 11:01:48 PST
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.
Ojan Vafai
Comment 3 2012-11-14 12:14:54 PST
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.
Carlos Garcia Campos
Comment 4 2012-11-15 01:49:35 PST
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.
Ahmad Saleem
Comment 5 2024-07-20 09:41:24 PDT
@Sammy - is it still applicable?
Sammy Gill
Comment 6 2024-07-22 09:30:33 PDT
I think so since there is still a FIXME that references this bugzilla in RenderFlexibleBox::applyStretchAlignmentToFlexItem
Note You need to log in before you can comment on or make changes to this bug.