[Affected versions]: - latest Nightly 50.0a1 [Affected platforms]: - Windows 10 64-bit [Steps to reproduce]: 1. Launch Firefox 2. Open the Inspector. 3. Repeatedly right click on any node from the Markup View. [Expected result]: Context menu is displayed once per click. [Actual result]: Context menu remains displayed at every click. [Regression range]: - Last good Nightly → 2016-06-11 - First bad Nightly → 2016-06-12 - Pushlog: https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=b6f7d0eb61b1878d3d906bd231edf225463ece3f&tochange=016e0f47e8ad66ba6eb11fe28958e3a69ef9e53d - I think this one too is a regression from bug 1266478 [Additional notes]: - Screenshot → https://i.imgur.com/iFvYyB2.png - If a different tab is selected, the context menu is still visible on that page: screenshot → https://i.imgur.com/W5eKgVs.png

A key difference here between Linux and Windows is that the context menu hides the second time you right click on linux, whereas it opens another one on Windows. Also, it looks like you have to right click in the exact same position, otherwise it closes the old and open a new one. Or at least it is more reproducible that way.

Note that this is also reproducible with the frames command button.

It looks like on Windows, when we create another <xul:menupopup> and call its openPopupAtScreen, any previously opened one won't be dismissed. Nor will it be dismissed by the right-click/context-menu event. And that, only during a race. You have to open two in a short period of time. One way to address that is to use ID on Menus and remove any existing one that isn't already hidden'n removed. Julian, I'm flagging you for review, but do not hesitate to forward to whoever makes sense!

Same for the frames button. If defining an ID is an issue, we may also automatically compute an ID for all menus from Menu's constructor if none is given...

Wouldn't it be simpler to empty the contents of the popupset everytime a popup is dismissed ? This would remove the need for an ID.

If emptying the popupset removes some existing hardcoded XUL popups, we can always create a special popupset for the Menu API.

I wasn't sure if we would ever want to be able to display more than one menu at a time? The xul API seems to allow that? But yes, we could also do that. We can remove all the elements matching: `popupset > [menu-api="true"]`.

Comment on attachment 8769760 [details] [diff] [review] patch v1 Review of attachment 8769760 [details] [diff] [review]: ----------------------------------------------------------------- Looks good, but I would prefer a global fix, so either : - ntim's proposal: hide all "menu-api" popups - your proposal: generate an id for each menu +1 comment regarding using hidePopup() instead of remove() ::: devtools/client/framework/menu.js @@ +67,5 @@ > + let popupset = doc.querySelector("popupset"); > + // See bug 1285229, on Windows, opening the same popup multiple times in a > + // row ends up duplicating the popup. The newly inserted popup doesn't > + // dismiss the old one. > + if (this.id) { I like ntim's suggestion to hide all popups automatically before opening a new one. Our menu API is very high level and offers little control: once you open a popup, you have no way to change it, to hide it. So I think it makes sense to enforce a sensible default behavior here, and to hide all menu popups when opening a new one. @@ +70,5 @@ > + // dismiss the old one. > + if (this.id) { > + let previousPopup = popupset.querySelector("#" + this.id); > + if (previousPopup) { > + previousPopup.remove(); Calling .remove() will not fire the "popuphidden", which means the menu will not fire the corresponding "close" event. I think we should call hidePopup() here (it also removes the menupopup element), and maybe wait until we receive the popuphidden event before proceeding to open the new popup?

Here is the patch with previous popup removal. I only remove the first one. Thanks to this code, we shouldn't have more than one in the popupset, right?!

Comment on attachment 8770120 [details] [diff] [review] patch v2 Review of attachment 8770120 [details] [diff] [review]: ----------------------------------------------------------------- ::: devtools/client/framework/menu.js @@ +72,5 @@ > + // opening a new one. > + let popup = popupset.querySelector("menupopup[menu-api=\"true\"]"); > + if (popup) { > + popup.hidePopup(); > + yield this.once("close"); "close" will be fired from "this" only if the popup being closed was opened by this Menu instance. Could fail if there is a popup displayed by another Menu instance, which for some reason was not dismissed. If we are waiting for an event we should wait for "popuphidden" on the popup instance. But it should actually be enough to hide the popup without waiting for any event. It will simplify the implementation and make it synchronous. This way we are sure we have at most one match for menupopup[menu-api="true"]. With an async implementation we could still run into edge cases were several popups would be created and then we would have to use querySelectorAll, hide all popups, wait for all events etc... -> let's remove the Task wrapper and the yield here.

Without tasks!

https://hg.mozilla.org/integration/fx-team/rev/31cab0ba8bbf1d88b08abeae45bcdf317645355a Bug 1285229 - Prevent duplicated context menu in the Inspector on Windows. r=jdescottes

Verified fixed with latest Nightly 50.0a1, under Windows 10 64-bit, Mac OS X 10.11.1 and Ubuntu 16.04 64-bit.