On/Off opacity changes shouldn't create active layers

REOPENED
Assigned to

Status

()

REOPENED
3 years ago
3 days ago

People

(Reporter: mstange, Assigned: mstange)

Tracking

(Blocks: 1 bug)

Trunk
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox44 affected)

Details

(Whiteboard: [qf])

Attachments

(1 attachment)

(Assignee)

Description

3 years ago
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 hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 4

a year ago
mozreview-review
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.
Attachment #8895953 - Flags: review?(matt.woodrow) → review+

Comment 5

a year ago
Pushed by mstange@themasta.com:
https://hg.mozilla.org/integration/autoland/rev/2c33b1101844
Don't consider opacity as animated if it's changed between discrete values. r=mattwoodrow
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
Flags: needinfo?(mstange)
Assignee: nobody → mstange
(Assignee)

Comment 7

a year ago
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?
Flags: needinfo?(mstange) → needinfo?(bugmail)
(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 :/
Flags: needinfo?(bugmail)
(Assignee)

Comment 9

a year ago
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.
Hey mstange, any status on this?
Flags: needinfo?(mstange)
Flags: needinfo?(mconley)
(Assignee)

Comment 11

4 months ago
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.
Flags: needinfo?(mstange)
Given comment 12, is it worth trying to re-land this?
Flags: needinfo?(mconley) → needinfo?(mstange)
(Assignee)

Comment 14

4 months ago
Oh, nice! I'll click the buttons to land it.
Flags: needinfo?(mstange)

Comment 15

4 months ago
Pushed by mstange@themasta.com:
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

4 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/07344dd47432
Status: NEW → RESOLVED
Last Resolved: 4 months ago
status-firefox62: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla62

Updated

4 months ago
Status: RESOLVED → REOPENED
status-firefox62: fixed → ---
Resolution: FIXED → ---
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
Target Milestone: mozilla62 → ---
Whiteboard: [qf]
You need to log in before you can comment on or make changes to this bug.