contextMenus is strange when context doesn't apply

RESOLVED FIXED in Firefox 48

Status

()

Toolkit
WebExtensions: Untriaged
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: wbamberg, Assigned: jdj, Mentored)

Tracking

unspecified
mozilla48
Points:
---
Bug Flags:
blocking-webextensions +

Firefox Tracking Flags

(firefox48 fixed)

Details

(Whiteboard: [contextMenus][good first bug] triaged)

Attachments

(3 attachments)

(Reporter)

Description

2 years ago
Created attachment 8722701 [details]
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.
(Reporter)

Comment 1

2 years ago
Created attachment 8722702 [details]
context-menu.png

Comment 2

2 years ago
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@mozilla.com
Whiteboard: [ContextMenu] triaged → [contextMenus][good first bug] triaged
(Assignee)

Comment 4

2 years ago
Hey everyone,

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

Cheers!
Have fun :)
Assignee: nobody → jdinartejesus

Updated

2 years ago
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)
(Assignee)

Comment 7

2 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

2 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

2 years ago
Created attachment 8741497 [details] [diff] [review]
Fixed bug contextMenu for when the context doesn't apply and still show empty options
(Assignee)

Updated

2 years ago
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+
(Assignee)

Updated

2 years ago
Attachment #8741497 - Attachment description: Fix extension context menu for images → Fix bug on extension context menu
(Assignee)

Updated

2 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 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

2 years ago
Attachment #8741497 - Flags: review+ → checkin+

Updated

2 years ago
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
(Assignee)

Comment 13

2 years ago
[:aswan] Thanks for adding the keyword. Always learning :D
Attachment #8741497 - Flags: review+

Comment 14

2 years ago
https://hg.mozilla.org/integration/fx-team/rev/234126a918c8
Keywords: checkin-needed

Comment 15

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/234126a918c8
Status: NEW → RESOLVED
Last Resolved: 2 years ago
status-firefox48: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla48
Depends on: 1270099
You need to log in before you can comment on or make changes to this bug.