Bug 87905 – Can avoid a second layout of flex-items in some cases

• Component: Layout and Rendering
• Status: NEW

[Ojan Vafai]

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.

[Carlos Garcia Campos]

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 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 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]

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]

@Sammy - is it still applicable?

[Sammy Gill]

I think so since there is still a FIXME that references this bugzilla in RenderFlexibleBox::applyStretchAlignmentToFlexItem