Closed Bug 1250631 Opened 8 years ago Closed 8 years ago

contextMenus "onClicked" event does not work

Categories

(WebExtensions :: Untriaged, defect, P2)

defect

Tracking

(Not tracked)

RESOLVED FIXED
mozilla48
Iteration:
48.2 - Apr 4

People

(Reporter: wbamberg, Assigned: mattw)

Details

(Whiteboard: [contextMenus] triaged)

Attachments

(3 files)

Attached file context-menu.xpi
I've attached a minimal WebExtension that just adds a context menu item labeled "Click me!". It uses contextMenus.onClicked[0] to listen for the click event, and logs a message to the console when it hears the event.

On Firefox (47.0a1) I don't see this message in the Browser Console.

The same add-on works as expected in Chrome.

I've attached another WebExtension which is the same, but which uses the "onclick" property in the argument to contextMenus.create()[1]. This one works fine.

[0] https://developer.mozilla.org/en-US/Add-ons/WebExtensions/API/ContextMenus/onClicked
[1] [https://developer.mozilla.org/en-US/Add-ons/WebExtensions/API/contextMenus/create#Parameters]
Attached file context-menu-2.xpi
should be working with other context menu
Flags: blocking-webextensions?
Whiteboard: [contextMenu] triaged
(In reply to :shell escalante from comment #2)
> should be working with other context menu

Hm?
Matt, can you take a look at this?

The comment for onClicked suggests that it hasn't been implemented, because it's waiting for event pages. But I don't think that should be necessary. They should work with other contexts too.

I'm not sure how the "onclick" properties are supposed to be handled when they're registered from a window that's destroyed... so we should probably look into that too, at some point. But that's a different bug.
Assignee: nobody → mwein
Flags: blocking-webextensions? → blocking-webextensions+
Priority: -- → P2
Iteration: --- → 48.2 - Apr 4
Comment on attachment 8733796 [details]
MozReview Request: Bug 1250631 Implement chrome.contextMenus.onClicked r?kmag

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/41949/diff/1-2/
Attachment #8733796 - Attachment description: MozReview Request: Bug 1250631 Implement chrome.contextMenus.onClicked r?rpl → MozReview Request: Bug 1250631 Implement chrome.contextMenus.onClicked f?rpl r?kmag
Attachment #8733796 - Flags: review?(lgreco) → review?(kmaglione+bmo)
https://reviewboard.mozilla.org/r/41949/#review38431

::: browser/components/extensions/ext-contextMenus.js:184
(Diff revision 2)
>          item.checked = true;
>        }
>  
>        item.tabManager.addActiveTabPermission();
>        if (item.onclick) {
>          let clickData = item.getClickData(contextData, event);

I noticed that the chrome API documentation specifies a slightly different signature for the two onclick handlers - they pass the tab object as a second parameter to the callbacks rather than bundling it with the click info. What do you think we should do about this?
https://reviewboard.mozilla.org/r/41947/#review38461

Hi Matthew,
Follows a couple of lines from the patch that I have some question about:

::: browser/components/extensions/ext-contextMenus.js:185
(Diff revision 2)
>        }
>  
>        item.tabManager.addActiveTabPermission();
>        if (item.onclick) {
>          let clickData = item.getClickData(contextData, event);
> +        item.extension.emit("webext-contextmenu-menuitem-click", clickData);

This event seems to be the one that is subscribed by the onClicked listeners, shouldn't we emit this event even if there is no `onclick` callback for the context menu item?

::: browser/components/extensions/ext-contextMenus.js:268
(Diff revision 2)
>    // If the item is not the root and has no parent
>    // it must be a child of the root.
>    if (!isRoot && !this.parent) {
>      this.root.addChild(this);
>    }
> +  EventEmitter.decorate(this);

The `"webext-contentmenu-menuitem-click"` event is currently emitted on the `extension` object, and so this seems to be unused.

If we don't need to decorate the `MenuItem` objets with an `EventEmitter`, we can also remove its import.
https://reviewboard.mozilla.org/r/41949/#review38431

> I noticed that the chrome API documentation specifies a slightly different signature for the two onclick handlers - they pass the tab object as a second parameter to the callbacks rather than bundling it with the click info. What do you think we should do about this?

I'm looking into the chrome API docs ([onclick](https://developer.chrome.com/extensions/contextMenus#method-create) and [onClicked](https://developer.chrome.com/extensions/contextMenus#event-onClicked)) and in both the cases it seems to pass the tab as the second parameter instead of info.tab, and our json schema seems to be consistent with the chrome API docs. 

am I missing something?

it is very likely that we have to change our implementation to use the second parameter as well, it is a chrome incompatibilities that we can easily prevent.

I'm not sure if we really need to keep a compatible behavior with the current implementation, but if we want to do so, we can put the tab details into both the info.tab attribute and the tab parameter.
Attachment #8733796 - Flags: review?(kmaglione+bmo)
Comment on attachment 8733796 [details]
MozReview Request: Bug 1250631 Implement chrome.contextMenus.onClicked r?kmag

https://reviewboard.mozilla.org/r/41949/#review38643

I think my comments are pretty much the same as Luca's.

::: browser/components/extensions/ext-contextMenus.js:185
(Diff revision 2)
>        }
>  
>        item.tabManager.addActiveTabPermission();
>        if (item.onclick) {
>          let clickData = item.getClickData(contextData, event);
> +        item.extension.emit("webext-contextmenu-menuitem-click", clickData);

We should emit this whether or not there's an `onclick` listener for the item.

::: browser/components/extensions/ext-contextMenus.js:268
(Diff revision 2)
>    // If the item is not the root and has no parent
>    // it must be a child of the root.
>    if (!isRoot && !this.parent) {
>      this.root.addChild(this);
>    }
> +  EventEmitter.decorate(this);

This is unused.

::: browser/components/extensions/test/browser/browser_ext_contextMenus.js:25
(Diff revision 2)
>        function genericOnClick(info) {
> -        browser.test.sendMessage("menuItemClick", info);
> +        browser.test.sendMessage("onclick", info);
>        }
>  
> +      browser.contextMenus.onClicked.addListener(info => {
> +        browser.test.sendMessage("browser.contextMenus.onClicked", info);

Please test that this is also sent for an item with no `onclick` property.
(In reply to Luca Greco [:rpl] from comment #10)
> https://reviewboard.mozilla.org/r/41949/#review38431
> 
> > I noticed that the chrome API documentation specifies a slightly different signature for the two onclick handlers - they pass the tab object as a second parameter to the callbacks rather than bundling it with the click info. What do you think we should do about this?
> 
> I'm looking into the chrome API docs
> ([onclick](https://developer.chrome.com/extensions/contextMenus#method-
> create) and
> [onClicked](https://developer.chrome.com/extensions/contextMenus#event-
> onClicked)) and in both the cases it seems to pass the tab as the second
> parameter instead of info.tab, and our json schema seems to be consistent
> with the chrome API docs. 
> 
> am I missing something?
> 
> it is very likely that we have to change our implementation to use the
> second parameter as well, it is a chrome incompatibilities that we can
> easily prevent.
> 
> I'm not sure if we really need to keep a compatible behavior with the
> current implementation, but if we want to do so, we can put the tab details
> into both the info.tab attribute and the tab parameter.

We should definitely stay compatible here. If we were OK with breaking compatibility with event listeners, I'd change the signature of just about all of the tab events before I changed this one.
Comment on attachment 8733796 [details]
MozReview Request: Bug 1250631 Implement chrome.contextMenus.onClicked r?kmag

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/41949/diff/2-3/
Attachment #8733796 - Flags: review?(kmaglione+bmo)
https://reviewboard.mozilla.org/r/41949/#review38431

> I'm looking into the chrome API docs ([onclick](https://developer.chrome.com/extensions/contextMenus#method-create) and [onClicked](https://developer.chrome.com/extensions/contextMenus#event-onClicked)) and in both the cases it seems to pass the tab as the second parameter instead of info.tab, and our json schema seems to be consistent with the chrome API docs. 
> 
> am I missing something?
> 
> it is very likely that we have to change our implementation to use the second parameter as well, it is a chrome incompatibilities that we can easily prevent.
> 
> I'm not sure if we really need to keep a compatible behavior with the current implementation, but if we want to do so, we can put the tab details into both the info.tab attribute and the tab parameter.

Yeah, I don't think it makes sense to have an incompatible implementation here. I modified the API to fix this.
https://reviewboard.mozilla.org/r/41949/#review38643

> We should emit this whether or not there's an `onclick` listener for the item.

Oops, thanks, I fixed this in my next revision.

> This is unused.

Thanks, removed.

> Please test that this is also sent for an item with no `onclick` property.

Yeah that makes sense, thanks, I added a test for this.
Comment on attachment 8733796 [details]
MozReview Request: Bug 1250631 Implement chrome.contextMenus.onClicked r?kmag

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

::: browser/components/extensions/test/browser/browser_ext_contextMenus.js
(Diff revision 4)
>  
>        browser.contextMenus.create({
>          title: "radio-group-1",
>          type: "radio",
>          checked: true,
> -        contexts: ["page"],

I removed these because "page" is the default context when it isn't provided.

::: browser/components/extensions/test/browser/browser_ext_contextMenus.js:174
(Diff revision 4)
> -    function checkClickInfo(info) {
> +    function checkClickInfo(info, tab) {
>        for (let i of Object.keys(expectedClickInfo)) {
>          is(info[i], expectedClickInfo[i],
>             "click info " + i + " expected to be: " + expectedClickInfo[i] + " but was: " + info[i]);
>        }
> -      is(expectedClickInfo.pageSrc, info.tab.url);
> +      if (tab) {

For some reason, tab is "undefined" only when the menuitme that has its title modified to show the selection text is clicked (it has the ID "ext-selection"). I'm not sure why that happens in this case. Any ideas?
https://reviewboard.mozilla.org/r/41949/#review38687

> For some reason, tab is "undefined" only when the menuitme that has its title modified to show the selection text is clicked (it has the ID "ext-selection"). I'm not sure why that happens in this case. Any ideas?

Because you're not passing the `tab` argument to `genericOnClick` from that handler.
Comment on attachment 8733796 [details]
MozReview Request: Bug 1250631 Implement chrome.contextMenus.onClicked r?kmag

https://reviewboard.mozilla.org/r/41949/#review38697

Looks good aside from some nits. Thanks!

::: browser/components/extensions/ext-contextMenus.js:169
(Diff revision 4)
>        element.setAttribute("disabled", "true");
>      }
>  
>      element.addEventListener("command", event => {  // eslint-disable-line mozilla/balanced-listeners
> +      // Prevent the event from propagating up to the parent element.
> +      event.stopPropagation();

Why?

::: browser/components/extensions/ext-contextMenus.js:186
(Diff revision 4)
>          item.checked = true;
>        }
>  
>        item.tabManager.addActiveTabPermission();
> +
> +      let tab = TabManager.convert(item.extension, contextData.tab);

`let tab = item.tabManager.convert(contextData.tab)`

::: browser/components/extensions/test/browser/browser_ext_contextMenus.js:174
(Diff revision 4)
> -    function checkClickInfo(info) {
> +    function checkClickInfo(info, tab) {
>        for (let i of Object.keys(expectedClickInfo)) {
>          is(info[i], expectedClickInfo[i],
>             "click info " + i + " expected to be: " + expectedClickInfo[i] + " but was: " + info[i]);
>        }
> -      is(expectedClickInfo.pageSrc, info.tab.url);
> +      if (tab) {

We should always get a tab in these tests, so please remove the `if`.
Attachment #8733796 - Flags: review?(kmaglione+bmo) → review+
Comment on attachment 8733796 [details]
MozReview Request: Bug 1250631 Implement chrome.contextMenus.onClicked r?kmag

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/41949/diff/4-5/
Attachment #8733796 - Attachment description: MozReview Request: Bug 1250631 Implement chrome.contextMenus.onClicked f?rpl r?kmag → MozReview Request: Bug 1250631 Implement chrome.contextMenus.onClicked r=kmag
https://reviewboard.mozilla.org/r/41949/#review38687

> Because you're not passing the `tab` argument to `genericOnClick` from that handler.

Ah okay, thanks!
https://reviewboard.mozilla.org/r/41949/#review38697

> Why?

Without this, the "command" event was firing twice, once for the menuitem and again for the parent menuitem of the extension.  Because of this, the onCommand listener was getting called twice each time a menuitem was clicked, and the clickInfo wasn't correct when it was called on the parent menuitem.  Should I add a comment about this or maybe fix it differently?
https://reviewboard.mozilla.org/r/41949/#review38697

> Without this, the "command" event was firing twice, once for the menuitem and again for the parent menuitem of the extension.  Because of this, the onCommand listener was getting called twice each time a menuitem was clicked, and the clickInfo wasn't correct when it was called on the parent menuitem.  Should I add a comment about this or maybe fix it differently?

If that's unexpected, then you need to check the target of the event in your other "command" listeners, rather than having them depend on other code to stop the propagation of events from their descendents.
https://reviewboard.mozilla.org/r/41949/#review38697

> If that's unexpected, then you need to check the target of the event in your other "command" listeners, rather than having them depend on other code to stop the propagation of events from their descendents.

Okay that makes sense.  Since browser.contextMenus.onClicked doesn't receive an event, I wasn't able to check the target..  I was able to use `info.parentMenuItemId` to get the behavior I wanted though, since this parameter is undefined when the event fires for the parent menu item.  Does this sound okay?
Comment on attachment 8733796 [details]
MozReview Request: Bug 1250631 Implement chrome.contextMenus.onClicked r?kmag

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

> Okay that makes sense.  Since browser.contextMenus.onClicked doesn't receive an event, I wasn't able to check the target..  I was able to use `info.parentMenuItemId` to get the behavior I wanted though, since this parameter is undefined when the event fires for the parent menu item.  Does this sound okay?

No, I think the check should probably be done in the "command" event listener. Something like `event.target === event.currentTarget`. Unless `onClicked` listeners fire in Chrome when a sub-menu item was clicked (which I haven't tested).
Comment on attachment 8733796 [details]
MozReview Request: Bug 1250631 Implement chrome.contextMenus.onClicked r?kmag

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/41949/diff/6-7/
Attachment #8733796 - Attachment description: MozReview Request: Bug 1250631 Implement chrome.contextMenus.onClicked r=kmag → MozReview Request: Bug 1250631 Implement chrome.contextMenus.onClicked r?kmag
Comment on attachment 8733796 [details]
MozReview Request: Bug 1250631 Implement chrome.contextMenus.onClicked r?kmag

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/41949/diff/7-8/
https://reviewboard.mozilla.org/r/41949/#review38697

> No, I think the check should probably be done in the "command" event listener. Something like `event.target === event.currentTarget`. Unless `onClicked` listeners fire in Chrome when a sub-menu item was clicked (which I haven't tested).

Sounds good. I tested this out in Chrome and the onClicked listeners fired for all child-less menu-items that were constructed by the extension.
Comment on attachment 8733796 [details]
MozReview Request: Bug 1250631 Implement chrome.contextMenus.onClicked r?kmag

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/41949/diff/8-9/
Attachment #8733796 - Attachment description: MozReview Request: Bug 1250631 Implement chrome.contextMenus.onClicked r?kmag → MozReview Request: Bug 1250631 Implement chrome.contextMenus.onClicked r?kmag try: -b do -p linux64,macosx64,win32,win64 -u mochitest,xpcshell -n
Comment on attachment 8733796 [details]
MozReview Request: Bug 1250631 Implement chrome.contextMenus.onClicked r?kmag

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/41949/diff/9-10/
Attachment #8733796 - Attachment description: MozReview Request: Bug 1250631 Implement chrome.contextMenus.onClicked r?kmag try: -b do -p linux64,macosx64,win32,win64 -u mochitest,xpcshell -n → MozReview Request: Bug 1250631 Implement chrome.contextMenus.onClicked r?kmag
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/6934dc0c95b8
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla48
Whiteboard: [contextMenu] triaged → [contextMenus] triaged
Product: Toolkit → WebExtensions
I am also getting error message "browser.contextMenus.onClicked.addListener(...) is undefined". What causes this? Here is the relevant part of my background code:

	// Remove any existing context menu items on extension reload to prevent error
	browser.contextMenus.removeAll().then(p2).then(p3).catch(err);

function p2()
	{
	// Create context menu item
	browser.contextMenus.create(
		{
		id: "ServInt",
		title: "ServInt Reports"
		}, function()
		{
		if (browser.runtime.lastError)
			err(browser.runtime.lastError);
		}); // Sync
	} // p2

// Create context menu command to run PM_fg.js

function p3()
	{
	browser.contextMenus.onClicked.addListener(function(info, tab)
		{
		if (info.menuItemId == "ServInt")
			{
			var p=browser.tabs.executeScript({file:"/PM_fg.js"}).then(fgReady).catch(err);
			}
		}).then(empty).catch(err);
	} // p3
(In reply to David Spector from comment #36)
> function p3()
> 	{
> 	browser.contextMenus.onClicked.addListener(function(info, tab)
> 		{
> 		if (info.menuItemId == "ServInt")
> 			{
> 			var
> p=browser.tabs.executeScript({file:"/PM_fg.js"}).then(fgReady).catch(err);
> 			}
> 		}).then(empty).catch(err);
> 	} // p3

This code is incorrect.  You're treating addListener() as if it returns a Promise (ie, you're calling .then() on its return value).  But addListener() does not return a Promise.

For further questions like this about getting help with debugging your extension, please use forums like discourse, comments on resolved bugs are likely to be missed/ignored.
It is hard to know which functions are Promises because there is no naming convention.

Thank you for referring me to Discourse, which seems to be a priced product for development teams.

Can you give me any more detailed information about where I can find debugging help? I hate to misuse this bug reporting system to learn the very obscure Webextensions environment, but I must if I have no other options. Please point to a specific forum, and my deepest thanks.
You need to log in before you can comment on or make changes to this bug.