Closed
Bug 1274860
Opened 5 years ago
Closed 5 years ago
ContextType "page" is not applied correctly
Categories
(WebExtensions :: Untriaged, defect, P2)
WebExtensions
Untriaged
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)
5.85 KB,
application/zip
|
Details | |
2.01 KB,
patch
|
Details | Diff | Splinter Review |
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.
Updated•5 years ago
|
Whiteboard: [contextMenus]
Updated•5 years ago
|
Priority: -- → P2
Whiteboard: [contextMenus] → [contextMenus] good first bug, triaged
Updated•5 years ago
|
Mentor: kmaglione+bmo
Whiteboard: [contextMenus] good first bug, triaged → [contextMenus][good first bug][triaged]
Comment 1•5 years ago
|
||
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
Assignee | ||
Comment 2•5 years ago
|
||
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 3•5 years ago
|
||
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)
Assignee | ||
Comment 4•5 years ago
|
||
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)
Updated•5 years ago
|
Keywords: good-first-bug
Whiteboard: [contextMenus][good first bug][triaged] → [contextMenus][triaged]
Comment 5•5 years ago
|
||
What is the status of this bug? Asking because we ran into this issue on an extension I work on.
Comment 6•5 years ago
|
||
Are you still able to work on this bug Deepjyoti? Can I be of assistance?
Flags: needinfo?(djmdeveloper060796)
Assignee | ||
Comment 7•5 years ago
|
||
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)
Comment 8•5 years ago
|
||
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)
Comment 9•5 years ago
|
||
Hey Deepjyoti, are you still planning to work on this? Do you need help with anything?
Flags: needinfo?(djmdeveloper060796)
Assignee | ||
Comment 10•5 years ago
|
||
(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)
Assignee | ||
Comment 11•5 years ago
|
||
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 12•5 years ago
|
||
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+
Comment 13•5 years ago
|
||
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)
Assignee | ||
Comment 14•5 years ago
|
||
Attachment #8808952 -
Attachment is obsolete: true
Assignee | ||
Updated•5 years ago
|
Keywords: checkin-needed
Whiteboard: [contextMenus][triaged] → [contextMenus][triaged
Comment 15•5 years ago
|
||
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
Comment 16•5 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/49d1c7361c57
Status: NEW → RESOLVED
Closed: 5 years ago
status-firefox52:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla52
Updated•3 years ago
|
Product: Toolkit → WebExtensions
You need to log in
before you can comment on or make changes to this bug.
Description
•