Closed Bug 715647 Opened 12 years ago Closed 12 years ago

I want to be able to remove nodes from the Tilt view

Categories

(DevTools :: Inspector, defect)

x86
macOS
defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED
Firefox 12

People

(Reporter: davidb, Assigned: vporof)

References

Details

(Keywords: dev-doc-complete, Whiteboard: [tilt])

Attachments

(3 files, 1 obsolete file)

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.
Assignee: nobody → vporof
Status: NEW → ASSIGNED
Whiteboard: [tilt]
This kind of feature should be "surface-agnostic". I should work with the 2D inspector as well.
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.
Whiteboard: [tilt]
Whiteboard: [tilt]
Attached patch v1 (obsolete) — Splinter Review
Wip patch. No tests. Lot of queue underneath, probably not safe to apply.
Attachment #588724 - Flags: feedback?(rcampbell)
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?
Attached patch v2Splinter Review
Try is green.
https://tbpl.mozilla.org/?tree=Try&rev=99f836e9fef1
Attachment #588724 - Attachment is obsolete: true
Attachment #588724 - Flags: feedback?(rcampbell)
Attachment #588817 - Flags: review?(rcampbell)
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.
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).
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
(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 :)
Depends on: 718281
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?
I'm also not keen on the right click to destroy nodes control. What if I'm translating and mess it up?
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?
(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 :)
Attached patch v2.1Splinter Review
Removed the right-click-to-remove functionality.
Attachment #591378 - Flags: review?(rcampbell)
Attachment #588817 - Flags: review?(rcampbell)
Attachment #591378 - Flags: review?(rcampbell) → review+
(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.
(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?
Whiteboard: [tilt] → [tilt][land-in-fx-team]
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.
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.
https://hg.mozilla.org/integration/fx-team/rev/397db1eba037
Whiteboard: [tilt][land-in-fx-team] → [tilt][fixed-in-fx-team]
https://hg.mozilla.org/mozilla-central/rev/397db1eba037
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Whiteboard: [tilt][fixed-in-fx-team] → [tilt]
Target Milestone: --- → Firefox 12
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.
Attachment #591378 - Flags: approval-mozilla-aurora?
Attachment #591378 - Flags: approval-mozilla-aurora?
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: