208500 - Copy XPath of Node in DOMInspector

Status: RESOLVED INCOMPLETE

(Other Applications :: DOM Inspector, enhancement)

Description

[Paul Clark]

User-Agent:       Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.0.0) Gecko/20020623 Debian/1.0.0-0.woody.1
Build Identifier: All

When using DOMInspector to navigate the tree of nodes, there is an option (on
right-click) to copy the XML of the selected node.

It would also be useful to have an option to copy the XPath of the selected node.

Reproducible: Always

Steps to Reproduce:
1. Go Tools -> Web Development -> DOMInspector
2. Put a URL in the location box, and hit Enter to load a page.
3. Right click on a node in the tree.  No option "Copy XPath" appears in the menu!



Expected Results:  
Offered an option to "Copy XPath".  Mozilla would then place the XPath of the
selected node into the user's clipboard.

This is a feature request for which I have a patch.  I would like a committer to
sponsor it.

Comment 1

[Paul Clark]

Attachment: Patch to add Copy XPath context-menu option to DOMInspector

The patch is best applied from directory:
mozilla/extensions/inspector/resources

Comment 2

[Peter Van der Beken [:peterv]]

This is quite broken: doesn't handle namespaces, node.nodeName won't work for
textnodes, comment nodes, document nodes, ... "/node.nodename" won't work for
attribute nodes.

Comment 3

[Alex Vincent [:WeirdAl]]

Valid RFE, confirming for radar.

Comment 4

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

oh, and you might also want to stop walking up the tree if you hit an element
with an id and use the id() function instead.

Comment 5

[Christopher Aillon (sabbatical, not receiving bugmail)]

Comment on attachment 125048 [details] [diff] [review]
Patch to add Copy XPath context-menu option to DOMInspector

based on comments

Comment 6

[Christopher Aillon (sabbatical, not receiving bugmail)]

On a separate implementation note, I don't feel that this should be something in
the context menu.

I think perhaps on the DOM Node Object view, as an additional row would make
more sense.

I also don't like the fact that DOM Inspector must provide this functionality. 
I'd prefer seeing it elsewhere.  Perhaps nsIDOMNS3Node?

Comment 7

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

No, inside the node itself seems like the wrong approach to me since we have
multiple implementations of nodes. Maybe in xmlextras or as a separate object
provided by the transformiix dll

Comment 8

[Christopher Aillon (sabbatical, not receiving bugmail)]

Yeah that sounds better.

Comment 9

[Christopher Aillon (sabbatical, not receiving bugmail)]

Mass re-assigning bugs to [e-mail]

Comment 10

[Daniel Gredler]

Attachment: Revised Patch

I'm attaching another patch that fixes a couple of small bugs in the previous
patch, including dealing correctly with all the different node types. Here's a
list of things I did not address, with their corresponding reasons:

- I did not implement the id( ) shortcut suggested by Jonas because a lot of
real-world HTML out there has multiple nodes with the same id, kind of like a
extra name attribute. IE allows multiple nodes to have the same id, so people
do it.
- I'm not sure if extra namespace handling is needed. The node names for
element nodes include the namespace name if they have one, so I'm not sure what
else there is to do.
- As far as the UI goes and whether to expose this in the context menu or in
the DOM Node Object view, I had thought of both possibilities, and they both
seem intuitive. The previous patch put it in the context menu, so all things
being equal, that's where I put it as well. Much easier, since I've never
messed with Mozilla code before ;-)
- Finally, I agree that the methods getXPath( ) and countTwins( ) would be
better off somewhere other than dom.js, but that's about the extent of my input
since as I've already mentioned I know zero about the Mozilla internals.
Perhaps Peter can comment on this, since he's the TransforMiix module owner.

Comment 11

[Alex Vincent [:WeirdAl]]

Comment on attachment 165654 [details] [diff] [review]
Revised Patch

Note that I don't know much about XPath, so this is just opinion.

>diff -u -r1.33 dom.js
>--- extensions/inspector/resources/content/viewers/dom/dom.js	21 Apr 2004 15:09:27 -0000	1.33
>+++ extensions/inspector/resources/content/viewers/dom/dom.js	12 Nov 2004 04:17:02 -0000
>@@ -411,6 +411,91 @@
>   },
>   
>   ////////////////////////////////////////////////////////////////////////////
>+  //// XPath retrieval
>+  
>+  cmdCopyXPath: function()

Could we get a few more lines of context here?	I realize you're just adding
code, but where? :)

>+      var myIndex = this.countTwins(node, node.nodeName);
>+      var totalIndex = this.countTwins(pNode.lastChild, node.nodeName);

I'm not sure we should rely on node.nodeName.  Would it be wiser to rely on the
namespaceURI and localName? :)

>+        case Node.CDATA_SECTION_NODE:
>+          // This should never happen.
>+          nodeName = "";
>+          break;
>+        case Node.ENTITY_REFERENCE_NODE:
>+          // This should never happen.
>+          nodeName = "";
>+          break;
>+        case Node.ENTITY_NODE:
>+          // This should never happen.
>+          nodeName = "";
>+          break;


Perhaps offer a dump() statement, or possibly throw an exception?


>+  countTwins: function(node, nodeName)
>+  {
>+    var count = 0;
>+    while (node) {
>+      if (node.nodeName == nodeName)

Again, would checking namespace URI and localName explicitly be wise?

Comment 12

[Daniel Gredler]

Attachment: Revised Patch 2

Here's an updated version of the patch, modified based on Alex's input.

A brief synopsis of the patch as a whole:

- dom.js:
  - added method cmdCopyXPath( ) that copies the XPath for the selected node
into the clipboard.
  - added helper method getXPath( ) that recursively retrieves the XPath string
for a specific node.
  - added helper method countTwins( ) that counts a node's twin siblings (ie,
where namespaceURI and localName match).
  - added helper method getIdentifier( ) that retrieves the unique identifier
for a node (namespaceURI if it has one, plus localName).

- dom.xul:
  - added the command and menu item for the "Copy XPath" context menu item.

- dom.dtd:
  - added the entity declaration for the "Copy XPath" context menu item label.

As Alex pointed out, relying on nodeName to uniquely identify a node is sooooo
DOM Level 1... at DOM Level 2 you use the namespaceURI and localName, so that's
fixed ;-) Finally, I added dump statements to the cases in getXPath( ) for
invalid node types.

Comment 13

[timeless]

Comment on attachment 165667 [details] [diff] [review]
Revised Patch 2

>+  cmdCopyXPath: function()

name your functions.

>+        case Node.ELEMENT_NODE:

I think we should consider using Components.interfaces.nsIDOM3Node or whatever
it is instead of Node.

>   <!ENTITY cmdCopyXML.label "Copy XML">
>+  <!ENTITY cmdCopyXPath.label "Copy XPath">

I'd rather we use flavors than add extra items to the context menu.

Comment 14

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

> >+        case Node.ELEMENT_NODE:
> I think we should consider using Components.interfaces.nsIDOM3Node or whatever
> it is instead of Node.

Why? The w3c DOM spec says that Node should work. And it's not likly to go
anywhere in mozilla since we already have lots of code that depend on it.

Using Node.ELEMENT_NODE rather then '1' is just for readability anyway, and
Components.interfaces.nsIDOM3Node.ELEMENT_NODE isn't very readable.

Comment 15

[Alex Vincent [:WeirdAl]]

> Why? The w3c DOM spec says that Node should work. And it's not likly to go
> anywhere in mozilla since we already have lots of code that depend on it.

The properties of Node are overrideable.  caillon and I have discussed that at
length.

Comment 16

[Daniel Gredler]

Here are some quick comments on the latest round of suggestions:

> >+  cmdCopyXPath: function()
> name your functions.

I'm not sure if you want a better name? The name of the function is 
cmdCopyXPath, following the naming convention of other JavaScript functions in 
the same file (dom.js) that get invoked by XUL commands (cmdCopySelectedXML( ), 
cmdInspectInNewWindow( ), cmdInspectBrowser( ), etc).

> I think we should consider using Components.interfaces.nsIDOM3Node
> or whatever it is instead of Node.

I did a quick search of the code and cannot find any JavaScript files in the 
codebase that reference nsIDOM3Node. There are, however, references to the node 
type constants in Node throughout the JavaScript files (including dom.js). It 
seems that it is standard to use Node. This change would seem to be beyond the 
scope of this enhancement.

> I'd rather we use flavors than add extra items to the context menu.

Someone will have to enlighten me as to what this means. A link (or some such) 
would be nice. As I mentioned before, I'm a newbie here ;)

> The properties of Node are overrideable. caillon and I have
> discussed that at length.

I don't even know whether this is an argument for or against using Node ;)

Comment 17

[timeless]

> I'm not sure if you want a better name? The name of the function is 
cmdCopyXPath

no, it has no name, cmdCopyXPath is the name of a property which has an
anonymous function.

flavours can be found at:
http://lxr.mozilla.org/seamonkey/source/widget/public/nsITransferable.idl

fwiw, internally, we use nsIDOM3Node and friends instead of Node and friends.

Comment 18

[Daniel Gredler]

> no, it has no name, cmdCopyXPath is the name of a property which
> has an anonymous function

Ah, I see what you mean. I just assumed that was ok since that seems to be the 
convention throughout the rest of the code. I'll change it for the next patch.

> flavours can be found at:
> http://lxr.mozilla.org/seamonkey/source/widget/public/nsITransferable.idl

Looking at the existing code, nsITransferable seems to be used mostly for drag-
and-drop operations. Do you mean you would like to eliminate the context menu 
item in favor of dragging and dropping nodes from the inspector into, say, a 
text editor?

The dom.js file has some global constants at the top that are used to access 
the clipboard elsewhere. I just used the constants the same way they were being 
used in other parts of the code.

Comment 19

[Christian :Biesinger (don't email me, ping me on IRC)]

clipboards also have flavors, consider
http://lxr.mozilla.org/seamonkey/source/widget/public/nsIClipboard.idl#48

Comment 20

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

I might be talking out of my butt here since I don't really know what these
flavours are, but:

Note that copying part of a DOM-tree and then pasting it in a text-only area
such as a texteditor or a textfield should probably paste the text-contents
rather then the source markup. Same thing for drag-n-drop.

So if that is what flavours are then we need separate 'copy' and 'copy xml' (or
'copy markup') menuitems. 'copy' would copy part of the DOM (later pasted as
whatever is appropriate) and 'copy xml' would copy the markup as a string.

Comment 21

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Oh, wrt the id-shortcut. I'm not sure that using duplicated IDs are that common.
How about using the id-shortcut whenever you find a node that

a) has an id, and
b) |document.getElementById(elem.id) == elem| is true.

?

Comment 22

[timeless]

sicking: the idea is you have one copy item, a paste special option in your
favorite apps, and maybe a clipboard manager /somewhere/. Having n Copy items
sucks for various reasons.

Comment 23

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Well, none of my favourite texteditors have a 'paste as X' or a 'paste special'
that would work here option. Neither does any textareas in mozilla. And going
through a clipboard manager (that currently doesn't exist) seems like a huge pain.

Until that changes I think we'll just have to live with having multiple copy items.

Also I just realized that we're talking about the xpath, not the markup here.
I'd say that it would be very hard to figure out that you need to copy the node
and then do some 'paste as XPath', that's certainly non-intuitive UI, IMHO. I'd
never find such a feature.

If 'copy' is an offending term here then maybe having something like 'show
XPath' is better that would open a dialog showing the path expression which
could then be copied like normal text.

Comment 24

[timeless]

hrm, showing it as an edit field might actually make me happier. there are
advantages to being able to scan things instead of having to 'copy magical
thing', 'paste' to find out what the magical thing is.

Comment 25

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

(sorry, forgot to say this in my last post)

OTOH, we're talking about a feature in a development tool here. People using
this care more about easy access to features rather then userfriendly UI. For
this reason my vote still goes towards a 'copy xpath' menuitem.

Comment 26

[Daniel Gredler]

Attachment: Revised Patch 3

Changes from the previous patch:

- Named the JavaScript functions, rather than using anonymous functions.
- Fixed a small bug in the getIdentifier() function.
- Added the id() shortcut using Jonas' excellent idea.

Interface-wise, it is still a context menu. I had initially wanted to implement
it as a textbox in the DOM Node view (somewhat like Jonas is suggesting) but
the DOM Node view only gets displayed when the current node is an element node,
and I wanted this option to be accessible for all nodes (text, comment, etc).

Comment 27

[Alex Vincent [:WeirdAl]]

iirc, you'll find ELEMENT_NODE and its bosom buddies under nsIDOMNode, not
nsIDOM3Node.

Comment 28

[Alex Vincent [:WeirdAl]]

Comment on attachment 166189 [details] [diff] [review]
Revised Patch 3

requesting review to get on radar.

Comment 29

[Jason Barnabe (np)]

timeless has recently reviewed some of my DOM Inspector patches, he'd probably be a better choice for a reviewer.

Comment 30

[Shawn Wilsher :sdwilsh]

Comment on attachment 166189 [details] [diff] [review]
Revised Patch 3

Requesting Review from timeless as per Comment #29.  This would be nice to have.

Comment 31

[timeless]

Comment on attachment 166189 [details] [diff] [review]
Revised Patch 3

*grumble* firefox ate my homework.

data:Text/xml,<?xml version="1.0"?><!DOCTYPE q [ <!ENTITY aaa "hi"> ]><q><![CDATA[a]]></q>

this yields

+      case Node.CDATA_SECTION_NODE:
+        dump("Trying to get the XPath for a node of type CDATA_SECTION_NODE; this should never happen.\n");

+      case Node.DOCUMENT_TYPE_NODE:
+        dump("Trying to get the XPath for a node of type DOCUMENT_TYPE_NODE; this should never happen.\n");

--
+      return ( node.namespaceURI ? node.namespaceURI + ":" + node.localName : node.localName );
+    }
+    else {
+      return node.nodeName;

no point in the "else {", you can't possibly reach that line unless the if was false because the if returned. so drop |else {| and the matching |}| and outdent the return.

--
note that this code will generate very useless "xpaths" for soap responses where the prefixes are randomized. (you're still using nodeName which sicking doesn't like.)

I'd suggest that you switch from just a single "xpath" to an "xpath" with optional extra lines describing all prefixes:

a1:b/b1:c
a1=>http://foopy/a
b1=>http://foo.y/b

input:
data:text/xml,<a xmlns="http://a"><b/><b xmlns="http://b"></b></a>

clearly you don't want to just use a nodeName. that isn't handled properly by your code.

Comment 32

[Helder Ribeiro]

I'm sorry, but doesn't the Firefox extension XPather (http://xpath.alephzarro.com/index) already do this? If it's a feature worth integrating in the DOM Inspector itself, wouldn't it be a simple matter of using some of XPather's code?

Off-topic: I came to this bug because I was trying to find a way to access the xpath of an element from the DOM object, but didn't find a corresponding method/property in the DOM reference. Maybe I can use code from this bug or from XPather to fill that need. If anyone knows of another solution please send me an email.

Thanks,

Helder

Comment 33

[Biju]

see also Bug 382065 - Implement Copy Selector feature

Comment 34

[sallads1]

I found that you can't copy the value you're editing, unless you do ctrl+x instead of ctrl+c. Hope it helps someone.

Comment 35

[Firefox Bug Husbandry Bot]

Bulk close. This component is no longer supported or maintained.

https://bugzilla.mozilla.org/show_bug.cgi?id=1499023

Comment 36

[Firefox Bug Husbandry Bot]

Bulk close. This component is no longer supported or maintained.

https://bugzilla.mozilla.org/show_bug.cgi?id=1499023