Bug 102352

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.

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

(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).

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?

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.

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

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.

(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.

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.

(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.

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

(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.

(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.

(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.

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.

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.

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

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

Created attachment 176849 [details] Patch

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.

Created attachment 176919 [details] Updated patch Updated patch with the isColumnFlow minor improvement I commented before.

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.

Created attachment 177008 [details] Patch

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.

Comment on attachment 177008 [details] Patch yay!

Comment on attachment 177008 [details] Patch Clearing flags on attachment: 177008 Committed r136324: <http://trac.webkit.org/changeset/136324>

All reviewed patches have been landed. Closing bug.