1217748 - On/Off opacity changes shouldn't create active layers

Status: NEW

(Core :: Layout, defect)

Description

[Markus Stange [:mstange]]

If opacity is toggled between just two values, for example from a timer during a custom caret blink animation (bug 1217253) or due to a :hover style, we shouldn't create an active layer for the opacity.

Attachment 581241 [details] shows the problem with the :hover style: When a box is hovered and then unhovered within a short time span, we create an active layer for it, and due to overlap this causes the whole rest of the page to be repainted in a different layer.

Comment 1

[Markus Stange [:mstange]]

Attachment: Bug 1217748 - Don't consider opacity as animated if it's changed between discrete values.

This means that JS-implemented opacity animations will no longer be detected as animations.
I hope that's fine. The current heuristic makes us detect opacity 'animations' in lots of
places where there isn't an actual animation, and the resulting layerization changes from
that detected animation cause expensive repaints.

Review commit: https://reviewboard.mozilla.org/r/167220/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/167220/

Comment 2

[Markus Stange [:mstange]]

Comment on attachment 8895953 [details]
Bug 1217748 - Don't consider opacity as animated if it's changed between discrete values.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/167220/diff/1-2/

Comment 3

[Markus Stange [:mstange]]

Comment on attachment 8895953 [details]
Bug 1217748 - Don't consider opacity as animated if it's changed between discrete values.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/167220/diff/2-3/

Comment 4

[Matt Woodrow (:mattwoodrow)]

Comment on attachment 8895953 [details]
Bug 1217748 - Don't consider opacity as animated if it's changed between discrete values.

https://reviewboard.mozilla.org/r/167220/#review172490

Yeah, I think this is a good idea. We've definitely seen plenty of cases where being overly aggresive with layerization changes hurts us.

Comment 5

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/2c33b1101844
Don't consider opacity as animated if it's changed between discrete values. r=mattwoodrow

Comment 6

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

Backed out for failing gfx/layers/apz/test/mochitest/test_group_touchevents.html on Android 4.3 debug:

https://hg.mozilla.org/integration/autoland/rev/aa7a1476a7faefa7dbd5cd631a98cbcce686fa48

Push with failures: https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=2c33b1101844129b31ee6581ba70f60306e996c6&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&filter-resultStatus=retry&filter-resultStatus=runnable&filter-resultStatus=usercancel
Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=122808501&repo=autoland

[task 2017-08-13T02:01:20.900409Z] 02:01:20     INFO -  171 INFO None172 INFO TEST-START | gfx/layers/apz/test/mochitest/test_group_touchevents.html
[task 2017-08-13T02:10:06.004860Z] 02:10:06     INFO -  TEST-INFO | started process screentopng
[task 2017-08-13T02:10:06.360978Z] 02:10:06     INFO -  TEST-INFO | screentopng: exit 0
[task 2017-08-13T02:10:13.313574Z] 02:10:13  WARNING -  TEST-UNEXPECTED-FAIL | gfx/layers/apz/test/mochitest/test_group_touchevents.html | application timed out after 330 seconds with no output

Comment 7

[Markus Stange [:mstange]]

The last relevant output before the timeout is the string "Waiting for pan-x to propagate..." at https://treeherder.mozilla.org/logviewer.html#?job_id=122808501&repo=autoland&lineNumber=2786
which makes me think that the waitForAllPaintsFlushed at http://searchfox.org/mozilla-central/rev/e5b13e6224dbe3182050cf442608c4cb6a8c5c55/gfx/layers/apz/test/mochitest/helper_touch_action.html#31 never completes.

I don't really know what to make of the screenshot: https://public-artifacts.taskcluster.net/FmhG7U7jT_ud-YFRYB3j7A/0/public/test_info//mozilla-test-fail-screenshot_UtVihC.png

The test itself doesn't even seem to be visible on the screen. I wonder if all the action is happening in a background tab.

I don't see "opacity" being used anywhere in this test so I have no idea how the patch in this bug could be responsible. In fact, the only occurrence of the string "opacity" in that test directory is in helper_bug1271432.html, which didn't even get a chance run in the failing test runs.

I'm going to set up an Android mochitest environment locally now. In the meantime, kats, do you have any idea what might be happening here?

Comment 8

[Kartikaya Gupta (email:[e-mail])]

(In reply to Markus Stange [:mstange] from comment #7)
> The last relevant output before the timeout is the string "Waiting for pan-x
> to propagate..." at
> https://treeherder.mozilla.org/logviewer.
> html#?job_id=122808501&repo=autoland&lineNumber=2786
> which makes me think that the waitForAllPaintsFlushed at
> http://searchfox.org/mozilla-central/rev/
> e5b13e6224dbe3182050cf442608c4cb6a8c5c55/gfx/layers/apz/test/mochitest/
> helper_touch_action.html#31 never completes.

Agreed.

> I don't really know what to make of the screenshot:
> https://public-artifacts.taskcluster.net/FmhG7U7jT_ud-YFRYB3j7A/0/public/
> test_info//mozilla-test-fail-screenshot_UtVihC.png
>
> The test itself doesn't even seem to be visible on the screen. I wonder if
> all the action is happening in a background tab.

Yeah, this is a pre-existing issue with the android setup. It doesn't always foreground the test that's running.

> I don't see "opacity" being used anywhere in this test so I have no idea how
> the patch in this bug could be responsible. In fact, the only occurrence of
> the string "opacity" in that test directory is in helper_bug1271432.html,
> which didn't even get a chance run in the failing test runs.
> 
> I'm going to set up an Android mochitest environment locally now. In the
> meantime, kats, do you have any idea what might be happening here?

This is quite puzzling indeed. I don't know why the test would be affected by your patch, sorry :/

Comment 9

[Markus Stange [:mstange]]

After failing again to setup a local Android emulator mochitest environment, I'm guessing I'll have to push logging patches to try once more.

Comment 10

[Mike Conley (:mconley) (:⚙️)]

Hey mstange, any status on this?

Comment 11

[Markus Stange [:mstange]]

I think this is still worth doing. The test failure is what prevented me from landing it and I didn't spend the time to debug it.

Comment 12

[Ryan VanderMeulen [:RyanVM]]

Looks pretty green on Try these days, FWIW.
https://treeherder.mozilla.org/#/jobs?repo=try&revision=b1e1c87bd7db726f7bb428f9431d3815d90cb8ab

Comment 13

[Mike Conley (:mconley) (:⚙️)]

Given comment 12, is it worth trying to re-land this?

Comment 14

[Markus Stange [:mstange]]

Oh, nice! I'll click the buttons to land it.

Comment 15

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/07344dd47432
Don't consider opacity as animated if it's changed between discrete values. r=mattwoodrow

Comment 16

[Andrei Ciure[:aciure]]

https://hg.mozilla.org/mozilla-central/rev/07344dd47432

Comment 17

[Noemi Erli[:noemi_erli]]

Backed out changeset 07344dd47432 (bug 1217748) for increasing the frequency of https://bugzilla.mozilla.org/show_bug.cgi?id=1439979 a=backout

https://hg.mozilla.org/mozilla-central/rev/d99634619b05dd667c6f85e6ce51805a1165f3a1

Comment 18

[Markus Stange [:mstange]]

I think this still makes sense to do, but I don't have the time at the moment to look into the intermittent test failures that it causes.