Last Comment Bug 712096 - Exiting 3D (Tilt) mode of the developer tools should show a transition back to a flat page
: Exiting 3D (Tilt) mode of the developer tools should show a transition back t...
Status: RESOLVED FIXED
[tilt][fixed-in-fx-team]
:
Product: Firefox
Classification: Client Software
Component: Developer Tools (show other bugs)
: Trunk
: All All
: -- normal (vote)
: Firefox 12
Assigned To: Victor Porof [:vporof][:vp]
:
Mentors:
Depends on: 712029
Blocks: 718281
  Show dependency treegraph
 
Reported: 2011-12-19 12:30 PST by Jared Wein [:jaws] (please needinfo? me)
Modified: 2012-01-28 16:07 PST (History)
2 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
v1 (15.81 KB, patch)
2012-01-15 04:12 PST, Victor Porof [:vporof][:vp]
no flags Details | Diff | Review
v2 (15.80 KB, patch)
2012-01-15 05:20 PST, Victor Porof [:vporof][:vp]
no flags Details | Diff | Review
v3 (12.36 KB, patch)
2012-01-15 23:12 PST, Victor Porof [:vporof][:vp]
rcampbell: review+
Details | Diff | Review

Description Jared Wein [:jaws] (please needinfo? me) 2011-12-19 12:30:46 PST
Exiting 3D (Tilt) mode of the developer tools should show a transition back to a flat page. Further, it would be nice if the selected element was kept in view when transitioned back.
Comment 1 Victor Porof [:vporof][:vp] 2012-01-14 03:20:41 PST
The selected element should indeed be kept in view when Tilt is closed. I am wondering if the transition animation back to a flat page wouldn't be annoying. (Suppose I want to instantly go back to the 3D view?).
Comment 2 Victor Porof [:vporof][:vp] 2012-01-14 03:28:38 PST
Err.. instantly go back to the 2D view.
Comment 3 Victor Porof [:vporof][:vp] 2012-01-14 06:29:51 PST
On the other hand, I just tested this and it would look totally awesome. We should keep this and maybe add a pref for disabling the transition if there are any issues.
Comment 4 Victor Porof [:vporof][:vp] 2012-01-15 04:12:26 PST
Created attachment 588727 [details] [diff] [review]
v1
Comment 5 Dão Gottwald [:dao] 2012-01-15 04:20:58 PST
Comment on attachment 588727 [details] [diff] [review]
v1

>+        this.chromeWindow.gBrowser.selectedBrowser.contentWindow.focus();

this.chromeWindow.gBrowser.selectedBrowser.focus();
Comment 6 Victor Porof [:vporof][:vp] 2012-01-15 05:20:26 PST
Created attachment 588732 [details] [diff] [review]
v2

(In reply to Dão Gottwald [:dao] from comment #5)
> Comment on attachment 588727 [details] [diff] [review]
> v1
> 
> >+        this.chromeWindow.gBrowser.selectedBrowser.contentWindow.focus();
> 
> this.chromeWindow.gBrowser.selectedBrowser.focus();

Hmm, this slipped through quite a long time ago. Thank you, fixed.
Comment 7 Victor Porof [:vporof][:vp] 2012-01-15 06:09:03 PST
Filed and added patch in bug 718281 for hiding the animations under a pref.
Comment 8 Victor Porof [:vporof][:vp] 2012-01-15 23:12:24 PST
Created attachment 588815 [details] [diff] [review]
v3

Cleaned up, fixed a typo & some fails in try.
https://tbpl.mozilla.org/?tree=Try&rev=99f836e9fef1
Comment 9 Rob Campbell [:rc] (:robcee) 2012-01-19 13:16:55 PST
Comment on attachment 588815 [details] [diff] [review]
v3

-      this.presenter.tiltUI.destroy(this.presenter.tiltUI.currentWindowId);
+      this.presenter.tiltUI.destroy(this.presenter.tiltUI.currentWindowId, 1);

, 1? or true? I think you're mixing those. Or are you just doing that to avoid a longer line?

I think this'll do!
Comment 10 Rob Campbell [:rc] (:robcee) 2012-01-19 13:20:52 PST
Comment on attachment 588815 [details] [diff] [review]
v3

and, I spoke too soon. I got this on a local build when exiting:

Timestamp: 2012-01-19 17:19:55
Error: controller.arcball.reset is not a function
Source File: resource:///modules/devtools/Tilt.jsm
Line: 162
Comment 11 Rob Campbell [:rc] (:robcee) 2012-01-19 13:23:18 PST
gosh I'm dumb!
Comment 12 Rob Campbell [:rc] (:robcee) 2012-01-19 13:24:59 PST
Comment on attachment 588815 [details] [diff] [review]
v3

changed the depencies so this applied. I am sorry I r-d you.
Comment 13 Victor Porof [:vporof][:vp] 2012-01-19 14:05:38 PST
(In reply to Rob Campbell [:rc] (robcee) from comment #9)
> Comment on attachment 588815 [details] [diff] [review]
> v3
> 
> -      this.presenter.tiltUI.destroy(this.presenter.tiltUI.currentWindowId);
> +      this.presenter.tiltUI.destroy(this.presenter.tiltUI.currentWindowId,
> 1);
> 
> , 1? or true? I think you're mixing those. Or are you just doing that to
> avoid a longer line?
> 
> I think this'll do!

I tried avoiding a longer line. However, it bugged me too so I *did* change it to true and made the line shorter somewhere later in the patch queue.
Comment 14 Rob Campbell [:rc] (:robcee) 2012-01-20 10:47:14 PST
https://hg.mozilla.org/integration/fx-team/rev/66fcdc7716dc
Comment 15 Rob Campbell [:rc] (:robcee) 2012-01-21 07:38:12 PST
https://hg.mozilla.org/mozilla-central/rev/66fcdc7716dc
Comment 16 Rob Campbell [:rc] (:robcee) 2012-01-27 07:49:07 PST
Comment on attachment 588815 [details] [diff] [review]
v3

[Approval Request Comment]
Regression caused by (bug #): New feature
User impact if declined: User won't see an exit transition when closing Tilt. They may be jarred by the shock of returning to 2D viewing.
Testing completed (on m-c, etc.): on m-c
Risk to taking this patch (and alternatives if risky): Very not risky! It is also fairly low down in our patchqueue and makes landing the downstream patches possible. This has been tested fairly extensively.

Note You need to log in before you can comment on or make changes to this bug.