1366060 - Tab moving/reordering animations should be disabled if toolkit.cosmeticAnimations.enabled=false

Status: VERIFIED FIXED

(Firefox :: Tabbed Browser, defect, P1)

Description

[u595641]

User Agent: Mozilla/5.0 (Windows; Windows NT 5.1; rv:54.0) Gecko/20100101 Firefox/54.0
Build ID: 20170518030213

Steps to reproduce:

As per request bug 1352069#c39 disabling animations and sliding tabs to reorder still shows animations.
Just the animation is a bit slower, just disable animations and slide tabs around , there is an animation happening even in default settings innew firefox 55 with
toolkit.cosmeticAnimations.enabled false
browser.tabs.animate false

have a very old laptop and animations make it slower like tab/bookmark/panel animations and disabling it improves performance but this bug is making things painful.

Comment 1

[u595641]

Filed please fill what else is required :)

Comment 2

[Jared Wein [:jaws] (please needinfo? me)]

Thanks!

Comment 3

[Virtual_ManPL [:Virtual] 🇵🇱 - (please needinfo? me - so I will see your comment/reply/question/etc.)]

FYI - "browser.tabs.animate" was removed,
there is now only "toolkit.cosmeticAnimations.enabled",
so I'm fixing the summary.

Comment 4

[:Gijs (he/him)]

We should probably not stop the reordering, just remove animations...

Comment 5

[Jared Wein [:jaws] (please needinfo? me)]

Attachment: Bug 1366060 - Disable the tab reordering animation if the cosmeticAnimations pref is disabled.

Review commit: https://reviewboard.mozilla.org/r/151068/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/151068/

Comment 7

[Jared Wein [:jaws] (please needinfo? me)]

[Tracking Requested - why for this release]: The tab reordering animation was introduced in 55 and having a way to disable it in 55 will be helpful for any users who had other UI animations disabled (also see bug 1374052).

The attached patch is also low-risk and very simple.

Comment 8

[:Gijs (he/him)]

Comment on attachment 8879729 [details]
Bug 1366060 - Disable the tab reordering animation if the cosmeticAnimations pref is disabled.

https://reviewboard.mozilla.org/r/151068/#review156090

::: browser/base/content/tabbrowser.xml:7042
(Diff revision 1)
>            let dropIndex = "animDropIndex" in draggedTab._dragData &&
>                            draggedTab._dragData.animDropIndex;
>            if (dropIndex && dropIndex > draggedTab._tPos)
>              dropIndex--;
>  
> -          if (oldTranslateX && oldTranslateX != newTranslateX) {
> +          let animate = Services.prefs.getBoolPref("toolkit.cosmeticAnimations.enabled");

We check this pref in 2 other places already. Maybe we should just have a defineLazyPreferenceGetter() on the tabbrowser for this?

::: browser/base/content/tabbrowser.xml:7043
(Diff revision 1)
>                            draggedTab._dragData.animDropIndex;
>            if (dropIndex && dropIndex > draggedTab._tPos)
>              dropIndex--;
>  
> -          if (oldTranslateX && oldTranslateX != newTranslateX) {
> +          let animate = Services.prefs.getBoolPref("toolkit.cosmeticAnimations.enabled");
> +          if (oldTranslateX && oldTranslateX != newTranslateX && animate) {

Ugh. I mean, this code isn't hot, so I guess it doesn't matter much, but you could move the `dropIndex` changes to the top of the else if clause, and then check the animate bool first, before bothering doing the calculations for the translateX value. That said, because it would split the if condition, the complexity of the code would go up as well, and we can't return early from this method because we're meant to also delete the `_dragData` off the draggedTab all the way at the bottom, and we're already in an if statement so triple-nesting them (which would be the result) doesn't seem attractive either.

Really, this code needs refactoring so it isn't a single 100-line method, but never mind that for now... Maybe file a followup? Trying to handle all the different drops in a single function without delegating to more specific ones makes this code harder to work with, IMO.

Comment 9

[Jared Wein [:jaws] (please needinfo? me)]

Thanks for the quick review!

In trying to have a simple patch that we can uplift to 55, I'll file follow up bugs for both of your issues.

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/0ba540ea0d54
Disable the tab reordering animation if the cosmeticAnimations pref is disabled. r=Gijs

Comment 11

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/0ba540ea0d54

Comment 12

[Julien Cristau [:jcristau]]

Tracking for 55, I guess you'll request uplift once this is verified?

Comment 13

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Hi Jared, could you please nominate a patch for uplift to Beta55? Thanks!

Comment 14

[Jared Wein [:jaws] (please needinfo? me)]

Comment on attachment 8879729 [details]
Bug 1366060 - Disable the tab reordering animation if the cosmeticAnimations pref is disabled.

Approval Request Comment
[Feature/Bug causing the regression]: The tab reordering animation was introduced in 55 and having a way to disable it in 55 will be helpful for any users who had other UI animations disabled (also see bug 1374052).
[User impact if declined]: Users will be unable to disable tab reordering animations
[Is this code covered by automated tests?]: no
[Has the fix been verified in Nightly?]: no
[Needs manual test from QE? If yes, steps to reproduce]: no, manually tested locally and the patch is very simple 
[List of other uplifts needed for the feature/fix]: none
[Is the change risky?]: no
[Why is the change risky/not risky?]:
[String changes made/needed]:

Comment 15

[Julien Cristau [:jcristau]]

Comment on attachment 8879729 [details]
Bug 1366060 - Disable the tab reordering animation if the cosmeticAnimations pref is disabled.

looks reasonable, let's get this in 55.0b6

Comment 16

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/bbf775f1de45

Comment 17

[Stefan [:StefanG_QA]]

Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:56.0) Gecko/20100101 Firefox/56.0 (20170714030205)

This issue is verified as fixed with Nightly build 56.0a1 from (20170714030205) and on 55.0b7 from (20170706085221).

Comment 18

[Charles Belov]

Re-broken in Firefox 62.0.3

Comment 19

[Julien Cristau [:jcristau]]

(In reply to Charles Belov from comment #18)
> Re-broken in Firefox 62.0.3

Please file a new bug for this.

Comment 20

[Bill Mayhew]

broken in 74 - toolkit.cosmeticAnimations.enabled appears to be IGNORED - no way to suppress faulty animations!

Comment 21

[:Gijs (he/him)]

(In reply to Bill Mayhew from comment #20)

broken in 74 - toolkit.cosmeticAnimations.enabled appears to be IGNORED - no way to suppress faulty animations!

Tracked in bug 1499937