Closed Bug 1352069 Opened 7 years ago Closed 7 years ago

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

Categories

(Firefox :: Settings UI, enhancement, P1)

Product:
Firefox
Component:
Settings UI
Version:
52 Branch
Type:
enhancement
Points:
2

Tracking

()

Status:
VERIFIED FIXED
Milestone:
Firefox 55
Iteration:
55.4 - May 1
Tracking Flags:
Tracking Status
firefox55 --- fixed

People

(Reporter: jaws, Assigned: squib)

References

(Depends on 1 open bug, Blocks 4 open bugs)

Details

(Whiteboard: [photon-animation])

Attachments

(1 file)

Bug 1352069 - Introduce a pref that allows for disabling cosmetic animations
(deleted), text/x-review-board-request
jaws
: review+
Details 
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.
Whiteboard: [photon]
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.
Taking this.
Assignee: nobody → squibblyflabbetydoo
Status: NEW → ASSIGNED
(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)
(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.
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.
Blocks: 1354473
(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)
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)
(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.
Whiteboard: [photon] → [photon-animation]
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.
Flags: qe-verify+
Priority: -- → P1
QA Contact: jwilliams
Attachment #8857087 - Flags: review?(jaws)
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
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 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.
Just realized I did migration wrong; I'll have a patch up tomorrow that fixes it.
Blocks: 1357349
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 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.
Iteration: --- → 55.4 - May 1
Pushed by squibblyflabbetydoo@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/77760e0b239f
Introduce a pref that allows for disabling cosmetic animations r=jaws
https://hg.mozilla.org/mozilla-central/rev/77760e0b239f
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
status-firefox55: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 55
Depends on: 1358059
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)
toolkit.cosmeticAnimations.enabled
Everything looks good on the current version of nightly. Marking as Verified.
Status: RESOLVED → VERIFIED
Flags: needinfo?(jaws)
Flags: qe-verify+
Summary: Introduce a pref that allows for disabling animations → Introduce a pref that allows for disabling animations (toolkit.cosmeticAnimations.enabled)
Blocks: 1362128
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)
(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)
(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)
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.
Will this new property also control the animations mentioned in bug 556717 and bug 601546 (that currently do not have a dedicated property), or it would have to be introduced for every case one-by-one?
toolkit.cosmeticAnimations.enabled = false  is not disabling all animations in the UI as expected:

1) The animation that occurs when the pointer is hovered over an unfocused tab.  It still animates a "growing" grey line across the top of the tab.
2) The animated "busy" icon that appears when loading or reloading a tab (we need something to indicate busy, but it could be far slower and less intense)
3) The closing of a tab still animates the resizing of the remaining tabs (try closing a tab in the middle of others)
4) The "fade in" and "fade out" animations when hovering over a link and the opening/closing of traditional menus or right-click menus (this might be my window manager in KDE)

Shouldn't this setting stop those, too?  I am unable to find a bug reports that relate to these, but would be happy to create them if appropriate....
(In reply to crxssi from comment #42)
> toolkit.cosmeticAnimations.enabled = false  is not disabling all animations
> in the UI as expected:[...] Shouldn't this setting stop those, too?

I forgot to add the now 16-year old bug 111373 report seeking to get Firefox to stop animating favicons, which are essentially part of the UI.  This should also be covered under toolkit.cosmeticAnimations.enabled
Depends on: 1442238
Blocks: 1470776
No longer depends on: 1470776
Blocks: 1417776, 1380065
Blocks: 1409825
Blocks: 1446713
Blocks: 1431237
Blocks: 1478597
Not working in 62.0.3
(In reply to Charles Belov from comment #44)
> Not working in 62.0.3

Which animations are still happening when you have the pref set to false?
Flags: needinfo?(charles.belov)
(In reply to Jared Wein [:jaws] (Regression Engineering Owner for 65) (please needinfo? me) from comment #45)
> (In reply to Charles Belov from comment #44)
> > Not working in 62.0.3
> 
> Which animations are still happening when you have the pref set to false?

I can't speak for Charles Belov, but all those in comments 42 and 43.  Nothing has changed with any of those through Firefox 62; they all still animate with the pref set to false.
(In reply to crxssi from comment #46)
> > Which animations are still happening when you have the pref set to false?
> 
> I can't speak for Charles Belov, but all those in comments 42 and 43. 
> Nothing has changed with any of those through Firefox 62; they all still
> animate with the pref set to false.

It would be good to file each as a separate bug so we can debate them individually. They represent quite different problems even if they all fall under the general umberally of "animation". You are welcome to make a case for each and why having the means to disable or dial back each animation is important. I don't agree that all those should fall under this one pref. If the pref needs renaming to better describe what it is intended to do, we could consider that also.
While I'm perfectly fine with being able to fine-tune each of the settings, I think those who want to turn off all the animations (like me), would also prefer a way to do this in a single step, as these animations still have one thing in common: they are wasting time.
(In reply to LC from comment #48)
> While I'm perfectly fine with being able to fine-tune each of the settings,
> I think those who want to turn off all the animations (like me), would also
> prefer a way to do this in a single step, as these animations still have one
> thing in common: they are wasting time.

And distracting, and slowing down rendering, using battery (if mobile), and consuming bandwidth (which slows everything) if using Firefox on a thin client or through any type of remote desktop (X11 remote, ssh, citrix, RDP, whatever).

I am fine with whatever preferences that give the option to turn them off, all, separate, or in groups; even better if there are sub-options for "reducing" their impact (like frame rate, run-through-once, or animate never).
My preference would be that there is one setting that disables *all* UI animations. And that said, in Firefox 63, I would expect this to be set by my system preference in Windows 10 to not animate (which I believe is a separate issue) and not to need a separate preference.

Now reporting on Firefox 63, here are the remaining animations that I notice which would be covered by this issue:

- Tabs
- Click anything in the right-hand >> or hamburger menus with a > indicating an additional menu level
- Click < to return to the previous menu level
- Opening the left sidebar
- Clicking the V in the history sidebar to show the history menu
Flags: needinfo?(charles.belov)
(In reply to Charles Belov from comment #50)
> My preference would be that there is one setting that disables *all* UI
> animations. And that said, in Firefox 63, I would expect this to be set by
> my system preference in Windows 10 to not animate (which I believe is a
> separate issue) and not to need a separate preference.

That is the eventual goal, filed as bug 1478597. 

> Now reporting on Firefox 63, here are the remaining animations that I notice
> which would be covered by this issue:
> 
> - Tabs
> - Click anything in the right-hand >> or hamburger menus with a > indicating
> an additional menu level
> - Click < to return to the previous menu level
> - Opening the left sidebar
> - Clicking the V in the history sidebar to show the history menu

If you haven't already, please flie bugs for these issues. This bug is already verified fixed so no further work will take place here.
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: