Closed Bug 1504065 Opened Last year Closed Last year

Run background-color animations on the compositor for non-WebRender

Categories

(Core :: Layout, enhancement, P3)

enhancement

Tracking

()

RESOLVED FIXED
mozilla65
Tracking Status
firefox65 --- fixed

People

(Reporter: hiro, Assigned: hiro)

References

(Blocks 1 open bug, Regressed 1 open bug)

Details

(Whiteboard: [qf:p2:responsiveness])

Attachments

(5 files)

background-color animations is the third topmost property being used in the wild [1], it would be nice to do it.

I know Core:Layout is probably not a right component for this issue, but the code for this will be across layout and gfx and servo. :)

[1] https://www.chromestatus.com/metrics/css/animated
I will do this along with bug 1479173 to prove the new setup introduced in bug 1479173 work.
Assignee: nobody → hikezoe
Status: NEW → ASSIGNED
Whiteboard: [qf]
Replying the concern raised by Markus in bug 1371101 comment 3.

It seems we don't allow background-color animations on :visited pseudo class.  And Chrome doesn't either.  Attaching file is an example for it.
CCing Markus, I did check the behavior of background-color animation on :visited class.
That's just because `animation` is not one of the properties that are allowed in :visited. You could trigger transitions though, and that may or may not be problematic.
I actually try transition but it didn't work either.
(I'm not sure we trigger transitions based on visited, if not _that_ is the bug to fix)
(In reply to Emilio Cobos Álvarez (:emilio) from comment #6)
> (I'm not sure we trigger transitions based on visited, if not _that_ is the
> bug to fix)

Err, this was meant to be "if so _that_ is the bug to fix". Triggering transitions for :visited seems bad.
Attached file different testcase
Here's what I had in mind: An animation that changes the background color only on non-visited links. On visited links, the animation animates between purple and purple.
currentColor, an interesting test case. Thanks!  It noticed me that my current WIP patch doesn't handle it correctly. :)

It seems to me that we will leak the visited/non-visited information in the test case by an optimization in ColorLayerProperties::ComputeChangeInternal()[1].  I wouldn't worry too much since the difference is pretty negligible, but yeah we shouldn't underestimate it.

[1] https://hg.mozilla.org/mozilla-central/file/2dd516cee24f/gfx/layers/LayerTreeInvalidation.cpp#l597
Depends on: 1504884
Whiteboard: [qf] → [qf:p2]
Whiteboard: [qf:p2] → [qf:p2:responsiveness]
With the patches for this bug, a few reftests start failing on Android;  All the failed test cases have a background-color animation.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=60aaa231273e4647bf491d4e984f47ca61cf2fe1&selectedJob=210956062

I did investigate what happened there, and it turned out that on *non-E10S* there are two places we flush pending styles in reftest.jsm so we stuck at STATE_WAITING_TO_FIRE_INVALIDATE_EVENT (in the case where the animation started in the first place) or stuck at STATE_WAITING_TO_FINISH.  The one place we flush pending styles is that getBoundingClientRect call for browser window (that's the reason why the failure doesn't happen on E10S) in DoDrawWindow, the other is drawWindow itself, it flushes styles without specifying DRAWWINDOW_DO_NOT_FLUSH.

I am going to create a patch to skip those flushes when reftest-no-flush is specified in test case.
See Also: → 1510030
Changes for nsIDOMWindowUtils.getOMTAValue is in the next commit with come test
cases.
In reviewing this I'm reminded of the various timing attacks against :visited brought up in [1]. When we come to implement AnimationWorklet we'll need to be careful that we don't accidentally expose when the visited style changes for background-color animations.

[1] https://cseweb.ucsd.edu/~dstefan/pubs/smith:2018:browser.pdf
Depends on: 1510120
Pushed by hikezoe@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/359e81b35cfb
Run background-color animations on the compositor. r=birtles
https://hg.mozilla.org/integration/autoland/rev/129188370231
Support background-color animations on the compositor for nsIDOMWindowUtils::GetOMTAValue. r=birtles
Looks like subpixel rendering fuzziness (I am not sure what is called) on Windows 7 GPU, I will drop the 'child' text from the reftest;

https://treeherder.mozilla.org/#/jobs?repo=try&revision=84899670ac741c20b79c988400c1646a802f65be
Flags: needinfo?(hikezoe)
The try looks good.  Though the reftest change should be in the first commit, but I am going to add a new commit and send a new review request for that.
Pushed by hikezoe@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/d99d8f275d8b
Run background-color animations on the compositor. r=birtles
https://hg.mozilla.org/integration/autoland/rev/a30f77050399
Support background-color animations on the compositor for nsIDOMWindowUtils::GetOMTAValue. r=birtles
https://hg.mozilla.org/integration/autoland/rev/94ecc40729d0
Drop text in the child element inside background-color animated element to avoid fuzziness on Windows 7 GPU. r=birtles
Depends on: 1512754
Depends on: 1518802
See Also: → 1516948
Depends on: 1525203
Blocks: 1535532

Moving dependencies to meta bug 1535532 instead.

No longer blocks: 1371101
No longer depends on: 1510120, 1518802
Regressions: 1546856
You need to log in before you can comment on or make changes to this bug.