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)
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)
17.53 KB,
patch
|
Details | Diff | Splinter Review | |
17.37 KB,
patch
|
rcampbell
:
review+
|
Details | Diff | Splinter Review |
297.50 KB,
image/png
|
Details |
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 | ||
Updated•12 years ago
|
Assignee: nobody → vporof
Status: NEW → ASSIGNED
Whiteboard: [tilt]
Comment 1•12 years ago
|
||
This kind of feature should be "surface-agnostic". I should work with the 2D inspector as well.
Assignee | ||
Comment 2•12 years ago
|
||
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]
Assignee | ||
Updated•12 years ago
|
Whiteboard: [tilt]
Assignee | ||
Comment 3•12 years ago
|
||
Wip patch. No tests. Lot of queue underneath, probably not safe to apply.
Assignee | ||
Updated•12 years ago
|
Attachment #588724 -
Flags: feedback?(rcampbell)
Assignee | ||
Comment 4•12 years ago
|
||
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?
Assignee | ||
Comment 5•12 years ago
|
||
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)
Comment 6•12 years ago
|
||
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.
Assignee | ||
Comment 7•12 years ago
|
||
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•12 years ago
|
||
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
Assignee | ||
Comment 9•12 years ago
|
||
(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•12 years ago
|
||
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•12 years ago
|
||
I'm also not keen on the right click to destroy nodes control. What if I'm translating and mess it up?
Comment 12•12 years ago
|
||
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?
Assignee | ||
Comment 13•12 years ago
|
||
(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 :)
Assignee | ||
Comment 14•12 years ago
|
||
Removed the right-click-to-remove functionality.
Attachment #591378 -
Flags: review?(rcampbell)
Updated•12 years ago
|
Attachment #588817 -
Flags: review?(rcampbell)
Updated•12 years ago
|
Attachment #591378 -
Flags: review?(rcampbell) → review+
Comment 15•12 years ago
|
||
(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.
Assignee | ||
Comment 16•12 years ago
|
||
(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?
Updated•12 years ago
|
Whiteboard: [tilt] → [tilt][land-in-fx-team]
Comment 17•12 years ago
|
||
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.
Reporter | ||
Comment 18•12 years ago
|
||
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•12 years ago
|
||
https://hg.mozilla.org/integration/fx-team/rev/397db1eba037
Whiteboard: [tilt][land-in-fx-team] → [tilt][fixed-in-fx-team]
Comment 20•12 years ago
|
||
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 21•12 years ago
|
||
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?
Updated•12 years ago
|
tracking-firefox11:
--- → ?
Updated•12 years ago
|
Attachment #591378 -
Flags: approval-mozilla-aurora?
Updated•12 years ago
|
tracking-firefox11:
? → ---
Updated•12 years ago
|
Keywords: dev-doc-needed
Comment 22•12 years ago
|
||
Documentation updated: https://developer.mozilla.org/en/Tools/Page_Inspector/3D_view#Controlling_the_3D_view Mentioned on Firefox 12 for developers.
Keywords: dev-doc-needed → dev-doc-complete
Updated•6 years ago
|
Product: Firefox → DevTools
You need to log in
before you can comment on or make changes to this bug.
Description
•