Closed
Bug 1275116
Opened 8 years ago
Closed 8 years ago
documentUrlPatterns does not work in the contextMenus API
Categories
(WebExtensions :: Untriaged, defect, P2)
WebExtensions
Untriaged
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
Comment 1•8 years ago
|
||
Could you take a look please matt?
Assignee: nobody → mwein
Priority: -- → P2
Whiteboard: [contextMenus]triaged
Comment 2•8 years ago
|
||
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.
Assignee | ||
Comment 3•8 years ago
|
||
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.
Assignee | ||
Comment 4•8 years ago
|
||
Review commit: https://reviewboard.mozilla.org/r/64398/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/64398/
Attachment #8771151 -
Flags: review?(kmaglione+bmo)
Comment 5•8 years ago
|
||
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)
Assignee | ||
Comment 6•8 years ago
|
||
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 7•8 years ago
|
||
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+
Assignee | ||
Comment 8•8 years ago
|
||
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/
Assignee | ||
Updated•8 years ago
|
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
Comment 10•8 years ago
|
||
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
Comment 11•8 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/39ebdb2e59f2 https://hg.mozilla.org/mozilla-central/rev/ccbf505eff51
Status: NEW → RESOLVED
Closed: 8 years ago
status-firefox50:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla50
Comment 12•8 years ago
|
||
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
Comment 13•8 years ago
|
||
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
Reporter | ||
Comment 14•8 years ago
|
||
Done: https://developer.mozilla.org/en-US/Add-ons/WebExtensions/API/contextMenus#Browser_compatibility https://developer.mozilla.org/en-US/Add-ons/WebExtensions/API/contextMenus/create https://developer.mozilla.org/en-US/Add-ons/WebExtensions/API/contextMenus/update
Keywords: dev-doc-needed → dev-doc-complete
Comment 15•8 years ago
|
||
The compat table at https://developer.mozilla.org/en-US/Add-ons/WebExtensions/API/contextMenus#Browser_compatibility does not show any new compat note (https://developer.mozilla.org/en-US/Add-ons/WebExtensions/API/contextMenus$history). Did you save the changes?
Flags: needinfo?(wbamberg)
Reporter | ||
Comment 16•8 years ago
|
||
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)
Updated•6 years ago
|
Product: Toolkit → WebExtensions
You need to log in
before you can comment on or make changes to this bug.
Description
•