Closed Bug 1356415 Opened 7 years ago Closed 7 years ago

Extract the logic to craft a CSS selector outside of DevTools

Categories

(DevTools :: Framework, defect, P3)

Product:
DevTools
Component:
Framework
Type:
defect

Tracking

(firefox55 fixed)

Status:
RESOLVED FIXED
Milestone:
Firefox 55
Tracking Flags:
Tracking Status
firefox55 --- fixed

People

(Reporter: jdescottes, Assigned: jdescottes)

References

Details

Attachments

(2 files, 1 obsolete file)

Bug 1356415 - get CSS selector in content for contextmenu to fix devtools CPOW;
59 bytes, text/x-review-board-request
mixedpuppy
: review+
Details
Bug 1356415 - move devtools helper findCssSelector to shared module in toolkit;
59 bytes, text/x-review-board-request
mixedpuppy
: review+
Details 
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.
Blocks: 1355996
DevTools bug triage (filter on CLIMBING SHOES).
Priority: -- → P3
Blocks: dt-addon
Attachment #8861868 - Attachment is obsolete: true
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
(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.
Flags: needinfo?(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.
Flags: needinfo?(kernp25)
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?
Thanks! 

Fixed in the last patch, and removed a CPOW issue I introduced earlier (try https://treeherder.mozilla.org/#/jobs?repo=try&revision=381be576113b31e84a3bc3b10624c20cb8c667bd)
Shane: flagged you for review since you started looking at the patches, but let me know if I should redirect to somebody else.
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 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+
Attachment #8861867 - Flags: review?(mixedpuppy) → review+
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?
Thanks for the reviews!
Let me know if you're comfortable with the current patch or if you want me to update it.
(ni? just to make sure you see my answer in comment 23)
Flags: needinfo?(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
Attachment #8861866 - Flags: review?(mixedpuppy) → review+
Sounds good.  I just pushed to try, given that all works out fine it's ready to land.
Flags: needinfo?(mixedpuppy)
Thanks!

Looking at try, all the failures seem to be unrelated intermittents, so I'm landing this.
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)
Pushed by jdescottes@mozilla.com:
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
https://hg.mozilla.org/mozilla-central/rev/52a37a9d52a3
https://hg.mozilla.org/mozilla-central/rev/aa507b3a8893
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
status-firefox55: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 55
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: