Inspector should always provide a unique selector

RESOLVED FIXED in Firefox 21

Status

()

Firefox
Developer Tools: Inspector
P3
enhancement
RESOLVED FIXED
6 years ago
4 years ago

People

(Reporter: dietrich, Assigned: Christos Stathis)

Tracking

unspecified
Firefox 21
x86
Mac OS X
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [good first bug][mentor=jwalker][lang=js])

Attachments

(1 attachment, 2 obsolete attachments)

(Reporter)

Description

6 years ago
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.
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
Severity: normal → enhancement
(Reporter)

Comment 2

6 years ago
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.
Depends on: 705636
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.
Blocks: 674134

Comment 4

5 years ago
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
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?
Assignee: nobody → jwalker
Status: NEW → ASSIGNED
Priority: -- → P3
(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.
Assignee: jwalker → nobody
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.

Updated

5 years ago
Status: ASSIGNED → NEW

Updated

5 years ago
Component: Developer Tools → Developer Tools: Inspector
QA Contact: developer.tools → developer.tools.inspector

Updated

5 years ago
Blocks: 705947

Updated

5 years ago
Duplicate of this bug: 703383

Updated

5 years ago
Whiteboard: [good first bug][mentor=paul][lang=js]
(Assignee)

Comment 9

4 years ago
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

4 years ago
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/
Attachment #708068 - Flags: feedback+

Comment 11

4 years ago
Once the test has been added, can you ask ":jwalker" to review this patch?

Thank you!
Flags: needinfo?

Comment 12

4 years ago
(clearing needinfo)
Flags: needinfo?

Comment 13

4 years ago
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

Updated

4 years ago
Blocks: 831711
(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'.
Assignee: nobody → chstath
(Assignee)

Comment 15

4 years ago
Thanks for the quick feedback! I 'll have a test and a correct patch ASAP.
Whiteboard: [good first bug][mentor=paul][lang=js] → [good first bug][mentor=jwalker][lang=js]
(Assignee)

Comment 16

4 years ago
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.
Attachment #708068 - Attachment is obsolete: true
Attachment #708813 - Flags: review?(jwalker)
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.
Attachment #708813 - Flags: review?(jwalker)

Comment 18

4 years ago
FYI, a Selectors.jsm module is coming (bug 831693). That might be a better place.
(Assignee)

Comment 19

4 years ago
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
Attachment #708813 - Attachment is obsolete: true
Attachment #711468 - Flags: review?(jwalker)
Attachment #711468 - Flags: review?(jwalker) → review+
Whiteboard: [good first bug][mentor=jwalker][lang=js] → [good first bug][mentor=jwalker][lang=js][land-in-fx-team]
https://hg.mozilla.org/integration/fx-team/rev/d4862b10556c
Whiteboard: [good first bug][mentor=jwalker][lang=js][land-in-fx-team] → [good first bug][mentor=jwalker][lang=js][fixed-in-fx-team]
https://hg.mozilla.org/mozilla-central/rev/d4862b10556c
Status: NEW → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Whiteboard: [good first bug][mentor=jwalker][lang=js][fixed-in-fx-team] → [good first bug][mentor=jwalker][lang=js]
Target Milestone: --- → Firefox 21
No longer blocks: 831711
You need to log in before you can comment on or make changes to this bug.