1142686 - When checking if flex item has correct final size, use its content-box rect, not frame-rect

Status: RESOLVED FIXED

(Core :: Layout, defect)

Description

[Daniel Holbert [:dholbert]]

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

Comment 1

[Daniel Holbert [:dholbert]]

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)

Comment 2

[Daniel Holbert [:dholbert]]

Attachment: fix v1

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

Comment 3

[Daniel Holbert [:dholbert]]

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 4

[Mats Palmgren (inactive)]

Comment on attachment 8576865 [details] [diff] [review]
fix v1

GetContentRectRelativeToSelf() is slightly faster since it doesn't
add in the position.

Comment 5

[Daniel Holbert [:dholbert]]

Attachment: fix v2, with GetContentRectRelativeToSelf [r=mats]

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

Comment 6

[Daniel Holbert [:dholbert]]

Attachment: fix v3 (with mochitest)

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

Comment 7

[Daniel Holbert [:dholbert]]

Attachment: fix v3a (with mochitest)

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

Comment 8

[Daniel Holbert [:dholbert]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/6cd4d71818ee

Comment 9

[Daniel Holbert [:dholbert]]

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 10

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/c17b8f486fd5

Comment 12

[Daniel Holbert [:dholbert]]

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 13

[Sylvestre Ledru [:Sylvestre]]

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

Comment 14

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/8852b9ffe7c2