Closed Bug 1285229 Opened 4 years ago Closed 4 years ago

Context menu remains displayed when repeatedly right clicking on any node from the Markup View

Categories

(DevTools :: Inspector, defect, P1)

50 Branch
All
Windows
defect

Tracking

(firefox47 unaffected, firefox48 unaffected, firefox49 unaffected, firefox50 verified)

VERIFIED FIXED
Firefox 50
Iteration:
50.3 - Jul 18
Tracking Status
firefox47 --- unaffected
firefox48 --- unaffected
firefox49 --- unaffected
firefox50 --- verified

People

(Reporter: adalucinet, Assigned: ochameau)

References

Details

(Keywords: regression, Whiteboard: [devtools-html])

Attachments

(1 file, 3 obsolete files)

[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
QA Whiteboard: [qe-dthtml]
Whiteboard: [devtools-html][triage]
Flags: qe-verify+
QA Contact: alexandra.lucinet
Blocks: 1266478
Priority: -- → P1
Whiteboard: [devtools-html][triage] → [devtools-html]
Assignee: nobody → poirot.alex
Status: NEW → ASSIGNED
Iteration: --- → 50.3 - Jul 18
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.
Attached patch patch v1 (obsolete) — Splinter Review
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!
Attachment #8769760 - Flags: review?(jdescottes)
Attached patch frames menu fix (obsolete) — Splinter Review
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...
Attachment #8769762 - Flags: review?(jdescottes)
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?
Attachment #8769760 - Flags: review?(jdescottes)
Attachment #8769762 - Flags: review?(jdescottes)
Attached patch patch v2 (obsolete) — Splinter Review
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?!
Attachment #8770120 - Flags: review?(jdescottes)
Attachment #8769760 - Attachment is obsolete: true
Attachment #8769762 - Attachment is obsolete: true
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.
Attachment #8770120 - Flags: review?(jdescottes) → review+
Attached patch patch v3Splinter Review
Without tasks!
Attachment #8770142 - Flags: review+
Attachment #8770120 - Attachment is obsolete: true
https://hg.mozilla.org/integration/fx-team/rev/31cab0ba8bbf1d88b08abeae45bcdf317645355a
Bug 1285229 - Prevent duplicated context menu in the Inspector on Windows. r=jdescottes
https://hg.mozilla.org/mozilla-central/rev/31cab0ba8bbf
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 50
Verified fixed with latest Nightly 50.0a1, under Windows 10 64-bit, Mac OS X 10.11.1 and Ubuntu 16.04 64-bit.
Status: RESOLVED → VERIFIED
Flags: qe-verify+
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.