Closed
Bug 1285229
Opened 8 years ago
Closed 8 years ago
Context menu remains displayed when repeatedly right clicking on any node from the Markup View
Categories
(DevTools :: Inspector, defect, P1)
Tracking
(firefox47 unaffected, firefox48 unaffected, firefox49 unaffected, firefox50 verified)
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)
1.84 KB,
patch
|
ochameau
:
review+
|
Details | Diff | Splinter Review |
[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
Reporter | ||
Updated•8 years ago
|
QA Whiteboard: [qe-dthtml]
Whiteboard: [devtools-html][triage]
Updated•8 years ago
|
Updated•8 years ago
|
Priority: -- → P1
Whiteboard: [devtools-html][triage] → [devtools-html]
Updated•8 years ago
|
Assignee: nobody → poirot.alex
Status: NEW → ASSIGNED
Iteration: --- → 50.3 - Jul 18
Assignee | ||
Comment 1•8 years ago
|
||
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.
Comment 2•8 years ago
|
||
Note that this is also reproducible with the frames command button.
Assignee | ||
Comment 3•8 years ago
|
||
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)
Assignee | ||
Comment 4•8 years ago
|
||
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)
Assignee | ||
Comment 5•8 years ago
|
||
Comment 6•8 years ago
|
||
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.
Comment 7•8 years ago
|
||
If emptying the popupset removes some existing hardcoded XUL popups, we can always create a special popupset for the Menu API.
Assignee | ||
Comment 8•8 years ago
|
||
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 9•8 years ago
|
||
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)
Updated•8 years ago
|
Attachment #8769762 -
Flags: review?(jdescottes)
Assignee | ||
Comment 10•8 years ago
|
||
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)
Assignee | ||
Updated•8 years ago
|
Attachment #8769760 -
Attachment is obsolete: true
Attachment #8769762 -
Attachment is obsolete: true
Assignee | ||
Comment 11•8 years ago
|
||
Comment 12•8 years ago
|
||
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+
Assignee | ||
Updated•8 years ago
|
Attachment #8770120 -
Attachment is obsolete: true
Assignee | ||
Comment 14•8 years ago
|
||
Assignee | ||
Comment 15•8 years ago
|
||
https://hg.mozilla.org/integration/fx-team/rev/31cab0ba8bbf1d88b08abeae45bcdf317645355a
Bug 1285229 - Prevent duplicated context menu in the Inspector on Windows. r=jdescottes
Comment 16•8 years ago
|
||
bugherder |
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 50
Reporter | ||
Comment 17•8 years ago
|
||
Verified fixed with latest Nightly 50.0a1, under Windows 10 64-bit, Mac OS X 10.11.1 and Ubuntu 16.04 64-bit.
Updated•6 years ago
|
Product: Firefox → DevTools
You need to log in
before you can comment on or make changes to this bug.
Description
•