1457474 - Automatically add Manage Extension item in browserAction context menu

Status: VERIFIED FIXED

(WebExtensions :: Frontend, enhancement, P5)

Description

[Oriol Brufau [:Oriol]]

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.

Comment 1

[Caitlin Neiman [:caitmuenster]]

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

Comment 2

[Oriol Brufau [:Oriol]]

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.

Comment 3

[Caitlin Neiman [:caitmuenster]]

Will do. Thanks for the heads up. :)

Comment 4

[Tim Nguyen :ntim]

Attachment: Chrome's context menu

Comment 5

[Caitlin Neiman [:caitmuenster]]

This was approved in the design-decision meeting. Mconca to follow up with UX.

Comment 6

[Mike Conca [:mconca]]

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.

Comment 7

[emanuela [ux]]

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

Comment 8

[Oriol Brufau [:Oriol]]

I will add "Manage Extension", the other things can be added in other bugs.

Comment 9

[Oriol Brufau [:Oriol]]

Attachment: Bug 1457474 - Add 'Manage Extension' in browserAction context menu

Review commit: https://reviewboard.mozilla.org/r/244418/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/244418/

Comment 10

[Oriol Brufau [:Oriol]]

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.

Comment 11

[:Gijs (he/him)]

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.

Comment 12

[Oriol Brufau [:Oriol]]

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

Comment 13

[:Gijs (he/him)]

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

Comment 14

[:Gijs (he/him)]

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

Comment 15

[Shane Caraveo (:mixedpuppy)]

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

Comment 16

[Shane Caraveo (:mixedpuppy)]

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

Comment 17

[Oriol Brufau [:Oriol]]

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

Comment 18

[Oriol Brufau [:Oriol]]

(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 = [e-mail], true".
But with useAddonManager:"temporary", I get "id = undefined, false".

Comment 19

[Oriol Brufau [:Oriol]]

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

Comment 20

[Shane Caraveo (:mixedpuppy)]

(In reply to Oriol Brufau [:Oriol] from comment #18)

> Without useAddonManager, I get "id = [e-mail], 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.

Comment 21

[Oriol Brufau [:Oriol]]

(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: "[e-mail]"},
      },

But if I also add useAddonManager:"temporary", then extension.id is undefined.

Comment 22

[Shane Caraveo (:mixedpuppy)]

(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: "[e-mail]"},
>       },
> 
> 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

Comment 23

[Oriol Brufau [:Oriol]]

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

Comment 24

[:Gijs (he/him)]

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

[Oriol Brufau [:Oriol]]

Comment on attachment 8976224 [details]
Bug 1457474 - Add 'Manage Extension' in browserAction context menu

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/244418/diff/1-2/

Comment 26

[Oriol Brufau [:Oriol]]

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.

Comment 27

[:Gijs (he/him)]

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.

Comment 28

[Oriol Brufau [:Oriol]]

Attachment: 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 29

[:Gijs (he/him)]

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.

Comment 30

[Oriol Brufau [:Oriol]]

Comment on attachment 8976224 [details]
Bug 1457474 - Add 'Manage Extension' in browserAction context menu

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/244418/diff/2-3/

Comment 31

[Oriol Brufau [:Oriol]]

For some reason I didn't get that JS error. Well, now it seems to work.

Comment 32

[:Gijs (he/him)]

Comment on attachment 8976224 [details]
Bug 1457474 - Add 'Manage Extension' in browserAction context menu

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

Comment 33

[Shane Caraveo (:mixedpuppy)]

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

Comment 34

[Oriol Brufau [:Oriol]]

Comment on attachment 8976224 [details]
Bug 1457474 - Add 'Manage Extension' in browserAction context menu

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

Comment 35

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/73bbbe5c59b8
Add 'Manage Extension' in browserAction context menu r=Gijs,mixedpuppy

Comment 36

[Narcis Beleuzu [:NarcisB]]

https://hg.mozilla.org/mozilla-central/rev/73bbbe5c59b8

Comment 37

[marius.santa]

Attachment: manage extensions button.gif

Tested and verified in Firefox 62.0a1 (20180530100110) on Windows 10 64Bit and MacOS 10.13.2.