Steps To Reproduce 1. Place addon in australis menu panel 2. Right click on addon for context menu Expected Result 3. Addon context menu should appear with customization menuitems appended to list Actual Result 3. Menu panel customization menu appears

Can you give an example add-on with which we can test this? Thank you! :-)

I noticed this with the x-notifier addon.

This was my first shot. It... looks like it works? Somewhat surprisingly? Jared, what do you think?

Comment on attachment 777067 [details] [diff] [review] should modify add-on context menu, not replace it How will this behave when add-ons populate their menus on the fly when opening them?

(In reply to Dão Gottwald [:dao] from comment #6) > Comment on attachment 777067 [details] [diff] [review] > should modify add-on context menu, not replace it > > How will this behave when add-ons populate their menus on the fly when > opening them? That would depend on how they populate the menus. If the menu doesn't exist at all, we will replace the context property. If it does exist but it's empty and/or they append items, our items might end up on (or closer to) the top. I mean, as far as I'm concerned we can't really fix this perfectly, considering add-ons can pretty much do anything. Do you think I should follow a different approach? If so, what do you have in mind?

If we can't do it reliably, the best option might be to just leave external menus alone. Maybe provide some hook for add-ons to add the items themselves.

(In reply to Dão Gottwald [:dao] from comment #8) > If we can't do it reliably, the best option might be to just leave external > menus alone. Maybe provide some hook for add-ons to add the items themselves. I'm not sure we should let the perfect be the enemy of the good here. It will work well for 90% of cases, less well for the rest, but I don't see how this would completely break add-ons' context menus - and the 10% is totally fixable by the add-ons themselves, even if they add items dynamically. I think I would prefer offering our menu for the majority of such add-on icons rather than having it be an opt-in thing for *all* add-ons to have to put work into in order to "play nice" in Australis. If we can make this transition easier for most add-ons, we should take that opportunity.

Comment on attachment 777067 [details] [diff] [review] should modify add-on context menu, not replace it Review of attachment 777067 [details] [diff] [review]: ----------------------------------------------------------------- Stealing this back as I see mconley's review queue is quite worrisome. ::: browser/components/customizableui/src/CustomizableUI.jsm @@ +407,5 @@ > } > }, > > ensureButtonContextMenu: function(aNode, ourContextMenu) { > + let currentCtxt = aNode.getAttribute("context") || s/currentCtxt/currentContextMenu/ because characters are cheap and it makes reading the code a bit easier. @@ +440,5 @@ > + } > + > + let ourMenu = doc.getElementById(kCustomizationContextMenu); > + let ourSeparator = doc.createElement("menuseparator"); > + ourSeparator.id = aMenu + kCustomizeMenuSeparator; Do we need to add some randomness to this? I imagine that many add-on context menus may just use some default ID (or none at all) and thus this will give us collisions on the ID. Maybe store a random string on menuToModify that can also be used in removeCustomizeMenuitems?

(In reply to Jared Wein [:jaws] from comment #10) > @@ +440,5 @@ > > + } > > + > > + let ourMenu = doc.getElementById(kCustomizationContextMenu); > > + let ourSeparator = doc.createElement("menuseparator"); > > + ourSeparator.id = aMenu + kCustomizeMenuSeparator; > > Do we need to add some randomness to this? I imagine that many add-on > context menus may just use some default ID (or none at all) and thus this > will give us collisions on the ID. Maybe store a random string on > menuToModify that can also be used in removeCustomizeMenuitems? Funny you should suggest that, because I initially did this, but then I removed it again. The thing is, context menu attributes work by IDs of the menus that they ought to be showing. If there were multiple identical IDs, things would already be broken for such add-ons (one would have the other's context menu). Does that make sense and can I push this with just the other nit fixed? :-)

Comment on attachment 777067 [details] [diff] [review] should modify add-on context menu, not replace it (In reply to :Gijs Kruitbosch from comment #9) > (In reply to Dão Gottwald [:dao] from comment #8) > > If we can't do it reliably, the best option might be to just leave external > > menus alone. Maybe provide some hook for add-ons to add the items themselves. > > I'm not sure we should let the perfect be the enemy of the good here. It > will work well for 90% of cases, less well for the rest, but I don't see how > this would completely break add-ons' context menus It can quite easily break add-ons' context menus. For instance, a common pattern that we use ourselves is to add a command event listener to a menupopup that would do the right thing for our own items but not for items others would inject unaskedly without special care for how our menu works.

OK, so here's an alternative. Let's not override the menu at all, and add-ons will have to add the contextmenu themselves, because even when my original approach worked, it'd still easily cause issues with conflicting access keys; it's probably better to have add-ons add these menuitems themselves.

(In reply to :Gijs Kruitbosch from comment #13) > Created attachment 778377 [details] [diff] [review] > Australis menupanel contextmenu shouldn't override add-on contextmenu > > OK, so here's an alternative. Let's not override the menu at all, and > add-ons will have to add the contextmenu themselves, because even when my > original approach worked, it'd still easily cause issues with conflicting > access keys; it's probably better to have add-ons add these menuitems > themselves. I think this is a reasonable compromise because users will have a readily available method to customize and remove items using the customize button and overriding addon menu seems to complicate the code unnecessarily.

Comment on attachment 778377 [details] [diff] [review] Australis menupanel contextmenu shouldn't override add-on contextmenu > ensureButtonContextMenu: function(aNode, ourContextMenu) { >+ let currentContextMenu = aNode.getAttribute("context") || >+ aNode.getAttribute("contextmenu"); >+ if (ourContextMenu && !currentContextMenu) { > aNode.setAttribute("context", kCustomizationContextMenu); >+ } else if (currentContextMenu == kCustomizationContextMenu) { >+ aNode.removeAttribute("context"); > } > }, "ourContextMenu" is confusing. Can you rename it such that it's clearly a boolean value meant to determine whether the context menu should be added or removed? Also add the "a" prefix while you're at it. It looks like you might enter the else branch when you shouldn't. How about: if (ourContextMenu) { if (!currentContextMenu) aNode.setAttribute("context", kCustomizationContextMenu); } else ...

Characters were cheap, I was told... :-)

Comment on attachment 778398 [details] [diff] [review] Australis menupanel contextmenu shouldn't override add-on contextmenu >+ ensureButtonContextMenu: function(aNode, aShouldHaveCustomizationMenu) { >+ let currentContextMenu = aNode.getAttribute("context") || >+ aNode.getAttribute("contextmenu"); >+ if (aShouldHaveCustomizationMenu) { >+ if (!currentContextMenu) >+ aNode.setAttribute("context", kCustomizationContextMenu); >+ } else if (currentContextMenu == kCustomizationContextMenu) { >+ aNode.removeAttribute("context"); > } might be even easier to follow if you write it like this: ... } else { if (currentContextMenu == kCustomizationContextMenu) aNode.removeAttribute("context"); }

(In reply to Dão Gottwald [:dao] from comment #17) > might be even easier to follow if you write it like this: > > ... > } else { > if (currentContextMenu == kCustomizationContextMenu) > aNode.removeAttribute("context"); > } Pushed with that change made. https://hg.mozilla.org/projects/ux/rev/47d9faf1a4be