Closed
Bug 1250685
Opened 8 years ago
Closed 8 years ago
contextMenus is strange when context doesn't apply
Categories
(WebExtensions :: Untriaged, defect)
WebExtensions
Untriaged
Tracking
(firefox48 fixed)
RESOLVED
FIXED
mozilla48
Tracking | Status | |
---|---|---|
firefox48 | --- | fixed |
People
(Reporter: wbamberg, Assigned: jdj, Mentored)
References
Details
(Whiteboard: [contextMenus][good first bug] triaged)
Attachments
(3 files)
I've attached a WebExtension that creates a context menu item with: chrome.contextMenus.create({ id: "clickme", title: "Click me!", contexts: ["image"] }); If the context does not apply, the context menu gets an item labeled with the extension ID, and an arrow that does nothing. See the other attachment.
Reporter | ||
Comment 1•8 years ago
|
||
Comment 2•8 years ago
|
||
context menue should work
Flags: blocking-webextensions?
Whiteboard: [ContextMenu] triaged
Comment 3•8 years ago
|
||
We do actually try to handle this: https://dxr.mozilla.org/mozilla-central/source/browser/components/extensions/ext-contextMenus.js#42 The problem is that we're checking `rootElement.childNodes.length`, which is always `1`, because actual child is a <menupopup> element, where the actual child elements live. We just need to check `rootElement.firstChild.childNodes.length` instead, and then add a test.
Mentor: kmaglione+bmo
Whiteboard: [ContextMenu] triaged → [contextMenus][good first bug] triaged
Assignee | ||
Comment 4•8 years ago
|
||
Hey everyone, I would like to fix the bug, someone could assign me to this task? Cheers!
Updated•8 years ago
|
Flags: blocking-webextensions? → blocking-webextensions+
Comment 6•8 years ago
|
||
How are you coming along with this? Do you have any questions, or would you like any advice?
Flags: needinfo?(jdinartejesus)
Assignee | ||
Comment 7•8 years ago
|
||
(In reply to Kris Maglione [:kmag] from comment #6) > How are you coming along with this? Do you have any questions, or would you > like any advice? [:kmag] I already fix, is just missing the tests. Test is what i'm struggling a little bit. But was waiting for a help from [:johannh]
Flags: needinfo?(jdinartejesus)
Assignee | ||
Comment 8•8 years ago
|
||
(In reply to Kris Maglione [:kmag] from comment #6) > How are you coming along with this? Do you have any questions, or would you > like any advice? [:kmag] I already fix, is just missing the tests. Test is what i'm struggling a little bit. But was waiting for a help from [:johannh]
Assignee | ||
Comment 9•8 years ago
|
||
Assignee | ||
Updated•8 years ago
|
Attachment #8741497 -
Flags: review?(kmaglione+bmo)
Comment 10•8 years ago
|
||
Comment on attachment 8741497 [details] [diff] [review] Fixed bug contextMenu for when the context doesn't apply and still show empty options Review of attachment 8741497 [details] [diff] [review]: ----------------------------------------------------------------- Excellent, thanks! Can you just change the commit summary so it's a little clearer about what this fixes?
Attachment #8741497 -
Flags: review?(kmaglione+bmo) → review+
Assignee | ||
Updated•8 years ago
|
Attachment #8741497 -
Attachment description: Fix extension context menu for images → Fix bug on extension context menu
Assignee | ||
Updated•8 years ago
|
Attachment #8741497 -
Attachment description: Fix bug on extension context menu → Fixed bug contextMenu for when the context doesn't apply and still show empty options
Attachment #8741497 -
Flags: review+ → review?(kmaglione+bmo)
Comment 11•8 years ago
|
||
Comment on attachment 8741497 [details] [diff] [review] Fixed bug contextMenu for when the context doesn't apply and still show empty options Review of attachment 8741497 [details] [diff] [review]: ----------------------------------------------------------------- If this is ready to land, please add the checkin-needed keyword Thanks!
Attachment #8741497 -
Flags: review?(kmaglione+bmo) → review+
Assignee | ||
Updated•8 years ago
|
Attachment #8741497 -
Flags: review+ → checkin+
Updated•8 years ago
|
Attachment #8741497 -
Flags: checkin+
Comment 12•8 years ago
|
||
jdinartejesus, the checkin+ flag would typically indicate that the patch has already been checked in, and checkin? would indicate that you're asking for it to be checked in. But, the per-attachment flag isn't used so much these days, we use the checkin-needed keyword on the bug itself. I've set it for you. Thanks for the contribution, I look forward to seeing more :-)
Keywords: checkin-needed
Assignee | ||
Comment 13•8 years ago
|
||
[:aswan] Thanks for adding the keyword. Always learning :D
Updated•8 years ago
|
Attachment #8741497 -
Flags: review+
Comment 14•8 years ago
|
||
https://hg.mozilla.org/integration/fx-team/rev/234126a918c8
Keywords: checkin-needed
Comment 15•8 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/234126a918c8
Status: NEW → RESOLVED
Closed: 8 years ago
status-firefox48:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla48
Updated•6 years ago
|
Product: Toolkit → WebExtensions
You need to log in
before you can comment on or make changes to this bug.
Description
•