1250685 - contextMenus is strange when context doesn't apply

Status: RESOLVED FIXED

(WebExtensions :: Untriaged, defect)

Description

[Will Bamberg [:wbamberg]]

Attachment: 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.

Comment 1

[Will Bamberg [:wbamberg]]

Attachment: context-menu.png

Comment 2

[:shell escalante]

context menue should work

Comment 3

[Kris Maglione [:kmag]]

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.

Comment 4

[Dinarte Jesus [:jdj]]

Hey everyone,

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

Cheers!

Comment 5

[Johann Hofmann [:johannh]]

Have fun :)

Comment 6

[Kris Maglione [:kmag]]

How are you coming along with this? Do you have any questions, or would you like any advice?

Comment 7

[Dinarte Jesus [:jdj]]

(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]

Comment 8

[Dinarte Jesus [:jdj]]

(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]

Comment 9

[Dinarte Jesus [:jdj]]

Attachment: Fixed bug contextMenu for when the context doesn't apply and still show empty options

Comment 10

[Kris Maglione [:kmag]]

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?

Comment 11

[Kris Maglione [:kmag]]

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!

Comment 12

[Andrew Swan [:aswan]]

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 :-)

Comment 13

[Dinarte Jesus [:jdj]]

[:aswan] Thanks for adding the keyword. Always learning :D

Comment 14

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/234126a918c8

Comment 15

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/234126a918c8