browser.menus.onShown fails to trigger in browser_action popup of WebExtension

VERIFIED FIXED in Firefox 62

Status

VERIFIED FIXED
7 months ago
6 months ago

People

(Reporter: c4609174, Assigned: robwu)

Tracking

60 Branch
mozilla62

Firefox Tracking Flags

(firefox62 verified)

Details

Attachments

(2 attachments)

(Reporter)

Description

7 months ago
User Agent: Mozilla/5.0 (X11; Fedora; Linux x86_64; rv:60.0) Gecko/20100101 Firefox/60.0
Build ID: 20180507144316

Steps to reproduce:

Created a web extension and this in the background script:

browser.menus.onShown.addListener(menuShown);

where menuShown is

function menuShown(info, tab) {
        console.log(JSON.parse(JSON.stringify(tab)));
        if (!tab.url.startsWith(browser.extension.getURL())) {
            return;
        }

        browser.menus.onHidden.addListener(createItems);

        browser.menus.remove(CONVERT_TEXT_SELECTION); // contants contain IDs, should just hide them
        browser.menus.remove(CONVERT_LINK_TEXT_SELECTION);

        browser.menus.refresh();
    }


Actual results:

On websites this actually works, but not when I open the popup of my extension (browser action), and show the context menu there.

I get this, instead:

WeakMap key must be an object, got undefined  ext-browser.js:274
	setId chrome://browser/content/ext-browser.js:274:5
	getId chrome://browser/content/ext-browser.js:254:5
	wrapTab chrome://browser/content/ext-browser.js:942:47
	TabManagerBase/this._tabs< chrome://extensions/content/ext-tabs-base.js:1694:44
	get resource://gre/modules/ExtensionUtils.jsm:92:15
	getWrapper chrome://extensions/content/ext-tabs-base.js:1765:12
	hasActiveTabPermission chrome://extensions/content/ext-tabs-base.js:1736:12
	listener chrome://browser/content/ext-menus.js:831:13
	emit resource://gre/modules/ExtensionUtils.jsm:227:40
	emit resource://gre/modules/Extension.jsm:1293:37
	dispatchOnShownEvent chrome://browser/content/ext-menus.js:397:7
	forEach self-hosted:5844:9
	afterBuildingMenu chrome://browser/content/ext-menus.js:403:7
	build chrome://browser/content/ext-menus.js:62:5
	observe chrome://browser/content/ext-menus.js:750:5
	CM_initMenu chrome://browser/content/nsContextMenu.js:132:7
	nsContextMenu chrome://browser/content/nsContextMenu.js:85:3
	onpopupshowing chrome://browser/content/browser.xul:1:119


Expected results:

With that code, I wanted to hide the add-on's own menu entries in the add-on's own popup, as this is quite senseless for my add-on at least.

Updated

7 months ago
Component: Untriaged → WebExtensions: Untriaged
Product: Firefox → Toolkit
Flags: needinfo?(rob)
(Assignee)

Comment 1

7 months ago
Reproduced. The issue is that there is no "tab" associated with the context menu in the popup (not even in the onClicked event).
The MDN documentation of onShown (and onClicked) both allow for the possibility of the menu being empty, so the onShown dispatcher in ext-menus.js needs to check whether the tab is set before trying to convert it.


Steps to reproduce:
1. Load the extension (described below) in Chrome or Firefox.
2. Click on the browser_action extension button to show the popup.
3. Right-click in the popup and click on the menu item "Some menu item 1".
4. Look at the console of the background page:

Result:
- In Chrome (66), the tab is printed.
- In Firefox (60), the tab parameter is undefined.


> background.js

chrome.contextMenus.create({
    id: "item1",
    title: "Some menu item 1",
    onclick(info, tab) {
        console.log("menus.onClicked", info, tab);
    },
});

browser.menus.onShown.addListener(function(info, tab) {
    console.log("menus.onShown", info, tab);
});


> manifest.json

{
    "name": "browserAction popup menu",
    "version": "1",
    "manifest_version": 2,
    "background": {
        "scripts": ["background.js"]
    },
    "browser_action": {
        "browser_style": true,
        "default_popup": "popup.html"
    },
    "permissions": [
        "activeTab",
        "contextMenus",
        "menus"
    ]
}


> popup.html

This is a popup,<br>
Right-click me.
Assignee: nobody → rob
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
Flags: needinfo?(rob)
Summary: browser.menus.onShown fails to trigger in browser_action of WebExtension → browser.menus.onShown fails to trigger in browser_action popup of WebExtension
Comment hidden (mozreview-request)
(Assignee)

Comment 3

7 months ago
I fixed the bug by checking for emptiness of the tab.

As I showed in comment 1, there is a difference in how Chrome and Firefox set the tab property (in onClicked, and I extend the logic to onShown too):
- In Chrome, the tab is set even if the menu is opened in a browserAction popup.
- In Firefox, the tab is not set.

I decided to continue with Firefox's logic of omitting the tab, since a menu *inside* an extension popup is clearly not associated with the tab. Extensions can always use tabs.query if they want to determine the current tab.
(Assignee)

Updated

7 months ago
Component: WebExtensions: Untriaged → WebExtensions: Frontend

Comment 4

7 months ago
mozreview-review
Comment on attachment 8976941 [details]
Bug 1461115 - Emit menus.onShown even if there is no tab

https://reviewboard.mozilla.org/r/245088/#review251032
Attachment #8976941 - Flags: review?(mixedpuppy) → review+

Comment 5

7 months ago
Pushed by rob@robwu.nl:
https://hg.mozilla.org/integration/autoland/rev/010fd502cf60
Emit menus.onShown even if there is no tab r=mixedpuppy

Comment 6

7 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/010fd502cf60
Status: ASSIGNED → RESOLVED
Last Resolved: 7 months ago
status-firefox62: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla62

Comment 7

7 months ago
Created attachment 8980570 [details]
Bug1461115.png

I can reproduce this issue on Firefox 60.0.1 (20180516032328) under Win 7 64-bit and Mac OS X 10.13.3.

The actual result from the description is displayed.

This issue is verified as fixed on Firefox 62.0a1 (20180525005138) under Win 7 64-bit and Mac OS X 10.13.3.

Updated

7 months ago
Status: RESOLVED → VERIFIED
status-firefox62: fixed → verified
FYI, in Firefox 60.0.1 this bug also affects menu entries created for the bookmark context, with the same error. I'll bet the same fix will work, but maybe it merits a separate test?

Updated

6 months ago
Product: Toolkit → WebExtensions
You need to log in before you can comment on or make changes to this bug.