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

RESOLVED FIXED in Firefox 38

Status

()

RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: dholbert, Assigned: dholbert)

Tracking

Trunk
mozilla39
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(firefox37 unaffected, firefox38 fixed, firefox39 fixed)

Details

Attachments

(1 attachment, 3 obsolete attachments)

(Assignee)

Description

4 years ago
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)

Updated

4 years ago
Assignee: nobody → dholbert
OS: Linux → All
Hardware: x86_64 → All
(Assignee)

Comment 1

4 years ago
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)
status-firefox37: --- → unaffected
status-firefox38: --- → affected
status-firefox39: --- → affected
(Assignee)

Comment 2

4 years ago
Created attachment 8576865 [details] [diff] [review]
fix v1

Here's the fix. (Still need to write some tests.)
Attachment #8576865 - Flags: review?(mats)
(Assignee)

Comment 3

4 years ago
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+
(Assignee)

Comment 5

4 years ago
Created attachment 8576896 [details] [diff] [review]
fix v2, with GetContentRectRelativeToSelf [r=mats]

Ah, good point -- thanks! Here's an updated patch with that.
Attachment #8576896 - Flags: review+
(Assignee)

Updated

4 years ago
Attachment #8576865 - Attachment is obsolete: true
(Assignee)

Updated

4 years ago
Flags: needinfo?(dholbert)
(Assignee)

Comment 6

4 years ago
Created attachment 8577545 [details] [diff] [review]
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.)
Attachment #8576896 - Attachment is obsolete: true
(Assignee)

Comment 7

4 years ago
Created attachment 8577547 [details] [diff] [review]
fix v3a (with mochitest)

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

Comment 8

4 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/6cd4d71818ee
Flags: needinfo?(dholbert)
Flags: in-testsuite?
Flags: in-testsuite+
(Assignee)

Comment 9

4 years ago
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
Last Resolved: 4 years ago
status-firefox39: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla39
(Assignee)

Updated

4 years ago
Duplicate of this bug: 1108772
(Assignee)

Comment 12

4 years ago
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+

Updated

4 years ago
Depends on: 1145868
(Assignee)

Updated

4 years ago
Blocks: 1141108
You need to log in before you can comment on or make changes to this bug.