Automatically add Manage Extension item in browserAction context menu

VERIFIED FIXED in Firefox 62

Status

enhancement
P5
normal
VERIFIED FIXED
a year ago
9 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)

(Assignee)

Description

a year ago
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
(Assignee)

Comment 2

a year ago
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

Comment 7

11 months ago
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)
(Assignee)

Comment 8

11 months ago
I will add "Manage Extension", the other things can be added in other bugs.
Assignee: nobody → oriol-bugzilla
Status: NEW → ASSIGNED
Comment hidden (mozreview-request)
(Assignee)

Comment 10

11 months ago
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.

Updated

11 months ago
Blocks: 1401610

Updated

11 months ago
Summary: Automatically add Options item in browserAction context menu → Automatically add Manage Extension item in browserAction context menu

Comment 11

11 months ago
mozreview-review
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+

Updated

11 months ago
Attachment #8976224 - Flags: review?(mixedpuppy)
(Assignee)

Comment 12

11 months ago
(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

11 months ago
(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

11 months ago
(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).
(Assignee)

Comment 17

11 months ago
(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.
(Assignee)

Comment 18

11 months ago
(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".
(Assignee)

Comment 19

11 months ago
(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.
(Assignee)

Comment 21

11 months ago
(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
(Assignee)

Comment 23

11 months ago
(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

11 months ago
(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 hidden (mozreview-request)
(Assignee)

Comment 26

11 months ago
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 27

11 months ago
mozreview-review
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+
(Assignee)

Comment 28

11 months ago
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 29

11 months ago
mozreview-review
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+
Comment hidden (mozreview-request)
(Assignee)

Comment 31

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

Comment 32

11 months ago
mozreview-review
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 33

11 months ago
mozreview-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+
Comment hidden (mozreview-request)
(Assignee)

Updated

11 months ago
Keywords: checkin-needed

Comment 35

11 months ago
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

Comment 36

11 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/73bbbe5c59b8
Status: ASSIGNED → RESOLVED
Last Resolved: 11 months ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla62

Comment 37

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

Updated

11 months ago
Status: RESOLVED → VERIFIED

Updated

10 months ago
Product: Toolkit → WebExtensions

Updated

9 months ago
Depends on: 1482480
You need to log in before you can comment on or make changes to this bug.