Tomislav Jovanovic :zombie Assignee
	Description • 8 years ago

+++ This bug was initially created as a clone of Bug #1253418 +++

Andy mentioned we want this.

	Blake Winton (:bwinton) (:☕️)
	Comment 1 • 8 years ago

As a side note, this is a requested feature for SnoozeTabs, an upcoming Test Pilot experiment that I'm hoping to write as a WebExtension…

	Blake Winton (:bwinton) (:☕️)
	Comment 2 • 8 years ago

Is this something I could prototype as a WebExtension Experiment (https://webextensions-experiments.readthedocs.io/en/latest/index.html)?  (I can see how I would add new APIs, but not how to extend existing APIs…)

	Tomislav Jovanovic :zombie Assignee
	Comment 3 • 8 years ago

AFAIK, we are already planning to do this, so it might not be the best candidate for an experiment (experiments are for things we are unsure about).

Also, I started looking into this before discovering bug 1253418 (which was higher priority as an existing thing).  I was planning to take this as soon as I'm done with that.

(which is just blocked on ux-review, hint hint ;)

	Andy McKay
	Comment 4 • 8 years ago

We can't really do that in experiments yet sadly, so would have to go through this path. Unless someone wants to try changing experiments... :)

	Kris Maglione [:kmag]
	Comment 5 • 8 years ago

There's not really much point in doing this as an experiment. As zombie said, this should be pretty simple now that bug 1253418 is fixed, and it's already some thing we know we want to do.

	Tomislav Jovanovic :zombie Assignee
	Comment 7 • 8 years ago

The situation is very similar to content context menu, so my implementation is virtually the same as well.

Multiple extensions can add a menu item to the tab context menu.  Those with multiple menu items get them grouped in a submenu with the name of the extension, "Generated extension" in this screenshot, with optional extension icon to the left of the name (not shown).

Also, I added the separator after the last built-in menu item, to serve as a visual and logical break.

	Tomislav Jovanovic :zombie Assignee
	Comment 8 • 8 years ago

While comparing this bug with the content context menu, I noticed we currently don't add a separator between built-in and extension menu items.  For the same reasons as above, as well as consistency, I went and added it.

Do you agree with this change Blake?

	Blake Winton (:bwinton) (:☕️)
	Comment 9 • 8 years ago

It looks good to me, but we should check with Markus, too…

Also, in terms of forward compatibility, what would happen to code that used this new API on versions of Firefox before it lands?  (Specifically if it's going to throw an error, can we also add a check so that I can write code that works in both situations?)

	Tomislav Jovanovic :zombie Assignee
	Comment 10 • 8 years ago

Markus seems to be on PTO.

This is implemented as a new "tab" context for the browser.contextMenus API [1].  Trying to create a menu item with that context would throw (reject the Promise) because of the unknown parameter in the Schema.

Other than catching that rejection, and creating the item without the unsupported context value, I'm not sure I understand what kind of check do you have in mind?

1) https://developer.mozilla.org/en-US/Add-ons/WebExtensions/API/contextMenus/create

	Blake Winton (:bwinton) (:☕️)
	Comment 11 • 8 years ago

Okay, lemme build it and check it out before I give the official ui-r.  (First thing tomorrow, I promise!)

I think I can live with adding a rejection handler to the promise.  Thanks!

	Blake Winton (:bwinton) (:☕️)
	Comment 12 • 8 years ago

(Oh, I guess I was thinking it would be nice if I could check for the existence of `contextMenus.ContextType.tab`, to see whether it would make sense to call the method in the first place.  Would that be possible?)

	Kris Maglione [:kmag]
	Comment 13 • 8 years ago

(In reply to Blake Winton (:bwinton) (:☕️) from comment #12)
> (Oh, I guess I was thinking it would be nice if I could check for the
> existence of `contextMenus.ContextType.tab`, to see whether it would make
> sense to call the method in the first place.  Would that be possible?)

Yes. Only it would be ContextType.TAB.

	Blake Winton (:bwinton) (:☕️)
	Comment 14 • 8 years ago

Comment on attachment 8815504 [details]
fx-tab-context-menu.png

This looks good.  My only complaint is that when I test it with the code at https://github.com/bwinton/SnoozeTabs/commit/2470ed73ed1d46fd44757fa8d9302aeb0609c90f I can't click on the parent menu to show the two sub-menus.  (And the onClicked listener doesn't fire.)

	Blake Winton (:bwinton) (:☕️)
	Comment 15 • 8 years ago

Comment on attachment 8815509 [details]
fx-content-context-menu.png

This looks good, after confirming on IRC that alpha and beta would be collapsed under a single parent item if they came from the same extension.

	Blake Winton (:bwinton) (:☕️)
	Comment 16 • 8 years ago

(In reply to Blake Winton (:bwinton) (:☕️) from comment #14)
> Comment on attachment 8815504 [details]
> fx-tab-context-menu.png
> 
> This looks good.  My only complaint is that when I test it with the code at
> https://github.com/bwinton/SnoozeTabs/commit/
> 2470ed73ed1d46fd44757fa8d9302aeb0609c90f I can't click on the parent menu to
> show the two sub-menus.  (And the onClicked listener doesn't fire.)

(That was my fault, kinda, because I didn't have a `context` on the child items.)

	Kris Maglione [:kmag]
	Comment 17 • 8 years ago
mozreview-review

Comment on attachment 8815503 [details]
Bug 1316020 - Implement tab context for browser.contextMenus

https://reviewboard.mozilla.org/r/96394/#review96802

This looks good to me. I'd r+ now, if not for the sub-menu context handling changes we discussed on IRC.

::: browser/components/extensions/ext-contextMenus.js:76
(Diff revision 1)
> +      if (firstItem) {
> +        firstItem = false;
> +        const separator = xulMenu.ownerDocument.createElement("menuseparator");
> +        this.itemsToCleanUp.add(separator);
> +        xulMenu.append(separator);
> +      }

Nit: Can you move this part to a separate patch, to make it clearer that it also applies to page context menus?

::: browser/components/extensions/ext-contextMenus.js:318
(Diff revision 1)
>    }
>  
> +  if (contextData.onTab) {
> +    contexts.add("tab");
> +  } else {
> +    contexts.add("all");

Should we also skip adding this for browserAction and pageAction?

::: browser/components/extensions/ext-contextMenus.js:534
(Diff revision 1)
> +function onTabContextMenu(event) {
> +  if (event.target.id === "tabContextMenu") {
> +    gMenuBuilder.build({
> +      tab: TabManager.activeTab,
> +      pageUrl: TabManager.activeTab.linkedBrowser.currentURI.spec,
> +      menu: event.target,
> +      onTab: true,
> +    });
> +  }
> +}
> +
> +function onWindowOpen(window) {
> +  const menu = window.document.getElementById("tabContextMenu");
> +  menu.addEventListener("popupshowing", onTabContextMenu);
> +}
> +
> +function registerListeners() {
> +  Services.obs.addObserver(contextMenuObserver, "on-build-contextmenu", false);
> +  for (const window of WindowListManager.browserWindows()) {
> +    onWindowOpen(window);
> +  }
> +  WindowListManager.addOpenListener(onWindowOpen);
> +}
> +
> +function unregisterListeners() {
> +  Services.obs.removeObserver(contextMenuObserver, "on-build-contextmenu");
> +  for (const window of WindowListManager.browserWindows()) {
> +    const menu = window.document.getElementById("tabContextMenu");
> +    menu.removeEventListener("popupshowing", onTabContextMenu);
> +  }
> +  WindowListManager.removeOpenListener(onWindowOpen);
> +}

Nit: Can you make these methods of a singleton object rather than separate globals?

::: browser/components/extensions/test/browser/head.js:274
(Diff revision 1)
>    }
>    return hidden;
>  }
>  
> +function openActionContextMenu(extension, kind, win = window) {
> +  SetPageProxyState("valid");

Would you mind adding a comment about the reason for this call (to make pageAction icons visible for tabs like about:blank)? I'm pretty sure there was one originally, but it seems to have been lost at some point.

	Tomislav Jovanovic :zombie Assignee
	Comment 20 • 8 years ago
mozreview-review-reply

Comment on attachment 8815503 [details]
Bug 1316020 - Implement tab context for browser.contextMenus

https://reviewboard.mozilla.org/r/96394/#review96802

That discussion was mainly predicated on (my) false statement that ommiting the context defaults to `all`.  Instead, it defaults to `page`, which means that other contexts like `link` or `image` also exibit the "problematic" behaviour noticed by @bwinton.

Because this makes it (still related but) a separate issue, and because I'm not even sure what the right solution would be, I'd rather leave that out of this bug.

> Should we also skip adding this for browserAction and pageAction?

I don't think so.  `browser_action` and `page_action` are explicitly documented [1] as included in `all`, and changing this now might break stuff.

On the other hand, it would make _some_ sense to include `tab` context in `all`, as semantically it is fairly close to the existing `page` context, as in "do something with this page loaded into this tab".

1) https://developer.mozilla.org/en-US/Add-ons/WebExtensions/API/contextMenus/ContextType

> Would you mind adding a comment about the reason for this call (to make pageAction icons visible for tabs like about:blank)? I'm pretty sure there was one originally, but it seems to have been lost at some point.

There is a comment some ~15 lines below.  It was even closer in a previous version of this code, so I didn't think it was necesery.  Added a pointer comment instead.

	Kris Maglione [:kmag]
	Comment 21 • 8 years ago
mozreview-review

Comment on attachment 8816940 [details]
Bug 1316020 - Add separator before extension items in context menus

https://reviewboard.mozilla.org/r/97416/#review99482

	Kris Maglione [:kmag]
	Comment 22 • 8 years ago
mozreview-review

Comment on attachment 8815503 [details]
Bug 1316020 - Implement tab context for browser.contextMenus

https://reviewboard.mozilla.org/r/96394/#review99484

Please file a follow-up bug to have child items inherit their parent's contexts by default.

	Pulsebot
	Comment 23 • 8 years ago

Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/dcb08ad3b67c
Implement tab context for browser.contextMenus r=kmag
https://hg.mozilla.org/integration/autoland/rev/6a907587ef77
Add separator before extension items in context menus r=kmag

	Phil Ringnalda (:philor)
	Comment 24 • 8 years ago
bugherder

https://hg.mozilla.org/mozilla-central/rev/dcb08ad3b67c
https://hg.mozilla.org/mozilla-central/rev/6a907587ef77

	Tomislav Jovanovic :zombie Assignee
	Comment 25 • 8 years ago

(In reply to Kris Maglione [:kmag] from comment #22)
> Please file a follow-up bug to have child items inherit their parent's
> contexts by default.

Filed bug 1324429.

	Vasilica Tamas, QA [:vtamas]
	Comment 26 • 8 years ago

Tested a bit around this bug on Firefox 53.0a1 (2016-12-27) under Windows 10 64-bit and Ubuntu 16.04 32-bit and noticed the following:

  - the separator is successfully displayed for both context menus in Firefox 53.0a1.

  - installed a few webextensions (Black Menu for Google, context-menu-demo and Tile Tabs WE) and noticed that only one of them is displayed in tab context menu (Black Menu for Google). See screenshots: http://i.imgur.com/nvRy0WA.jpg , http://i.imgur.com/nzIUjKR.jpg 

Should all the webextensions that appear in the page context menu be displayed in the tab context menu too? Or are there any conditions for this to happen?

	Tomislav Jovanovic :zombie Assignee
	Comment 27 • 8 years ago

(In reply to Vasilica Mihasca, QA [:vasilica_mihasca] from comment #26)
> Should all the webextensions that appear in the page context menu be
> displayed in the tab context menu too? Or are there any conditions for this
> to happen?

No, this is correct, there is a special "tab" context that extensions must specifically add to their code, so it is expected that most existing extensions don't add items to tab context menu.

	Vasilica Tamas, QA [:vtamas]
	Comment 28 • 8 years ago

(In reply to Tomislav Jovanovic :zombie from comment #27)
> No, this is correct, there is a special "tab" context that extensions must
> specifically add to their code, so it is expected that most existing
> extensions don't add items to tab context menu.

Thank you for your prompt reply!

Based on Comment 26 and Comment 27 I am marking this bug as Verified.

	Will Bamberg [:wbamberg]
	Comment 30 • 8 years ago

I've updated https://developer.mozilla.org/en-US/Add-ons/WebExtensions/API/contextMenus/ContextType.

Please let me know if it looks OK.

	Tomislav Jovanovic :zombie Assignee
	Comment 31 • 8 years ago

Looks good, thanks Will.