Closed Bug 1142686 Opened 5 years ago Closed 5 years ago

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

Categories

(Core :: Layout, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla39
Tracking Status
firefox37 --- unaffected
firefox38 --- fixed
firefox39 --- fixed

People

(Reporter: dholbert, Assigned: dholbert)

References

Details

Attachments

(1 file, 3 obsolete files)

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().
Assignee: nobody → dholbert
OS: Linux → All
Hardware: x86_64 → All
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)
Attached patch fix v1 (obsolete) — Splinter Review
Here's the fix. (Still need to write some tests.)
Attachment #8576865 - Flags: review?(mats)
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")
Flags: in-testsuite?
Comment on attachment 8576865 [details] [diff] [review]
fix v1

GetContentRectRelativeToSelf() is slightly faster since it doesn't
add in the position.
Attachment #8576865 - Flags: review?(mats) → review+
Ah, good point -- thanks! Here's an updated patch with that.
Attachment #8576896 - Flags: review+
Attachment #8576865 - Attachment is obsolete: true
Flags: needinfo?(dholbert)
Attached patch fix v3 (with mochitest) (obsolete) — Splinter Review
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.)
Attachment #8576896 - Attachment is obsolete: true
(reposting to fix a comment typo in the mochitest. I also added documentation for one more function in the mochitest)
Attachment #8577545 - Attachment is obsolete: true
https://hg.mozilla.org/integration/mozilla-inbound/rev/6cd4d71818ee
Flags: needinfo?(dholbert)
Flags: in-testsuite?
Flags: in-testsuite+
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
https://hg.mozilla.org/mozilla-central/rev/c17b8f486fd5
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla39
Duplicate of this bug: 1108772
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.
Attachment #8577547 - Flags: approval-mozilla-aurora?
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
Attachment #8577547 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Depends on: 1145868
Blocks: 1141108
You need to log in before you can comment on or make changes to this bug.