Last Comment Bug 701419 - Inspector should always provide a unique selector
: Inspector should always provide a unique selector
Status: RESOLVED FIXED
[good first bug][mentor=jwalker][lang...
:
Product: Firefox
Classification: Client Software
Component: Developer Tools: Inspector (show other bugs)
: unspecified
: x86 Mac OS X
: P3 enhancement (vote)
: Firefox 21
Assigned To: Christos Stathis
:
: Patrick Brosset <:pbro>
Mentors:
: 703383 (view as bug list)
Depends on: 705636
Blocks: 674134 705947
  Show dependency treegraph
 
Reported: 2011-11-10 09:52 PST by Dietrich Ayala (:dietrich)
Modified: 2013-07-16 09:34 PDT (History)
9 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Adds a "Copy Unique Selector" option in the inspected element menu (3.88 KB, patch)
2013-01-30 02:29 PST, Christos Stathis
paul: feedback+
Details | Diff | Splinter Review
Update previous patch with a test (5.37 KB, patch)
2013-01-31 14:44 PST, Christos Stathis
no flags Details | Diff | Splinter Review
Update previous patch based on jwalker 's suggestions (8.21 KB, patch)
2013-02-07 12:41 PST, Christos Stathis
jwalker: review+
Details | Diff | Splinter Review

Description Dietrich Ayala (:dietrich) 2011-11-10 09:52:49 PST
Currently when using the inspector, it sometimes shows "td". This is not helpful if I'm using Inspector to do actual things with the inspected element.

We should provide access to a truly unique selector for the inspected element.

It doesn't make much sense to do it in the breadcrumb, but you could definitely do it in the selector popup that displays on hover/click of an element.
Comment 1 Rob Campbell [:rc] (:robcee) 2011-11-14 08:20:26 PST
hey Dietrich,

I guess you mean that you're inspecting a <td> element in a table and you have no way to refer to it.

You can grab the currently-selected node in the web console via the $0 convenience variable, but maybe you want something else here. Some way to "copy a reference to this element".

We could provide a menu option (maybe on the breadcrumbs or the selected node itself) to copy the XPath for the node. Or generate a unique selector for it, which might be tricky.

... some quick googling ...

This could work:

http://stackoverflow.com/questions/3620116/get-css-path-from-dom-element
Comment 2 Dietrich Ayala (:dietrich) 2011-11-14 09:54:23 PST
Yes, I want a string CSS selector for the current node that uniquely identifies it.

This would close a workflow hole for actually doing things like writing CSS or JS code while using the Inspector.
Comment 3 Joe Walker [:jwalker] (needinfo me or ping on irc) 2011-11-28 07:54:22 PST
Here's the algo that I'm thinking of:

1. Does the element have an id? If so, check that getElementById returns that element (there could be duplicate ids) and use #id

2. Does the element have a class? If so querySelectorAll, and loop searching for the element, and use .class:nth-child(i)

3. Does the element have a parent? if not, loop over the children of the root element and use tagname:nth-child(i)

4. Do steps 1, 2 and 3 on the parent element and use 'parent > tagname:nth-child(i)'

There are lots of tweaks we could do to this, like looking for parent=body rather than parent=null in step 3. However I suspect this will come up with a fairly short unique CSS expression in all but pathalogical cases.

Optimizations welcome.
Comment 4 Brian Burg 2011-12-22 15:49:27 PST
FYI

Uniquely identifying a DOM node is a common dilemma for my research area, specifically capture/replay of DOM events. In order to reproduce event dispatch, one must of course re-locate the event target.

Pure JavaScript deterministic replay systems (f.e., Mugshot http://research.microsoft.com/apps/pubs/default.aspx?id=120937) use the algorithm as described above, though in terms of DOM traversal actions, not selectors. This is easy to understand but requires space roughly proportional to tree depth. My research project doesn't need to surface a unique selector to the user, so I instead identify a node by its order in a well-defined DOM node traversal. The code that does that in WK is below

http://trac.webkit.org/browser/trunk/Source/WebCore/dom/Document.cpp#L2613
Comment 5 Joe Walker [:jwalker] (needinfo me or ping on irc) 2012-01-26 04:24:59 PST
For what it's worth I have an implementation of this:

https://github.com/joewalker/gcli/blob/6e47ca629f159e0c98ba7fdbf6eb0f30ed850145/lib/gcli/util.js#L213

Seems to work OK. Has tests. Anyone desperate for this?
Comment 6 Joe Walker [:jwalker] (needinfo me or ping on irc) 2012-01-28 13:25:28 PST
(In reply to Joe Walker from comment #5)
> For what it's worth I have an implementation of this:
> 
> https://github.com/joewalker/gcli/blob/
> 6e47ca629f159e0c98ba7fdbf6eb0f30ed850145/lib/gcli/util.js#L213
> 
> Seems to work OK. Has tests. Anyone desperate for this?

I'm taking that silence as a 'no'. We can integrate when we are.
Comment 7 Rob Campbell [:rc] (:robcee) 2012-01-29 07:59:15 PST
desperate isn't quite accurate but I'd like to have it in one of our shared libs.

I'm working on a context menu for the HTML panel (bug 584407) that would benefit from this greatly.
Comment 8 Paul Rouget [:paul] 2012-12-03 08:01:11 PST
*** Bug 703383 has been marked as a duplicate of this bug. ***
Comment 9 Christos Stathis 2013-01-30 02:29:02 PST
Created attachment 708068 [details] [diff] [review]
Adds a "Copy Unique Selector" option in the inspected element menu

Hello all,

I gave it a try. The patch adds a "Copy Unique Selector" option in the inspected element 's menu. Any feedback is welcome.
Comment 10 Paul Rouget [:paul] 2013-01-30 03:16:18 PST
Comment on attachment 708068 [details] [diff] [review]
Adds a "Copy Unique Selector" option in the inspected element menu

(In reply to Christos Stathis from comment #9)
> I gave it a try. The patch adds a "Copy Unique Selector" option in the
> inspected element 's menu. Any feedback is welcome.

Wow, thanks! I've been waiting for that for a while :)
It appears to work well. Code is good.
Can you add a test? (see browser/devtools/inspector/test/)
If you need any assistance for this, please ping me on IRC (#devtools).

>diff -r 126f887730f5 browser/devtools/inspector/InspectorPanel.jsm
>--- a/browser/devtools/inspector/InspectorPanel.jsm	Tue Jan 29 11:40:50 2013 -0500
>+++ b/browser/devtools/inspector/InspectorPanel.jsm	Tue Jan 29 23:17:00 2013 +0200
>@@ -7,16 +7,19 @@
> const { classes: Cc, interfaces: Ci, utils: Cu, results: Cr } = Components;
> 
> this.EXPORTED_SYMBOLS = ["InspectorPanel"];
> 
> Cu.import("resource://gre/modules/XPCOMUtils.jsm");
> Cu.import("resource://gre/modules/Services.jsm");
> Cu.import("resource://gre/modules/commonjs/promise/core.js");
> Cu.import("resource:///modules/devtools/EventEmitter.jsm");
>+Cu.import("resource://gre/modules/devtools/Require.jsm");
>+Cu.import("resource:///modules/devtools/gcli.jsm");
>+var util = require('gcli/util');

s/var util/let gcliUtil/

> 
>   /**
>+   * Copy the unique selector of the selected Node to the clipboard.

s/the unique/a unique/
Comment 11 Paul Rouget [:paul] 2013-01-30 03:17:01 PST
Once the test has been added, can you ask ":jwalker" to review this patch?

Thank you!
Comment 12 Paul Rouget [:paul] 2013-01-30 03:17:49 PST
(clearing needinfo)
Comment 13 Paul Rouget [:paul] 2013-01-30 03:18:50 PST
Oh, and can you use this technique to create a patch:
https://developer.mozilla.org/en-US/docs/Mercurial_FAQ#How_can_I_generate_a_patch_for_somebody_else_to_check-in_for_me.3F
Comment 14 Panos Astithas [:past] 2013-01-30 04:25:07 PST
(In reply to Paul Rouget [:paul] from comment #13)
> Oh, and can you use this technique to create a patch:
> https://developer.mozilla.org/en-US/docs/
> Mercurial_FAQ#How_can_I_generate_a_patch_for_somebody_else_to_check-
> in_for_me.3F

...and by that I believe Paul means to make sure your patch has the metadata (author, comment, etc.) on top, like what you get from 'hg export'.
Comment 15 Christos Stathis 2013-01-30 04:29:26 PST
Thanks for the quick feedback! I 'll have a test and a correct patch ASAP.
Comment 16 Christos Stathis 2013-01-31 14:44:15 PST
Created attachment 708813 [details] [diff] [review]
Update previous patch with a test

I hope it 's ok now. I added the test in browser_inspector_menu.js as it seems appropriate.
Comment 17 Joe Walker [:jwalker] (needinfo me or ping on irc) 2013-02-04 05:20:27 PST
Comment on attachment 708813 [details] [diff] [review]
Update previous patch with a test

Review of attachment 708813 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks for this.
I've been thinking about the best way to do this, and I think we shouldn't reuse code from GCLI like this. Can I suggest that we copy the code into CssLogic, and then later, I'll move the code into "host.js" so we don't have this duplicated?
Thanks.
Comment 18 Paul Rouget [:paul] 2013-02-04 06:24:59 PST
FYI, a Selectors.jsm module is coming (bug 831693). That might be a better place.
Comment 19 Christos Stathis 2013-02-07 12:41:05 PST
Created attachment 711468 [details] [diff] [review]
Update previous patch based on jwalker 's suggestions

If I understood it correctly, I copied the findCssSelector function into CssLogic and called it from there instead of gcli.jsm
Comment 20 Panos Astithas [:past] 2013-02-11 01:03:46 PST
https://hg.mozilla.org/integration/fx-team/rev/d4862b10556c
Comment 21 Tim Taubert [:ttaubert] 2013-02-12 10:16:39 PST
https://hg.mozilla.org/mozilla-central/rev/d4862b10556c

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