1356415 - Extract the logic to craft a CSS selector outside of DevTools

Status: RESOLVED FIXED

(DevTools :: Framework, defect, P3)

Description

[Julian Descottes [:jdescottes]]

Discussed for Bug 1325814.

Web-extensions need to be able to craft a CSS selector for a given node for which there is currently a devtools utility in css-logic.js: http://searchfox.org/mozilla-central/rev/d4eaa9c2fa54d553349ac88f0c312155a4c6e89e/devtools/shared/inspector/css-logic.js#365

The findCssSelector method depends on: 
- positionInNodeList (local method, no other dependency)
- CSS.escape (https://developer.mozilla.org/en-US/docs/Web/API/CSS/escape)
- getRootBindingParent (http://searchfox.org/mozilla-central/rev/d4eaa9c2fa54d553349ac88f0c312155a4c6e89e/devtools/shared/layout/utils.js#506, no other dependency)

As DevTools are moving outside of mozilla-central, we should extract this logic to a separate module that can be safely used by code that might run when devtools are not installed.

Comment 2

[Patrick Brosset <:pbro>]

DevTools bug triage (filter on CLIMBING SHOES).

Comment 3

[Julian Descottes [:jdescottes]]

Attachment: Bug 1356415 - get CSS selector in content for contextmenu to fix devtools CPOW;

Review commit: https://reviewboard.mozilla.org/r/133880/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/133880/

Comment 4

[Julian Descottes [:jdescottes]]

Attachment: Bug 1356415 - move devtools helper findCssSelector to shared module in toolkit;

Review commit: https://reviewboard.mozilla.org/r/133882/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/133882/

Comment 5

[Julian Descottes [:jdescottes]]

Attachment: Bug 1356415 - remove findCssSelector wrapper in devtools/shared/inspector/css-logic

Review commit: https://reviewboard.mozilla.org/r/133884/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/133884/

Comment 6

[Julian Descottes [:jdescottes]]

Comment on attachment 8861866 [details]
Bug 1356415 - get CSS selector in content for contextmenu to fix devtools CPOW;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/133880/diff/1-2/

Comment 7

[Julian Descottes [:jdescottes]]

Comment on attachment 8861867 [details]
Bug 1356415 - move devtools helper findCssSelector to shared module in toolkit;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/133882/diff/1-2/

Comment 8

[Julian Descottes [:jdescottes]]

try run at https://treeherder.mozilla.org/#/jobs?repo=try&revision=d83ed6ab819be6f47870ae506ec145c3895aecfa

Comment 9

[kernp25]

I have 2 bugs found in findCssSelector [2]:

1) Some web pages changes class names on hover.
2) The tagName [3] also must be escaped (this is also bug in devtools (because it doesn't work when using Copy -> CSS Selector))

First bug:
When using findCssSelector in handleContentContextMenu [1] the selector will be ".hover > div:nth-child(2)" (using in content script will not work), right-click on element -> Copy -> CSS Selector gives "li.san_navItem:nth-child(1) > a:nth-child(1) > div:nth-child(2)" which will work in content script.
We can fix this by removing that that part with the class name [4] then the selector will be "div#navigationWrp > nav:nth-child(1) > ul:nth-child(1) > li:nth-child(1) > a:nth-child(1) > div:nth-child(2)".

Second Bug:
The tagName [3] also can contain ":" so it must be escaped (using CSS.escape)

[1] https://dxr.mozilla.org/mozilla-central/source/browser/base/content/content.js#92
[2] https://dxr.mozilla.org/mozilla-central/source/devtools/shared/inspector/css-logic.js#365
[3] https://dxr.mozilla.org/mozilla-central/source/devtools/shared/inspector/css-logic.js#421
[4] https://dxr.mozilla.org/mozilla-central/source/devtools/shared/inspector/css-logic.js#390

Comment 10

[Julian Descottes [:jdescottes]]

(In reply to kernp25 from comment #9)
> I have 2 bugs found in findCssSelector [2]:

Are these issues related to the patches I attached to this bug? 

If not I'd suggest opening a dedicated bug to address them. 
I would really prefer this bug to be focused on removing the findCssSelector code from devtools.

Comment 11

[Julian Descottes [:jdescottes]]

Comment on attachment 8861866 [details]
Bug 1356415 - get CSS selector in content for contextmenu to fix devtools CPOW;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/133880/diff/2-3/

Comment 12

[Julian Descottes [:jdescottes]]

Comment on attachment 8861867 [details]
Bug 1356415 - move devtools helper findCssSelector to shared module in toolkit;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/133882/diff/2-3/

Comment 13

[kernp25]

(In reply to Julian Descottes [:jdescottes] from comment #10)
> (In reply to kernp25 from comment #9)
> > I have 2 bugs found in findCssSelector [2]:
> 
> Are these issues related to the patches I attached to this bug? 
> 
> If not I'd suggest opening a dedicated bug to address them. 
> I would really prefer this bug to be focused on removing the findCssSelector
> code from devtools.

Yes, we should create new bugs to fix these issues.

Comment 14

[Shane Caraveo (:mixedpuppy)]

Comment on attachment 8861867 [details]
Bug 1356415 - move devtools helper findCssSelector to shared module in toolkit;

https://reviewboard.mozilla.org/r/133882/#review136972

::: browser/base/content/content.js:15
(Diff revision 3)
>  
>  var {classes: Cc, interfaces: Ci, utils: Cu, results: Cr} = Components;
>  
>  Cu.import("resource://gre/modules/XPCOMUtils.jsm");
> +/* globals findCssSelector */
> +Cu.import("resource://gre/modules/css-selector.js");

drive-by.  can this be kept as a lazy loader?

Comment 15

[Julian Descottes [:jdescottes]]

Comment on attachment 8861867 [details]
Bug 1356415 - move devtools helper findCssSelector to shared module in toolkit;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/133882/diff/3-4/

Comment 16

[Julian Descottes [:jdescottes]]

Thanks! 

Fixed in the last patch, and removed a CPOW issue I introduced earlier (try https://treeherder.mozilla.org/#/jobs?repo=try&revision=381be576113b31e84a3bc3b10624c20cb8c667bd)

Comment 17

[kernp25]

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

Comment 18

[Julian Descottes [:jdescottes]]

Comment on attachment 8861866 [details]
Bug 1356415 - get CSS selector in content for contextmenu to fix devtools CPOW;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/133880/diff/3-4/

Comment 19

[Julian Descottes [:jdescottes]]

Comment on attachment 8861867 [details]
Bug 1356415 - move devtools helper findCssSelector to shared module in toolkit;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/133882/diff/4-5/

Comment 20

[Julian Descottes [:jdescottes]]

Shane: flagged you for review since you started looking at the patches, but let me know if I should redirect to somebody else.

Comment 21

[Shane Caraveo (:mixedpuppy)]

Comment on attachment 8861866 [details]
Bug 1356415 - get CSS selector in content for contextmenu to fix devtools CPOW;

https://reviewboard.mozilla.org/r/133880/#review137378

I'm fine with this but want to address the questions before r+

::: browser/base/content/content.js:276
(Diff revision 4)
> + * item.
> + *
> + * @param  {Node}
> + *         The node for which the CSS selectors should be computed
> + * @return {Array} array of css selectors (strings).
> + */

are all the selectors returned content only, or do some selectors include chrome in the selectors?  If the latter, we will need to think about how that gets to a webextension, and whether we might want to split chrome selectors out from content selectors.

::: devtools/client/framework/devtools-browser.js:299
(Diff revision 4)
>        Services.ww.openWindow(null, "chrome://webide/content/", "webide", "chrome,centerscreen,resizable", null);
>      }
>    },
>  
> -  inspectNode: function (tab, node) {
> +  inspectNode: function (tab, nodeSelectors) {
>      let target = TargetFactory.forTab(tab);

Depending on the answer above, a solution may be to keep the selector gathering here, and make the above "targetSelectors" content-only paths.

Comment 22

[Shane Caraveo (:mixedpuppy)]

Comment on attachment 8861867 [details]
Bug 1356415 - move devtools helper findCssSelector to shared module in toolkit;

https://reviewboard.mozilla.org/r/133882/#review137380

Assuming any potential changes in the first patch doesn't affect anything here, r+

Comment 23

[Julian Descottes [:jdescottes]]

Comment on attachment 8861866 [details]
Bug 1356415 - get CSS selector in content for contextmenu to fix devtools CPOW;

https://reviewboard.mozilla.org/r/133880/#review137378

> are all the selectors returned content only, or do some selectors include chrome in the selectors?  If the latter, we will need to think about how that gets to a webextension, and whether we might want to split chrome selectors out from content selectors.

From what I can tell, looping on ownerGlobal.frameElement from a content element should never cross to the browser documents (per http://searchfox.org/mozilla-central/rev/068e6f292503df13515a52321fc3844e237bf6a9/dom/base/nsGlobalWindow.cpp#9685-9689 possibly?)

In my tests anyway the selectors have always been content only, whether e10s was enabled or not.

> Depending on the answer above, a solution may be to keep the selector gathering here, and make the above "targetSelectors" content-only paths.

We should only have content selectors. Should I make it more obvious with comments?

Comment 24

[Julian Descottes [:jdescottes]]

Thanks for the reviews!
Let me know if you're comfortable with the current patch or if you want me to update it.

Comment 25

[Julian Descottes [:jdescottes]]

(ni? just to make sure you see my answer in comment 23)

Comment 26

[Shane Caraveo (:mixedpuppy)]

Comment on attachment 8861866 [details]
Bug 1356415 - get CSS selector in content for contextmenu to fix devtools CPOW;

https://reviewboard.mozilla.org/r/133880/#review138564

Comment 27

[Shane Caraveo (:mixedpuppy)]

Sounds good.  I just pushed to try, given that all works out fine it's ready to land.

Comment 28

[Julian Descottes [:jdescottes]]

Thanks!

Looking at try, all the failures seem to be unrelated intermittents, so I'm landing this.

Comment 29

[Mozilla Autoland]

We're sorry, Autoland could not rebase your commits for you automatically. Please manually rebase your commits and try again.

hg error in cmd: hg rebase -s 49de95fb2780 -d 879d80dec207: rebasing 393595:49de95fb2780 "Bug 1356415 - get CSS selector in content for contextmenu to fix devtools CPOW;r=mixedpuppy"
merging browser/base/content/content.js
merging browser/base/content/nsContextMenu.js
merging devtools/client/framework/devtools-browser.js
warning: conflicts while merging devtools/client/framework/devtools-browser.js! (edit, then use 'hg resolve --mark')
unresolved conflicts (see hg resolve, then hg rebase --continue)

Comment 30

[Julian Descottes [:jdescottes]]

Rebased.

Try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=4c0bab1cc51347b10ff95c1072768c4105b87d7f

Comment 31

[Julian Descottes [:jdescottes]]

Comment on attachment 8861866 [details]
Bug 1356415 - get CSS selector in content for contextmenu to fix devtools CPOW;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/133880/diff/4-5/

Comment 32

[Julian Descottes [:jdescottes]]

Comment on attachment 8861867 [details]
Bug 1356415 - move devtools helper findCssSelector to shared module in toolkit;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/133882/diff/5-6/

Comment 33

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/52a37a9d52a3
get CSS selector in content for contextmenu to fix devtools CPOW;r=mixedpuppy
https://hg.mozilla.org/integration/autoland/rev/aa507b3a8893
move devtools helper findCssSelector to shared module in toolkit;r=mixedpuppy

Comment 34

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

https://hg.mozilla.org/mozilla-central/rev/52a37a9d52a3
https://hg.mozilla.org/mozilla-central/rev/aa507b3a8893