Closed Bug 1275116 Opened 4 years ago Closed 3 years ago

documentUrlPatterns does not work in the contextMenus API

Categories

(WebExtensions :: Untriaged, defect, P2)

defect

Tracking

(firefox50 verified)

VERIFIED FIXED
mozilla50
Tracking Status
firefox50 --- verified

People

(Reporter: wbamberg, Assigned: mattw)

References

Details

(Keywords: dev-doc-complete, Whiteboard: [contextMenus]triaged)

Attachments

(2 files)

I've attached a WebExtension that uses documentUrlPatterns to show the item only under developer.mozilla.org.

In Chrome:
* the item does not show up on bbc.co.uk
* the item does show up on developer.mozilla.org

In Firefox:
* the item never shows up
Could you take a look please matt?
Assignee: nobody → mwein
Priority: -- → P2
Whiteboard: [contextMenus]triaged
I think targetUrlPatterns does not work also, I already described this problem in bug:
Bug #1275126 - "targetUrlPatterns" in contextMenus fails with an error if the ContextType is "image" 

but this one seems to be more appropriate for that problem.
The problem is that `SingleMatchPattern.match(...)` takes an `nsIURI` object with properties like scheme, host, and path, and we are only passing in the URLs as strings.  We can fix this by converting the strings into `nsIURI`s using `Services.io.newURI(...)`.  I'm going to write up some tests for this and then I'll upload a patch.
Comment on attachment 8771151 [details]
Bug 1275116 - Fix {document|target}UrlPatterns by passing in the correct arguments to SingleMatchPattern.match.

https://reviewboard.mozilla.org/r/64398/#review62028

::: browser/components/extensions/ext-contextMenus.js:438
(Diff revision 1)
>      if (!this.contexts.some(n => contexts.has(n))) {
>        return false;
>      }
>  
>      let docPattern = this.documentUrlMatchPattern;
> -    if (docPattern && !docPattern.matches(contextData.pageUrl)) {
> +    let pagePattern = Services.io.newURI(contextData.pageUrl, null, null);

This should be named something like `pageURI`

::: browser/components/extensions/ext-contextMenus.js:449
(Diff revision 1)
> -    if (isMedia && targetPattern && !targetPattern.matches(contextData.srcURL)) {
> +    if (targetPattern) {
> +      let isMedia = contextData.onImage || contextData.onAudio || contextData.onVideo;
> +      if (!isMedia) {
> +        return false;
> +      }
> +      let srcPattern = Services.io.newURI(contextData.srcUrl, null, null);

This should be named something like `srcURI`.

::: browser/components/extensions/test/browser/browser_ext_contextMenus_urlPatterns.js:9
(Diff revision 1)
> +
> +add_task(function* () {
> +  let tab1 = yield BrowserTestUtils.openNewForegroundTab(gBrowser,
> +    "http://mochi.test:8888/browser/browser/components/extensions/test/browser/context.html");
> +
> +  gBrowser.selectedTab = tab1;

Not necessary.

::: browser/components/extensions/test/browser/browser_ext_contextMenus_urlPatterns.js:63
(Diff revision 1)
> +
> +      browser.contextMenus.create({
> +        title: "documentUrlPatterns-patternDoesNotMatch-contextImage",
> +        documentUrlPatterns: ["*://*/does-not-match"],
> +        contexts: ["image"],
> +      });

We also need tests for items using both `targetUrlPatterns` and `documentUrlPatterns`.

::: browser/components/extensions/test/browser/head.js:124
(Diff revision 1)
>    let popupHiddenPromise = BrowserTestUtils.waitForEvent(contentAreaContextMenu, "popuphidden");
>    contentAreaContextMenu.hidePopup();
>    yield popupHiddenPromise;
>  }
>  
> -function* openExtensionContextMenu() {
> +function* openExtensionContextMenu(id="#img1") {

s/id/selector/ (in both these helper functions)
Attachment #8771151 - Flags: review?(kmaglione+bmo)
Comment on attachment 8771151 [details]
Bug 1275116 - Fix {document|target}UrlPatterns by passing in the correct arguments to SingleMatchPattern.match.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/64398/diff/1-2/
Attachment #8771151 - Flags: review?(kmaglione+bmo)
Comment on attachment 8771151 [details]
Bug 1275116 - Fix {document|target}UrlPatterns by passing in the correct arguments to SingleMatchPattern.match.

https://reviewboard.mozilla.org/r/64398/#review62390
Attachment #8771151 - Flags: review?(kmaglione+bmo) → review+
Blocks: 1275126
Comment on attachment 8771151 [details]
Bug 1275116 - Fix {document|target}UrlPatterns by passing in the correct arguments to SingleMatchPattern.match.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/64398/diff/2-3/
Keywords: checkin-needed
Pushed by cbook@mozilla.com:
https://hg.mozilla.org/integration/fx-team/rev/39ebdb2e59f2
Fix {document|target}UrlPatterns by passing in the correct arguments to SingleMatchPattern.match. r=kmag
Keywords: checkin-needed
Pushed by archaeopteryx@coole-files.de:
https://hg.mozilla.org/integration/fx-team/rev/ccbf505eff51
Fix {document|target}UrlPatterns by passing in the correct arguments to SingleMatchPattern.match: add spaces to fix eslint errors. r=eslint-fix
https://hg.mozilla.org/mozilla-central/rev/39ebdb2e59f2
https://hg.mozilla.org/mozilla-central/rev/ccbf505eff51
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla50
I was able to reproduce the initial issue on Firefox 49.0a1 (2016-05-23) under Windows 10 64-bit.

Verified as fixed on Firefox 52.0a1 (2016-09-25), Firefox 51.0a2 (2016-09-26) and Firefox 50.0b1 (20160920155715). The context menu item appears only for developer.mozilla.org.
Status: RESOLVED → VERIFIED
MDN should be updated.

https://developer.mozilla.org/en-US/Add-ons/WebExtensions/API/contextMenus#Chrome_incompatibilities
"Firefox does not support: .... the documentUrlPatterns option to create()" should be removed, and the "Browser compatibility" tables (possibly including those at create/update) should mention that documentUrlPatterns is only properly supported since version 50.
Keywords: dev-doc-needed
The change here:
https://developer.mozilla.org/en-US/Add-ons/WebExtensions/API/contextMenus#Browser_compatibility 
is the *removal* of the "Firefox does not support..." note.

This table does not show the notes specific to create() and update(). The aggregate tables don't show all the detailed compat notes for all the APIs they contain. If they did, this section would be a morass of notes and would get very hard to read, especially in large modules with a lot of notes. What it does is: where there is a specific API which has compat notes, display an asterisk in the cell, and in the page for that API (e.g. https://developer.mozilla.org/en-US/Add-ons/WebExtensions/API/contextMenus/create), describe the compat issue itself.
Flags: needinfo?(wbamberg)
Product: Toolkit → WebExtensions
You need to log in before you can comment on or make changes to this bug.