Closed Bug 1131371 Opened 5 years ago Closed 5 years ago

Adding a CSS outline should just cause an update to overflow areas, instead of a full reflow

Categories

(Core :: Layout, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla38
Tracking Status
firefox38 --- fixed

People

(Reporter: dholbert, Assigned: dholbert)

References

(Blocks 1 open bug)

Details

Attachments

(1 file, 2 obsolete files)

Right now, if you add a CSS outline (and you change the outline-width and/or offset, which you probably will be doing), we trigger a reflow (via "AllReflowHints"):

642 nsChangeHint nsStyleOutline::CalcDifference(const nsStyleOutline& aOther) const
643 {
644   bool outlineWasVisible =
645     mCachedOutlineWidth > 0 && mOutlineStyle != NS_STYLE_BORDER_STYLE_NONE;
646   bool outlineIsVisible = 
647     aOther.mCachedOutlineWidth > 0 && aOther.mOutlineStyle != NS_STYLE_BORDER_STYLE_NONE;
648   if (outlineWasVisible != outlineIsVisible ||
649       (outlineIsVisible && (mOutlineOffset != aOther.mOutlineOffset ||
650                             mOutlineWidth != aOther.mOutlineWidth ||
651                             mTwipsPerPixel != aOther.mTwipsPerPixel))) {
652     return NS_CombineHint(nsChangeHint_AllReflowHints,
653                           nsChangeHint_RepaintFrame);

http://mxr.mozilla.org/mozilla-central/source/layout/style/nsStyleStruct.cpp?#642

This code basically dates back to bug 151375, which pushed "outline" outside of the border-box of frames, and made it influence its overflow region. It looks like (and roc confirms via IRC) we added nsChangeHint_AllReflowHints simply so that we could update the overflow region, because reflow was the only way to do this at that time.

However, now we have nsChangeHint_UpdateOverflow which we should be able to use, to directly target the overflow region without needing a reflow.

Filing this bug on making that change.

(billm says he's noticed this causing slowness when switching to a treeherder tab that has something focused deep down in the frame tree.  That focused thing becomes active when we switch tabs, which gives it a focus-outline, which triggers an avoidable reflow.)
Attached patch fix v1 (obsolete) — Splinter Review
I think this should do it. This adds UpdateOverflow and SchedulePaint (to kick off DLBI & be sure we don't just get a recompositing paint -- added in similar bug 1038781).

I'll give this a Try run, and request review if all is well.

(I'll also do a Try run without these new changehints, just to make sure we are actually testing this, and that our tests prove that UpdateOverflow is indeed required.)
tn points out that mozilla-central changeset 73eaf1199ff0 was intended to fix this -- returning  NS_CombineHint(nsChangeHint_UpdateOverflow, nsChangeHint_RepaintFrame) for this case.

But mats backed that out in mozilla-central changeset 14e550268f98 for some reason, a few weeks later.

mats, do you remember why we needed that backout? (I'm curious if there's anything I should test to find out if this new patch causes the same issue that triggered your eventual backout.)
Flags: needinfo?(mats)
Ah, looks like the backout is somewhat documented in bug 724432 comment 0, and (for a group of related patches) in bug 719177 comment 24. As dbaron noted later on bug 719177, it sounds like bug 984226 covered the work that was needed to be done before we could pick up the ball on using UpdateOverflow hints again.  Since bug 984226 is fixed, I suspect that means this should be OK now... (though I haven't looked at bug 984226 in detail yet).
Try run with this bug's patch:
  https://treeherder.mozilla.org/#/jobs?repo=try&revision=07d80f5799dd

And some probably-flawed exploratory Try runs...
 - just removing AllReflowHints:
    https://treeherder.mozilla.org/#/jobs?repo=try&revision=7769783afba4
 - Adding nsChangeHint_UpdateOverflow, but without SchedulePaint:
    https://treeherder.mozilla.org/#/jobs?repo=try&revision=18ae5a547061
D'oh, the Try runs of course has assertions because I forgot to update MaxDifference(). New patch / Try runs coming soon (maybe tomorrow) with that fixed.
Attached patch fix v2 (obsolete) — Splinter Review
Try run with MaxDifference / MaxDifferenceNeverInherited tweaked, to account for the updated changehints that we send now:
 https://treeherder.mozilla.org/#/jobs?repo=try&revision=ffa7fddbea4a

Tagging heycam to review, since he created MaxDifferenceNeverInherited recently, and I want to be sure I'm tweaking it correctly.
Attachment #8562294 - Flags: review?(cam)
(In reply to Daniel Holbert [:dholbert] from comment #5)
> And some probably-flawed exploratory Try runs...
>  - just removing AllReflowHints:
>     https://treeherder.mozilla.org/#/jobs?repo=try&revision=7769783afba4
>  - Adding nsChangeHint_UpdateOverflow, but without SchedulePaint:
>     https://treeherder.mozilla.org/#/jobs?repo=try&revision=18ae5a547061

For the record, both of these intentionally-flawed Try runs had reftest failures with missing outlines (and some ignorable assertions due to comment 6). Those reftest failures confirm that both "nsChangeHint_UpdateOverflow" and "nsChangeHint_SchedulePaint" are definitely needed here.
Attachment #8561759 - Attachment is obsolete: true
Flags: in-testsuite?
Added a mochitest, which supports making arbitrary style changes & asserting that no reflow/frame-construction cocurred.

Right now it just focuses on "outline", but I gave it a generic name so we can extend it. Verified locally that it passes w/ fix, fails w/out.

Try run: https://treeherder.mozilla.org/#/jobs?repo=try&revision=c5ebbb08d35f
Attachment #8562294 - Attachment is obsolete: true
Attachment #8562294 - Flags: review?(cam)
Attachment #8563175 - Flags: review?(cam)
Looks like you found the information you asked for.  Please make sure you don't
regress the test in bug 725664 (I'm not sure if that's in the tree or not).
Flags: needinfo?(mats)
Thanks! I checked, & it does not regress that test; the test looks the same with & without the patch.
Comment on attachment 8563175 [details] [diff] [review]
fix v3 (now with test)

Review of attachment 8563175 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good.

::: layout/style/test/test_dynamic_change_causing_reflow.html
@@ +21,5 @@
> +
> +/** Test for Bug 1131371 **/
> +
> +/**
> +

Remove blank line.
Attachment #8563175 - Flags: review?(cam) → review+
Thanks -- fixed that & landed:
 https://hg.mozilla.org/integration/mozilla-inbound/rev/4c0ccf99cb71
Flags: in-testsuite? → in-testsuite+
https://hg.mozilla.org/mozilla-central/rev/4c0ccf99cb71
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla38
See Also: → 1194480
Depends on: 1194346
Depends on: 1364338
You need to log in before you can comment on or make changes to this bug.