967201 - "Copy Unique Selector" doesn't place anything in the clipboard in e10s windows and Browser Toolbox

Status: RESOLVED FIXED

(DevTools :: Inspector, defect, P1)

Description

[Jared Wein [:jaws] (please needinfo? me)]

STR:
Open the Developer Toolbox for inspecting the browser chrome
Select a 'tab' element
Right-click and choose "Copy Unique Selector".

ER:
The clipboard has a selector that will apply for the selected tab.

AR:
The clipboard is empty.

Comment 1

[Brian Grinstead [:bgrins]]

Just to be clear, this is happening with the Browser Toolbox only, so it isn't just an issue with XUL (this works if you open a normal toolbox on about:addons, for instance)

Comment 2

[Andrew Halberstadt [:ahal]]

I'm very interested in the cause of this bug. Is there code somewhere in devtools land that *would* return a unique selector for anonymous nodes but for the fact that the ui is broken? Or is the issue that a unique selector for anonymous nodes is hard/impossible and therefore that feature doesn't exist yet?

If the former, could someone point me towards some code? Thanks!

Comment 3

[Brian Grinstead [:bgrins]]

(In reply to Andrew Halberstadt [:ahal] from comment #2)
> I'm very interested in the cause of this bug. Is there code somewhere in
> devtools land that *would* return a unique selector for anonymous nodes but
> for the fact that the ui is broken? Or is the issue that a unique selector
> for anonymous nodes is hard/impossible and therefore that feature doesn't
> exist yet?
> 
> If the former, could someone point me towards some code? Thanks!

Actually, I was wrong about the reason for this.  This is a problem because the copy unique selector is happening in the frontend only [0].  It is affecting e10s windows as well as the Browser Toolbox.  We need to add a getUniqueSelector or similar to the server.  Patrick, would this make more sense on the WalkerActor or NodeActor?  It seems more natural on the NodeActor since it only touches the node, but lots of other similar kinds of things are already on the walker (like innerHTML, outerHTML, editTagName, and probably others).

[0]: http://dxr.mozilla.org/mozilla-central/source/browser/devtools/inspector/inspector-panel.js#839

Comment 4

[Patrick Brosset <:pbro>]

(In reply to Brian Grinstead [:bgrins] from comment #3)
> (In reply to Andrew Halberstadt [:ahal] from comment #2)
> > I'm very interested in the cause of this bug. Is there code somewhere in
> > devtools land that *would* return a unique selector for anonymous nodes but
> > for the fact that the ui is broken? Or is the issue that a unique selector
> > for anonymous nodes is hard/impossible and therefore that feature doesn't
> > exist yet?
> > 
> > If the former, could someone point me towards some code? Thanks!
> 
> Actually, I was wrong about the reason for this.  This is a problem because
> the copy unique selector is happening in the frontend only [0].  It is
> affecting e10s windows as well as the Browser Toolbox.  We need to add a
> getUniqueSelector or similar to the server.  Patrick, would this make more
> sense on the WalkerActor or NodeActor?  It seems more natural on the
> NodeActor since it only touches the node, but lots of other similar kinds of
> things are already on the walker (like innerHTML, outerHTML, editTagName,
> and probably others).
> 
> [0]:
> http://dxr.mozilla.org/mozilla-central/source/browser/devtools/inspector/
> inspector-panel.js#839
Brian is right, the first thing to fix is to stop accessing the raw node on the front end. [0] is where we compute a unique css selector for the currently selected node, and we do this by accessing |this.selection.node| which stores the raw dom node. This has been deprecated for some time as we move closer to e10s and has for sure never worked for the browser toolbox or when debugging a remote device.

So as Brian said, we need a new protocol.js method on one of our inspector-domain actors. I don't see a big difference in adding this method to either NodeActor or WalkerActor. NodeActor does make more sense. Let's put it there (i.e. in this object: [1]).

This method will need to compute a unique selector using |CssLogic.findCssSelector(this.rawNode)| and should then return the resulting string back to the client side.
Actually storing the string in the clipboard still need to occur on the frontend though, otherwise the string would be stored in the remove device when debugging via a remove connection.

So it looks like we then just need to make |copyUniqueSelector| [2] an async function that calls the new protocol method, waits for the string and stores it in the clipboard.

[0] http://dxr.mozilla.org/mozilla-central/source/browser/devtools/inspector/inspector-panel.js#845
[1] http://dxr.mozilla.org/mozilla-central/source/toolkit/devtools/server/actors/inspector.js#193
[2] http://dxr.mozilla.org/mozilla-central/source/browser/devtools/inspector/inspector-panel.js#839

Comment 5

[Patrick Brosset <:pbro>]

s/remove/remote (twice!)

Comment 6

[Brian Grinstead [:bgrins]]

(In reply to Andrew Halberstadt [:ahal] from comment #2)
> I'm very interested in the cause of this bug. Is there code somewhere in
> devtools land that *would* return a unique selector for anonymous nodes but
> for the fact that the ui is broken? Or is the issue that a unique selector
> for anonymous nodes is hard/impossible and therefore that feature doesn't
> exist yet?
> 
> If the former, could someone point me towards some code? Thanks!

Hi Andrew, I couldn't tell exactly by your comment - are you planning to work on this?  The solution described in Comment 4 is the way to go if so.  We will be needing this for e10s support.

Comment 7

[Andrew Halberstadt [:ahal]]

Sorry, I wasn't planning to work on this. I was wondering if there existed an implementation of the css selector that worked with anonymous content (e.g xul bindings), but looks like there isn't.

Comment 8

[Brian Grinstead [:bgrins]]

(In reply to Andrew Halberstadt [:ahal] from comment #7)
> Sorry, I wasn't planning to work on this. I was wondering if there existed
> an implementation of the css selector that worked with anonymous content
> (e.g xul bindings), but looks like there isn't.

No problem.  By the way, we are going to need to somehow serialize selections for anonymous content in Bug 1037437 for reselecting pseudo elements and possibly xbl content on page reload.  I'm not actually sure if there is a way to do this with just a selector or if we will need to implement something else for it.

Comment 9

[Andrew Halberstadt [:ahal]]

(In reply to Brian Grinstead [:bgrins] from comment #8)
> No problem.  By the way, we are going to need to somehow serialize
> selections for anonymous content in Bug 1037437 for reselecting pseudo
> elements and possibly xbl content on page reload.  I'm not actually sure if
> there is a way to do this with just a selector or if we will need to
> implement something else for it.

I'm sort of working on the same thing in bug 1080764. Afaict, it's not possible to use selector. Mozmill (an in-house ui automation tool) invented a Lookup expression syntax [1] to do what you are suggesting.. but I'm not sure I'd recommend following suit :).

[1] https://developer.mozilla.org/en-US/docs/Mozmill/Finding_Mozmill_Elements#Lookup

Comment 10

[Heather Arthur [:harth]]

Attachment: Remote getting a unique selector

This makes the "Copy Unique Selector" item work in e10s and the browser toolbox. It now hides the menu item if it's not connected something that implements NodeActor.getUniqueSelector().

The test didn't need to be changed as it was just waiting for the clipboard.

Comment 11

[Patrick Brosset <:pbro>]

Comment on attachment 8506501 [details] [diff] [review]
Remote getting a unique selector

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

Short and nice. No problem there. R+ as long as try is green.
One thing though: I see css-logic is still required in the file, and I realized we actually do this:
http://mxr.mozilla.org/mozilla-central/source/browser/devtools/inspector/inspector-panel.js#408
which we need to remote too. Up to you if we want to do this in a follow-up or in this bug.

Comment 12

[Heather Arthur [:harth]]

(In reply to Patrick Brosset [:pbrosset] [:patrick] from comment #11)
> One thing though: I see css-logic is still required in the file, and I
> realized we actually do this:
> http://mxr.mozilla.org/mozilla-central/source/browser/devtools/inspector/
> inspector-panel.js#408
> which we need to remote too. Up to you if we want to do this in a follow-up
> or in this bug.

Thanks. I'll do that as part of bug 1036409. Try:

https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=3e9e65be6a0a

Comment 13

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/integration/fx-team/rev/8c38f996a82a

Comment 14

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/8c38f996a82a