Closed Bug 1461115 Opened 7 years ago Closed 7 years ago

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

Categories

(WebExtensions :: Frontend, defect)

60 Branch
defect
Not set
normal

Tracking

(firefox62 verified)

VERIFIED FIXED
mozilla62
Tracking Status
firefox62 --- verified

People

(Reporter: 5i13ghzt462u, Assigned: robwu)

Details

Attachments

(2 files)

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.
Component: Untriaged → WebExtensions: Untriaged
Product: Firefox → Toolkit
Flags: needinfo?(rob)
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
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.
Component: WebExtensions: Untriaged → WebExtensions: Frontend
Attachment #8976941 - Flags: review?(mixedpuppy) → review+
Pushed by rob@robwu.nl: https://hg.mozilla.org/integration/autoland/rev/010fd502cf60 Emit menus.onShown even if there is no tab r=mixedpuppy
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla62
Attached image 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.
Status: RESOLVED → 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?
Product: Toolkit → WebExtensions

@pikadudeno1+bugzilla@gmail.com To bump this up again, I think this question is very valid. Unit tests are likely useful. :)

You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: