Closed Bug 987877 Opened 10 years ago Closed 7 years ago

[markup view] Add "Copy XPath" context menu item for nodes

Categories

(DevTools :: Inspector, defect, P3)

Product:
DevTools
Component:
Inspector
Type:
defect

Tracking

(firefox56 verified, firefox57 verified)

Status:
VERIFIED FIXED
Milestone:
Firefox 56
Tracking Flags:
Tracking Status
firefox56 --- verified
firefox57 --- verified

People

(Reporter: bgrins, Assigned: pbro)

References

(Blocks 2 open bugs)

Details

(Keywords: dev-doc-needed)

Attachments

(1 file)

Bug 987877 - Add Copy XPath menu item to the inspector;
59 bytes, text/x-review-board-request
miker
: review+
benjamin
: review+
Details 
Similar to "Copy Unique Selector" we could have an entry that copies the XPath for a node.
Priority: -- → P3
I'm willing to bet that the only people that want this are WebDriver people. I wonder if the best place for this is a WebDriver extension?
See Also: → 1298359
One notes the Chromium browser has 'Copy Xpath', but it apparently abbreviates.
//*[@id="switchdiv"]/table/tbody/tr[2]/td/table/tbody/tr[2]/td/table/tbody/tr[8]/td[5]
https://getfirebug.com/firstrun#Firebug%202.0.17 says it will be merged into Firefox soon anyway.
As a footnote, I can now also turn the xpath into a Perl XML::TreePP path!
$ set '/html/body/div/div[3]/div[2]/div[2]/div[2]/table/tbody/tr[2]/td/table/tbody/tr[2]/td/table/tbody/tr[7]/td[5]'
$ echo $@|perl -pwle 's!/(\w+)!->{$1}!g'
->{html}->{body}->{div}->{div}[3]->{div}[2]->{div}[2]->{div}[2]->{table}->{tbody}->{tr}[2]->{td}->{table}->{tbody}->{tr}[2]->{td}->{table}->{tbody}->{tr}[7]->{td}[5]
As far as we are aware the only call for this is people that use webdriver. I'm assuming that this is your use-case? In general I'm not sure I've seen use of XPath outside of webdriver for a while.

We're considering this bug as part of the firebug-gaps project (bug 991806) which closes off gaps where the builtin tools are missing functions that are in Firebug.

As an aside comments on Bugzilla are subject to Etiquette and Contributor Guidelines. https://bugzilla.mozilla.org/page.cgi?id=etiquette.html
Your comment violates point 2, point 3 and maybe point 1, please be more civil in the future.
> (In reply to Joe Walker [:jwalker] (needinfo me or ping on irc) from comment #1)
> I'm willing to bet that the only people that want this are WebDriver people.

We use this at our company, and I'm not sure I even know what WebDriver is.

We have a tool that is configured using XPaths, and we normally get these via Firebug.  Therefore, we have lost some vital functionality with the recent 'upgrade' that disabled Firebug for us!
(In reply to Mark Clements from comment #10)
> > (In reply to Joe Walker [:jwalker] (needinfo me or ping on irc) from comment #1)
> We have a tool that is configured using XPaths, and we normally get these
> via Firebug.  Therefore, we have lost some vital functionality with the
> recent 'upgrade' that disabled Firebug for us!

Reminder: e10s is still off in release by default, so Firebug works fine there. It still works anywhere where e10 is is off, which includes DevEdition and Nightly if the window is opened in non-e10s mode, so as far as I'm aware, you can still run Firebug on all releases of Firefox.
Correction: e10s is still off in release by default *if you have addons that don't work in e10s mode* (which include FB).
(In reply to Joe Walker [:jwalker] (needinfo me or ping on irc) from comment #11)
> (In reply to Mark Clements from comment #10)
> Reminder: e10s is still off in release by default, so Firebug works fine

You're right - my mistake.  I'd read the Firebug release notes which said it now just shows an upgrade notice and therefore started using the developer tools on the assumption that Firebug was no-longer available.  But having tested, it appears Firebug is still working.

My point still stands - there are other people who use this functionality.  But so long as we are able to continue using Firebug this is not such an urgent issue (so long as it is fixed before Firebug gets switched off).
Btw. Firebug additionally had an option 'Copy Minimal XPath' for elements with an 'id' attribute. It should be considered to also add that option, which is probably the more valuable one in regard of automatic testing.

Sebastian
+1 for this feature.

With the pending disappearance of firebug, and its xpath extension, there seems to be no good way to automatically determine xpaths for elements on a page. I use xpaths to parse, scrape, and test web sites using python code (independent of firefox itself, or selenium webdriver).

The converse functionality is also needed: after much search I did find the developer console function $x() that returns a list of xpath results on a page, but it would also be helpful to have a tool that shows those results in the inspect view automatically (as firebug's xpath extension did). Currently it doesn't appear possible to view multiple results, or things like '/text()' contents of elements, in the inspector.
This came up again on IRC today. We have a "copy" menu group in the inspector's context menu, it already contains things like "copy selector", "copy HTML", etc. Adding one more item to copy the xpath doesn't seem like a big deal to me.
And since this seems like a very requested feature and is easy to implement, let's flag it as such, and I'll set myself as a mentor.

So, for anyone who wants to get started, please go through our contribution guide first: https://developer.mozilla.org/en-US/docs/Tools/Contributing (let me know if you don't manage to get the dev environment ready).
In terms of code, there will be 2 main things to change:

1. First, the server-side, that's the part of the DevTools code that runs in the same process as the page being inspected. That's the part that has access to the DOM and that will be able to compute the xpath string.
If you look inside \devtools\server\actors\inspector.js you'll find the NodeActor class. When instantiated, this is an object that represents a single DOM node on the page for DevTools' usage. 
This class already has methods like getUniqueSelector and getCssPath. We should add a new method like getXPath.
The actual implementation of this method could be based on Firebug's which seems to be here: https://github.com/firebug/firebug/blob/313e5bece064c0fb0ab7db123a38ba3b90dfc922/extension/content/firebug/lib/xpath.js#L21
Maybe the implementation could actually go inside \devtools\shared\inspector\css-logic.js just like we do for getCssPath.

Once that's done, we still need to do 2 things for this server-side part. First, the new method needs to be declared in a special interface file so that the rest of DevTools can call it: see getCssPath in \devtools\shared\specs\node.js for an example.

And also, we need a test to make sure our xpath logic works in all cases, by adding some automated tests.
Here again, we can basically copy what we have for getCssPath: \devtools\shared\tests\mochitest\test_css-logic-getCssPath.html

2. Secondly, the client-side. That's the part of DevTools that you see when you use the tools. This is what we call the toolbox. It contains the tabs, panels, menus, buttons, etc.
In the inspector panel, right-clicking on any node brings up the context menu, this menu has a sub menu item for copying various things, and this one has several options, including CSS Path. So once again, let's do the same thing here.
If you do a search for the string csspath in the folder \devtools\client\, you should find all the places that need to be copied.

For instance, \devtools\client\inspector\inspector.js needs to append the menu item and actually call the getXPath method when clicked. And \devtools\client\locales\en-US\inspector.properties needs to contain the new localized strings for this menu item.
Finally, we need automated integration tests to cover this new client-side feature. 2 tests need to be changed/enhanced:
\devtools\client\inspector\test\browser_inspector_menu-01-sensitivity.js
\devtools\client\inspector\test\browser_inspector_menu-02-copy-items.js

Right, that's about it I think.
Let me know if you need more help.
Mentor: pbrosset
Keywords: good-first-bug
firefish6000: you expressed interest in fixing this bug. Please see comment 16 for more info. I'll assign the bug to you for now. Feel free to ping me.
Assignee: nobody → firefish6000
Status: NEW → ASSIGNED
Got a bit of a late start due to school and network changes at home (gained ipv6, switched to nftables).
Its currently working, but I am questioning the functionality we should provide. First, I'll start by describing how Firebug did things.

If the user selects a node with an id, Firebug shortened the XPath to just that node. (eg. '//tag[@id="identifies"]')
If the user selects a node without an id, Firebug used the full XPath with indexes. (eg. '/html/body/div[3]/tag[2]'). Note it used the full XPath even if a parent node had an id, shortening only happened when the selected node had an id.

I would like to propose adding two selectors for each case, 1 for grabbing the short uniq XPath (compliments 'Copy -> CSS Selector', and somewhat mimics chrome). And another for grabbing full XPath (compliments 'Copy -> CSS Path').

I personally would like to go even further, as someone who semi-frequently parses sites with XPath, I often find myself wishing that these tools gave a little more context. I actually went ahead and added some functionality, resulting in it outputting this from https://developer.mozilla.org/en-US/docs/Tools
//div[@id='Subnav'][@class='subnav']/ol[@class='accordion']/li[5][@class='toggleable'][@data-closed='true']/a[@href='#'][@class='toggler'][text()='Extending the devtools']
However, I decided something like this was probably better off in the realm of an extension, so I will leave it out.

At the code (near-fully stolen from firebug) appears to be working. I just need to know what route we should take. I currently have it written to compliment CSS Selector('//tag[@id='ident']') and CSS Path('/path/from/doc/root/tag'). I will post a patch once I clean out the code of my comments, write the tests, and figure out mercurial. I also plan to add in the xpath search functionality, unless there are any objections to that.

As a last note, I would like to say that I also have no clue what WebDriver is. I use XPath primarily for parsing html with perl/javascript/python/etc.
(In reply to firefish6000 from comment #18)
> If the user selects a node with an id, Firebug shortened the XPath to just
> that node. (eg. '//tag[@id="identifies"]')
> If the user selects a node without an id, Firebug used the full XPath with
> indexes. (eg. '/html/body/div[3]/tag[2]'). Note it used the full XPath even
> if a parent node had an id, shortening only happened when the selected node
> had an id.
> 
> I would like to propose adding two selectors for each case, 1 for grabbing
> the short uniq XPath (compliments 'Copy -> CSS Selector', and somewhat
> mimics chrome). And another for grabbing full XPath (compliments 'Copy ->
> CSS Path').
Just so I understand correctly, are you proposing to add 2 menu items inside the "copy" menu group?
I would personally prefer to only add 1. Our "copy" menu already contains 5 (somewhat unrelated) items and the more we add, the less easy it becomes to find any of them.
What do you think about adding just one Copy XPath menu, as well as a setting somewhere people can use to switch between short and full mode?

> I personally would like to go even further, as someone who semi-frequently
> parses sites with XPath, I often find myself wishing that these tools gave a
> little more context. I actually went ahead and added some functionality,
> resulting in it outputting this from
> https://developer.mozilla.org/en-US/docs/Tools
> //div[@id='Subnav'][@class='subnav']/ol[@class='accordion']/
> li[5][@class='toggleable'][@data-closed='true']/
> a[@href='#'][@class='toggler'][text()='Extending the devtools']
> However, I decided something like this was probably better off in the realm
> of an extension, so I will leave it out.
If we do add a setting to switch between modes, we could easily add this 3rd mode.

> At the code (near-fully stolen from firebug) appears to be working. I just
> need to know what route we should take. I currently have it written to
> compliment CSS Selector('//tag[@id='ident']') and CSS
> Path('/path/from/doc/root/tag'). I will post a patch once I clean out the
> code of my comments, write the tests, and figure out mercurial.
If we go for the setting route. Then you'll need to add a new pref, that can take 3 different string values, and has a default value. Then you'll need to read the value for this pref in your code.
New prefs go here: /devtools/client/preferences/devtools.js
As you can see, there's already a bunch of them related to the inspector.
Here's an example of how you can read a pref's value from code: https://dxr.mozilla.org/mozilla-central/source/devtools/client/inspector/inspector.js#601

> I also plan
> to add in the xpath search functionality, unless there are any objections to
> that.
We have bug 963933 for this if you're interested in tackling this too.
I'd say that the option is not really necessary, as the XPaths above ('//tag[@id="identifies"]' and '/html/body/div[3]/tag[2]') are both meant to select the same element.

Firebug did have two different options for elements with ID, 'Copy XPath' and 'Copy Minimal XPath' and I formally created a request to make the 'Copy Minimal Xpath' option also available for other elements[1].
Though I believe the only use case for copying XPaths from the Inspector nowadays is using them in automated testing like within Selenium.
Note that this assumption only applies to copying XPaths. Features like searching for elements via XPaths as requested in bug 963933 should of course allow to search for all kinds of XPaths.

Sebastian

[1] https://github.com/firebug/firebug/issues/7304
(In reply to Sebastian Zartner [:sebo] from comment #20)
> Though I believe the only use case for copying XPaths from the Inspector
> nowadays is using them in automated testing like within Selenium.

Why do people keep saying that?  There are numerous comments in this thread from people indicating they have other use-cases, which appear to be wide and varied.
(In reply to Mark Clements from comment #21)
> (In reply to Sebastian Zartner [:sebo] from comment #20)
> > Though I believe the only use case for copying XPaths from the Inspector
> > nowadays is using them in automated testing like within Selenium.
> 
> Why do people keep saying that?  There are numerous comments in this thread
> from people indicating they have other use-cases, which appear to be wide
> and varied.

Skimming through the page I can see two people commenting about their use cases. Jtbr's use case in comment 15 is to "parse, scrape, and test web sites using python code", which sounds like automatic testing. And you stated in comment 10 that "We have a tool that is configured using XPaths.".

So, maybe you could explain a bit more detailed what this tool does exactly to understand your use case better and outline why you require both options. As I stated in comment 14, I think the functionality behind 'Copy Minimal XPath' is the more valuable one, because it is independent of the document's structure.
Therefore, I think it should be enough to only add one 'Copy XPath' option, which tries to minimize the XPath as Firebug did and maybe even try to be a bit smarter, e.g. when parent elements have an ID, as firefish6000 mentioned in comment 16, or when the element only has a class.

Sebastian
Assignee: firefish6000 → nobody
Status: ASSIGNED → NEW
> > (In reply to Sebastian Zartner [:sebo] from comment #20)
> > > Though I believe the only use case for copying XPaths from the Inspector
> > > nowadays is using them in automated testing like within Selenium.
> > 
> > Why do people keep saying that?  There are numerous comments in this thread
> > from people indicating they have other use-cases, which appear to be wide
> > and varied.
> 
> Skimming through the page I can see two people commenting about their use
> cases. Jtbr's use case in comment 15 is to "parse, scrape, and test web
> sites using python code", which sounds like automatic testing. And you
> stated in comment 10 that "We have a tool that is configured using XPaths.".

You're right - I've rechecked and there are only two mentions on this page, but I'm pretty sure I've seen similar comments in other XPath related bugs.

My point was to counter the assumption that XPath is only used by Selenium users, which has come up a number of times and which is patently wrong.
 
> So, maybe you could explain a bit more detailed what this tool does exactly
> to understand your use case better and outline why you require both options.

Our use-case is for generating ini files for a bookmarklet provided as part of our web application.  The bookmarklet allows users to extract structured text from public online journal resources to add to their local clipboard within the application for later use.  These ini files use XPath specifiers to define where the various fields (Title, Author, Abstract, etc.) are located within the page content of each site.

> As I stated in comment 14, I think the functionality behind 'Copy Minimal
> XPath' is the more valuable one, because it is independent of the document's
> structure.
> Therefore, I think it should be enough to only add one 'Copy XPath' option,
> which tries to minimize the XPath as Firebug did and maybe even try to be a
> bit smarter, e.g. when parent elements have an ID, as firefish6000 mentioned
> in comment 16, or when the element only has a class.

I wasn't really commenting on whether there should be two options or just one, just on the point that only Selenium users use this feature.

For our use-case, minimal XPath is normally the most useful option.  

I don't have an opinion as to whether multiple options would be useful.  I don't know what Selenium users require nor what is most appropriate for other use-cases.  I can see a theoretical use for a longer XPath, one which represents the full tag structure of the selected node (i.e. it includes an entry for every parent node).  I don't currently have a practical use myself, but I can see how it would be useful for certain types of automation/analysis where you want to know the full set of containers of an element.
I'm parsing some HTML and I need the XPath of an element. To my great surprise the Developer Tools don't have Copy XPath, only CSS Path which I can't use because the Python library I'm using doesn't accept with it. There are others, but why add bloat to the project I'm working on only because a missing feature of Firefox? I had to run Chrome to get the XPath of that element.

I consider this a kind of regression because Firefox had Copy To XPath thanks to Firebug and now that Firebug had to go we don't have it anymore. I used Copy to XPath in Firebug many times over the years and I'd like to get it back.
Assignee: nobody → pbrosset
Mentor: pbrosset
Status: NEW → ASSIGNED
Flags: qe-verify+
Keywords: good-first-bug
Comment on attachment 8876755 [details]
Bug 987877 - Add Copy XPath menu item to the inspector;

https://reviewboard.mozilla.org/r/148088/#review152446

Awesome, wouldn't change a thing.
Attachment #8876755 - Flags: review?(mratcliffe) → review+
Pushed by pbrosset@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/4f7877809d79
Add Copy XPath menu item to the inspector; r=miker
Comment on attachment 8876755 [details]
Bug 987877 - Add Copy XPath menu item to the inspector;

Oh I forgot, do I need a review from you on the Scalars.yaml changes?
Attachment #8876755 - Flags: feedback?(benjamin)
(In reply to Patrick Brosset <:pbro> from comment #30)
> Comment on attachment 8876755 [details]
> Bug 987877 - Add Copy XPath menu item to the inspector;
> 
> Oh I forgot, do I need a review from you on the Scalars.yaml changes?

@Ben: Do you still want us to ask for review from you about simple probes like this? I ask because our probes pretty much all follow the same pattern.
https://hg.mozilla.org/mozilla-central/rev/4f7877809d79
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
status-firefox56: --- → fixed
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → Firefox 56
Comment on attachment 8876755 [details]
Bug 987877 - Add Copy XPath menu item to the inspector;

https://reviewboard.mozilla.org/r/148088/#review153622

Yes, we should have data review of every change. If there's going to be a lot of changes, talk to me about having a data steward peer from the devtools team.
Attachment #8876755 - Flags: review+
Attachment #8876755 - Flags: feedback?(benjamin)
I managed to reproduce the issue on Firefox 55.0b12, under Windows 10x64.
The XPath option is correctly displayed and functional on Firefox 56.0b3 and on Firefox 57.0a1 (2017-08-16).
Tests were performed under Windows 10x64, Mac OS X 10.11.6 and under Ubuntu 16.04x64.
Status: RESOLVED → VERIFIED
status-firefox56: fixed → verified
status-firefox57: --- → verified
Flags: qe-verify+
See Also: → 1410810
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.