Closed Bug 1380065 Opened 7 years ago Closed 4 years ago

Disable arrow-panel (doorhanger) animations with the toolkit.cosmeticAnimations.enabled pref

Categories

(Toolkit :: UI Widgets, defect, P4)

defect

Tracking

()

RESOLVED DUPLICATE of bug 1636057
Tracking Status
firefox57 --- wontfix
firefox58 --- wontfix
firefox59 --- wontfix
firefox60 --- affected
firefox61 --- affected
firefox62 --- affected

People

(Reporter: sfoster, Unassigned)

References

Details

(Whiteboard: [reserve-photon-animation])

Attachments

(1 file, 2 obsolete files)

We should be able to enable/disable the opening and closing transitions used by arrow-panel popups with the toolkit.cosmeticAnimations.enabled pref. 

This will also allow us to stabilize tests that are race-y because of these animations, which is blocking landing of work on 1352075
Status: NEW → ASSIGNED
Iteration: --- → 56.3 - Jul 24
Flags: qe-verify+
Priority: -- → P1
QA Contact: jwilliams
Whiteboard: [photon-animation] → [photon-animation]
Component: XP Toolkit/Widgets: XUL → XUL Widgets
Product: Core → Toolkit
Known issues:
* Logic looks right, but setting animate attribute in popupshowing is too late to affect how the panel opens, we only see the change the next time. I dont fully understand this, do we need to trigger a style flush (if so, how?) or is this somehow set during InitializePopup

* Using animate=false attribute causes some state issue - a click on the anchor while the popup is already open should close it, but instead it closes and re-opens. Currently trying to track down what's going on with this.
Attachment #8885879 - Flags: feedback?(jaws)
Comment on attachment 8885879 [details] [diff] [review]
WIP patch to update the animate attribute and transitioning behavior based on the toolkit.cosmeticAnimations.enabled pref

Review of attachment 8885879 [details] [diff] [review]:
-----------------------------------------------------------------

I tested out the patch and I couldn't get animations to disable when changing the pref through about:config. I also wasn't able to get the popup to reopen instead of closing when clicking on the anchor. Maybe this is a difference between which platform we were using (I am using Windows atm). I attached a patch that worked fine for me with neither of the issues you mentioned.
Attachment #8885879 - Flags: feedback?(jaws) → feedback+
Comment on attachment 8886285 [details]
Bug 1380065 - Disable arrow-panel animations if the cosmeticAnimations pref is set to false.

https://reviewboard.mozilla.org/r/157040/#review163100

I still get the re-opening behaviour when animate=false, on OSX. I think we need to get an answer to that before we can move forward with this. The pref/animation logic does indeed work better with this patch though.
Attachment #8886285 - Flags: review?(sfoster)
To be clear, this re-opening behavior is an existing issue with the handling of animate="false" in the popup code, for OSX only.
Comment on attachment 8887235 [details]
Bug 1380065 - Disable arrow-panel animations if the cosmeticAnimations pref is set to false.

https://reviewboard.mozilla.org/r/158032/#review163144

Huh, I guess I lost the authorship on this patch. The point of request my review was to approve :jaws' changes but now I'm reviewing myself.
Attachment #8887235 - Flags: review?(sfoster) → review+
Comment on attachment 8887235 [details]
Bug 1380065 - Disable arrow-panel animations if the cosmeticAnimations pref is set to false.

https://reviewboard.mozilla.org/r/158032/#review163152

well, if you want you can assign the bug to me :)
Attachment #8887235 - Flags: review?(jaws) → review+
Try results are coming in: https://treeherder.mozilla.org/#/jobs?repo=try&revision=e6d640e7598e4894f8081dde1f7338afa27cd2ed

Its still running, but so far I see lots of failures on Linux. Which is unexpected as we dont animate arrow panel opening/closing at all on Linux today. The couple I've looked into fail reliably locally without the popup.xml changes - simply toggling cosmeticAnimations.enabled in prefs_general.js is enough to cause failures. E.g. 

39 INFO TEST-UNEXPECTED-FAIL | browser/components/migration/tests/browser/browser_undo_notification.js | Notification should be closing - Got [object XULElement], expected null
Stack trace:
chrome://mochikit/content/browser-test.js:test_is:967
chrome://mochitests/content/browser/browser/components/migration/tests/browser/browser_undo_notification.js:autoMigrationUndoNotificationShows:70 

Jim, have you run into any of this - any ideas? The idea is to stabilize otherwise flaky tests that need to interact with popups (arrow-panels specifically) and have to workaround the timing issues presented by these transitions. I wasn't expecting this kind of fallout..
Flags: needinfo?(squibblyflabbetydoo)
Looks like the issue is that the test expects the *notification* to be animated, not necessarily the popup. `._closedNotification` is for when the notification is in the middle of closing, I think. Maybe there's another way to write a similar test, but I'm not sure as I never had to update tests for that code in my bug.
Flags: needinfo?(squibblyflabbetydoo)
Attachment #8885879 - Attachment is obsolete: true
Attachment #8886285 - Attachment is obsolete: true
(In reply to Sam Foster [:sfoster] from comment #10)
> Try results are coming in:
> https://treeherder.mozilla.org/#/
> jobs?repo=try&revision=e6d640e7598e4894f8081dde1f7338afa27cd2ed
> 
> Its still running, but so far I see lots of failures on Linux. Which is
> unexpected as we dont animate arrow panel opening/closing at all on Linux
> today. The couple I've looked into fail reliably locally without the
> popup.xml changes - simply toggling cosmeticAnimations.enabled in
> prefs_general.js is enough to cause failures. E.g. 
> 
> 39 INFO TEST-UNEXPECTED-FAIL |
> browser/components/migration/tests/browser/browser_undo_notification.js |
> Notification should be closing - Got [object XULElement], expected null
> Stack trace:
> chrome://mochikit/content/browser-test.js:test_is:967
> chrome://mochitests/content/browser/browser/components/migration/tests/
> browser/browser_undo_notification.js:autoMigrationUndoNotificationShows:70 
> 
> Jim, have you run into any of this - any ideas? The idea is to stabilize
> otherwise flaky tests that need to interact with popups (arrow-panels
> specifically) and have to workaround the timing issues presented by these
> transitions. I wasn't expecting this kind of fallout..

Let's just enable the cosmeticAnimations pref in these notification tests that are failing.
I went through the test failures on https://treeherder.mozilla.org/#/jobs?repo=try&revision=e6d640e7598e4894f8081dde1f7338afa27cd2ed&selectedJob=115009803 and have adjusted tests where appropriate to set the cosmeticAnimations pref to true. A number of the failures were due to a missing Services.jsm dependency, so I've added that in to the binding. 

New try push: 
https://treeherder.mozilla.org/#/jobs?repo=try&revision=695bacfdad1b539ff60abe1910846a02f9e334fa

Once completed, builds and logs will be available at:
https://archive.mozilla.org/pub/firefox/try-builds/sfoster@mozilla.com-695bacfdad1b539ff60abe1910846a02f9e334fa/
Comment on attachment 8887235 [details]
Bug 1380065 - Disable arrow-panel animations if the cosmeticAnimations pref is set to false.

Clearing r+ as this patch now touches lots of test files as well as loading the Services.jsm dependency in the arrowpanel binding.
Attachment #8887235 - Flags: review+ → review?(jaws)
Comment on attachment 8887235 [details]
Bug 1380065 - Disable arrow-panel animations if the cosmeticAnimations pref is set to false.

https://reviewboard.mozilla.org/r/158032/#review163904

::: commit-message-e0b08:5
(Diff revision 2)
> +Bug 1380065 - Disable arrow-panel animations if the cosmeticAnimations pref is set to false. r?jaws
> +
> +* Toggle animate=false attribute on arrow panels when toolkit.cosmeticAnimations.enabled is false
> +* Import Services in the arrowpanel binding as this is a new dependency
> +* Disable this pref during to remove a source of instability and timing-based test failures in chrome/UI tests.

s/during to/during tests to/

::: browser/base/content/test/alerts/browser_notification_tab_switching.js:11
(Diff revision 2)
> +var origAnimationsPrefValue = Services.prefs.getBoolPref("toolkit.cosmeticAnimations.enabled");
> +
> +// run these tests with notification animations enabled
> +Services.prefs.setBoolPref("toolkit.cosmeticAnimations.enabled", true);

Please use SpecialPowers.pushPrefEnv so we don't have to cache the original value, and this will also clean up after itself when the test completes.

http://searchfox.org/mozilla-central/rev/a83a4b68974aecaaacdf25145420e0fe97b7aa22/testing/specialpowers/content/specialpowersAPI.js#1037

::: toolkit/content/tests/chrome/test_arrowpanel.xul:197
(Diff revision 2)
> +    // Run this check with animations enabled
> +    let origPrefValue = Services.prefs.getBoolPref("toolkit.cosmeticAnimations.enabled");
> +    Services.prefs.setBoolPref("toolkit.cosmeticAnimations.enabled", true);

Tests like this where we want to put the pref back to its original value before the test has finished probably shouldn't use pushPrefEnv, though I'm not sure if we need to clean up so quickly in this test.

::: toolkit/content/widgets/popup.xml:478
(Diff revision 2)
> +        let Services = Components.utils.import("resource://gre/modules/Services.jsm", {}).Services;
> +        let animationsEnabled = Services.prefs.getBoolPref("toolkit.cosmeticAnimations.enabled");

Please use the Components.classes[1] here as I see that's more popular in the /toolkit/content/widgets. Sorry for misleading you.

[1] http://searchfox.org/mozilla-central/rev/a83a4b68974aecaaacdf25145420e0fe97b7aa22/toolkit/content/widgets/notification.xml#48-50
Attachment #8887235 - Flags: review?(jaws) → review+
Comment on attachment 8887235 [details]
Bug 1380065 - Disable arrow-panel animations if the cosmeticAnimations pref is set to false.

https://reviewboard.mozilla.org/r/158032/#review163904

> Tests like this where we want to put the pref back to its original value before the test has finished probably shouldn't use pushPrefEnv, though I'm not sure if we need to clean up so quickly in this test.

I fixed this to use SpecialPowers.set/setBoolPref, but otherwise left it as-is. We do want the rest of the test to run with the default test prefs. This last test specifically tests for transitionend, but we dont want to leave a booby trap for when someone adds a subsequent test. I didn't see an obvious way to use the asyn pushPrefEnv here without re-writing how the tests are invoked..
For reasons I don't fully understand, browser/base/content/test/tabcrashed/browser_noPermanentKey.js also seems to be sensitive to this pref change. I've re-enabled the pref in this test, as well as browser/base/content/test/general/browser_alltabslistener.js which appear to be the last stand-outs. 

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

Mike, I'm putting this on your radar as if this doesnt fix it I'm going to be looking for new ideas :) I'm trying not to boil the ocean with this patch - the idea was that disabling this pref would be a win for stability and the fastest path to getting 1352075 landed. If that turns out not to be the case I'll want to reconsider the approach here.
Flags: needinfo?(mconley)
That last try push looks pretty good, I'm going to try landing this. 

To summarize, the patch disables the toolkit.cosmeticAnimations.enable pref for tests, and makes the animated open/close behavior of arrow-panels contingent on this pref being enabled. The animations controlled by the cosmeticAnimations pref introduce unnecessary time and complexity where these UI components arent directly under test. For this reason, the patch disables the pref by default for all tests. A number of tests either explicitly looked for a transitionend event to know if for example a notification was visible, or were more subtly affected by this change. For each of those affected tests - and tests directly concerned with the animating behavior - I've re-enabled the pref to get them to pass again.
We're sorry, Autoland could not rebase your commits for you automatically. Please manually rebase your commits and try again.

hg error in cmd: hg rebase -s 7d9fb445a378 -d 394b3d22db19: rebasing 408556:7d9fb445a378 "Bug 1380065 - Disable arrow-panel animations if the cosmeticAnimations pref is set to false. r=jaws" (tip)
merging browser/base/content/test/performance/browser_tabclose_grow_reflows.js
merging browser/base/content/test/performance/browser_tabclose_reflows.js
merging browser/base/content/test/performance/browser_tabopen_reflows.js
merging browser/base/content/test/performance/browser_tabopen_squeeze_reflows.js
merging testing/profiles/prefs_general.js
merging toolkit/content/widgets/popup.xml
warning: conflicts while merging testing/profiles/prefs_general.js! (edit, then use 'hg resolve --mark')
unresolved conflicts (see hg resolve, then hg rebase --continue)
Pushed by sfoster@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/49c4c4a3fb81
Disable arrow-panel animations if the cosmeticAnimations pref is set to false. r=jaws
The browser_aboutStopReload.js failure is because it is a new test that wasn't in the previous pushes to tryserver but it should also get the toolkit.cosmeticAnimations.pref enabled for the test since it tests the presence and lack of animations.

I looked at the browser_ext_popup_select.js and don't see why this could have failed.
(In reply to Wes Kocher (:KWierso) from comment #28)
> I had to back this out for failures like
> https://treeherder.mozilla.org/logviewer.html#?job_id=116544674&repo=autoland
> 
> https://hg.mozilla.org/integration/autoland/rev/
> 24f784daaae26f082dc8b4a1222f93c3406e947f

Thanks for backing out :KWierso. I'll get a new patch together with the fix for the browser_aboutStopReload.js test. I've seen failures from browser_ext_popup_select.js as I've been testing the patch I landed, but like :jaws, I looked at the test and didn't see how this pref change could affect it. I can give it the same re-enable pref treatment and see how it goes, but it would be voodoo if it worked :)
Flags: needinfo?(sfoster)
Pushed by sfoster@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/7e0990dca8d1
Disable arrow-panel animations if the cosmeticAnimations pref is set to false. r=jaws
(In reply to Wes Kocher (:KWierso) from comment #34)
> sfoster asked for another backout, so here it is:
> https://hg.mozilla.org/integration/autoland/rev/
> 52b49a63629c78042d7683691fc46fb072a81fcb

Yep, thanks. Was too quick on the trigger there. Kris Maglione confirmed in irc that the browser_ext_popup_select.js test is indeed known to run better with animations enabled, so I'll update the patch and re-land.
Flags: needinfo?(sfoster)
Pushed by sfoster@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/2e0c2b3bc28b
Disable arrow-panel animations if the cosmeticAnimations pref is set to false. r=jaws
Pushed by sfoster@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/29131feeed60
Disable arrow-panel animations if the cosmeticAnimations pref is set to false. r=jaws
(In reply to Wes Kocher (:KWierso) from comment #38)
> Out again for eslint failures :(
> https://treeherder.mozilla.org/logviewer.html#?job_id=117198202&repo=autoland
> 
> https://hg.mozilla.org/integration/autoland/rev/
> c008db4b7874f353bc66a727d402127d3d2cef9b

Doh. /me installs pre-commit linting hook 
Once more with feeling :)
backed out for perma failures on windows like https://treeherder.mozilla.org/logviewer.html#?job_id=117267786&repo=autoland , sorry Sam!
Flags: needinfo?(sfoster)
I would strongly recommend a full try run, all builds and tests. At this point that's probably going to be better than us playing whackamole for another week.
Backout by cbook@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/95824646489a
Backed out changeset 29131feeed60 for windows 7 perma failure in test_bug884693.xul | Got expected message count - got 1, expected +0
Iteration: 56.3 - Jul 24 → 56.4 - Aug 1
(In reply to Wes Kocher (:KWierso) from comment #43)
> I would strongly recommend a full try run, all builds and tests. At this
> point that's probably going to be better than us playing whackamole for
> another week.

I had seen some failures in test_bug884693.xul over the last week, but after looking at what the test was testing, I dismissed them as noise. I can certainly do a full try run before attempting to re-land, but more importantly I have to treat my patch as guilty until proven innocent for every orange in each run. If you have any tips on how to use the tools we have to establish culpability and causality ahead of landing, I would appreciate them.
Flags: needinfo?(sfoster) → needinfo?(wkocher)
You can ask a sheriff to look at your try results before you do the actual landing.
Flags: needinfo?(wkocher)
Assignee: sfoster → nobody
Status: ASSIGNED → NEW
Iteration: 56.4 - Aug 1 → ---
Priority: P1 → P2
I'm putting this on the back-burner for now. It may not be practical at this point to toggle this pref off for all tests, especially as toolkit.cosmeticAnimations.enabled controls such a broad range of behavior across both browser and toolkit. However this exercise has highlighted some strange magic that exists today in our code and tests; signals of problems that are currently papered over. If we end up closing wontfix, we should first file separate bugs for these test failures - turning off cosmetic animations for tests is a thing we /should/ be able to do, and the fallout which doing so creates represents bad code smells at minimum, if not outright bugs.
Sorry I didn't get back in time to help with this. :( Clearing ni? due to comment 47.
Flags: needinfo?(mconley)
I think it'd be useful to at least land the non-test code here. Disabling the doorhanger animations seems like a sensible change for users who toggle that pref.
(In reply to Jim Porter (:squib) from comment #49)
> I think it'd be useful to at least land the non-test code here. Disabling
> the doorhanger animations seems like a sensible change for users who toggle
> that pref.

Agreed, I'm reversing the dependency though as I want to land the new animation first, then hook up the preference to control it. As we're going to expose this pref to users in bug 1357349 we do want to get tests running with this pref disabled sooner or later
No longer blocks: 1352075
Depends on: 1352075
Whiteboard: [photon-animation] → [reserve-photon-animation]
Priority: P2 → P3
Priority: P3 → P4
QA Contact: jwilliams → stefan.georgiev
Depends on: 1352069
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → DUPLICATE
You need to log in before you can comment on or make changes to this bug.