59 bytes, text/x-review-board-request
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 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+
Pushed by email@example.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
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 :/
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?
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.
Looks pretty green on Try these days, FWIW. https://treeherder.mozilla.org/#/jobs?repo=try&revision=b1e1c87bd7db726f7bb428f9431d3815d90cb8ab
Given comment 12, is it worth trying to re-land this?
Flags: needinfo?(mconley) → needinfo?(mstange)
Oh, nice! I'll click the buttons to land it.
Pushed by firstname.lastname@example.org: https://hg.mozilla.org/integration/autoland/rev/07344dd47432 Don't consider opacity as animated if it's changed between discrete values. r=mattwoodrow
Status: NEW → RESOLVED
Last Resolved: 7 months ago
status-firefox62: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla62
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 → ---
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.
Assignee: mstange → nobody
Status: REOPENED → NEW
You need to log in before you can comment on or make changes to this bug.