Automatically add Manage Extension item in browserAction context menu

VERIFIED FIXED in Firefox 62

Status

enhancement
P5
normal
VERIFIED FIXED
Last year
11 months ago

People

(Reporter: Oriol, Assigned: Oriol)

Tracking

unspecified
mozilla62
Dependency tree / graph

Firefox Tracking Flags

(firefox62 verified)

Details

(Whiteboard: [design-decision-approved])

Attachments

(4 attachments)

If I right-click a pageAction, there is the "Manage Extension..." item that opens about:addons where I can configure the add-on.

But nothing like this happens for browserAction. I think that, if an add-on defines options_ui in the manifest, a link to it should automatically appear in the browserAction context menu, without needing the menus permission.

However, some addons are adding it manually. To avoid duplicity, maybe it should be automatically added only if the menus permission is not requested. Or require some manifest flag in options_ui or browser_action.
Hi Oriol, this has been added to the agenda for the WebExtensions APIs triage on May 8. Would you be able to join us? 

Here’s a quick overview of what to expect at the triage: 

* We normally spend 5 minutes per bug
* The more information in the bug, the better
* The goal of the triage is to give a general thumbs up or thumbs down on a proposal; we won't be going deep into implementation details

Relevant Links: 

* Wiki for the meeting: https://wiki.mozilla.org/WebExtensions/Triage#Next_Meeting
* Meeting agenda: https://docs.google.com/document/d/1TRkZ2u3GQXwlHpfP4-P_SXcwIwvlQz5Sm-C8I89Ya_o/edit#
* Vision doc for WebExtensions: https://wiki.mozilla.org/WebExtensions/Vision
I will join on IRC.
You may want to update https://wiki.mozilla.org/WebExtensions/Triage#Details_.26_How_to_Join, it says 18:30 UTC instead of 17:30 UTC which appears in google docs.
Will do. Thanks for the heads up. :)
This was approved in the design-decision meeting. Mconca to follow up with UX.
Flags: needinfo?(mconca)
Whiteboard: [design-decision-needed] → [design-decision-approved]
Emanuela, this feature request was approved by during our design-decision meeting. I wanted to ping UX and see if you have any concerns with adding a "Manage Extension..." menu item to the Browser Action context menu.  It would be similar (identical?) to the one Firefox adds to the context menu for Page Actions.
Severity: normal → enhancement
Flags: needinfo?(mconca) → needinfo?(emanuela)
Priority: -- → P5
Hey Mike, if I understood the feature correctly, I don't have any concerns with this. I even have a mock from that show other options I'd like to add on right click:

- learn more > goes to extension's amo detail page
- remove > removes the extension via a modal
- manage > opens about:addons 

https://mozilla.invisionapp.com/share/T7DZLW5CR#/258734408_Right-Click-Extension
Flags: needinfo?(emanuela)
I will add "Manage Extension", the other things can be added in other bugs.
Assignee: nobody → oriol-bugzilla
Status: NEW → ASSIGNED
I used "E" accesskey because "M" is already taken by "Menu Bar". Or would "A" be better?
And in the overflow menu, "M" is available, but I reused the other one for consistency.
Blocks: 1401610
Summary: Automatically add Options item in browserAction context menu → Automatically add Manage Extension item in browserAction context menu
Comment on attachment 8976224 [details]
Bug 1457474 - Add 'Manage Extension' in browserAction context menu

https://reviewboard.mozilla.org/r/244418/#review250550

Looks OK to me besides the minor notes below. I do think this probably wants sign-off from an actual webextension peer though, so gonna get mixedpuppy to check this over in case I'm missing something at 00:25am... *yawn*. :-)

::: browser/base/content/browser.js:6390
(Diff revision 1)
>  }
>  
> +function UpdateManageExtension(popup) {
> +  let checkbox = popup.querySelector(".customize-context-manageExtension");
> +  let separator = checkbox.nextElementSibling;
> +  let isWebExt = popup.triggerNode && popup.triggerNode.id.endsWith("-browser-action");

Couldn't we just check for the `data-extension` attribute (or `triggerNode.dataset.extension` or something) ? The string match on the ID of the node feels fragile...

::: browser/components/extensions/parent/ext-browserAction.js:183
(Diff revision 1)
>  
>        onCreated: node => {
>          node.classList.add("badged-button");
>          node.classList.add("webextension-browser-action");
>          node.setAttribute("constrain-size", "true");
> +        node.setAttribute("data-extension", this.tabContext.extension.id);

This is an arrow function so it's bound to the browser action object itself, right? So I think you can just use `this.extension.id` ?

::: browser/components/extensions/test/browser/browser_ext_browserAction_contextMenu.js:113
(Diff revision 1)
> +      "applications": {
> +        "gecko": {id},
> +      },

The other example add-on in this file doesn't do this, so I doubt we need to do it here.

::: browser/components/extensions/test/browser/browser_ext_browserAction_contextMenu.js:137
(Diff revision 1)
> +    is(manageExtension.hidden, !visible, `Manage Extension is ${visible ? "visible" : "hidden"}`);
> +    is(separator.hidden, !visible, `Separator after Manage Extension is ${visible ? "visible" : "hidden"}`);

Nitpicky nit: can you use "should be" instead of "is" in these messages? Some people prefer writing assertion messages with the desired state, some with the state that would cause the messages to appear (ie the failing state) - meaning you're never sure which it is unless the message itself is explicit about what it means. :-)

::: browser/components/extensions/test/browser/browser_ext_browserAction_contextMenu.js:205
(Diff revision 1)
> +  let shown = BrowserTestUtils.waitForEvent(menu, "popupshown");
> +  overflowButton.click();
> +  await shown;
> +
> +  info("Check overflow menu context menu in another button");
> +  let overflowMenu = await openContextMenu("customizationPanelItemContextMenu", otherButtonId);

Nit: this name is a bit confusing, because the result is the overflow menu's *context menu*, not the menu itself. Can you clarify the naming?

::: browser/locales/en-US/chrome/browser/browser.dtd:431
(Diff revision 1)
> +<!ENTITY customizeMenu.manageExtension.label "Manage Extension">
> +<!ENTITY customizeMenu.manageExtension.accesskey "E">

Ugh, half these strings are dead anyway ( bug 1455965 ). Anyway, from code inspection I think these accesskeys are OK.
Attachment #8976224 - Flags: review?(gijskruitbosch+bugs) → review+
Attachment #8976224 - Flags: review?(mixedpuppy)
(In reply to :Gijs (he/him) from comment #11)
> ::: browser/base/content/browser.js:6390
> (Diff revision 1)
> >  }
> >  
> > +function UpdateManageExtension(popup) {
> > +  let checkbox = popup.querySelector(".customize-context-manageExtension");
> > +  let separator = checkbox.nextElementSibling;
> > +  let isWebExt = popup.triggerNode && popup.triggerNode.id.endsWith("-browser-action");
> 
> Couldn't we just check for the `data-extension` attribute (or
> `triggerNode.dataset.extension` or something) ? The string match on the ID
> of the node feels fragile...

I tried something like this but it didn't work in the Overflow menu or in the customize mode, not sure.

> ::: browser/components/extensions/parent/ext-browserAction.js:183
> (Diff revision 1)
> >  
> >        onCreated: node => {
> >          node.classList.add("badged-button");
> >          node.classList.add("webextension-browser-action");
> >          node.setAttribute("constrain-size", "true");
> > +        node.setAttribute("data-extension", this.tabContext.extension.id);
> 
> This is an arrow function so it's bound to the browser action object itself,
> right? So I think you can just use `this.extension.id` ?

It didn't work, not sure if `this.extension.id` or `this.extension` was null at that point. Probably something else changes it. I had to use this hack to access the original good value.

> :::
> browser/components/extensions/test/browser/
> browser_ext_browserAction_contextMenu.js:113
> (Diff revision 1)
> > +      "applications": {
> > +        "gecko": {id},
> > +      },
> 
> The other example add-on in this file doesn't do this, so I doubt we need to
> do it here.

I need this to know the id. `extension.id` is null when using `useAddonManager: "temporary"`. And if I don't use it, the add-on doesn't appear in about:addons. What a big mess.
(In reply to Oriol Brufau [:Oriol] from comment #12)
> (In reply to :Gijs (he/him) from comment #11)
> > ::: browser/base/content/browser.js:6390
> > (Diff revision 1)
> > >  }
> > >  
> > > +function UpdateManageExtension(popup) {
> > > +  let checkbox = popup.querySelector(".customize-context-manageExtension");
> > > +  let separator = checkbox.nextElementSibling;
> > > +  let isWebExt = popup.triggerNode && popup.triggerNode.id.endsWith("-browser-action");
> > 
> > Couldn't we just check for the `data-extension` attribute (or
> > `triggerNode.dataset.extension` or something) ? The string match on the ID
> > of the node feels fragile...
> 
> I tried something like this but it didn't work in the Overflow menu or in
> the customize mode, not sure.

In customize mode the node will be wrapped, so I expect the triggering node is the wrapper node? In that case you can get the nested actual toolbar button / item by just getting the first child of the wrapper to get the actual add-on node.

> > ::: browser/components/extensions/parent/ext-browserAction.js:183
> > (Diff revision 1)
> > >  
> > >        onCreated: node => {
> > >          node.classList.add("badged-button");
> > >          node.classList.add("webextension-browser-action");
> > >          node.setAttribute("constrain-size", "true");
> > > +        node.setAttribute("data-extension", this.tabContext.extension.id);
> > 
> > This is an arrow function so it's bound to the browser action object itself,
> > right? So I think you can just use `this.extension.id` ?
> 
> It didn't work, not sure if `this.extension.id` or `this.extension` was null
> at that point. Probably something else changes it. I had to use this hack to
> access the original good value.

Uhhh... maybe Shane knows. But that sounds dodgy!
(In reply to :Gijs (he/him) from comment #13)
> (In reply to Oriol Brufau [:Oriol] from comment #12)
> > (In reply to :Gijs (he/him) from comment #11)
> > > ::: browser/base/content/browser.js:6390
> > > (Diff revision 1)
> > > >  }
> > > >  
> > > > +function UpdateManageExtension(popup) {
> > > > +  let checkbox = popup.querySelector(".customize-context-manageExtension");
> > > > +  let separator = checkbox.nextElementSibling;
> > > > +  let isWebExt = popup.triggerNode && popup.triggerNode.id.endsWith("-browser-action");
> > > 
> > > Couldn't we just check for the `data-extension` attribute (or
> > > `triggerNode.dataset.extension` or something) ? The string match on the ID
> > > of the node feels fragile...
> > 
> > I tried something like this but it didn't work in the Overflow menu or in
> > the customize mode, not sure.
> 
> In customize mode the node will be wrapped, so I expect the triggering node
> is the wrapper node? In that case you can get the nested actual toolbar
> button / item by just getting the first child of the wrapper to get the
> actual add-on node.

To be clear, presumably you'd need to do that anyway to get the menu to actually work and get the right add-on ID to browse to in about:addons, as you get the ID for the faux addons:// URL out of the extension id attribute. :-)
(In reply to Oriol Brufau [:Oriol] from comment #12)
> (In reply to :Gijs (he/him) from comment #11)

> > ::: browser/components/extensions/parent/ext-browserAction.js:183
> > (Diff revision 1)
> > >  
> > >        onCreated: node => {
> > >          node.classList.add("badged-button");
> > >          node.classList.add("webextension-browser-action");
> > >          node.setAttribute("constrain-size", "true");
> > > +        node.setAttribute("data-extension", this.tabContext.extension.id);
> > 
> > This is an arrow function so it's bound to the browser action object itself,
> > right? So I think you can just use `this.extension.id` ?
> 
> It didn't work, not sure if `this.extension.id` or `this.extension` was null
> at that point. Probably something else changes it. I had to use this hack to
> access the original good value.

Neither should ever be the case.  Other handlers, e.g. onBeforeCreated, are using this.extension, and id should never be null.  Can you give me an STR that reproduces that?
Flags: needinfo?(oriol-bugzilla)
(In reply to Oriol Brufau [:Oriol] from comment #12)

> I need this to know the id. `extension.id` is null when using
> `useAddonManager: "temporary"`.

(within a test) The applications id is just used to have a consistent id across tests, or to provide an id that you might want to otherwise check from the test.  useAddonManager doesn't change the extension id (but I dont recall if you have to provide one when using the flag).

If you're getting null there, then something is wrong, either in the new code, the test, or something deeper (unlikely).
(In reply to :Gijs (he/him) from comment #14)
> To be clear, presumably you'd need to do that anyway to get the menu to
> actually work and get the right add-on ID to browse to in about:addons, as
> you get the ID for the faux addons:// URL out of the extension id attribute.
> :-)

You are right, 'Manage Extension' is shown but it doesn't really work in customize mode.
I guess I should also test this.
(In reply to Shane Caraveo (:mixedpuppy) from comment #16)
> (In reply to Oriol Brufau [:Oriol] from comment #12)
> 
> > I need this to know the id. `extension.id` is null when using
> > `useAddonManager: "temporary"`.
> 
> (within a test) The applications id is just used to have a consistent id
> across tests, or to provide an id that you might want to otherwise check
> from the test.  useAddonManager doesn't change the extension id (but I dont
> recall if you have to provide one when using the flag).
> 
> If you're getting null there, then something is wrong, either in the new
> code, the test, or something deeper (unlikely).

Sorry, not null, but the id is not defined. After extension.startup(), I add

  info("id = " + extension.id + ", " + ("id" in extension));

Without useAddonManager, I get "id = addon_id@example.com, true".
But with useAddonManager:"temporary", I get "id = undefined, false".
(In reply to Shane Caraveo (:mixedpuppy) from comment #15)
> Neither should ever be the case.  Other handlers, e.g. onBeforeCreated, are
> using this.extension, and id should never be null.  Can you give me an STR
> that reproduces that?

OK, I tried to reproduce this again but couldn't. Maybe it was some incompatibility with the debugger or something.
Flags: needinfo?(oriol-bugzilla)
(In reply to Oriol Brufau [:Oriol] from comment #18)

> Without useAddonManager, I get "id = addon_id@example.com, true".
> But with useAddonManager:"temporary", I get "id = undefined, false".

I'm just getting slightly confused here.  If you're saying that the id IS defined when you are using useAddonManager and you are adding the id to the manifest, then this is fine.  If you are defining the id in the manifest but extension.id is undefined, then that is a problem.
(In reply to Shane Caraveo (:mixedpuppy) from comment #20)
> I'm just getting slightly confused here.  If you're saying that the id IS
> defined when you are using useAddonManager and you are adding the id to the
> manifest, then this is fine.  If you are defining the id in the manifest but
> extension.id is undefined, then that is a problem.

Exactly, I define the id in the manifest

      "applications": {
        "gecko": {id: "addon_id@example.com"},
      },

But if I also add useAddonManager:"temporary", then extension.id is undefined.
(In reply to Oriol Brufau [:Oriol] from comment #21)
> (In reply to Shane Caraveo (:mixedpuppy) from comment #20)
> > I'm just getting slightly confused here.  If you're saying that the id IS
> > defined when you are using useAddonManager and you are adding the id to the
> > manifest, then this is fine.  If you are defining the id in the manifest but
> > extension.id is undefined, then that is a problem.
> 
> Exactly, I define the id in the manifest
> 
>       "applications": {
>         "gecko": {id: "addon_id@example.com"},
>       },
> 
> But if I also add useAddonManager:"temporary", then extension.id is
> undefined.

hrm, are you talking about the extension var in the test, or the extension in e.g. ext-browserAction.js?  I thought we were discussing the latter.

The extension var in the test is different[1] if you're using useAddonManager than it is when you don't.

Your test is passing fine for me, so I don't see a problem here.

[1] https://searchfox.org/mozilla-central/rev/da499aac682d0bbda5829327b60a865cbc491611/toolkit/components/extensions/ExtensionTestCommon.jsm#359
(In reply to Shane Caraveo (:mixedpuppy) from comment #22)
> hrm, are you talking about the extension var in the test, or the extension
> in e.g. ext-browserAction.js?  I thought we were discussing the latter.

Ah, yes, this is in the test. The other problem was in ext-browserAction.js but I can no longer reproduce it.

> Your test is passing fine for me, so I don't see a problem here.

It's just that Gijs didn't like that I manually specified the id in the manifest.
(In reply to Oriol Brufau [:Oriol] from comment #23)
> (In reply to Shane Caraveo (:mixedpuppy) from comment #22)
> > hrm, are you talking about the extension var in the test, or the extension
> > in e.g. ext-browserAction.js?  I thought we were discussing the latter.
> 
> Ah, yes, this is in the test. The other problem was in ext-browserAction.js
> but I can no longer reproduce it.
> 
> > Your test is passing fine for me, so I don't see a problem here.
> 
> It's just that Gijs didn't like that I manually specified the id in the
> manifest.

It wasn't clear to me that we needed to specify an ID, given that the other cases in the same file don't specify an ID. I assumed the test framework/head.js code would take care of this. Anyway, if we need it we need it, so then keep it.

FWIW, I also think it's strange that this in an `applications: gecko: {}` bit - it looks like compatibility information which would specify the ID of the app, but instead it's the ID of the webextension, but I guess that ship has long sailed.
Comment on attachment 8976224 [details]
Bug 1457474 - Add 'Manage Extension' in browserAction context menu

Please check if the code to unwrap the toolbar item is good.
And in the test I had to use some ugly code to exit the customize mode, just using gCustomizeMode.exit() didn't always close the tab, and the test complained about this extra tab. Not sure if you have better ideas.
Attachment #8976224 - Flags: review+ → review?(gijskruitbosch+bugs)
Comment on attachment 8976224 [details]
Bug 1457474 - Add 'Manage Extension' in browserAction context menu

https://reviewboard.mozilla.org/r/244418/#review251234

The trigger node stuff looks good, some nits in the test - but please don't manually remove a tab, that'll just lead to intermittents. CustomizeMode should remove the tab for you (I pointed out the code that does this). You may want to wait for that to happen, but it'll happen without manually closing the tab yourself. :-)

::: browser/components/extensions/test/browser/browser_ext_browserAction_contextMenu.js:233
(Diff revisions 1 - 2)
> +  await main(true);
> +
> +  info("Exit customize mode and make sure its tab is closed");
> +  let customizeModeClosed = Promise.all([
> +    new Promise(resolve => window.addEventListener("TabClose", resolve, {once: true})),
> +    new Promise(resolve => gNavToolbox.addEventListener("aftercustomization", resolve, {once: true})),

Nit: you can use the helper:

`BrowserTestUtils.waitForEvent(gNavToolbox, "aftercustomization")`

Same for the window/TabClose listener.

You can also do so for the `customizationready` bit but I guess it doesn't really help much there as you'd need to store the promise and then call .enter() and then await the promise, so maybe the current code is more clear.

::: browser/components/extensions/test/browser/browser_ext_browserAction_contextMenu.js:236
(Diff revisions 1 - 2)
> +  gCustomizeMode.exit();
> +  gBrowser.removeTab(tab);

Customize Mode should definitely remove its own tab (see https://dxr.mozilla.org/mozilla-central/source/browser/components/customizableui/CustomizeMode.jsm#407-418 ) so please remove this. With the tabclose listener and aftercustomization listener you should be fine.
Attachment #8976224 - Flags: review?(gijskruitbosch+bugs) → review+
Posted image screenshot.png
(In reply to :Gijs (he/him) from comment #27)
> Customize Mode should definitely remove its own tab (see
> https://dxr.mozilla.org/mozilla-central/source/browser/components/
> customizableui/CustomizeMode.jsm#407-418 ) so please remove this. With the
> tabclose listener and aftercustomization listener you should be fine.

It does not when the test runs with OOP extensions. I get

   FAIL Found an unexpected tab at the end of test run: about:blank

If I await new Promise(r => setTimeout(r, 5e3)), the first time the test runs with normal extensions and it works. But the second time it's with OOP extensions and I can see the tab is effectively not closed, it just becomes blank. See attached screenshot.
Comment on attachment 8976224 [details]
Bug 1457474 - Add 'Manage Extension' in browserAction context menu

https://reviewboard.mozilla.org/r/244418/#review251238

::: browser/components/extensions/test/browser/browser_ext_browserAction_contextMenu.js:142
(Diff revision 2)
> +    info(`Create an empty tab`);
> +    let tab = gBrowser.addTab("about:blank");
> +    gBrowser.selectedTab = tab;
> +

This is the issue. I actually get a different error:

 0:08.08 INFO Run tests in customize mode
 0:08.22 INFO Test toolbar context menu in browserAction
 0:08.22 INFO Create an empty tab
 0:08.24 INFO Open browserAction context menu in toolbar-context-menu
 0:08.29 INFO Check visibility
 0:08.29 INFO Choosing 'Manage Extension' in toolbar-context-menu should load options
 0:08.43 GECKO(55529) JavaScript error: chrome://browser/content/browser.js, line 6406: TypeError: getUnwrappedTriggerNode(...) is null
 0:08.43 INFO Console message: [JavaScript Error: "TypeError: getUnwrappedTriggerNode(...) is null" {file: "chrome://browser/content/browser.js" line: 6406}]
openAboutAddonsForContextAction@chrome://browser/content/browser.js:6406:12
oncommand@chrome://browser/content/browser.xul:1:1



You're opening a tab while you're in customize mode, which exits customize mode, which then breaks all the testing.
Attachment #8976224 - Flags: review+
For some reason I didn't get that JS error. Well, now it seems to work.
Comment on attachment 8976224 [details]
Bug 1457474 - Add 'Manage Extension' in browserAction context menu

https://reviewboard.mozilla.org/r/244418/#review251250
Attachment #8976224 - Flags: review?(gijskruitbosch+bugs) → review+
Comment on attachment 8976224 [details]
Bug 1457474 - Add 'Manage Extension' in browserAction context menu

https://reviewboard.mozilla.org/r/244418/#review251540

::: browser/components/extensions/parent/ext-browserAction.js:183
(Diff revision 3)
>  
>        onCreated: node => {
>          node.classList.add("badged-button");
>          node.classList.add("webextension-browser-action");
>          node.setAttribute("constrain-size", "true");
> +        node.setAttribute("data-extension", this.extension.id);

lets call this data-extensionid
Attachment #8976224 - Flags: review?(mixedpuppy) → review+
Keywords: checkin-needed
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/73bbbe5c59b8
Add 'Manage Extension' in browserAction context menu r=Gijs,mixedpuppy
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/73bbbe5c59b8
Status: ASSIGNED → RESOLVED
Closed: Last year
Resolution: --- → FIXED
Target Milestone: --- → mozilla62
Tested and verified in Firefox 62.0a1 (20180530100110) on Windows 10 64Bit and MacOS 10.13.2.
Status: RESOLVED → VERIFIED
Product: Toolkit → WebExtensions
Depends on: 1482480
You need to log in before you can comment on or make changes to this bug.