Last Comment Bug 715647 - I want to be able to remove nodes from the Tilt view
: I want to be able to remove nodes from the Tilt view
Status: RESOLVED FIXED
[tilt]
: dev-doc-complete
Product: Firefox
Classification: Client Software
Component: Developer Tools: Inspector (show other bugs)
: unspecified
: x86 Mac OS X
: -- normal (vote)
: Firefox 12
Assigned To: Victor Porof [:vporof][:vp]
:
Mentors:
Depends on: 718281
Blocks: 718303
  Show dependency treegraph
 
Reported: 2012-01-05 13:25 PST by David Bolter [:davidb]
Modified: 2012-04-24 06:09 PDT (History)
5 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
v1 (7.65 KB, patch)
2012-01-15 04:03 PST, Victor Porof [:vporof][:vp]
no flags Details | Diff | Splinter Review
v2 (17.53 KB, patch)
2012-01-15 23:14 PST, Victor Porof [:vporof][:vp]
no flags Details | Diff | Splinter Review
v2.1 (17.37 KB, patch)
2012-01-25 00:06 PST, Victor Porof [:vporof][:vp]
rcampbell: review+
Details | Diff | Splinter Review
controls helper screenshot (297.50 KB, image/png)
2012-01-25 10:39 PST, Victor Porof [:vporof][:vp]
no flags Details

Description David Bolter [:davidb] 2012-01-05 13:25:06 PST
I might want the combo of Tilt + the destroy the web extension (https://addons.mozilla.org/en-US/firefox/addon/destroy-the-web/).

This came up while I was trying to click on the blinking cursor in the ace editor demo:
http://ajaxorg.github.com/ace/build/kitchen-sink.html

Essentially I want improved ability to select nodes.

Tilt rocks.
Comment 1 Paul Rouget [:paul] 2012-01-05 14:47:40 PST
This kind of feature should be "surface-agnostic". I should work with the 2D inspector as well.
Comment 2 Victor Porof [:vporof][:vp] 2012-01-15 01:31:56 PST
I am thinking of using right click for this functionality. Something also tells me that this should have a key for doing this, although picking a node itself cannot be done with the keyboard. Since right click currently does nothing, I see three possibilities:

1. click to select, then right click (on the same node!) to delete
2. right click to automatically delete node under the cursor
3. click to select, press x, or accel+x to delete node

I like the the (2, 3) combination.
Comment 3 Victor Porof [:vporof][:vp] 2012-01-15 04:03:10 PST
Created attachment 588724 [details] [diff] [review]
v1

Wip patch. No tests. Lot of queue underneath, probably not safe to apply.
Comment 4 Victor Porof [:vporof][:vp] 2012-01-15 05:25:11 PST
Using right click to delete node may interfere with the current "right click + drag = panning around", although I personally don't find it a problem. Should we stick only to using a key to delete the currently selected node?
Comment 5 Victor Porof [:vporof][:vp] 2012-01-15 23:14:14 PST
Created attachment 588817 [details] [diff] [review]
v2

Try is green.
https://tbpl.mozilla.org/?tree=Try&rev=99f836e9fef1
Comment 6 Rob Campbell [:rc] (:robcee) 2012-01-16 11:07:42 PST
this deletes the node from the underlying DOM, does it not?

If the HTML panel is open, what happens? Haven't looked at the patch yet so if this is addressed, I applaud your comprehensiveness.

Also, I'm working on a patch to add similar functionality to bug 584407 (wip patch, have a local version that is more current) which you should be able to hook into if the HTML panel is present.

again, will review more fully after I've cleared out some earlier review requests.
Comment 7 Victor Porof [:vporof][:vp] 2012-01-16 11:15:29 PST
These are only visual changes. This means that the underlying dom is unaltered, but elements are invisible in the 3D view and can't be picked (clicked) in the Tilt view, but maintain an outer shadow to let users know that *something was there*.
If a node in the breadcrumbs or html panel is selected, Tilt will show a highlight quad exactly over where the node used to be (this also means on the z axis).
Comment 8 Rob Campbell [:rc] (:robcee) 2012-01-19 13:37:55 PST
getting an error that DOM_VK_X is undefined so I missed a patch in your queue while testing this. Strangely, I don't seem to be able to find that defined anywhere in the patch queue. I am taking this to mean that I am tired and unable to review more patches this evening!

Timestamp: 2012-01-19 17:37:02
Error: code is not defined
Source File: resource:///modules/devtools/TiltVisualizer.jsm
Line: 1064
Comment 9 Victor Porof [:vporof][:vp] 2012-01-19 13:57:16 PST
(In reply to Rob Campbell [:rc] (robcee) from comment #8)
> getting an error that DOM_VK_X is undefined so I missed a patch in your
> queue while testing this. Strangely, I don't seem to be able to find that
> defined anywhere in the patch queue. I am taking this to mean that I am
> tired and unable to review more patches this evening!
> 
> Timestamp: 2012-01-19 17:37:02
> Error: code is not defined
> Source File: resource:///modules/devtools/TiltVisualizer.jsm
> Line: 1064

Missed a patch :)
Comment 10 Rob Campbell [:rc] (:robcee) 2012-01-24 10:48:17 PST
Comment on attachment 588817 [details] [diff] [review]
v2

+
+    if ("function" === typeof this.onSetupTexture) {
+      this.onSetupTexture();
+      this.onSetupTexture = null;
+    }

when is onSetupTexture not a function? When it's null? Appears only in the one place. Please to Explain!

+  deleteNode: function TVP_deleteNode(aNodeIndex)
+  {
+    // we probably don't want to delete the html or body node.. just sayin'

word.

Codewise this is OK, but it's a little weird. It'd be nice if it wasn't quite so destructive.

Do we really want this?
Comment 11 Rob Campbell [:rc] (:robcee) 2012-01-24 11:00:01 PST
I'm also not keen on the right click to destroy nodes control. What if I'm translating and mess it up?
Comment 12 Kevin Dangoor 2012-01-24 11:12:03 PST
I think this may be a useful way to help visually select nodes without having to resort to breadcrumbs and HTML view. It seems like an interesting feature to try.

I concur with Rob, though, regarding the right click. Perhaps UX can comment on the implementation?
Comment 13 Victor Porof [:vporof][:vp] 2012-01-24 22:55:32 PST
(In reply to Rob Campbell [:rc] (robcee) from comment #11)
> I'm also not keen on the right click to destroy nodes control. What if I'm
> translating and mess it up?

I agree. See comment #2 and comment #4 (I asked).
Currently you can both delete nodes with right click, and by pressing X while a node is selected. We can stick only to keyboard interaction.


(In reply to Kevin Dangoor from comment #12)
> I think this may be a useful way to help visually select nodes without
> having to resort to breadcrumbs and HTML view. It seems like an interesting
> feature to try.
> 

I definitely think we need this. I'll post an updated patch with only keyboard control.


(In reply to Rob Campbell [:rc] (robcee) from comment #10)
> +
> +    if ("function" === typeof this.onSetupTexture) {
> +      this.onSetupTexture();
> +      this.onSetupTexture = null;
> +    }
> 
> when is onSetupTexture not a function? When it's null? Appears only in the
> one place. Please to Explain!

There are two major setup steps in tilt: creating the texture and the mesh. I added a onSetupMesh function (needed by tests) and an onSetupTexture function (to "mirror" the previous one, but not currently needed, although I bet it will be needed really (!) soon). This is because the lack of expressive notifications currently in Tilt, but fixed by the patch in bug 718973.


(In reply to Rob Campbell [:rc] (robcee) from comment #10)
> 
> Codewise this is OK, but it's a little weird. It'd be nice if it wasn't
> quite so destructive.

What's destructive about this patch? The only "removed" thing is actually a merge between onClick and onMouseDown handlers. Other than that, it's 4 tests added and the deleteNode function :)
Comment 14 Victor Porof [:vporof][:vp] 2012-01-25 00:06:44 PST
Created attachment 591378 [details] [diff] [review]
v2.1

Removed the right-click-to-remove functionality.
Comment 15 Kevin Dangoor 2012-01-25 10:24:31 PST
(In reply to Victor Porof from comment #13)
> I definitely think we need this. I'll post an updated patch with only
> keyboard control.

Seems reasonable to me, but this is completely undiscoverable (unless the user bumps the 'x' key when trying to press 's' :)

Perhaps we need a bug on Tilt control discoverability in general. I'll look to see if we have one and file one if we don't.
Comment 16 Victor Porof [:vporof][:vp] 2012-01-25 10:39:33 PST
Created attachment 591540 [details]
controls helper screenshot

(In reply to Kevin Dangoor from comment #15)
> 
> Perhaps we need a bug on Tilt control discoverability in general. I'll look
> to see if we have one and file one if we don't.

I don't think we have one.
The extension had a help dialog. Maybe we can somehow steal the idea?
Comment 17 Rob Campbell [:rc] (:robcee) 2012-01-25 10:47:40 PST
I think we should have a 3D view page on MDN as a bare minimum. Probably as a subset for all of the Page Inspector's documentation. Need a separate bug for that stuff.
Comment 18 David Bolter [:davidb] 2012-01-25 10:50:57 PST
Drive-by aside: I'd like to see the keyboard shortcut info HUD created (and be context based) which is bug 492557. Could be helpful if '?' becomes a user reflex.
Comment 19 Rob Campbell [:rc] (:robcee) 2012-01-25 11:39:48 PST
https://hg.mozilla.org/integration/fx-team/rev/397db1eba037
Comment 20 Tim Taubert [:ttaubert] (on PTO, back Aug 29th) 2012-01-25 23:56:42 PST
https://hg.mozilla.org/mozilla-central/rev/397db1eba037
Comment 21 Rob Campbell [:rc] (:robcee) 2012-01-27 07:42:24 PST
Comment on attachment 591378 [details] [diff] [review]
v2.1

[Approval Request Comment]
Regression caused by (bug #): New feature.
User impact if declined: Some nodes may obscure other nodes in the visualization.
Testing completed (on m-c, etc.): m-c
Risk to taking this patch (and alternatives if risky): This patch does introduce a new keyboard control. It does change the contents of the visualization, but the underlying web page is unaffected by it. I believe it is low risk.
Comment 22 Eric Shepherd [:sheppy] 2012-04-24 06:09:46 PDT
Documentation updated:

https://developer.mozilla.org/en/Tools/Page_Inspector/3D_view#Controlling_the_3D_view

Mentioned on Firefox 12 for developers.

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