Emil Ghitta, QA [:emilghitta] Reporter
	Description • 6 years ago

[Affected versions]:
63.0a1 (BuildId:20180817100105)
62.0b18 (BuildId:20180816151750)

[Unaffected versions]: 
61.0.2 (BuildId:20180807170231)
60.1.0esr (BuildId:20180621121604)

[Affected platforms]:
Windows 10 64bit.

[Steps to reproduce]:
1. Launch Firefox.
2. Bookmark a website using the CTRL + D keyboard combination.
3. Click the "Hamburger menu" button several times as soon as the Bookmark animation starts.

[Expected result]:
The "Hamburger menu" is successfully displayed.

[Actual result]:
The "Hamburger menu" breaks.

[Regression range]:
This seems to be a regression:

Last good revision: 885ed481dee338ed5952fcbc83699ab3975f6465
First bad revision: b8939927a9b9f394e4317301913ed1635896ea03

Pushlog:
https://hg.mozilla.org/integration/autoland/pushloghtml?fromchange=885ed481dee338ed5952fcbc83699ab3975f6465&tochange=b8939927a9b9f394e4317301913ed1635896ea03

[Note]
Please note that I didn't managed to reproduce this issue on Linux or macOS.
This issue is reproducible on the "Page actions" button as well.
This is quite an Edge case scenario.
For further information regarding this issue please observe the attached screencast.

	Dão Gottwald [:dao]
	Comment 1 • 6 years ago

Can you reproduce this with the Copy Link confirmation from the page action menu too?

	Emil Ghitta, QA [:emilghitta] Reporter
	Comment 2 • 6 years ago

I didn't managed to reproduce this issue with the Copy Link confirmation.

	Mike Conley (:mconley) (:⚙️)
	Comment 3 • 6 years ago

Gijs and I just talked about this. The STR for this bug makes it sound lower priority, but we're worried that we're hitting a PanelMultiView bug here that can be triggered by other means, and we want to understand it (see bug 1483124, which might be some kind of dupe).

paolo, do you have easy access to a Windows machine? I seem to only be able to reproduce on Windows. If so, when you have a second, can you try to figure out what's happening here, assuming it's something in PanelMultiView?

I guess once we understand what's going wrong here, we can make a more informed call on how important it is.

	Mike Conley (:mconley) (:⚙️)
	Comment 4 • 6 years ago

(Setting to P1 since alternative and simpler STR are currently unknown, but the resulting symptom is pretty awful)

	:Paolo Amadini Assignee
	Comment 5 • 6 years ago

(In reply to Mike Conley (:mconley) (:⚙️) from comment #3)
> we're worried that we're hitting a PanelMultiView bug here
> that can be triggered by other means

Good call, there is a platform race condition, whose root cause I don't know, affecting PanelMultiView.

The bug is easily reproducible on Windows if you close the main menu by clicking the main menu button while another popup is displayed at the same time. The mechanism that consumes this click and prevents it from reopening the main menu doesn't work while there is another popup open, so we try to reopen the main menu immediately.

This is all supported and handled fine by PanelMultiView, which goes all the way to the new openPopup platform call. The issue is that under these conditions the openPopup call doesn't work, but doesn't raise an exception either, and the popup is still closed. Since the main view is in the "open" state but the popup is closed, the next time the main menu button is clicked and we try to open the view, we notice that the view is already open, and close its panel immediately, which is the main menu itself.

I'm fixing this by checking that the popup is actually opening after the openPopup call. If this is not the case, we raise the missing "popuphidden" event ourselves, which properly resets the state.

I tried to write a test case, but unfortunately synthesized clicks behave differently than actual clicks. Anyways, I'll post a patch where you can run the test and click on the main menu button manually when it stops:

  ./mach build faster && ./mach test browser/components/customizableui/test/browser_1484275_PanelMultiView_toggle_with_other_popup.js

I ran this manually on Windows and Mac and verified that the issue is fixed on Windows. If you have any idea on how to fix the synthesized click issue, let me know, otherwise we may want to land this with manual testing only.

I know that at some point we'll make the bookmarks star menu a PanelMultiView, and when this happens the PanelMultiView code will already make sure that two popups can't be opened at the same time, so this would make this test obsolete anyways.

	:Paolo Amadini Assignee
	Comment 8 • 6 years ago

Note that I tried both "PanelUI-button" and "PanelUI-menu-button" and also with only "mousedown" type events.

	:Gijs (he/him)
	Comment 9 • 6 years ago

(In reply to :Paolo Amadini from comment #8)
> Note that I tried both "PanelUI-button" and "PanelUI-menu-button" and also
> with only "mousedown" type events.

There used to be native click synthesis helpers on EventUtils. Right now I only see a macOS one. Could try that?

	:Paolo Amadini Assignee
	Comment 10 • 6 years ago

(In reply to :Gijs (he/him) from comment #9)
> There used to be native click synthesis helpers on EventUtils. Right now I
> only see a macOS one. Could try that?

Oh, cool, that works for Mac.

    let button = document.getElementById("PanelUI-button");
    let clickFn = () => EventUtils.synthesizeNativeOSXClick(
      button.boxObject.screenX + button.boxObject.width / 2,
      button.boxObject.screenY + button.boxObject.height / 2);

Which isn't particularly helpful since the bug is on Windows, but you mentioned maybe there used to be a version for Windows too?

	:Gijs (he/him)
	Comment 11 • 6 years ago

(In reply to :Paolo Amadini from comment #10)
> (In reply to :Gijs (he/him) from comment #9)
> > There used to be native click synthesis helpers on EventUtils. Right now I
> > only see a macOS one. Could try that?
> 
> Oh, cool, that works for Mac.
> 
>     let button = document.getElementById("PanelUI-button");
>     let clickFn = () => EventUtils.synthesizeNativeOSXClick(
>       button.boxObject.screenX + button.boxObject.width / 2,
>       button.boxObject.screenY + button.boxObject.height / 2);
> 
> Which isn't particularly helpful since the bug is on Windows, but you
> mentioned maybe there used to be a version for Windows too?

There used to be code that tested drag and dropping the new tab tiles. ttaubert wrote it. But it's possible that it used non-native mousedowns and native mousemoves (to get the drag events). Or something. I don't know off-hand - could look at the hg history of EventUtils, or try to find the old dnd tests for the old new tab implementation in hg history...

	:Paolo Amadini Assignee
	Comment 12 • 6 years ago

Found it! Also, bug 1126772.

	:Paolo Amadini Assignee
	Comment 13 • 6 years ago

The new patch works locally on Windows and Mac, and fails without the fix on Windows. I haven't tested Linux yet as I'm still downloading the most recent build prerequisites, and I've only started automation now:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=94d116fe24170e3c46792879b5690fa235e0aeb4

There may be subtle differences that require changes, but I think the general concept will be the same, so I'm already asking for the final review.

	:Gijs (he/him)
	Comment 14 • 6 years ago

Comment on attachment 9003136 [details]
Bug 1484275 - Fix opening the main menu while another popup is open on Windows. r=Gijs

:Gijs (he/him) has approved the revision.

	:Gijs (he/him)
	Comment 15 • 6 years ago

Thanks for digging into this so quickly, Paolo!

	:Paolo Amadini Assignee
	Comment 16 • 6 years ago

https://treeherder.mozilla.org/#/jobs?repo=try&revision=a55685558922482fcd06726fe64da9a45c4e8cfe

	:Paolo Amadini Assignee
	Comment 17 • 6 years ago

Also relevant for the event ordering changes is bug 1438507. Neither the old nor the new order is the one we want eventually, but the new one is probably closer.

	Pulsebot
	Comment 18 • 6 years ago

Pushed by paolo.mozmail@amadzone.org:
https://hg.mozilla.org/integration/mozilla-inbound/rev/abd29769ce45
Fix opening the main menu while another popup is open on Windows. r=Gijs

	Pulsebot
	Comment 19 • 6 years ago

Pushed by paolo.mozmail@amadzone.org:
https://hg.mozilla.org/integration/mozilla-inbound/rev/3eff5274ee78
ESLint quick fix. r=me

	Pulsebot
	Comment 20 • 6 years ago

Pushed by paolo.mozmail@amadzone.org:
https://hg.mozilla.org/integration/mozilla-inbound/rev/81d946cdc170
ESLint quick fix, again. r=me

	Tiberius Oros[:tiberius_oros]
	Comment 21 • 6 years ago
bugherder

https://hg.mozilla.org/mozilla-central/rev/abd29769ce45
https://hg.mozilla.org/mozilla-central/rev/3eff5274ee78
https://hg.mozilla.org/mozilla-central/rev/81d946cdc170

	Emil Ghitta, QA [:emilghitta] Reporter
	Comment 22 • 6 years ago

This issue is verified fixed using Fx 63.0a1 (BuildId:20180827100129) on Windows 10 64bit.

	Ryan VanderMeulen [:RyanVM]
	Comment 23 • 6 years ago

Per IRC discussion, this patch makes changes to event ordering which would benefit from some proper bake time on Beta. Let's let this fix ride 63.