Closed Bug 1274860 Opened 3 years ago Closed 3 years ago

ContextType "page" is not applied correctly

Categories

(WebExtensions :: Untriaged, defect, P2)

defect

Tracking

(firefox52 fixed)

RESOLVED FIXED
mozilla52
Tracking Status
firefox52 --- fixed

People

(Reporter: wbamberg, Assigned: djmdeveloper060796, Mentored)

Details

(Keywords: good-first-bug, Whiteboard: [contextMenus][triaged)

Attachments

(2 files, 2 obsolete files)

Attached file 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.
Whiteboard: [contextMenus]
Priority: -- → P2
Whiteboard: [contextMenus] → [contextMenus] good first bug, triaged
Mentor: kmaglione+bmo
Whiteboard: [contextMenus] good first bug, triaged → [contextMenus][good first bug][triaged]
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
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.
Attachment #8767283 - Flags: review?(kmaglione+bmo)
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!
Attachment #8767283 - Flags: review?(kmaglione+bmo)
I am not well acquainted with writing tests, so it will be great if you can provide me with some references.
Flags: needinfo?(kmaglione+bmo)
Keywords: good-first-bug
Whiteboard: [contextMenus][good first bug][triaged] → [contextMenus][triaged]
What is the status of this bug? Asking because we ran into this issue on an extension I work on.
Are you still able to work on this bug Deepjyoti? Can I be of assistance?
Flags: needinfo?(djmdeveloper060796)
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.
Flags: needinfo?(djmdeveloper060796) → needinfo?(amckay)
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.
Flags: needinfo?(amckay)
Hey Deepjyoti, are you still planning to work on this?  Do you need help with anything?
Flags: needinfo?(djmdeveloper060796)
(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.
Flags: needinfo?(djmdeveloper060796)
Attached patch bug1274860.patch (obsolete) — Splinter Review
I have updated the test file. I have ran the tests locally,all of them are passing without any error
Attachment #8767283 - Attachment is obsolete: true
Attachment #8808952 - Flags: review?(kmaglione+bmo)
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.
Attachment #8808952 - Flags: review?(kmaglione+bmo) → review+
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.
Assignee: nobody → djmdeveloper060796
Flags: needinfo?(kmaglione+bmo)
Attached patch bug1274860.patchSplinter Review
Attachment #8808952 - Attachment is obsolete: true
Keywords: checkin-needed
Whiteboard: [contextMenus][triaged] → [contextMenus][triaged
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/49d1c7361c57
Modify getContexts function so that page context is applied properly. r=kmag
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/49d1c7361c57
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla52
Product: Toolkit → WebExtensions
You need to log in before you can comment on or make changes to this bug.