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

Status: RESOLVED FIXED

(Core :: Layout, enhancement, P3)

Description

[Hiroyuki Ikezoe (:hiro)]

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

Comment 1

[Hiroyuki Ikezoe (:hiro)]

I will do this along with bug 1479173 to prove the new setup introduced in bug 1479173 work.

Comment 2

[Hiroyuki Ikezoe (:hiro)]

Attachment: background-color animation on :visited

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.

Comment 3

[Hiroyuki Ikezoe (:hiro)]

CCing Markus, I did check the behavior of background-color animation on :visited class.

Comment 4

[Emilio Cobos Álvarez (:emilio)]

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.

Comment 5

[Hiroyuki Ikezoe (:hiro)]

I actually try transition but it didn't work either.

Comment 6

[Emilio Cobos Álvarez (:emilio)]

(I'm not sure we trigger transitions based on visited, if not _that_ is the bug to fix)

Comment 7

[Emilio Cobos Álvarez (:emilio)]

(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.

Comment 8

[Markus Stange [:mstange]]

Attachment: 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.

Comment 9

[Hiroyuki Ikezoe (:hiro)]

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

Comment 10

[Hiroyuki Ikezoe (:hiro)]

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.

Comment 11

[Hiroyuki Ikezoe (:hiro)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=d9aef70058d1d38a736245b68ce09489726078be

I think the failure cause has been fixed by bug 1506988.

Comment 12

[Hiroyuki Ikezoe (:hiro)]

Attachment: Bug 1504065 - Run background-color animations on the compositor.

Changes for nsIDOMWindowUtils.getOMTAValue is in the next commit with come test
cases.

Comment 13

[Hiroyuki Ikezoe (:hiro)]

Attachment: Bug 1504065 - Support background-color animations on the compositor for nsIDOMWindowUtils::GetOMTAValue. r?birtles!

Depends on D13001

Comment 14

[Brian Birtles (:birtles)]

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

Comment 15

[Pulsebot]

Pushed by [e-mail]:
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

Comment 16

[Andreea Pavel [:apavel]]

Backed out for failing win reftest at child-in-animating-element-display-none.html

Push that caused the failures: https://treeherder.mozilla.org/#/jobs?repo=autoland&selectedJob=214113559&revision=1291883702312b5ca9c44983121ac39fd2b8bdf6
 
Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=214113559&repo=autoland&lineNumber=33905

Backout: https://hg.mozilla.org/integration/autoland/rev/20898bcaaa0069be066b088fde0262169fc71261

Comment 17

[Hiroyuki Ikezoe (:hiro)]

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

Comment 18

[Hiroyuki Ikezoe (:hiro)]

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.

Comment 19

[Hiroyuki Ikezoe (:hiro)]

Attachment: Bug 1504065 - Drop text in the child element inside background-color animated element to avoid fuzziness on Windows 7 GPU. r?birtles!

Depends on D13002

Comment 20

[Pulsebot]

Pushed by [e-mail]:
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

Comment 21

[Narcis Beleuzu [:NarcisB]]

https://hg.mozilla.org/mozilla-central/rev/d99d8f275d8b
https://hg.mozilla.org/mozilla-central/rev/a30f77050399
https://hg.mozilla.org/mozilla-central/rev/94ecc40729d0

Comment 22

[Brian Birtles (:birtles)]

Moving dependencies to meta bug 1535532 instead.