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

VERIFIED FIXED in Firefox 4.0

Status

P3
enhancement
VERIFIED FIXED
8 years ago
3 years ago

People

(Reporter: gomita, Assigned: mitcho)

Tracking

unspecified
Firefox 4.0
Dependency tree / graph

Details

Attachments

(1 attachment, 5 obsolete attachments)

(Reporter)

Description

8 years ago
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.
(Assignee)

Comment 1

8 years ago
I agree this would be appropriate as an about:config pref.
Status: UNCONFIRMED → NEW
Ever confirmed: true
Priority: -- → P4

Comment 2

8 years ago
+1 on about:config
Priority: P4 → P3
Target Milestone: --- → Firefox 4.0
(Assignee)

Updated

8 years ago
Assignee: nobody → mitcho
(Assignee)

Comment 3

8 years ago
Created attachment 476711 [details] [diff] [review]
Patch v1, requires 593157 patch to apply first
Attachment #476711 - Flags: feedback?(ian)
(Assignee)

Comment 4

8 years ago
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.
(Assignee)

Updated

8 years ago
Depends on: 593157
(Assignee)

Updated

8 years ago
Severity: normal → enhancement
Hardware: x86 → All
(Assignee)

Updated

8 years ago
Blocks: 591705
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.
Attachment #476711 - Flags: feedback?(ian) → feedback-
Blocks: 597043
(Assignee)

Comment 6

8 years ago
Created attachment 477819 [details] [diff] [review]
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.
Attachment #476711 - Attachment is obsolete: true
Attachment #477819 - Flags: feedback?(ian)
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.
Attachment #477819 - Flags: feedback?(ian) → feedback+
(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?
(Assignee)

Comment 9

8 years ago
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.
(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!
Created attachment 478714 [details] [diff] [review]
Patch v3, now with a test
Attachment #477819 - Attachment is obsolete: true
Attachment #478714 - Flags: feedback?(ian)
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.
Attachment #478714 - Flags: feedback?(raymond)
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();
+  }
+
Status: NEW → ASSIGNED
Created attachment 478780 [details] [diff] [review]
Patch v4, fixed test as per Raymond's comment

Sent to try: 97b2d74743a7
Attachment #478714 - Attachment is obsolete: true
Attachment #478780 - Flags: feedback?(ian)
Attachment #478714 - Flags: feedback?(raymond)
Attachment #478714 - Flags: feedback?(ian)
Comment on attachment 478780 [details] [diff] [review]
Patch v4, fixed test as per Raymond's comment

Test looks good
Attachment #478780 - Flags: review?(dolske)
Attachment #478780 - Flags: feedback?(ian)
Attachment #478780 - Flags: feedback+
Passed try!
(Assignee)

Updated

8 years ago
Summary: Add a pref to disable zoom-in animation when selecting a tab from "Group Your Tabs" panel → Add a pref to disable Panorama zoom-in/out animation
(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 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. :)
Attachment #478780 - Flags: review?(dolske) → review+
Created attachment 481738 [details] [diff] [review]
Patch v5, fixed all nits
Attachment #478780 - Attachment is obsolete: true
Attachment #481738 - Flags: approval2.0?
Comment on attachment 481738 [details] [diff] [review]
Patch v5, fixed all nits

a=beltzner
Attachment #481738 - Flags: approval2.0? → approval2.0+
Created attachment 482066 [details] [diff] [review]
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.
Attachment #481738 - Attachment is obsolete: true
Passed try. Requesting checkin.
Keywords: checkin-needed
http://hg.mozilla.org/mozilla-central/rev/5aac28860585
Status: ASSIGNED → RESOLVED
Last Resolved: 8 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
verified with nightly minefield builds of 20101021
Status: RESOLVED → VERIFIED
Product: Firefox → Firefox Graveyard
You need to log in before you can comment on or make changes to this bug.