Introduce a pref that allows for disabling animations (toolkit.cosmeticAnimations.enabled)

VERIFIED FIXED in Firefox 55

Status

()

Firefox
Preferences
P1
normal
VERIFIED FIXED
3 months ago
20 hours ago

People

(Reporter: jaws, Assigned: squib)

Tracking

(Blocks: 4 bugs)

52 Branch
Firefox 55
Points:
2
Dependency tree / graph

Firefox Tracking Flags

(firefox55 fixed)

Details

(Whiteboard: [photon-animation])

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(1 attachment)

Similar to browser.tabs.animate, browser.fullscreen.animate, browser.download.animateNotifications, and alerts.disableSlidingEffect, we should have a pref that covers disabling/enabling UI animations in the browser. This pref could also cover the aforementioned prefs, disabling them if this pref is disabled (though not the other way around).

We will be adding other animations (see bug 1352068 and bug 1352063 for two examples) and this would prevent us from creating a pref for each and every animation.
See Also: → bug 556717, bug 456620

Updated

3 months ago
Whiteboard: [photon]

Updated

3 months ago
I don't know the backstory on the other/existing prefs, but it would be ideal to unify them to a single disable-animations pref as part of this.
(Assignee)

Comment 2

3 months ago
Taking this.
Assignee: nobody → squibblyflabbetydoo
Status: NEW → ASSIGNED

Comment 3

3 months ago
(In reply to Justin Dolske [:Dolske] from comment #1)
I still like having individual control over what animations I want and don't want, though. 

I disabled the tab anim, full screen anim and alert effect, but kept the download notification on since I do like that one. Even if I disliked it, with it disabled, it makes it impossible to know whether I finished a download or not when I have several downloads in progress. (The arrow will just stay blue)
(Assignee)

Comment 4

3 months ago
(In reply to Lily Rose from comment #3)
> I disabled the tab anim, full screen anim and alert effect, but kept the
> download notification on since I do like that one. Even if I disliked it,
> with it disabled, it makes it impossible to know whether I finished a
> download or not when I have several downloads in progress. (The arrow will
> just stay blue)

If we intend to support disabling animations, I think we'd want to be sure that disabling animations doesn't regress any functionality. That would probably mean having some other indicator of a completed download.

While this doesn't address the case of simply not liking a handful of animations, we plan to increase our support for no-animations (i.e. we'll test things to make sure everything works ok without animations) and I don't want to overburden the QA folks with a big pile of prefs to test. My tentative plan is to merge the prefs for tabs, fullscreen, and alerts and to keep the download notification separate. However, when we have a fix for the download notification working well without animations, we'll merge that into the master pref too.
(Assignee)

Comment 5

3 months ago
Just for future reference, here are all the bugs that introduced the prefs enumerated in comment 0:

browser.tabs.animate: bug 380960
browser.fullscreen.animate: bug 240859 (refactored in bug 1176233)
browser.download.animateNotifications: bug 861613 (added to firefox.js in bug 897930)
alerts.disableSlidingEffect: bug 355846

While the discussion in many of these is quite long, I haven't seen anything that adds further complexity to this bug, aside from the issues already mentioned around the download animation. If I come across any further wrinkles, I'll update this bug with the details.

Updated

3 months ago
Blocks: 1354473

Comment 6

3 months ago
(In reply to Jim Porter (:squib) from comment #5)
> Just for future reference, here are all the bugs that introduced the prefs
> enumerated in comment 0:
> 
> browser.tabs.animate: bug 380960
> browser.fullscreen.animate: bug 240859 (refactored in bug 1176233)
> browser.download.animateNotifications: bug 861613 (added to firefox.js in
> bug 897930)
> alerts.disableSlidingEffect: bug 355846
> 
> While the discussion in many of these is quite long, I haven't seen anything
> that adds further complexity to this bug, aside from the issues already
> mentioned around the download animation. If I come across any further
> wrinkles, I'll update this bug with the details.

Hi Jim,

What is your schedule of this bug ?

The bug 1354473 is planing to add one checkbox in about:preferences.
That checkout is to toggle on/off all firefox UI animations, which is exactly what this bug's pref is doing.
And the target schedule of the bug 1354473 is the 2nd 55.
So we would like to check with you what is your target schedule for this bug - a master pref of Firefox UI animations.
If your schedule couldn't align with the bug 1354473, then we probably will toggle browser.tabs.animate, browser.fullscreen.animate, browser.download.animateNotifications, alerts.disableSlidingEffect for now, then turn into the master animation pref after this bug is completed.

Thank you
Flags: needinfo?(squibblyflabbetydoo)
(Assignee)

Comment 7

3 months ago
I was hoping to have a patch up today, but I didn't quite finish it. It should be up for review early this week (and will hopefully land not long after).
Flags: needinfo?(squibblyflabbetydoo)

Comment 8

3 months ago
(In reply to Jim Porter (:squib) from comment #7)
> I was hoping to have a patch up today, but I didn't quite finish it. It
> should be up for review early this week (and will hopefully land not long
> after).
Thank you. This sounds good. This master pref is going to have a usage in about:preferences.
(Reporter)

Updated

3 months ago
Whiteboard: [photon] → [photon-animation]
Comment hidden (mozreview-request)
(Reporter)

Updated

3 months ago
Points: --- → 2
As an aside, I wonder how feasible it'd be to have a special @media selector that matches when the animation pref is true so that we don't have to have JS do all of the logic for choosing when to apply animations.
(In reply to Mike Conley (:mconley) from comment #10)
> As an aside, I wonder how feasible it'd be to have a special @media selector
> that matches when the animation pref is true so that we don't have to have
> JS do all of the logic for choosing when to apply animations.

There's @supports -moz-bool-pref from bug 1259889. Would be nice if it worked in chrome stylesheets.
(In reply to Dão Gottwald [::dao] from comment #11)
> (In reply to Mike Conley (:mconley) from comment #10)
> > As an aside, I wonder how feasible it'd be to have a special @media selector
> > that matches when the animation pref is true so that we don't have to have
> > JS do all of the logic for choosing when to apply animations.
> 
> There's @supports -moz-bool-pref from bug 1259889. Would be nice if it
> worked in chrome stylesheets.

Which is bug 1267890 apparently.

Updated

3 months ago
Flags: qe-verify+
Priority: -- → P1
QA Contact: jwilliams
Comment hidden (mozreview-request)
(Assignee)

Updated

2 months ago
Attachment #8857087 - Flags: review?(jaws)
(Assignee)

Comment 14

2 months ago
I know this won't pass review, but I'm not 100% sure what to do with the one part that I know is failing try from my patch, and some of the other test failures worry me, but they don't *seem* to be caused by this patch...

The one failure that I know is my fault is:

> TEST-UNEXPECTED-ERROR | /home/worker/checkouts/gecko/browser/components/nsBrowserGlue.js:1570:15 | Method 'BG__migrateUI' has a complexity of 70. (complexity) 

Not sure how to fix that, since splitting that function into smaller chunks seems kind of pointless to be honest...
(In reply to Jim Porter (:squib) from comment #14)
> > TEST-UNEXPECTED-ERROR | /home/worker/checkouts/gecko/browser/components/nsBrowserGlue.js:1570:15 | Method 'BG__migrateUI' has a complexity of 70. (complexity) 
> 
> Not sure how to fix that, since splitting that function into smaller chunks
> seems kind of pointless to be honest...

We can just remove old UI migration steps. Filed bug 1356129.
Depends on: 1356129
(Reporter)

Comment 16

2 months ago
mozreview-review
Comment on attachment 8857087 [details]
Bug 1352069 - Introduce a pref that allows for disabling cosmetic animations

https://reviewboard.mozilla.org/r/128990/#review132542

::: browser/components/nsBrowserGlue.js:1570
(Diff revision 2)
>    _migrateUI: function BG__migrateUI() {
> -    const UI_VERSION = 43;
> +    const UI_VERSION = 44;

Now that bug 1356129 has landed, please compute the cyclomatic complexity of your patch on top of the one from bug 1356129 and update /browser/components/.eslintrc.js with the new (lower) max.

You can compute it by running `./mach eslint browser/components/nsBrowserGlue.js` and then using the score that it returns for this function.

::: browser/components/nsBrowserGlue.js:1909
(Diff revision 2)
> +      let animate = (Services.prefs.getBoolPref("browser.tabs.animate") &&
> +                     Services.prefs.getBoolPref("browser.fullscreen.animate") &&
> +                     Services.prefs.getBoolPref("alerts.disableSlidingEffect"));

no need for the parens here.

also, i feel like when this lands we should send an email to firefox-dev describing this preference change since some users who only had browser.tabs.animate=false will now be reverted back to browser.tabs.animate=true.
Attachment #8857087 - Flags: review?(jaws) → review+
(In reply to Jared Wein [:jaws] (please needinfo? me) from comment #16)
> Now that bug 1356129 has landed, please compute the cyclomatic complexity of
> your patch on top of the one from bug 1356129 and update
> /browser/components/.eslintrc.js with the new (lower) max.

Please do not do this while we're actively discussing what the path forward with this method should be in bug 1326071.
Asa noted that's there's also an ancient browser.preferences.animateFadeIn pref, which I think is obsolete. Filed bug 1356710 for removing it.
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Comment 22

2 months ago
mozreview-review-reply
Comment on attachment 8857087 [details]
Bug 1352069 - Introduce a pref that allows for disabling cosmetic animations

https://reviewboard.mozilla.org/r/128990/#review132542

> Now that bug 1356129 has landed, please compute the cyclomatic complexity of your patch on top of the one from bug 1356129 and update /browser/components/.eslintrc.js with the new (lower) max.
> 
> You can compute it by running `./mach eslint browser/components/nsBrowserGlue.js` and then using the score that it returns for this function.

Filed bug 1356322 to handle this once and for all.

> no need for the parens here.
> 
> also, i feel like when this lands we should send an email to firefox-dev describing this preference change since some users who only had browser.tabs.animate=false will now be reverted back to browser.tabs.animate=true.

Fixed the parens (and I also fixed the logic of the migration, since "alerts.disableSlidingEffect" is inverted compared to the others).

I'll send a note to firefox-dev, but the behavior now should be: if *any* of the existing cosmetic animations are disabled, we'll disable all of them after we migrate the pref.
(Assignee)

Comment 23

2 months ago
Just realized I did migration wrong; I'll have a patch up tomorrow that fixes it.

Updated

2 months ago
Blocks: 1357349
Comment hidden (mozreview-request)
(Reporter)

Comment 25

2 months ago
mozreview-review
Comment on attachment 8857087 [details]
Bug 1352069 - Introduce a pref that allows for disabling cosmetic animations

https://reviewboard.mozilla.org/r/128990/#review133768

::: browser/components/nsBrowserGlue.js:1856
(Diff revisions 5 - 6)
> -      // Merge the various cosmetic animation prefs into one.
> -      let animate = Services.prefs.getBoolPref("browser.tabs.animate") &&
> -                    Services.prefs.getBoolPref("browser.fullscreen.animate") &&
> -                    !Services.prefs.getBoolPref("alerts.disableSlidingEffect");
> +      let getOldBoolPref = (pref, value) => {
> +        if (Services.prefs.prefHasUserValue(pref)) {
> +          value = Services.prefs.getBoolPref(pref);
> +          Services.prefs.clearUserPref(pref);
> +        }

Please use Preferences.jsm's Preferences.get which allow's for default values:
http://searchfox.org/mozilla-central/source/toolkit/modules/Preferences.jsm#50

The UI migration for < 41 imports that same JSM.
(In reply to Jared Wein [:jaws] (please needinfo? me) from comment #25)
> Comment on attachment 8857087 [details]
> Bug 1352069 - Introduce a pref that allows for disabling cosmetic animations
> 
> https://reviewboard.mozilla.org/r/128990/#review133768
> 
> ::: browser/components/nsBrowserGlue.js:1856
> (Diff revisions 5 - 6)
> > -      // Merge the various cosmetic animation prefs into one.
> > -      let animate = Services.prefs.getBoolPref("browser.tabs.animate") &&
> > -                    Services.prefs.getBoolPref("browser.fullscreen.animate") &&
> > -                    !Services.prefs.getBoolPref("alerts.disableSlidingEffect");
> > +      let getOldBoolPref = (pref, value) => {
> > +        if (Services.prefs.prefHasUserValue(pref)) {
> > +          value = Services.prefs.getBoolPref(pref);
> > +          Services.prefs.clearUserPref(pref);
> > +        }
> 
> Please use Preferences.jsm's Preferences.get which allow's for default
> values:
> http://searchfox.org/mozilla-central/source/toolkit/modules/Preferences.
> jsm#50

nsIPrefBranch.getBoolPref supports default values too (since bug 1338306).
Comment hidden (mozreview-request)
(Assignee)

Comment 28

2 months ago
mozreview-review-reply
Comment on attachment 8857087 [details]
Bug 1352069 - Introduce a pref that allows for disabling cosmetic animations

https://reviewboard.mozilla.org/r/128990/#review133768

> Please use Preferences.jsm's Preferences.get which allow's for default values:
> http://searchfox.org/mozilla-central/source/toolkit/modules/Preferences.jsm#50
> 
> The UI migration for < 41 imports that same JSM.

Per Florian's comment on the bug and our IRC discussion, I'm using nsIPrefBranch's fancy new default pref value thing now.

Updated

2 months ago
Iteration: --- → 55.4 - May 1

Comment 29

2 months ago
Pushed by squibblyflabbetydoo@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/77760e0b239f
Introduce a pref that allows for disabling cosmetic animations r=jaws

Comment 30

2 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/77760e0b239f
Status: ASSIGNED → RESOLVED
Last Resolved: 2 months ago
status-firefox55: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 55
Depends on: 1358059
(Assignee)

Updated

2 months ago
Depends on: 1358197
For verification purposes, what is the name of the new pref that would disable animations? I looked through the comments here and I did seem to see it.
Flags: needinfo?(jaws)
(Assignee)

Comment 32

2 months ago
toolkit.cosmeticAnimations.enabled
Everything looks good on the current version of nightly. Marking as Verified.
Status: RESOLVED → VERIFIED
Flags: needinfo?(jaws)

Updated

2 months ago
Flags: qe-verify+
Summary: Introduce a pref that allows for disabling animations → Introduce a pref that allows for disabling animations (toolkit.cosmeticAnimations.enabled)
(Assignee)

Updated

2 months ago
Blocks: 1362128

Comment 34

a month ago
Maybe this is not the right bug but disabling animations and sliding tabs to reorder still shows animation.
Just the animation is a bit slower, just disable animations and slide tabs around , there is an animation happening 

new firefox 55 with
toolkit.cosmeticAnimations.enabled false
browser.tabs.animate false
Flags: needinfo?(jaws)
Can you please file a new bug for this so it can be investigated separately from this bug (1352069)?
Flags: needinfo?(jaws)

Comment 36

a month ago
(In reply to Jared Wein [:jaws] (please needinfo? me) from comment #35)
> Can you please file a new bug for this so it can be investigated separately
> from this bug (1352069)?

how do I do that?
Already provided the steps
Flags: needinfo?(jaws)
I could file the bug for you, but after you file your first bug you will learn how easy it is and will be able to file more bugs as you find them. Visit https://bugzilla.mozilla.org/enter_bug.cgi and the form should step you through filing your bug.
Flags: needinfo?(jaws)

Comment 38

a month ago
(In reply to Jared Wein [:jaws] (please needinfo? me) from comment #37)
> I could file the bug for you, but after you file your first bug you will
> learn how easy it is and will be able to file more bugs as you find them.
> Visit https://bugzilla.mozilla.org/enter_bug.cgi and the form should step
> you through filing your bug.


Bug 1355507

Tab moving/reordering should always slide tabs in to the final place and should use the photon animation curve in doing so. Please see the attached UX design video for better description.

was about to file a bug on your request and found this,

it's not using the set pref

So what now?
Flags: needinfo?(jaws)

Updated

a month ago
Flags: needinfo?(dao+bmo)
Yes, that is the bug that introduced the sliding you are talking about. If you file your new bug, you can mark that new bug as "blocking" bug 1355507 and bug 1352069. I can also set that on the bug for you.
Flags: needinfo?(jaws)
Flags: needinfo?(dao+bmo)
Depends on: 1366060
(In reply to Mike Conley (:mconley) from comment #10)
> As an aside, I wonder how feasible it'd be to have a special @media selector
> that matches when the animation pref is true so that we don't have to have
> JS do all of the logic for choosing when to apply animations.

There is a @media(prefers-reduced-motion) in CSS Media Queries Level 5 and bug 1365045 to implement it.
You need to log in before you can comment on or make changes to this bug.