While playing with bug 1141108's testcase, I discovered that the optimization in bug 1054010 is checking the wrong rect for having the correct final size. Right now it has this check: > 3679 nsSize finalFlexedPhysicalSize = > 3680 aAxisTracker.PhysicalSizeFromLogicalSizes(item->GetMainSize(), > 3681 item->GetCrossSize()); [...] > 3690 // Check if we actually need to reflow the item -- if we already reflowed > 3691 // it with the right size, we can just reposition it as-needed. [...] > 3696 if (item->Frame()->GetSize() == finalFlexedPhysicalSize) { > 3697 // It has the correct size --> no need to reflow! Just make sure it's > 3698 // at the right position. > 3699 itemNeedsReflow = false; In the size-check there, nsIFrame::GetSize() returns the *border-box size*, whereas finalFlexedPhysicalSize is the *content-box size*. We should really be checking nsIFrame::GetContentRect().GetSize() -- not GetSize().

The upshot of this is that we don't necessarily skip in the right situations, for flex items with border/padding. This means we miss out on some cases where we could apply the optimization, and we also incorrectly apply the optimization in some cases when we should not. (which means we could end up with the wrong layout)

Here's the fix. (Still need to write some tests.)

This patch passes existing automated tests for the flexbox layout algorithm, FWIW. (reftests/flexbox, reftests/w3c-css/submitted/flexbox/, and the mochitest "test_flexbox_layout.html")

Comment on attachment 8576865 [details] [diff] [review] fix v1 GetContentRectRelativeToSelf() is slightly faster since it doesn't add in the position.

Ah, good point -- thanks! Here's an updated patch with that.

Here's the patch with a mochitest included, to check that border/padding on flex items doesn't cause us to do extra reflows. (I verified that this fails without the fix & passes with the fix.) I'll land this in the next day or so. (Tree's closed right now.)

(reposting to fix a comment typo in the mochitest. I also added documentation for one more function in the mochitest)

Sorry, comment 8 was missing my last test-tweaks -- forgot to pull from my patch queue in my inbound repo before landing. Backed out the patch that landed in comment 8: https://hg.mozilla.org/integration/mozilla-inbound/rev/841bf22704e0 and re-landed the right version: https://hg.mozilla.org/integration/mozilla-inbound/rev/c17b8f486fd5

Comment on attachment 8577547 [details] [diff] [review] fix v3a (with mochitest) I'd like to backport this to Aurora. Approval Request Comment [Feature/regressing bug #]: CSS flexbox -- specifically, an optimization to avoid redundant flexbox reflow, in bug 1054010. That patch had a bug in its code that checks whether the optimization could be applied -- this patch fixes that bug. [User impact if declined]: Jank on sites that make heavy use of CSS flexbox. (Not a regression; just a continuation of old jank, in some scenarios that bug 1054010 was *supposed* to let us avoid.) See e.g. bug 1108772 for one example. [Describe test coverage new/current, TreeHerder]: We have pretty comprehensive test coverage for flex layout. This has also been on trunk & getting nightly testing for the better part of a week, too. [Risks and why]: It's possible this will uncover more issues like bug 1128354 (a regression from bug 1054010), where the optimization causes us to skip a reflow that we actually need to perform (for subtle reasons). However, I don't think that risk should stop us from backporting this patch here. For any bug that's triggered by this patch, I think there'd be an equivalent bug (with a modified testcase) that would be triggered by bug 1054010 on its own. So, if we encounter any such regressions, and can't fix them, then the right course of action would probably be to disable bug 1054010's optimization altogether, rather than to pull out [or avoid backporting] this particular fix. [String/UUID change made/needed]: None.

Comment on attachment 8577547 [details] [diff] [review] fix v3a (with mochitest) Test + in m-c for a few days + small patch, taking it in aurora. thanks