kernp25 Reporter
	Description • 7 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

	kernp25 Reporter
	Comment 1 • 7 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.

	kernp25 Reporter
	Comment 2 • 7 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)

	Caitlin Neiman [:caitmuenster]
	Comment 3 • 7 years ago

This has been added to the March 7 WebExtensions Triage mtg. agenda. 

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

	Luca Greco [:rpl] [:luca] [:lgreco]
	Comment 4 • 7 years ago

We discussed about this during the last WebExtensions Triage meeting and we decided that we can approve this.

	Luca Greco [:rpl] [:luca] [:lgreco]
	Comment 5 • 7 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.

	Shane Caraveo (:mixedpuppy)
	Comment 8 • 7 years ago

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.

	Shane Caraveo (:mixedpuppy)
	Comment 9 • 7 years ago

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?

	Patrick Brosset <:pbro>
	Comment 10 • 7 years ago

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

	kernp25 Reporter
	Comment 11 • 7 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

	Julian Descottes [:jdescottes]
	Comment 12 • 7 years ago

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.

	Kris Maglione [:kmag]
	Comment 13 • 7 years ago

(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.

	Julian Descottes [:jdescottes]
	Comment 14 • 7 years ago

(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.

	Alexandre Poirot [:ochameau]
	Comment 15 • 7 years ago

(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.

	Rob Wu [:robwu] Assignee
	Comment 16 • 7 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.

	kernp25 Reporter
	Comment 17 • 7 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.

	Tetsuharu OHZEKI [:tetsuharu] (UTC+9)
	Comment 18 • 7 years ago

(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.

	kernp25 Reporter
	Comment 19 • 7 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?

	Tetsuharu OHZEKI [:tetsuharu] (UTC+9)
	Comment 20 • 7 years ago

(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.

	Shane Caraveo (:mixedpuppy)
	Comment 24 • 7 years ago

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.

	Shane Caraveo (:mixedpuppy)
	Comment 25 • 6 years ago

seems this bug got dropped a while back, are you still on it?

	kernp25 Reporter
	Comment 26 • 6 years ago

I have asked wisniewskit@gmail.com if he can take this bug and finish it.

	Thomas Wisniewski
	Comment 27 • 6 years ago

I'll see if I can find some spare time to finish this, but I can't guarantee it will be soon.

	Shane Caraveo (:mixedpuppy)
	Comment 35 • 6 years ago
mozreview-review

Comment on attachment 8997825 [details]
Bug 1325814 - Clarify file overview comment in ExtensionChild.jsm

https://reviewboard.mozilla.org/r/261534/#review268532

	Shane Caraveo (:mixedpuppy)
	Comment 36 • 6 years 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.

	Shane Caraveo (:mixedpuppy)
	Comment 37 • 6 years 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.

	Shane Caraveo (:mixedpuppy)
	Comment 38 • 6 years ago

I pushed that review comment 37 early to point out the sensitive data part, forgot about my notes which probably would have changed.

	Shane Caraveo (:mixedpuppy)
	Comment 43 • 6 years ago
mozreview-review

Comment on attachment 8997859 [details]
Bug 1325814 - Support shadow DOM in menus.getTargetElement + test

https://reviewboard.mozilla.org/r/261568/#review268974

	Shane Caraveo (:mixedpuppy)
	Comment 44 • 6 years 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.

	Shane Caraveo (:mixedpuppy)
	Comment 45 • 6 years 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.

	Shane Caraveo (:mixedpuppy)
	Comment 46 • 6 years 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"

	Shane Caraveo (:mixedpuppy)
	Comment 52 • 6 years ago
mozreview-review

Comment on attachment 8997827 [details]
Bug 1325814 - Add extension API to find menu target

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

	Pulsebot
	Comment 53 • 6 years 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

	Andreea Pavel [:apavel]
	Comment 54 • 6 years ago
bugherder

https://hg.mozilla.org/mozilla-central/rev/51d530820c10
https://hg.mozilla.org/mozilla-central/rev/f47fc4512dfb
https://hg.mozilla.org/mozilla-central/rev/e08c04702ab4
https://hg.mozilla.org/mozilla-central/rev/a7c2208c1f85
https://hg.mozilla.org/mozilla-central/rev/68dbca6faa6e

	Brett Zamir
	Comment 55 • 6 years ago

Very exciting to have this land!

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

	kernp25 Reporter
	Comment 56 • 6 years ago

(In reply to Brett Zamir from comment #55)
> Could documentation on MDN be added here for the to-dos as well?

	kernp25 Reporter
	Comment 57 • 6 years 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"]
});

	Rob Wu [:robwu] Assignee
	Comment 58 • 6 years 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).

	kernp25 Reporter
	Comment 59 • 6 years ago

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

Yes, this works :)

	Victor Carciu
	Comment 60 • 6 years ago

Can you please add some steps to reproduce this bug or attach a test webextension in order to verify this issue?

	Rob Wu [:robwu] Assignee
	Comment 61 • 6 years 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.

	Victor Carciu
	Comment 62 • 6 years 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.

	Irene Smith
	Comment 64 • 6 years 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?

	Rob Wu [:robwu] Assignee
	Comment 65 • 6 years 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 )

	Irene Smith
	Comment 66 • 6 years ago

Thank you for the feedback. I will work on the page later today.