712096 - Exiting 3D (Tilt) mode of the developer tools should show a transition back to a flat page

Status: RESOLVED FIXED

(DevTools :: General, defect)

Description

[Jared Wein [:jaws] (please needinfo? me)]

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]]

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]]

Err.. instantly go back to the 2D view.

Comment 3

[Victor Porof [:vporof][:vp]]

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]]

Attachment: v1

Comment 5

[Dão Gottwald [:dao]]

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]]

Attachment: 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]]

Filed and added patch in bug 718281 for hiding the animations under a pref.

Comment 8

[Victor Porof [:vporof][:vp]]

Attachment: v3

Cleaned up, fixed a typo & some fails in try.
https://tbpl.mozilla.org/?tree=Try&rev=99f836e9fef1

Comment 9

[Rob Campbell [:rc] (:robcee)]

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)]

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)]

gosh I'm dumb!

Comment 12

[Rob Campbell [:rc] (:robcee)]

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]]

(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)]

https://hg.mozilla.org/integration/fx-team/rev/66fcdc7716dc

Comment 15

[Rob Campbell [:rc] (:robcee)]

https://hg.mozilla.org/mozilla-central/rev/66fcdc7716dc

Comment 16

[Rob Campbell [:rc] (:robcee)]

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.