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]
:
: J. Ryan Stinnett [:jryans] (use ni?)
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 | Splinter Review
v2 (15.80 KB, patch)
2012-01-15 05:20 PST, Victor Porof [:vporof][:vp]
no flags Details | Diff | Splinter Review
v3 (12.36 KB, patch)
2012-01-15 23:12 PST, Victor Porof [:vporof][:vp]
rcampbell: review+
Details | Diff | Splinter Review

Description User image 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 User image 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 User image Victor Porof [:vporof][:vp] 2012-01-14 03:28:38 PST
Err.. instantly go back to the 2D view.
Comment 3 User image 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 User image Victor Porof [:vporof][:vp] 2012-01-15 04:12:26 PST
Created attachment 588727 [details] [diff] [review]
v1
Comment 5 User image 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 User image 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 User image 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 User image 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 User image 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 User image 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 User image Rob Campbell [:rc] (:robcee) 2012-01-19 13:23:18 PST
gosh I'm dumb!
Comment 12 User image 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 User image 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 User image Rob Campbell [:rc] (:robcee) 2012-01-20 10:47:14 PST
https://hg.mozilla.org/integration/fx-team/rev/66fcdc7716dc
Comment 15 User image Rob Campbell [:rc] (:robcee) 2012-01-21 07:38:12 PST
https://hg.mozilla.org/mozilla-central/rev/66fcdc7716dc
Comment 16 User image 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.