1274860 - ContextType "page" is not applied correctly

Status: RESOLVED FIXED

(WebExtensions :: Untriaged, defect, P2)

Description

[Will Bamberg [:wbamberg]]

Attachment: context-menu.zip

I've attached a WebExtension that creates two context menu items: one is displayed in "page" context, and the other is displayed in "frame" context:

chrome.contextMenus.create({
  id: "clickme-frame",
  title: "Frame!",
  contexts: ["frame"]
});

chrome.contextMenus.create({
  id: "clickme-page",
  title: "Page!",
  contexts: ["page"]
});

It's not documented, but from Chrome's behavior it seems as though "page" should apply when the user clicks on the page and none of the other contexts apply: for example, it's not on a link, or a nested iframe, or ...

In Firefox (Developer Edition) it seems as though the "page" item is displayed for all clicks on the page, including clicks on  images, frames, &c.

Comment 1

[Kris Maglione [:kmag]]

To fix this, we just need to update the `getContexts` function to only apply the "page" context when we haven't applied any other relevant contexts:

https://dxr.mozilla.org/mozilla-central/source/browser/components/extensions/ext-contextMenus.js#219

I think the easiest way to do that would be to check `contexts.size == 0` after adding the other contexts.

For reference, this is where the `contextData` object is created, and shows what other properties are available:

https://dxr.mozilla.org/mozilla-central/rev/4d63dde701b47b8661ab7990f197b6b60e543839/browser/base/content/nsContextMenu.js#46

Comment 2

[Deepjyoti Mondal]

Attachment: modified getContexts function so that page context is applied properly.

Hi Kris, I checked contexts.size == 0 after all the other contexts were added but on doing that the page option never appeared in the context menu. I think this is because of the first statement in the getContexts function : let contexts = new Set(["all"]) . This makes the minimum size of the set 1. So I used the condition contexts.size == 1. Now the page context no more appears on clicking links,pictures or frames.

Comment 3

[Kris Maglione [:kmag]]

Comment on attachment 8767283 [details] [diff] [review]
modified getContexts function so that page context is applied properly.

Review of attachment 8767283 [details] [diff] [review]:
-----------------------------------------------------------------

Hi,

Sorry for the delay in reviewing this. I'm still catching up from my vacation.

This looks good, but we should also add a test for this to test/browser/browser_ext_contextMenus.js to make sure that this context continues to work as expected. For the most part, it should be pretty straightforward, but that test script is a bit hairy, so let me know if you'd like some pointers.

Thanks!

Comment 4

[Deepjyoti Mondal]

I am not well acquainted with writing tests, so it will be great if you can provide me with some references.

Comment 5

[vishaalkal]

What is the status of this bug? Asking because we ran into this issue on an extension I work on.

Comment 6

[Andy McKay]

Are you still able to work on this bug Deepjyoti? Can I be of assistance?

Comment 7

[Deepjyoti Mondal]

Yeah I can still work on this. The previous patch I uploaded (#8767283) seemed to have solved the problem. Could please help me with updating the test file as mentioned by Kris. Any kind of references will help.

Comment 8

[Andy McKay]

Kris is talking about the file:

https://dxr.mozilla.org/mozilla-central/source/browser/components/extensions/test/browser/browser_ext_contextMenus.js

If I run that test with your patch it all passes:

./mach test browser/components/extensions/test/browser/browser_ext_contextMenus.js
[..]
Browser Chrome Test Summary
	Passed: 58

So we should probably add a test for this case. These lines are probably the most relevant...

https://dxr.mozilla.org/mozilla-central/source/browser/components/extensions/test/browser/browser_ext_contextMenus.js#7-38

these lines create an image and then open a context menu on the body or img and check what context menu appears. I added in the example from comment 0 and changed this test to read:

      browser.contextMenus.create({
        id: "clickme-image",
        title: "Click me!",
        contexts: ["image"],
      });
      browser.contextMenus.create({
        id: "clickme-page",
        title: "Click me!",
        contexts: ["page"],
      });

And the tests failed with: 

./mach test browser/components/extensions/test/browser/browser_ext_contextMenus.js
[..]
Browser Chrome Test Summary
	Passed: 57
        Failed: 1
[..]
13 INFO Console message: [JavaScript Error: "MenuItem cannot be an ancestor (or self) of its new parent." {file: "chrome://browser/content/ext-contextMenus.js" line: 333}]
ensureValidParentId@chrome://browser/content/ext-contextMenus.js:333:15

So I'm not sure if your patch is right, or perhaps its comment 0. Once you've got that working, the next step would be to ensure that on an image, the image context menu is shown and when on the body the page context menu is shown.

That's just my quick reading of the bug, but hopefully it makes sense.

Comment 9

[Tomislav Jovanovic :zombie]

Hey Deepjyoti, are you still planning to work on this?  Do you need help with anything?

Comment 10

[Deepjyoti Mondal]

(In reply to Tomislav Jovanovic :zombie from comment #9)
> Hey Deepjyoti, are you still planning to work on this?  Do you need help
> with anything?

Sorry for being so late, I am still willing work on this bug. I will be uploading a patch soon.

Comment 11

[Deepjyoti Mondal]

Attachment: bug1274860.patch

I have updated the test file. I have ran the tests locally,all of them are passing without any error

Comment 12

[Kris Maglione [:kmag]]

Comment on attachment 8808952 [details] [diff] [review]
bug1274860.patch

Review of attachment 8808952 [details] [diff] [review]:
-----------------------------------------------------------------

r=me with the indentation fixed.

Thanks!

::: browser/components/extensions/ext-contextMenus.js
@@ +254,5 @@
>      contexts.add("audio");
>    }
>  
> +  if (contexts.size == 1) {
> +  	contexts.add("page");

Please expand the tabs in this line, so there are 4 spaces of indentation.

Comment 13

[Kris Maglione [:kmag]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=37e09460db9cbb558fee4c1daa21f80bf1977ec5

Please add the checkin-needed keyword when you've fixed the whitespace issue and are ready for this to land.

Comment 14

[Deepjyoti Mondal]

Attachment: bug1274860.patch

Comment 15

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/49d1c7361c57
Modify getContexts function so that page context is applied properly. r=kmag

Comment 16

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/49d1c7361c57