contextMenus.OnClickData should provide selector of the clicked element

NEW
Assigned to

Status

()

Toolkit
WebExtensions: General
P3
normal
4 months ago
15 hours ago

People

(Reporter: kernp25, Assigned: kernp25, Mentored)

Tracking

(Depends on: 1 bug)

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [design-decision-approved][contextMenus] triaged)

Attachments

(1 attachment, 1 obsolete attachment)

(Assignee)

Description

4 months ago
I'm developing a WebExtension called "Copy Link Title".

To get the text of the link (if any ofc), i need the element that the user rigth-clicked on (e.g. link)

Now i'm doing this: 
function menuClick(info, tab) {
  let url = new URL(info.linkUrl);
  browser.tabs.executeScript(tab.id, {
    code: `
      var link = document.querySelector('a[href="${url.pathname}"]');
      ....
    `
  });

This works ok (not always (must modifie this to get this working properlie)) but i think it would be helpful if the info would contain the css selector of the element.

It should have the same CSS Selectors when using the Inspector: Tools -> Web Developer -> Inspector.
Rigth click on element -> Copy -> CSS Selector
(Assignee)

Comment 1

4 months ago
But, the code i posted will not work 100% because, if the page contains the same link twice (with different title) then it will not work. That's why i need the css selector from the element.
(Assignee)

Comment 2

4 months ago
I can also use:
let clickedEl = null;
document.addEventListener("contextmenu", function(event) {
	clickedEl = event.target;
}, true);

But i think this is ugly. (content-script running in every page, adding contextmenu event listener)

Updated

4 months ago
Whiteboard: [design-decision-needed] triaged

Updated

3 months ago
Whiteboard: [design-decision-needed] triaged → [design-decision-needed][contextMenus] triaged
This has been added to the March 7 WebExtensions Triage mtg. agenda. 

Agenda: https://docs.google.com/document/d/1zzfedbTKAHUmm4UctxSOIJV3iKayXQ8TuXPodXW8wC0/edit#

Comment 4

2 months ago
We discussed about this during the last WebExtensions Triage meeting and we decided that we can approve this.
Status: UNCONFIRMED → NEW
Ever confirmed: true
Priority: -- → P3
Whiteboard: [design-decision-needed][contextMenus] triaged → [design-decision-approved][contextMenus] triaged

Comment 5

2 months ago
As a side note (and as I said during the meeting):

it is possible that the generated CSS selector can be "not valid" anymore when the content scripts receive it (because the content of the page can have been changed in the meantime) and so we have to document this behavior in the MDN docs once this feature has been implemented, but it will be definitely cleaner then the alternative workarounds.
(Assignee)

Comment 6

14 days ago
Created attachment 8856480 [details] [diff] [review]
bug-1325814-fix.patch
Attachment #8856480 - Flags: feedback?(kmaglione+bmo)
(Assignee)

Comment 7

14 days ago
Created attachment 8856485 [details] [diff] [review]
bug-1325814-fix-v2.patch
Attachment #8856480 - Attachment is obsolete: true
Attachment #8856480 - Flags: feedback?(kmaglione+bmo)
Attachment #8856485 - Flags: feedback?(kmaglione+bmo)

Updated

13 days ago
Assignee: nobody → kernp25
Mentor: mixedpuppy@gmail.com
(Assignee)

Updated

12 days ago
Attachment #8856485 - Flags: feedback?(kmaglione+bmo) → feedback?(mixedpuppy)
Comment on attachment 8856485 [details] [diff] [review]
bug-1325814-fix-v2.patch


>   let selectionInfo = BrowserUtils.getSelectionDetails(content);
> 
>   let loadContext = docShell.QueryInterface(Ci.nsILoadContext);
>   let userContextId = loadContext.originAttributes.userContextId;
> 
>+  function isHTMLLink(aNode) {
>+    // Be consistent with what nsContextMenu.js does.
>+    return ((aNode instanceof Ci.nsIDOMHTMLAnchorElement && aNode.href) ||
>+            (aNode instanceof Ci.nsIDOMHTMLAreaElement && aNode.href) ||
>+            aNode instanceof Ci.nsIDOMHTMLLinkElement);
>+  }
>+
>+  let node = event.target;
>+  while (node && !isHTMLLink(node)) {
>+    node = node.parentNode;
>+  }
>+  
>+  let selector = CssLogic.findCssSelector(node || event.target);
>+  

I have a few initial reactions here.  

1. this is specific to one use case and maybe we should be more generic and just do:

let selector = CssLogic.findCssSelector(event.target);

The content script can then dig further if it is looking for a specific type of target (e.g. link).

2. *maybe* this should go into BrowserUtils.getSelectionDetails.

3. we need to find out if CssLogic will a) remain a part of the distribution, and b) perform reasonably here.  I'll try to find someone to give that feedback.
Attachment #8856485 - Flags: feedback?(mixedpuppy) → feedback+
Comment on attachment 8856485 [details] [diff] [review]
bug-1325814-fix-v2.patch

Patrick, what do you think of the use of devtools here to get the selector of the target of a context menu?
Attachment #8856485 - Flags: feedback?(pbrosset)
Comment on attachment 8856485 [details] [diff] [review]
bug-1325814-fix-v2.patch

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

Using source files from /devtools is unfortunately going to be incompatible with DevTools' project to become an addon and live outside of m-c.
Here's a description of this project: https://docs.google.com/document/d/1tod1YH8uuzZykKawNvrLyyxuxxYNUfYsvTuHFa2Jf9s/edit
We are actively working on this now. Knowing exactly when we'll make the jump is hard because there is still a lot of work to do, but it will happen this year.
In any case, relying on this file is a bad idea now because it will cease to exist soon-ish.

Maybe the logic to craft the selector can be copied to another module.

Having said that, I know that Firefox is going to still have a small DevTools part which will be responsible for adding the DevTools menus and key shortcuts. One of these is the Inspect Element menu, and I know that it also needs this CSS selector, so maybe that part will actually stay in m-c! 
Let's ping Julian who will know better what stays and what doesn't
Attachment #8856485 - Flags: feedback?(pbrosset) → feedback?(jdescottes)
(Assignee)

Comment 11

11 days ago

(In reply to Shane Caraveo (:mixedpuppy) from comment #8)
> Comment on attachment 8856485 [details] [diff] [review]
> bug-1325814-fix-v2.patch
> 
> 
> >   let selectionInfo = BrowserUtils.getSelectionDetails(content);
> > 
> >   let loadContext = docShell.QueryInterface(Ci.nsILoadContext);
> >   let userContextId = loadContext.originAttributes.userContextId;
> > 
> >+  function isHTMLLink(aNode) {
> >+    // Be consistent with what nsContextMenu.js does.
> >+    return ((aNode instanceof Ci.nsIDOMHTMLAnchorElement && aNode.href) ||
> >+            (aNode instanceof Ci.nsIDOMHTMLAreaElement && aNode.href) ||
> >+            aNode instanceof Ci.nsIDOMHTMLLinkElement);
> >+  }
> >+
> >+  let node = event.target;
> >+  while (node && !isHTMLLink(node)) {
> >+    node = node.parentNode;
> >+  }
> >+  
> >+  let selector = CssLogic.findCssSelector(node || event.target);
> >+  
> 
> I have a few initial reactions here.  
> 
> 1. this is specific to one use case and maybe we should be more generic and
> just do:
> 
> let selector = CssLogic.findCssSelector(event.target);
> 
> The content script can then dig further if it is looking for a specific type
> of target (e.g. link).

I have no problem with that. At least it is better then the alternative workarounds i posted above.

I have tried to add CssLogic.findCssSelector to https://dxr.mozilla.org/mozilla-central/source/browser/base/content/nsContextMenu.js but there were many unsafe/forbidden CPOW usage warnings in console.

Right-click -> Inspect Element also shows many unsafe/forbidden CPOW usage warnings in console. (using nightly browser 55.0a1 (2017-04-13) (64-bit))

https://dxr.mozilla.org/mozilla-central/source/devtools/client/framework/devtools-browser.js#299
Comment on attachment 8856485 [details] [diff] [review]
bug-1325814-fix-v2.patch

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

(In reply to Patrick Brosset <:pbro> from comment #10)
> Comment on attachment 8856485 [details] [diff] [review]
> bug-1325814-fix-v2.patch
> 
> Review of attachment 8856485 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Using source files from /devtools is unfortunately going to be incompatible
> with DevTools' project to become an addon and live outside of m-c.
> Here's a description of this project:
> https://docs.google.com/document/d/
> 1tod1YH8uuzZykKawNvrLyyxuxxYNUfYsvTuHFa2Jf9s/edit
> We are actively working on this now. Knowing exactly when we'll make the
> jump is hard because there is still a lot of work to do, but it will happen
> this year.
> In any case, relying on this file is a bad idea now because it will cease to
> exist soon-ish.
> 

The roadmap is not clear yet, but we'd like the migration to happen during the Firefox 56 timeframe, 57 at the latest.

> Maybe the logic to craft the selector can be copied to another module.
> 

Right now we are identifying the devtools modules that should be extracted from the devtools folder to remain in mozilla-central after our migration. In this case, if the logic to craft a selector is only in devtools at the moment, it's the occasion to extract it to a shared non-devtools module. I looked at the code, and the dependencies needed to extract only this method seems pretty small so we should be able to do it fairly easily.

> Having said that, I know that Firefox is going to still have a small
> DevTools part which will be responsible for adding the DevTools menus and
> key shortcuts. One of these is the Inspect Element menu, and I know that it
> also needs this CSS selector, so maybe that part will actually stay in m-c! 

Just to clarify what is happening to the Inspect Element menu item:
- if devtools addon is installed, the menu item is displayed and relies on the devtools code to perform its command
- if devtools addon is not installed, the menu item is replaced by another one that triggers the installation of the devtools addon (not 100% sure, but you get the idea, the behavior is different, and for that you don't need the CSS selector)

Which means that so far this code path was not identified as something that should remain in mozilla-central.

I think the way forward is to create a blocking bug to extract this API outside of devtools.
Attachment #8856485 - Flags: feedback?(jdescottes)
Depends on: 1356415
(In reply to Julian Descottes [:jdescottes] from comment #12)
> > Having said that, I know that Firefox is going to still have a small
> > DevTools part which will be responsible for adding the DevTools menus and
> > key shortcuts. One of these is the Inspect Element menu, and I know that it
> > also needs this CSS selector, so maybe that part will actually stay in m-c! 
> 
> Just to clarify what is happening to the Inspect Element menu item:
> - if devtools addon is installed, the menu item is displayed and relies on
> the devtools code to perform its command
> 
> Which means that so far this code path was not identified as something that
> should remain in mozilla-central.

On that subject, see bug 1355996. We need to move the logic that generates
those selectors into the content process, which ideally means moving out of
devtools and into content.js/nsContextMenu.js.

> - if devtools addon is not installed, the menu item is replaced by another
> one that triggers the installation of the devtools addon (not 100% sure, but
> you get the idea, the behavior is different, and for that you don't need the
> CSS selector)

Why not? Presumably we'd want to save the stored selector, and open the
inspector to it if the page is still active when the installation finishes.
Depends on: 1356417
(In reply to Kris Maglione [:kmag] from comment #13)
> (In reply to Julian Descottes [:jdescottes] from comment #12)
> 
> > - if devtools addon is not installed, the menu item is replaced by another
> > one that triggers the installation of the devtools addon (not 100% sure, but
> > you get the idea, the behavior is different, and for that you don't need the
> > CSS selector)
> 
> Why not? Presumably we'd want to save the stored selector, and open the
> inspector to it if the page is still active when the installation finishes.

Maybe, UX is still pending on all the paths that would lead to a devtools install.
If we want to seamlessly install devtools and open them after the install, then yes we'll need to craft the selector as well.
(In reply to Kris Maglione [:kmag] from comment #13)
> > - if devtools addon is not installed, the menu item is replaced by another
> > one that triggers the installation of the devtools addon (not 100% sure, but
> > you get the idea, the behavior is different, and for that you don't need the
> > CSS selector)
> 
> Why not? Presumably we'd want to save the stored selector, and open the
> inspector to it if the page is still active when the installation finishes.

A reference to the DOM Element can be kept around, no need for a selector at this point.
You need to log in before you can comment on or make changes to this bug.