contextMenus.OnClickData should provide selector of the clicked element

VERIFIED FIXED in Firefox 63

Status

P3
enhancement
VERIFIED FIXED
2 years ago
3 months ago

People

(Reporter: kernp25, Assigned: robwu, Mentored)

Tracking

(Blocks: 1 bug, {dev-doc-needed})

unspecified
mozilla63
dev-doc-needed
Dependency tree / graph

Firefox Tracking Flags

(firefox63 verified)

Details

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

Attachments

(8 attachments, 2 obsolete attachments)

(Reporter)

Description

2 years 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
(Reporter)

Comment 1

2 years 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.
(Reporter)

Comment 2

2 years 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

2 years ago
Whiteboard: [design-decision-needed] triaged

Updated

2 years 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 years 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 years 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.
(Reporter)

Comment 6

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

Comment 7

2 years 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

2 years ago
Assignee: nobody → kernp25
Mentor: mixedpuppy
(Reporter)

Updated

2 years 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)
(Reporter)

Comment 11

2 years 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)
(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.
(Assignee)

Comment 16

2 years ago
Bug 1210990 discusses a way to retrieve the DOM element related to a webRequest event (the 9th comment onwards).
If that is implemented, the underlying may be useful for the contextMenus.onClicked event too.
See Also: → bug 1210990
(Reporter)

Comment 17

2 years ago
We need to fix these bugs first:
https://bugzilla.mozilla.org/show_bug.cgi?id=1360136
https://bugzilla.mozilla.org/show_bug.cgi?id=1360132

Otherwise, it won't work correctly.
(In reply to kernp25 from comment #0)
> 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)


I also face to the same problem on porting my extension to WebExtensions.
However, I think that it's not proper way to add `selector` field to contextmenu.OnClickData.

Simply, I feel it's more suitable to add `linkText` propery to contextmenu.OnClickData for this problem.
(Reporter)

Comment 19

2 years ago
Please look here:
https://bugzilla.mozilla.org/show_bug.cgi?id=1343236

Tell them that you need it too, maybe they approve it?
(In reply to kernp25 from comment #19)
> Please look here:
> https://bugzilla.mozilla.org/show_bug.cgi?id=1343236
> 
> Tell them that you need it too, maybe they approve it?

Ah, okay. Thank you.
Duplicate of this bug: 1392568
(Reporter)

Comment 22

a year ago
Created attachment 8931890 [details] [diff] [review]
bug_1325814_fix_v3.patch
Attachment #8856485 - Attachment is obsolete: true
Attachment #8931890 - Flags: feedback?(mixedpuppy)
(Reporter)

Comment 23

a year ago
Created attachment 8931971 [details] [diff] [review]
contextMenus.OnClickData should provide selector of the clicked element
Comment on attachment 8931890 [details] [diff] [review]
bug_1325814_fix_v3.patch

I think this is generally fine.  This currently will only pass the css selector for the frame in which the context menu happened.  That should be ok since the extension also gets frameId.

I'd want to additionally ensure that targetSelector only contains selectors from content (ie. not from firefox chrome).  Right now it looks like that is the case (ie. only set via content.js).  I think just having tests that verify an extension does not get a css selector when the context menu is on chrome will be enough.
Attachment #8931890 - Flags: feedback?(mixedpuppy) → feedback+
seems this bug got dropped a while back, are you still on it?
Flags: needinfo?(kernp25)
(Reporter)

Comment 26

11 months ago
I have asked wisniewskit@gmail.com if he can take this bug and finish it.
Flags: needinfo?(kernp25) → needinfo?(wisniewskit)

Comment 27

11 months ago
I'll see if I can find some spare time to finish this, but I can't guarantee it will be soon.
Flags: needinfo?(wisniewskit)

Updated

11 months ago
Duplicate of this bug: 1437497
Duplicate of this bug: 1439384

Updated

9 months ago
See Also: → bug 1418424

Updated

7 months ago
Product: Toolkit → WebExtensions

Updated

6 months ago
Blocks: 1466876
Severity: normal → enhancement
(Assignee)

Updated

6 months ago
Assignee: kernp25 → rob
Status: NEW → ASSIGNED
Iteration: --- → 63.2 - July 23
(Assignee)

Updated

6 months ago
Iteration: 63.2 - July 23 → 63.3 - Aug 6
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 35

5 months ago
mozreview-review
Comment on attachment 8997825 [details]
Bug 1325814 - Clarify file overview comment in ExtensionChild.jsm

https://reviewboard.mozilla.org/r/261534/#review268532
Attachment #8997825 - Flags: review?(mixedpuppy) → review+

Comment 36

5 months ago
mozreview-review
Comment on attachment 8997826 [details]
Bug 1325814 - Remove unnecessary schemas/menus_internal.json

https://reviewboard.mozilla.org/r/261536/#review268534

::: commit-message-e8147:12
(Diff revision 1)
> +
> +Note: menus_internal.json specified the "menusInternal" namespace
> +which we do indeed implement in parent/ext-menus.js (and use in
> +child/ext-menus.js). However, none of the methods that we add to
> +menusInternal are actually defined in the schema.
> +This use of menusInternal was introduced in part 2 of bug 13333403 and

That's a big bug number.
Attachment #8997826 - Flags: review?(mixedpuppy) → review+

Comment 37

5 months ago
mozreview-review
Comment on attachment 8997827 [details]
Bug 1325814 - Add extension API to find menu target

https://reviewboard.mozilla.org/r/261538/#review268574

I'm not feeling fond of the implementation on this.  First is the additional permission, which is conflicting to some extent with the includeSensitiveData part.  It also feels like a lot of overhead to deal with holding on to an element for a short bit of time.  So after thinking about this, I'd like to suggest something slightly different.

Move the "on-prepare-contextmenu" observer to child/ext-menu and hold onto a single weakRef (lastContextNode).  The observer can be added/removed based on listener use rather than a permission.  The notification shouldn't need all the data, just the target element.  Add a child handler for onShown.  Use lastContextNode to add info.targetElement to the data fired for onShown and onClicked, respecting includeSensitiveData for onShown.

::: browser/components/extensions/child/ext-menus-child.js:12
(Diff revision 1)
> +  getAPI(context) {
> +    return {
> +      menus: {
> +        getTargetElement(targetElementId) {
> +          let element = ExtensionMenuChild.resolveTargetElementId(targetElementId);
> +          if (element && context.contentWindow.document.contains(element)) {

I'm not clear on this restriction, since, in the tests, it seems you can get around it anyway.  Is there a technical reason we have to do this?

::: browser/components/extensions/parent/ext-menus.js:484
(Diff revision 1)
> +  let {extensionInfo} = contextData;
> +  if (extensionInfo) {
> +    if (extensionInfo.targetElementId !== undefined &&
> +        extension.hasPermission("menus.getTargetElement")) {
> +      info.targetElementId = extensionInfo.targetElementId;
> +    }
> +  }

move to the includeSensitiveData section below.
I pushed that review comment 37 early to point out the sensitive data part, forgot about my notes which probably would have changed.
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 43

5 months ago
mozreview-review
Comment on attachment 8997859 [details]
Bug 1325814 - Support shadow DOM in menus.getTargetElement + test

https://reviewboard.mozilla.org/r/261568/#review268974
Attachment #8997859 - Flags: review?(mixedpuppy) → review+

Comment 44

5 months ago
mozreview-review
Comment on attachment 8997827 [details]
Bug 1325814 - Add extension API to find menu target

https://reviewboard.mozilla.org/r/261538/#review268978

::: browser/components/extensions/parent/ext-menus.js:498
(Diff revision 2)
> +  // menus.getTargetElement requires the "menus" permission, so do not set
> +  // targetElementId for extensions with only the "contextMenus" permission.
> +  if (contextData.timeStamp && extension.hasPermission("menus")) {
> +    // Convert to integer, in case the DOMHighResTimeStamp has a fractional part.
> +    info.targetElementId = Math.floor(contextData.timeStamp);
> +  }

I still want this in the includeSensitiveData block below.  No point in exposing it if it cannot be used.
Attachment #8997827 - Flags: review?(mixedpuppy) → review+

Comment 45

5 months ago
mozreview-review
Comment on attachment 8997827 [details]
Bug 1325814 - Add extension API to find menu target

https://reviewboard.mozilla.org/r/261538/#review269162

un-reviewing for now, see my email for thoughts on some changes.
Attachment #8997827 - Flags: review+ → review?(mixedpuppy)

Comment 46

5 months ago
mozreview-review
Comment on attachment 8997828 [details]
Bug 1325814 - Add tests for menus.getTargetElement API

https://reviewboard.mozilla.org/r/261540/#review269164

::: browser/components/extensions/test/browser/browser_ext_menus_targetElement_extension.js:26
(Diff revision 2)
> +      // This test exercises that scenario, and checks that the element is still
> +      // available. By using getViews(), the getTargetElement method can be
> +      // verified synchronously in response to the onShown event.
> +      let [tabGlobal] = browser.extension.getViews({type: "tab"});
> +      elem = tabGlobal.browser.menus.getTargetElement(info.targetElementId);
> +      browser.test.assertEq("BUTTON", elem.tagName, "should get tab element");

All the test messages in this file have this nit. 

The element retrieved is not the tab, sidebar, or popup, it is always the button in those content areas.  

Try something along the lines of "should get button from tab content"
Attachment #8997828 - Flags: review?(mixedpuppy) → review+
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 52

5 months ago
mozreview-review
Comment on attachment 8997827 [details]
Bug 1325814 - Add extension API to find menu target

https://reviewboard.mozilla.org/r/261538/#review269260
Attachment #8997827 - Flags: review?(mixedpuppy) → review+

Comment 53

5 months ago
Pushed by rob@robwu.nl:
https://hg.mozilla.org/integration/autoland/rev/51d530820c10
Clarify file overview comment in ExtensionChild.jsm r=mixedpuppy
https://hg.mozilla.org/integration/autoland/rev/f47fc4512dfb
Remove unnecessary schemas/menus_internal.json r=mixedpuppy
https://hg.mozilla.org/integration/autoland/rev/e08c04702ab4
Add extension API to find menu target r=mixedpuppy
https://hg.mozilla.org/integration/autoland/rev/a7c2208c1f85
Add tests for menus.getTargetElement API r=mixedpuppy
https://hg.mozilla.org/integration/autoland/rev/68dbca6faa6e
Support shadow DOM in menus.getTargetElement + test r=mixedpuppy

Comment 55

5 months ago
Very exciting to have this land!

Could documentation on MDN be added here for the to-dos as well?
(Reporter)

Comment 56

5 months ago
(In reply to Brett Zamir from comment #55)
> Could documentation on MDN be added here for the to-dos as well?
Flags: needinfo?(rob)
(Reporter)

Comment 57

5 months ago
I have run this code but it returns null:

browser.menus.onClicked.addListener((info, tab) => {
  if (info.menuItemId == "context-test") {
    let elem = browser.menus.getTargetElement(info.targetElementId);
    console.log(elem);
  }
});

browser.menus.create({
  id: "context-test",
  title: "Test",
  contexts: ["link"]
});
(Assignee)

Comment 58

5 months ago
The summary of the API changes are as follows.

= browser.menus.onShown event includes the info.targetElementId integer (if the extension has access to the tab/view where the menu was opened).
- browser.menus.onClicked (and the "onclick" event in menus) include the info.targetElementId integer if the user clicks on the extension menu item.
- browser.menus.getTargetElement is a new method, available to all extension script contexts (content scripts, background pages and other extension pages) that returns the element for a given info.targetElementId, provided that the element still exists in the document where the method is invoked. A strong advantage of this new method is that it allows extensions to detect the clicked element without having to insert a content script in every page.
- targetElementId expires when another contextmenu is opened.
- These APIs all require the "menus" permission.

Here is a simple example to remove an element:

browser.menus.create({
  title: "Remove element",
  documentUrlPatterns: ["*://*/*"],
  contexts: ["audio", "editable", "frame", "image", "link", "page", "password", "video"],
  onclick(info, tab) {
    browser.tabs.executeScript(tab.id, {
      frameId: info.frameId,
      code: `browser.menus.getTargetElement(${info.targetElementId}).remove();`,
    });
  },
});

I have posted a more advanced example at https://github.com/mdn/webextensions-examples/pull/369

For documentation writers, here are more examples:
- https://searchfox.org/mozilla-central/source/browser/components/extensions/test/browser/browser_ext_menus_targetElement.js
- https://searchfox.org/mozilla-central/source/browser/components/extensions/test/browser/browser_ext_menus_targetElement_extension.js
- https://searchfox.org/mozilla-central/source/browser/components/extensions/test/browser/browser_ext_menus_targetElement_shadow.js

MDN needs to be updated to refer to this example and to mention the above API changes at
https://developer.mozilla.org/en-US/docs/Mozilla/Add-ons/WebExtensions/API/menus/OnClickData
https://developer.mozilla.org/en-US/docs/Mozilla/Add-ons/WebExtensions/API/menus/getTargetElement (new article)
OnClickData and the new getTargetElement should be updated at: https://github.com/mdn/browser-compat-data/blob/6544e487c1720b3e8916079243504b16a9c09f9a/webextensions/api/menus.json

It is probably worth mentioning this method at the general overview of the menus API, since this allows extensions to customize the menu for contextual information that is not (yet) available as another property in the menus.onShown/onClicked/onclick events.

(In reply to kernp25 from comment #57)
> I have run this code but it returns null:
> 
> browser.menus.onClicked.addListener((info, tab) => {
>   if (info.menuItemId == "context-test") {
>     let elem = browser.menus.getTargetElement(info.targetElementId);
>     console.log(elem);
>   }
> });
> 
> browser.menus.create({
>   id: "context-test",
>   title: "Test",
>   contexts: ["link"]
> });

That code will only work if the menu is opened in the current document (see the browser_ext_menus_targetElement_extension.js for examples).
You cannot get access to a DOM element from a different tab or frame; use a content script for that (see the Github example that I linked before, and the other two test files).
Flags: needinfo?(rob)
Keywords: dev-doc-needed
(Reporter)

Comment 59

5 months ago
browser.tabs.executeScript(tab.id, {
  frameId: info.frameId,
  code: `console.log(browser.menus.getTargetElement(${info.targetElementId}));`,
});

Yes, this works :)

Comment 60

5 months ago
Can you please add some steps to reproduce this bug or attach a test webextension in order to verify this issue?
Flags: needinfo?(rob)
(Assignee)

Comment 61

5 months ago
str
See comment 58, in particular the full example and documentation in:
https://github.com/mdn/webextensions-examples/pull/369

1. Load extension from https://github.com/mdn/webextensions-examples/pull/369
2. Visit example.com
3. Right-click on "Example Domain" in the page and click on the "Remove element" menu item.

Expected:
- Element should be removed.

Actual:
- Element is not removed.

Note: If you want to repeat step 3 again, refresh the page first, because the example has a built-in fallback in unsupported Firefox versions, that results in the expected behavior when step 3 is run twice.
Flags: needinfo?(rob)

Comment 62

5 months ago
Thanks for the steps.

I verified the issue in latest FF63 on Windows10x64\macOS 10.13.2 and I was able to remove the elements(I also tested that in earlier versions I was not able to remove them).

I will attach a postfix video.
Status: RESOLVED → VERIFIED

Updated

5 months ago
status-firefox63: fixed → verified

Comment 63

5 months ago
Created attachment 9004208 [details]
Postfix video

Comment 64

4 months ago
Created the page for the new method: https://developer.mozilla.org/en-US/docs/Mozilla/Add-ons/WebExtensions/API/menus/getTargetElement

This page: https://developer.mozilla.org/en-US/docs/Mozilla/Add-ons/WebExtensions/API/menus/OnClickData already has a reference to using the getTargetElement method to retrieve the element associated with the targetElementId

Added a reference to the new method on this page: https://developer.mozilla.org/en-US/docs/Mozilla/Add-ons/WebExtensions/API/menus

Added a mention of the method to https://developer.mozilla.org/en-US/docs/Mozilla/Firefox/Releases/63 under changes for add-on developers.

Still need to add a discussion of the advantages of using this method and links to the other examples listed in the comments above. I used the simplest example directly on the page and know that it needs more, but I wanted to make sure that I haven't said anything wrong in the information I have already inserted.

Also, it would be helpful to know if any exceptions are raised in cases where the id is no longer valid. Does it fail silently and return null or undefined or does it raise an error?
Flags: needinfo?(rob)
(Assignee)

Comment 65

4 months ago
(In reply to Irene Smith from comment #64)
> Created the page for the new method:

Thanks! Here is some feedback:

> https://developer.mozilla.org/en-US/docs/Mozilla/Add-ons/WebExtensions/API/menus/getTargetElement

- "passed to the menus.onClick handler" -> passed to the (link)menus.onClicked or (link)menus.onShown event.
- Use code formatting when referring to method names or properties (i.e. info.targetElementId).
- Emphasize that the targetElementId identifies an element in a different document, and that menus.getTargetElement will only return that element if called in the same context of the document, for example using content scripts (as shown in the example). Calling the method in the wrong context will be a very common mistake, and in comment 57 you can already see a user who made that mistake.
- Please include all information from comment 58.

> https://developer.mozilla.org/en-US/docs/Mozilla/Add-ons/WebExtensions/API/menus

- Do not add "New in Firefox 63", version availability belongs to the compat tables.
- Use code formatting when referring to method names or properties (i.e. info.targetElementId - similar as above).
 
> https://developer.mozilla.org/en-US/docs/Mozilla/Firefox/Releases/63

- When a developer reads the summary, they will not know what the method does. The first sentence looks brief and great, but can you expand the second sentence to explain, e.g. "[... targetElementId parameter] that identifies the clicked element."

> Also, it would be helpful to know if any exceptions are raised in cases
> where the id is no longer valid. Does it fail silently and return null or
> undefined or does it raise an error?

When the *integer* ID is invalid, null is returned.

(If anything other than an integer is set, then an error is thrown. But that is normal extension API behavior and probably not needing an explicit explanation )
Flags: needinfo?(rob)

Comment 66

4 months ago
Thank you for the feedback. I will work on the page later today.
You need to log in before you can comment on or make changes to this bug.