595804 - Add a pref to disable Panorama zoom-in/out animation

Status: VERIFIED FIXED

(Firefox Graveyard :: Panorama, enhancement, P3)

Description

[Gomita]

User-Agent:       Mozilla/5.0 (Windows NT 5.1; rv:2.0b6pre) Gecko/20100912 Firefox/4.0b6pre
Build Identifier: Mozilla/5.0 (Windows NT 5.1; rv:2.0b6pre) Gecko/20100912 Firefox/4.0b6pre

I hope we can pref'ed off the zoom-in animation when selecting a tab from "Group Your Tabs" panel.

Reproducible: Always

Steps to Reproduce:
1.Open 'Group Your Tabs' panel
2.Select a tab
Actual Results:  
Switch to the tab with zoom-in animation

Expected Results:  
Users can choose the behavior when selecting a tab;
1) Switch to the tab with zoom-in animation (default)
2) Switch to the tab immediately without any animation

I prefer the 2nd behavior since I feel that the zoom-in animation takes too much time.

Comment 1

[Michael Yoshitaka Erlewine [:mitcho]]

I agree this would be appropriate as an about:config pref.

Comment 2

[Aza Raskin [:aza]]

+1 on about:config

Comment 3

[Michael Yoshitaka Erlewine [:mitcho]]

Attachment: Patch v1, requires 593157 patch to apply first

Comment 4

[Michael Yoshitaka Erlewine [:mitcho]]

Note for Ian: this patch I think is complete, except for the fact that, for me, it disables the option-space key command for zooming into (well, no zooming, but opening) a tab from Panoarama. If you have an idea of how to fix that, I'd appreciate it.

Comment 5

[Ian Gilman [:iangilman]]

Comment on attachment 476711 [details] [diff] [review]
Patch v1, requires 593157 patch to apply first

* Wrapping a setTimeout around the activeTab.zoomIn in the spacebar handler in UIManager._setTabViewFrameKeyHandlers fixes the issue with option+space not exiting the Panorama UI while animation is turned off. Perhaps there's some limitation on switching panels in a xul:deck while you're in the key event handler for the panel you're switching away from? Though if this were the case, we'd have the same problem with the return key and its ilk, but we don't. Anyway, there's a clue for you. It works; just would be nice to know why. 

* If you switch the pref off while running Minefield, when you go back to the Panorama UI you've got a "zoom prep" tab sitting there that never resolves itself. This should be fixed.

-  useConsole: true, // as opposed to dump
+  useConsole: false, // as opposed to dump

Take this out of the final patch.

Otherwise looks great.

Comment 6

[Michael Yoshitaka Erlewine [:mitcho]]

Attachment: Patch v2

Fixed the logging, and I believe I fixed the zoom prep issue.

As for the ctrl-space not working, the key event is being fired. I tracked it down all the way to UI.hideTabView, behaving correctly, and I couldn't for the life of me figure out why UI.hideTabView was not working as expected. I just wrapped a setTimeout around onZoomDone in the no-animation case. If we land this as is, we ought to file a bug for this, just for the future.

Comment 7

[Ian Gilman [:iangilman]]

Comment on attachment 477819 [details] [diff] [review]
Patch v2

You should add a comment to that setTimeout to explain why it exists. 

Also, seems like this patch needs a test?

Otherwise looks great.

Comment 8

[Ian Gilman [:iangilman]]

(In reply to comment #6)
> As for the ctrl-space not working, the key event is being fired. I tracked it
> down all the way to UI.hideTabView, behaving correctly, and I couldn't for the
> life of me figure out why UI.hideTabView was not working as expected. I just
> wrapped a setTimeout around onZoomDone in the no-animation case. If we land
> this as is, we ought to file a bug for this, just for the future.

I've filed bug 599157 to follow up on this.

Also, I ran across the same issue while fixing bug 595374, so it includes a setTimeout in the key handler, which I think a better place than in the onZoomDone. Perhaps we can say the patch for this bug requires the patch for 595374?

Comment 9

[Michael Yoshitaka Erlewine [:mitcho]]

Hmm, I would prefer to put the setTimeout on the onZoomDone so that setTimeout is only used in the case where we have the animation off. However, if you've noticed that it helps to put the whole routine (animation on or off) in a setTimeout, then we should go with that and drop the patch in here.

Comment 10

[Ian Gilman [:iangilman]]

(In reply to comment #9)
> Hmm, I would prefer to put the setTimeout on the onZoomDone so that setTimeout
> is only used in the case where we have the animation off. However, if you've
> noticed that it helps to put the whole routine (animation on or off) in a
> setTimeout, then we should go with that and drop the patch in here.

No, you're right... I wasn't thinking about that. I'll move mine to a lower level as well (it's for flipping app tabs, so it's not in the same code). Thanks!

Comment 11

[Michael Yoshitaka Erlewine [:mitcho]]

Attachment: Patch v3, now with a test

Comment 12

[Michael Yoshitaka Erlewine [:mitcho]]

Comment on attachment 478714 [details] [diff] [review]
Patch v3, now with a test

Adding Raymond for additional feedback: for some reason this patch seems to make the bug595943 test choke up.

Comment 13

[Raymond Lee [:raymondlee]]

Comment on attachment 478714 [details] [diff] [review]
Patch v3, now with a test

+  function wrapup() {
+    prefsBranch.clearUserPref("animate_zoom");
+    ok(animateZoom(), "it's true by default again");

You need to hide the tabview before calling finish(), otherwise, it would stop bug595943 test from working. 

+    finish();
+  }
+

Comment 14

[Michael Yoshitaka Erlewine [:mitcho]]

Attachment: Patch v4, fixed test as per Raymond's comment

Sent to try: 97b2d74743a7

Comment 15

[Ian Gilman [:iangilman]]

Comment on attachment 478780 [details] [diff] [review]
Patch v4, fixed test as per Raymond's comment

Test looks good

Comment 16

[Michael Yoshitaka Erlewine [:mitcho]]

Passed try!

Comment 17

[Ian Gilman [:iangilman]]

(In reply to comment #4)
> Note for Ian: this patch I think is complete, except for the fact that, for me,
> it disables the option-space key command for zooming into (well, no zooming,
> but opening) a tab from Panoarama. If you have an idea of how to fix that, I'd
> appreciate it.

Looks like this is no longer an issue now that we're using command+e... try it for yourself, but I think you can remove the timeout.

Comment 18

[Justin Dolske [:Dolske]]

Comment on attachment 478780 [details] [diff] [review]
Patch v4, fixed test as per Raymond's comment

>+++ b/browser/base/content/tabview/tabitems.js
...
>+      let animateZoom = true;
>+      if (gPrefBranch.prefHasUserValue("animate_zoom"))
>+        animateZoom = gPrefBranch.getBoolPref("animate_zoom");
>+      if (animateZoom) {

Let's just add this pref to browser/app/profile/firefox.js, so it has a default value and can skip the prefHasUserValue check.

>+        setTimeout(onZoomDone,0);

Nit: space after comma.

>+    let onZoomDone = function() {

Nit: |function onZoomDone() {|

>+    } else
>+      onZoomDone();

Nit: when you brace one block of an if/else, brace both (even if the other is 1 line).


>+++ b/browser/base/content/test/tabview/browser_tabview_bug595804.js
...
>+  function part2() {
>+    prefsBranch.setBoolPref("animate_zoom", false);

Probably best to use registerCleanupFunction() so that the pref is always reset, even if the test has a problem. See https://developer.mozilla.org/en/Browser_chrome_tests

>\ No newline at end of file

Nit. :)

Comment 19

[Michael Yoshitaka Erlewine [:mitcho]]

Attachment: Patch v5, fixed all nits

Comment 20

[Mike Beltzner [:beltzner, not reading bugmail]]

Comment on attachment 481738 [details] [diff] [review]
Patch v5, fixed all nits

a=beltzner

Comment 21

[Michael Yoshitaka Erlewine [:mitcho]]

Attachment: Patch for checkin

Sent to try first to verify it all passes, as it hasn't been try-ed against some of the recent p-c changes.

Comment 22

[Michael Yoshitaka Erlewine [:mitcho]]

Passed try. Requesting checkin.

Comment 23

[Michael Yoshitaka Erlewine [:mitcho]]

http://hg.mozilla.org/mozilla-central/rev/5aac28860585

Comment 24

[Tracy Walker [:tracy]]

verified with nightly minefield builds of 20101021