Rob Wu [:robwu] Assignee
	Description • 6 years ago

TL;DR let documentUrlPatterns match moz-extension:-URL document URLs, even if menus.overrideContext is used.

In bug 1367160 I introduced menus.overrideContext to allow extensions to replace menus in their extension documents.
In bug 1280347 I extended the method to allow extensions to change the context type to tab/bookmark.
In bug 1416839 I added a "viewTypes" parameter to allow extensions to restrict the menu items to a specific menu.

Usually, "documentUrlPatterns" can be used to restrict menu items to certain documents.
When the menu of a document is overridden (bug 1280347), documentUrlPatterns does not behave in this way because I tried to avoid overloading the meaning of "documentUrlPatterns" (in the "tab" context, this is used to show menu items only on tabs with certain URLs).

I was hoping that viewTypes from bug 1416839 would be an acceptable alternative, but I realized that this is not sufficient, as there is no way to ensure that a menu item only appears in an extension (and not any others).

So I will add a patch (and get it in Firefox 64 where those other features have landed) that extends the behavior of documentUrlPatterns:

1. If viewTypes is present, check if match patterns with the moz-extension: scheme are present in documentUrlPatterns.
   If that is the case, require the document's URL (where the menu was opened) to match the pattern.

2. If moz-extension: is the only type of pattern in documentUrlPatterns, treat the menu item as if documentUrlPatterns is not set. This ensures that the menu item will not inadvertently be hidden in contexts where there is no document URL that could match (e.g. in the native bookmark context).

3. If there are any non-moz-extension:-patterns (e.g. http://localhost/*, <all_urls>, *://*/*),
   also perform the usual matching logic, if any.
   (e.g. for the "tab" context, the menu item will only be shown if the tab's URL matches any of the patterns).
   (this also means that the moz-extension:-pattern would always match; to avoid this one can use menus.onShown + menus.update with visible, from bug 1482529).

This API design brings the desired ability to restrict menus to existing documents, without backwards-compatibility issues (since both viewTypes and menus.overrideContext are new in Firefox 64).

	YUKI "Piro" Hiroshi
	Comment 1 • 6 years ago

How should the new behavior work on a case: both the sidebar and the context tab have "moz-extension:" URLs? For example, when the sidebar is "moz-extension://xxxx-xxxxx/sidebar/sidebar.html", the current tab is "moz-extension://yyyy-yyyyy/canvas.html" and the context menu is shown by "browser.menus.overrideContext({ contest: 'tab', tabId: XXX })", then should "documentUrlPatterns" match to one of xxxx-xxxxx or yyyy-yyyyy, or both?

	Rob Wu [:robwu] Assignee
	Comment 2 • 6 years ago

(In reply to YUKI "Piro" Hiroshi from comment #1)
> How should the new behavior work on a case: both the sidebar and the context
> tab have "moz-extension:" URLs?

That depends on whether there are other patterns in the documentUrlPatterns list.
The exact behavior is specified in comment 0 by:

> 2. If moz-extension: is the only type of pattern in documentUrlPatterns,
> treat the menu item as if documentUrlPatterns is not set.
and
>    (this also means that the moz-extension:-pattern would always match; to
> avoid this one can use menus.onShown + menus.update with visible, from bug 1482529).

So, in your example, if you have ['moz-extension://*/*'], then it would only match xxxx-xxxxx (if viewTypes is set; otherwise it would match yyyy-yyyyy).

If you have ['moz-extension://*/*', 'http://some.other.example.com'], then it would match both xxxx-xxxxx and also yyyy-yyyyy.


Another ambiguity is the meaning of onClickData.pageUrl. In the "tab" context, it is the same as tab.url.
A way to solve this ambiguity is to always set onClickData.frameUrl when menus.overrideContext is used.

	Rob Wu [:robwu] Assignee
	Comment 3 • 6 years ago

I investigated the API usage patterns of add-ons (including multiple versions of the same add-on) that use the menus or contextMenus permission on AMO, and specify a tab context.

There is no add-on that uses a moz-extension:-URL in documentUrlPatterns when "contexts" includes "tab".

Therefore I think that the change does not need to be tied to viewTypes, and the following change is sufficient.
We only need to *add* (not modify) the following logic:
1. If a pattern with the moz-extension: scheme is present in documentUrlPatterns,
    - Require the document's URL (where the menu was opened) to match the pattern.
    - If <all_urls> is present, let it match any tab.

The last condition about <all_urls> is because <all_urls> does not actually match all URLs, but only URLs with whitelisted schemes. E.g. about:blank is not matched because "about" is not a permitted scheme for <all_urls>.
Most of the ~300 extensions that have a tab menu don't specify documentUrlPatterns, so their menu items show up on every tab.
At most 30 add-ons do use documentUrlPatterns for tab contexts (and most of them are trying to match any host (*://*/, http://*/, https://*/, file://*/)).

There are four add-ons that use <all_urls> for tab contexts and all of them combine it with "page" (AMO IDs 11097 800531 892972 1000452). The proposed change does not affect these add-ons.


I also looked for add-ons that use documentUrlPatterns with the "bookmark" context. There is only one, Side View (and it has "tab", "bookmark" and "page" all in one list, together with "<all_urls>" as documentUrlPatterns:
https://github.com/mozilla/side-view/blob/389370c366291dab56efbeb8447564990da484af/addon/background.js#L90

Adding support for moz-extension in documentUrlPatterns in the "bookmark" context will not affect the functionality of these extensions.

	Shane Caraveo (:mixedpuppy)
	Comment 4 • 6 years ago

(In reply to Rob Wu [:robwu] from comment #3)

> The last condition about <all_urls> is because <all_urls> does not actually
> match all URLs, but only URLs with whitelisted schemes. E.g. about:blank is
> not matched because "about" is not a permitted scheme for <all_urls>.
> Most of the ~300 extensions that have a tab menu don't specify
> documentUrlPatterns, so their menu items show up on every tab.

I'm not really certain what to do with this right now.  I've a couple competing thoughts.

My initial inclination is to NOT change all_urls, and fix context menus to use all-urls if no documentUrlPatterns are defined.  This results in actually restricting extensions from placing context menus on tabs they otherwise usually cannot access.

My second inclination is to not change anything here.  

With both: I'd rather not have all_urls mean different things in different apis.

	Rob Wu [:robwu] Assignee
	Comment 5 • 6 years ago

(In reply to Shane Caraveo (:mixedpuppy) from comment #4)
> (In reply to Rob Wu [:robwu] from comment #3)
> 
> > The last condition about <all_urls> is because <all_urls> does not actually
> > match all URLs, but only URLs with whitelisted schemes. E.g. about:blank is
> > not matched because "about" is not a permitted scheme for <all_urls>.
> > Most of the ~300 extensions that have a tab menu don't specify
> > documentUrlPatterns, so their menu items show up on every tab.
> 
> I'm not really certain what to do with this right now.  I've a couple
> competing thoughts.
> 
> My initial inclination is to NOT change all_urls, and fix context menus to
> use all-urls if no documentUrlPatterns are defined.  This results in
> actually restricting extensions from placing context menus on tabs they
> otherwise usually cannot access.

If menus are only useful to inject scripts, then I'd agree. However, that is not the case. Examples:
- A tab menu item to move the tab elsewhere.
- A link menu item to open a link elsewhere.

I just checked the usage of all_urls in documentUrlPatterns and targetUrlPatterns. There are about 15 extensions that use <all_urls>, for mostly different reasons.

> My second inclination is to not change anything here.  
> 
> With both: I'd rather not have all_urls mean different things in different
> apis.

Next idea: To avoid complicated handling of documentUrlPatterns (special handling of moz-extension:-schemes as in comment 0, and/or special handling of <all_urls> as in comment 3), we could also change the behavior of documentUrlPatterns when viewTypes is set. When one uses viewTypes, it is reasonable to expect documentUrlPatterns to always match the document's URL.

Extension won't be able to use documentUrlPatterns to match on tab URL any more (when a tab context is spoofed in the sidebar). They can however see the tab URL in info.pageUrl (and even tab.url) and use menus.update during the menus.onShown event to hide the menu if desired.
Note that if viewTypes is not present, that documentUrlPatterns still matches based on tab URL, so there are no backward-compatibility issues.

The original document's URL (where the menu was opened from where menus.overrideContext was called) will be available as onClickData.frameUrl. The API looks still reasonably consistent: documentUrlPatterns matches frameUrl if present, and pageUrl otherwise.

	Rob Wu [:robwu] Assignee
	Comment 6 • 6 years ago

To support the above proposal, I note that there are very few extensions that rely on documentUrlPatterns for tab menus, and if they do, that it is used to select as many URLs as possible:

(In reply to Rob Wu [:robwu] from comment #3)
> Most of the ~300 extensions that have a tab menu don't specify
> documentUrlPatterns, so their menu items show up on every tab.
> At most 30 add-ons do use documentUrlPatterns for tab contexts (and most of
> them are trying to match any host (*://*/, http://*/, https://*/,
> file://*/)).

And it is not even documented on MDN that tab menus can be matched via documentUrlPatterns. Combined with the "viewTypes" property, I think that the idea in comment 5 is the most reasonable proposal so far.

	Rob Wu [:robwu] Assignee
	Comment 7 • 6 years ago

Usually, documentUrlPatterns applies to the URL of the document where
the context menu is opened. In some cases, there is no document, such
as menus on browser UI (extension action buttons, tools menu, tabs).
In these cases, `documentUrlPatterns` matches the (active) tab's URL.

This causes ambiguity when `browser.menus.overrideContext` is used to
change the context to the "tab" context.
Before this patch, `documentUrlPatterns` applied to the tab's URL.
After this patch, `documentUrlPatterns` applies to the original
document's URL *if* `viewTypes` is also set. Using this property is a
strong signal from the extension that the menu is meant to be shown in
a document rather than browser UI, so extensions can reasonably expect
`documentUrlPatterns` to match the original document's URL instead of
the URL of the spoofed tab context.

There was no existing test coverage for documentUrlPatterns on tab
contexts, so this does not only add tests for documentUrlPatterns on
overridden contexts (browser_ext_menus_replace_menu_context.js), but
also documentUrlPatterns in normal tab contexts (browser_ext_menus.js).

	Pulsebot
	Comment 8 • 6 years ago

Pushed by rob@robwu.nl:
https://hg.mozilla.org/integration/autoland/rev/ffbc071b41bd
Apply documentUrlPatterns to original target document when viewTypes is set r=mixedpuppy

	Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)
	Comment 9 • 6 years ago
bugherder

https://hg.mozilla.org/mozilla-central/rev/ffbc071b41bd

	Rob Wu [:robwu] Assignee
	Comment 10 • 6 years ago

Test case for verification:

1. Load attached extension, e.g. at about:debugging.
2. Upon load, a sidebar will open, with a button inside.
3. Right-click the button to open a context menu.
4. Count the number of menu items in the context menu.

Expected before patch (bad):
- Menu has one menu item.

Expected after patch (good):
- Menu has two menu items.

Bad : Firefox 64.0a1, buildID 20181012103207
Good: Firefox 65.0a1, buildID 20181022220734

	Rob Wu [:robwu] Assignee
	Comment 11 • 6 years ago

Comment on attachment 9018597 [details]
Bug 1498896 - Apply documentUrlPatterns to original target document when viewTypes is set

[Beta/Release Uplift Approval Request]

Feature/Bug causing the regression: Bug 1280347

User impact if declined: This fixes a defect in an extension API that was introduced in Firefox 64. Due to this bug, menu items from extensions may unexpectedly appear in documents of other extensions, or unexpectedly not be shown in their own document.

Not merging the fix forces extension authors to ignore the above problems, or develop work-arounds to account for the behavioral differences in Firefox 64 and 65.

Is this code covered by automated tests?: Yes

Has the fix been verified in Nightly?: Yes

Needs manual test from QE?: No

If yes, steps to reproduce: 

List of other uplifts needed: None

Risk to taking this patch: Low

Why is the change risky/not risky? (and alternatives if risky): This changes the behavior of a new feature that was added in Firefox 64. It is covered by automated tests and has been verified manually (comment 10).

String changes made/needed: N/A

	Ryan VanderMeulen [:RyanVM]
	Comment 12 • 6 years ago

Comment on attachment 9018597 [details]
Bug 1498896 - Apply documentUrlPatterns to original target document when viewTypes is set

Fixes a regression in Fx64 causing menu items from extensions to behave erratically. Approved for 64.0b4.

	Timea Zsoldos [:zstimi/tzsoldos], Desktop QA
	Comment 13 • 6 years ago

I have reproduced this issue using Firefox 64.0a1 (2018.10.14) on Win 10 x64.
I can confirm this issue is fixed, I verified using Firefox 65.0a1 (2018.10.23) on Win 10 x64, Mac OS X 10.13 and Ubuntu 16.04 x64. I wait for build 64.0b4 to verify this issue.

	Narcis Beleuzu [:NarcisB]
	Comment 14 • 6 years ago
bugherder uplift

https://hg.mozilla.org/releases/mozilla-beta/rev/498f4d6ade77

	YUKI "Piro" Hiroshi
	Comment 15 • 6 years ago

FYI: I've written a blog post to describe how to use the new behavior of documentUrlPatterns to define context menu items for each case. I hope it helps addon developers until official documents become available:
https://piro.sakura.ne.jp/latest/blosxom/mozilla/xul/2018-10-26_override-context-for-each-case.htm#topic2018-10-26_override-context-for-each-case

	Rob Wu [:robwu] Assignee
	Comment 16 • 6 years ago

Thanks again YUKI for the preliminary documentation.

After bug 1367160, bug 1280347, bug 1416839 is documented, the change from this bug should also be documented on MDN, at the following places:
- documentUrlPatterns property in the parameter for the menus.create and menus.update methods
- viewTypes property in the parameter for the menus.create and menus.update methods
- the frameUrl property of menus.OnClickData
- the menus.overrideContext method

	Timea Zsoldos [:zstimi/tzsoldos], Desktop QA
	Comment 17 • 6 years ago

I can confirm this issue is fixed, I verified using Firefox 64.0b4 on Win 10 x64, Mac OS X 10.13 and Ubuntu 16.04 x64.

	Chris Mills [:cmills]
	Comment 18 • 5 years ago

Note to MDN writers:

I have written a note to cover this in the Fx 64 rel notes; see the Menus section of https://developer.mozilla.org/en-US/docs/Mozilla/Firefox/Releases/64#API_changes. I'm not sure if this is quite right, so a review would be appreciated.

For the docs work, see :robwu's very useful summary of what needs documenting in https://bugzilla.mozilla.org/show_bug.cgi?id=1498896#c16