Closed Bug 1366060 Opened 7 years ago Closed 7 years ago

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

Categories

(Firefox :: Tabbed Browser, defect, P1)

55 Branch
defect

Tracking

()

VERIFIED FIXED
Firefox 56
Iteration:
56.1 - Jun 26
Tracking Status
firefox54 --- unaffected
firefox55 + verified
firefox56 --- verified

People

(Reporter: u595641, Assigned: jaws)

References

(Depends on 1 open bug, Regression)

Details

(Keywords: regression, Whiteboard: [reserve-photon-animation])

Attachments

(1 file)

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.
Filed please fill what else is required :)
Flags: needinfo?(jaws)
Thanks!
Blocks: 1355507, 1352069
Status: UNCONFIRMED → NEW
Component: Untriaged → Tabbed Browser
Ever confirmed: true
Flags: needinfo?(jaws)
FYI - "browser.tabs.animate" was removed,
there is now only "toolkit.cosmeticAnimations.enabled",
so I'm fixing the summary.
Summary: Tab moving/reordering should be disabled if toolkit.cosmeticAnimations.enabled false & browser.tabs.animate false → Tab moving/reordering should be disabled if toolkit.cosmeticAnimations.enabled=false
We should probably not stop the reordering, just remove animations...
Summary: Tab moving/reordering should be disabled if toolkit.cosmeticAnimations.enabled=false → Tab moving/reordering animations should be disabled if toolkit.cosmeticAnimations.enabled=false
Blocks: 1367136
Assignee: nobody → jaws
Status: NEW → ASSIGNED
[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 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.
Attachment #8879729 - Flags: review?(gijskruitbosch+bugs) → review+
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.
Depends on: 1375040
Depends on: 1375041
Pushed by jwein@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/0ba540ea0d54
Disable the tab reordering animation if the cosmeticAnimations pref is disabled. r=Gijs
Iteration: --- → 56.1 - Jun 26
Flags: qe-verify?
Priority: -- → P1
Whiteboard: [reserve-photon-animation]
https://hg.mozilla.org/mozilla-central/rev/0ba540ea0d54
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 56
Flags: qe-verify? → qe-verify+
QA Contact: jwilliams
Tracking for 55, I guess you'll request uplift once this is verified?
Hi Jared, could you please nominate a patch for uplift to Beta55? Thanks!
Flags: needinfo?(jaws)
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]:
Flags: needinfo?(jaws)
Attachment #8879729 - Flags: approval-mozilla-beta?
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
Attachment #8879729 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
QA Contact: jwilliams → stefan.georgiev
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).
Status: RESOLVED → VERIFIED
Re-broken in Firefox 62.0.3
(In reply to Charles Belov from comment #18)
> Re-broken in Firefox 62.0.3

Please file a new bug for this.
Depends on: 1499937
No longer blocks: 1355507
Regressed by: 1355507
Keywords: regression

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

(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

Keywords: regression
Has Regression Range: --- → yes
You need to log in before you can comment on or make changes to this bug.