Changes to background-color trigger invalidations, even on an "opacity:0" element

RESOLVED FIXED in Firefox 51

Status

()

RESOLVED FIXED
3 years ago
2 years ago

People

(Reporter: dholbert, Assigned: mattwoodrow)

Tracking

(Blocks: 1 bug)

unspecified
mozilla51
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox51 fixed)

Details

Attachments

(3 attachments)

(Reporter)

Description

3 years ago
STR:
 1. Enable paint flashing. ( nglayout.debug.paint_flashing )
 2. Load attached testcase.

ACTUAL RESULTS:
The area above the text gets invalidated (repainted in a different paint-flashing color) every 500ms, from the periodic style.backgroundColor tweak.

EXPECTED RESULTS: No invalidation, since the periodic backgroundColor tweak is happening on an element with "opacity:0".
(Reporter)

Comment 1

3 years ago
Created attachment 8704370 [details]
testcase 1
(Reporter)

Comment 2

3 years ago
Created attachment 8704373 [details]
reference case 1 (adding "will-change:opacity" to force layerization)

As shown here, we can work around this by adding "will-change: opacity" on the element whose background-color is changing (I think because we can figure out that the whole layer is opacity:0 and optimize away the change).
(Reporter)

Updated

3 years ago
Blocks: 1231946
(Reporter)

Comment 3

3 years ago
NOTE: I'm not 100% sure my EXPECTED RESULTS are reasonable here, or that this causes problems in the real world.
Component: Layout → CSS Parsing and Computation
(Reporter)

Comment 4

3 years ago
(Sorry, accidentally tweaked component; reverting.)

Also didn't finish my thought in comment 3 -- compared to this bug here, I'm *more* that related-bug 1237102 is problematic, given that bug's dependence on a plugin being present, and given that we hit a version of that bug in the wild at Twitch.
Component: CSS Parsing and Computation → Layout
I agree that this is a bug.

(In reply to Daniel Holbert [:dholbert] from comment #2)
> Created attachment 8704373 [details]
> reference case 1 (adding "will-change:opacity" to force layerization)
> 
> As shown here, we can work around this by adding "will-change: opacity" on
> the element whose background-color is changing

This actually not true. What happens here is that the paint flashing ends up in a PaintedLayer that's invisible. We're still invalidating and painting.
With will-change: opacity, Gecko makes sure that the element can be made visible at any time, so it needs to have up-to-date content in a (currently invisible) layer. In other words, with will-change:opacity, invalidating is in fact the right thing to do.
(Assignee)

Comment 6

2 years ago
Created attachment 8777662 [details] [diff] [review]
dont-invalidate-opacity-0-item

Changing the background-color is a style change which sets the 'invalid' bit on the frame.

This makes us treat the nsDisplayOpacity items belonging to the same frame as dirty, and we mark its bound as invalid in the PaintedLayer.
Assignee: nobody → matt.woodrow
Attachment #8777662 - Flags: review?(tnikkel)
Attachment #8777662 - Flags: review?(tnikkel) → review+

Comment 7

2 years ago
Pushed by mwoodrow@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/cbcdad90154e
Don't invalidate opacity:0 nsDisplayOpacity items. r=tnikkel

Comment 8

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/cbcdad90154e
Status: NEW → RESOLVED
Last Resolved: 2 years ago
status-firefox51: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla51
You need to log in before you can comment on or make changes to this bug.