Bug 102352 – Avoid a second layout of flex items in layoutAndPlaceChildren()

• Status: RESOLVED
• Resolution: FIXED
• Component: Layout and Rendering

[Carlos Garcia Campos]

In case of nested flexbox containers, it's possible that in layoutAndPlaceChildren(), the children have already been laid out in computeMainAxisPreferredSizes().

[Carlos Garcia Campos]

Created attachment 174370 [details]
Patch

I'm still getting a failure in css3/flexbox/auto-margins.html but as I commented in bug #87905 the output is correct, and the inspector shows the right values for margins, just reloading the page turns the FAIL into PASS.

[WebKit Review Bot]

Comment on attachment 174370 [details]
Patch

Attachment 174370 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/14857146

New failing tests:
fast/forms/datalist/update-range-with-datalist.html
fast/multicol/client-rects.html
mathml/presentation/sub.xhtml
mathml/presentation/mo-stretch.html
css3/flexbox/auto-margins.html
fast/css/unknown-pseudo-element-matching.html
fast/forms/range/input-appearance-range.html
fast/forms/range/range-thumb-height-percentage.html

[Tony Chang]

(In reply to comment #1)
> Created an attachment (id=174370) [details]
> Patch
> 
> I'm still getting a failure in css3/flexbox/auto-margins.html but as I commented in bug #87905 the output is correct, and the inspector shows the right values for margins, just reloading the page turns the FAIL into PASS.

This is a real failure. It means we're not layout out when we should.  The inspector is probably forcing an extra layout which is causing the margins to correct themselves.  The other failures are probably real too (mathml and the range form control both use flexbox).

[Tony Chang]

Comment on attachment 174370 [details]
Patch

I'm not sure why the tests are failing, maybe try using a debugger to see how auto-margins.html is different with and without this patch?

[Carlos Garcia Campos]

Created attachment 174641 [details]
Updated patch

Since the major performance issues happen with nested flexbox containers, we can try a more conservative approach and only avoid second layout when the child is a flexbox.

[WebKit Review Bot]

Comment on attachment 174641 [details]
Updated patch

Attachment 174641 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/14874072

New failing tests:
fast/forms/datalist/update-range-with-datalist.html
mathml/presentation/sub.xhtml
mathml/presentation/mo-stretch.html

[Tony Chang]

Comment on attachment 174641 [details]
Updated patch

Tests are still failing.  We need to debug the cause of the failure.  Applying the change less often isn't going to make the bug go away.

[Carlos Garcia Campos]

(In reply to comment #7)
> (From update of attachment 174641 [details])
> Tests are still failing.  We need to debug the cause of the failure.  Applying the change less often isn't going to make the bug go away.

Sure, I debugged the auto-margins case, and the problem was that the logical height is updated during the layout and it resets the margins, so when not laying out the margins already had the right value that is used to calculate the margin again, which ended up resetting to 0. So, just calling child->updateLogicalHeight() when avoiding the layout of the child, fixed the test, but I was not sure that was the right approach and that didn't fix the other tests. So, my guess is that when avoiding the layout, we should still do other things that happen during the layout even if they don't affect the size. I think the problem with the other failures has to do with the position, for some reason, when laying out multiple times the child is moved several times, but the final position is the same than when avoiding the layout. Next week I'll continue debugging the test failures.

[Carlos Garcia Campos]

I've been debugging fast/forms/range/input-appearance-range.html and the problem is that the slider thumb location is added every time the slider container is laid out. RenderSliderContainer::layout() sets the thumb location assuming the thumb is laid out, which might not be the case after this patch. I've filed it as a separate bug report:

https://bugs.webkit.org/show_bug.cgi?id=102817

I'm still looking at the mathml changes, the different is just a few pixels, for some reason the main axis is different (by one pixel) in some cases when doing the second layout to apply the stretch alignment.

[Carlos Garcia Campos]

(In reply to comment #9)
> I've been debugging fast/forms/range/input-appearance-range.html and the problem is that the slider thumb location is added every time the slider container is laid out. RenderSliderContainer::layout() sets the thumb location assuming the thumb is laid out, which might not be the case after this patch. I've filed it as a separate bug report:
> 
> https://bugs.webkit.org/show_bug.cgi?id=102817
> 
> I'm still looking at the mathml changes, the different is just a few pixels, for some reason the main axis is different (by one pixel) in some cases when doing the second layout to apply the stretch alignment.

I meant the cross axis, not the main axis.

[Tony Chang]

fixed by bug 102851:
fast/forms/datalist/update-range-with-datalist.html

fixed by bug 102817:
fast/forms/range/input-appearance-range.html
fast/forms/range/range-thumb-height-percentage.html

For these 2, it's because we normally compute margins during layout, but for flex items, we compute auto margins in the flexbox.  When we skip the flex item layout, we end up double computing the margin in a few cases.  There are probably a few different ways to solve this.
fast/multicol/client-rects.html
css3/flexbox/auto-margins.html

This is mysterious, the first :hover selector isn't applying, but the second test passes.  If I reorder the test cases, the first test always fails.
fast/css/unknown-pseudo-element-matching.html

Haven't looked at these yet.:
mathml/presentation/sub.xhtml
mathml/presentation/mo-stretch.html

[Carlos Garcia Campos]

(In reply to comment #11)
> For these 2, it's because we normally compute margins during layout, but for flex items, we compute auto margins in the flexbox.  When we skip the flex item layout, we end up double computing the margin in a few cases.  There are probably a few different ways to solve this.
> fast/multicol/client-rects.html
> css3/flexbox/auto-margins.html

I fixed this, by calling updateLogicalHeight() on the child when not laying out it.

> Haven't looked at these yet.:
> mathml/presentation/sub.xhtml
> mathml/presentation/mo-stretch.html

I'll continue debugging these today.

[Tony Chang]

(In reply to comment #11)
> This is mysterious, the first :hover selector isn't applying, but the second test passes.  If I reorder the test cases, the first test always fails.
> fast/css/unknown-pseudo-element-matching.html

This will be fixed by 102993.

[Carlos Garcia Campos]

(In reply to comment #12)
> > Haven't looked at these yet.:
> > mathml/presentation/sub.xhtml
> > mathml/presentation/mo-stretch.html
> 
> I'll continue debugging these today.

I'm very confused with these, mo-stretch.html fails for me even without the patch, so I'm not sure, and sub.xhtml renders the sub Y one pixel below the expected one, however, what I get when running manually the tests in chromium is different and looks correct even with the patch, so I don't know if it has to do with the fonts used by DRT.

[Tony Chang]

I debugged the mathml failures and they uncovered a real problem.  Here's an example:
http://plexode.com/u/#2N=0&-Mle2L%3A2K%0AEJheightEH5olid!EG%3Cdiv29%3E28%3Dj7K!!G!id26%22r54borderL!1px!sb4!styM86b*%3C%2Fdiv92!%20~http://pMxode.com/eval3/#ht=G4displayL-webkit-fMx6978aHblue%3B!JL!30px69a*78bHgreen%3B69b*K*&ohjNjt=document.getEMmentById(6a6).styM.J!8!620px6K&ojhNojjNcex=0


Press eval JS now to see the bug.  What happens is that we previously stretched a child and since we don't layout the second time, we keep using the stretched height, which is incorrect.  We should include this test as a layout test.

A simple fix is to ignore this optimization if the child is stretching (i.e., always relayout stretching children).

For the auto margins case, updateLogicalHeight should work, but we may be able to be smarter about it (maybe just set the margins to 0 if they style is auto?).

I'll try to put together a patch tomorrow.

[Carlos Garcia Campos]

Created attachment 176720 [details]
Updated patch

Do not skip the layout when stretching children that has auto size. This fixes the tests, but I think we are still missing some more cases where we could avoid a relayout.

[WebKit Review Bot]

Comment on attachment 176720 [details]
Updated patch

Attachment 176720 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/15054071

New failing tests:
fast/forms/datalist/input-appearance-range-with-datalist-rtl.html
fast/forms/datalist/input-appearance-range-with-padding-with-datalist.html
fast/forms/range/input-appearance-range-rtl.html

[Build Bot]

Comment on attachment 176720 [details]
Updated patch

Attachment 176720 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/15044429

New failing tests:
fast/forms/range/input-appearance-range-rtl.html

[Tony Chang]

Created attachment 176849 [details]
Patch

[Carlos Garcia Campos]

Comment on attachment 176849 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=176849&action=review

> Source/WebCore/rendering/RenderFlexibleBox.cpp:1074
> +    return (!isColumnFlow() && child->style()->logicalHeight().isAuto())
> +        || (isColumnFlow() && child->style()->logicalWidth().isAuto());

I've noticed we might be checking isColumnFlow twice here, it's very minor thing, but we could cache it in a variable like you are doing in resetAutoMarginsAndLogicalTopInCrossAxis() with isHorizontalFlow.

[Carlos Garcia Campos]

Created attachment 176919 [details]
Updated patch

Updated patch with the isColumnFlow minor improvement I commented before.

[Ojan Vafai]

Comment on attachment 176919 [details]
Updated patch

View in context: https://bugs.webkit.org/attachment.cgi?id=176919&action=review

> LayoutTests/css3/flexbox/stretch-after-sibling-size-change.html:17
> +  <div id=a class="flex-one" data-expected-width="50" data-expected-height="20" style="background-color: blue; height: 30px;"></div>
> +  <div id=b class="flex-one" data-expected-width="50" data-expected-height="20" style="background-color: green"></div>

Nit: 2 spec indent.

> Source/WebCore/html/shadow/SliderThumbElement.cpp:186
> +        // doesn't move the thumb to the wrong place. FIXME: Make a custom Render class

The FIXME should go at the beginning of a line.

> Source/WebCore/rendering/RenderFlexibleBox.cpp:1074
> +    bool isColumn = isColumnFlow();
> +    return (!isColumn && child->style()->logicalHeight().isAuto()) || (isColumn && child->style()->logicalWidth().isAuto());

I'd find this easier to read as follows:
var crossAxisLength = isColumnFlow() ? child->style->logicalWidth() : child->style->logicalHeight();
return crossAxisLength.isAuto();

> Source/WebCore/rendering/RenderFlexibleBox.cpp:1081
> +    Length start = isHorizontal ? child->style()->marginTop() : child->style()->marginLeft();
> +    Length end = isHorizontal ? child->style()->marginBottom() : child->style()->marginRight();

s/start/before
s/end/after

You could use flowAwareMarginBeforeForChild and flowAwareMarginAfterForChild.

> Source/WebCore/rendering/RenderFlexibleBox.cpp:1114
> +            resetAutoMarginsAndLogicalTopInCrossAxis(child);

This could use a comment explaining why we need to do this. I had to go read the comments in the bug to make sense of this.

[Tony Chang]

Created attachment 177008 [details]
Patch

[Tony Chang]

Comment on attachment 176919 [details]
Updated patch

View in context: https://bugs.webkit.org/attachment.cgi?id=176919&action=review

>> Source/WebCore/rendering/RenderFlexibleBox.cpp:1074
>> +    return (!isColumn && child->style()->logicalHeight().isAuto()) || (isColumn && child->style()->logicalWidth().isAuto());
> 
> I'd find this easier to read as follows:
> var crossAxisLength = isColumnFlow() ? child->style->logicalWidth() : child->style->logicalHeight();
> return crossAxisLength.isAuto();

The old code was wrong in the case of orthogonal flow.  I fixed it in the latest patch.

>> Source/WebCore/rendering/RenderFlexibleBox.cpp:1081
>> +    Length end = isHorizontal ? child->style()->marginBottom() : child->style()->marginRight();
> 
> s/start/before
> s/end/after
> 
> You could use flowAwareMarginBeforeForChild and flowAwareMarginAfterForChild.

Those return the computed margin, not the margin style. Fortunately, we had a different helper method that does what I want.

[Ojan Vafai]

Comment on attachment 177008 [details]
Patch

yay!

[WebKit Review Bot]

Comment on attachment 177008 [details]
Patch

Clearing flags on attachment: 177008

Committed r136324: <http://trac.webkit.org/changeset/136324>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.