The default bug view has changed. See this FAQ.

documentUrlPatterns in the contextMenus does not work on frames properly

RESOLVED FIXED in Firefox 55

Status

()

Toolkit
WebExtensions: Untriaged
P2
normal
RESOLVED FIXED
a month ago
6 days ago

People

(Reporter: Johny G., Assigned: mixedpuppy)

Tracking

54 Branch
mozilla55
Points:
---

Firefox Tracking Flags

(firefox55 fixed)

Details

(Whiteboard: [contextMenus], triaged)

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(1 attachment)

(Reporter)

Description

a month ago
User Agent: Mozilla/5.0 (Windows NT 6.1; Win64; x64; rv:54.0) Gecko/20100101 Firefox/54.0
Build ID: 20170213030206

Steps to reproduce:

documentUrlPatterns in the contextMenus does not work on frames properly

This bug applies to WebExtensions contextMenus API. 
Specification states that context "frame" use documentUrlPatterns to filter its src attribute,
in distinction of "image", "link" and "video" that use targetUrlPatterns.

1.
To reproduce issue, create simple extension compound of two files:
manifest.json:
{
"manifest_version": 2,
"version": "1.0",
"name": "frame context menu",
"permissions": ["contextMenus"],
"background": {"scripts": ["sample_menu.js"]}
}

sample_menu.js:
chrome.contextMenus.create({
	"title": "FRAME context menu",
	"contexts": ["frame"],
	"documentUrlPatterns": ["<all_urls>"]
});
function playOnClick(info, tab) {
    console.log(JSON.stringify(info));    
}
chrome.contextMenus.onClicked.addListener(playOnClick);

2.
Install extension at 'about:debugging' and click debug to show javascript console.
In new tab navigate to https://developers.google.com/youtube/youtube_player_demo
This is sample website with embedded youtube player that uses iframe.

Right click on player (right click twice, as first time player menu appears).
Our context menu shows, hover and click on it. 

Javascript console displays:
{"menuItemId":1, "editable":false, "parentMenuItemId":0, "linkUrl":"", "srcUrl":"",
"pageUrl":"https://developers.google.com/youtube/youtube_player_demo",
"frameUrl":"https://www.youtube.com/embed/M7lc1UVf-VE?...cut.as.not.important", "modifiers":[]}

Value in frameUrl confirms, that iframe src attribute is filled with url,
that matches pattern "https://www.youtube.com/embed/*"

3.
Edit sample_menu.js and replace <all_urls> in documentUrlPatterns to look like: 

"documentUrlPatterns": ["https://www.youtube.com/embed/*"]

Reload extension and once again on the same webpage right click on youtube player.
Our context menu does not show, but should. In Chrome this works and menu shows.

4.
Once again edit sample_menu.js and add to documentUrlPatterns "https://developers.google.com/*"
it will look like:
"documentUrlPatterns": ["https://developers.google.com/*", "https://www.youtube.com/embed/*"]

Reload extension and once again on the same webpage right click on youtube player.
Menu shows. This is a clue what might be wrong. Probably implementation first checks if documentUrlPatterns matches parent 
url, and if not, do not render menu even if there is "frame" among contexts attribute in the menu.

This was tested on 64bit Windows 7 and Firefox 51.0.1 64bit, and nightly build 54.0a1 (2017-02-13) 64bit.
I don't have any other extensions installed.
Component: Untriaged → WebExtensions: Untriaged
Product: Firefox → Toolkit

Updated

25 days ago
Priority: -- → P2
Whiteboard: [contextMenus], triaged

Comment 1

25 days ago
shane and kris were going to talk about this one
Flags: needinfo?(mixedpuppy)
Comment hidden (mozreview-request)
(Assignee)

Updated

16 days ago
Flags: needinfo?(mixedpuppy)
Attachment #8845191 - Flags: feedback?(kmaglione+bmo)
(Assignee)

Comment 3

16 days ago
The documentation is vague on what is expected here: 

documentUrlPatternsOptional
    array of string. Lets you restrict the item to apply only to documents whose URL matches one of the given match patterns. This applies to frames as well.

It does however make sense that the match is against the frameUrl, not the url of the parent.

At a minimum, we need to add a test form "frame" since one did not exist.
(Assignee)

Updated

11 days ago
Attachment #8845191 - Flags: review?(kmaglione+bmo)
(Assignee)

Updated

11 days ago
Attachment #8845191 - Flags: feedback?(kmaglione+bmo)
(Assignee)

Updated

11 days ago
Assignee: nobody → mixedpuppy

Comment 4

11 days ago
mozreview-review
Comment on attachment 8845191 [details]
Bug 1339440 use frameUrl to match context menus on frames

https://reviewboard.mozilla.org/r/118380/#review121786

Still not really sold on this behavior, but fine.

::: browser/components/extensions/ext-contextMenus.js:520
(Diff revision 1)
> -    let pageURI = Services.io.newURI(contextData.pageUrl);
> +    let pageURI = contextData.inFrame ? Services.io.newURI(contextData.frameUrl)
> +                                      : Services.io.newURI(contextData.pageUrl);

Please move the ternary inside the `newURI` call, or ideally something like `contextData[inFrame ? "frameUrl" : "pageUrl"]`
Attachment #8845191 - Flags: review?(kmaglione+bmo) → review+
Comment hidden (mozreview-request)

Comment 6

9 days ago
We're sorry, Autoland could not rebase your commits for you automatically. Please manually rebase your commits and try again.

hg error in cmd: hg rebase -s d78792d5e9a8 -d e2d9ac61dd7d: rebasing 382228:d78792d5e9a8 "Bug 1339440 use frameUrl to match context menus on frames r=kmag" (tip)
merging browser/components/extensions/test/browser/browser-common.ini
merging browser/components/extensions/test/browser/head.js
warning: conflicts while merging browser/components/extensions/test/browser/head.js! (edit, then use 'hg resolve --mark')
unresolved conflicts (see hg resolve, then hg rebase --continue)
Comment hidden (mozreview-request)

Comment 8

9 days ago
Pushed by mixedpuppy@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/075395ba45fe
use frameUrl to match context menus on frames r=kmag
Backed out in https://hg.mozilla.org/integration/autoland/rev/2b8f086b702f for https://treeherder.mozilla.org/logviewer.html#?job_id=84153815&repo=autoland
Driving by... looking at https://hg.mozilla.org/integration/autoland/rev/075395ba45fe#l1.13,

I'm pretty sure:
> let pageURI = Services.io.newURI(contextData["inFrame" ? "frameUrl" : "pageUrl"]);
should have been:
> let pageURI = Services.io.newURI(contextData[inFrame ? "frameUrl" : "pageUrl"]);

The actual code that landed should always return "frameUrl" since the string "inFrame" is always truthy. I don't see where the inFrame variable comes from though, but I'll assume it's present already since comment 4 mentions it.
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
Comment hidden (mozreview-request)

Comment 12

7 days ago
Pushed by mixedpuppy@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/9aa2aa7a42d2
use frameUrl to match context menus on frames r=kmag
https://hg.mozilla.org/mozilla-central/rev/9aa2aa7a42d2
Status: ASSIGNED → RESOLVED
Last Resolved: 6 days ago
status-firefox55: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
You need to log in before you can comment on or make changes to this bug.