Closed Bug 1250685 Opened 4 years ago Closed 4 years ago

contextMenus is strange when context doesn't apply

Categories

(WebExtensions :: Untriaged, defect)

defect
Not set

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)

Attached file context-menu.xpi
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.
Attached image context-menu.png
context menue should work
Flags: blocking-webextensions?
Whiteboard: [ContextMenu] triaged
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
Hey everyone,

I would like to fix the bug, someone could assign me to this task?

Cheers!
Have fun :)
Assignee: nobody → jdinartejesus
Flags: blocking-webextensions? → blocking-webextensions+
How are you coming along with this? Do you have any questions, or would you like any advice?
Flags: needinfo?(jdinartejesus)
(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)
(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]
Attachment #8741497 - Flags: review?(kmaglione+bmo)
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+
Attachment #8741497 - Attachment description: Fix extension context menu for images → Fix bug on extension context menu
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 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+
Attachment #8741497 - Flags: review+ → checkin+
Attachment #8741497 - Flags: checkin+
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
[:aswan] Thanks for adding the keyword. Always learning :D
Attachment #8741497 - Flags: review+
https://hg.mozilla.org/mozilla-central/rev/234126a918c8
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla48
Product: Toolkit → WebExtensions
You need to log in before you can comment on or make changes to this bug.