Hamburger menu breaks after force opening it during a bookmark animation

VERIFIED FIXED in Firefox 63

Status

()

defect
P1
major
VERIFIED FIXED
11 months ago
11 months ago

People

(Reporter: emilghitta, Assigned: Paolo)

Tracking

(Blocks 2 bugs, {regression})

Trunk
Firefox 63
x86_64
Windows 10
Points:
---
Dependency tree / graph
Bug Flags:
in-qa-testsuite +
in-testsuite +

Firefox Tracking Flags

(firefox-esr52 unaffected, firefox-esr60 unaffected, firefox61 unaffected, firefox62+ wontfix, firefox63+ verified)

Details

Attachments

(2 attachments)

Posted image IssueB.gif
[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.
Component: Bookmarks & History → Toolbars and Customization
Can you reproduce this with the Copy Link confirmation from the page action menu too?
Flags: needinfo?(emil.ghitta)
I didn't managed to reproduce this issue with the Copy Link confirmation.
Flags: needinfo?(emil.ghitta)
See Also: → 1483124
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.
Flags: needinfo?(paolo.mozmail)
Priority: -- → P1
(Setting to P1 since alternative and simpler STR are currently unknown, but the resulting symptom is pretty awful)
Severity: normal → major
Flags: in-qa-testsuite?
(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.
Assignee: nobody → paolo.mozmail
Status: NEW → ASSIGNED
Flags: needinfo?(paolo.mozmail)
Duplicate of this bug: 1483124
Note that I tried both "PanelUI-button" and "PanelUI-menu-button" and also with only "mousedown" type events.
(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?
(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?
(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...
Found it! Also, bug 1126772.
Blocks: 1126772
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.
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.
Attachment #9003136 - Flags: review+
Thanks for digging into this so quickly, Paolo!
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.
Blocks: 1438507
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
Flags: qe-verify+
Flags: in-testsuite+
This issue is verified fixed using Fx 63.0a1 (BuildId:20180827100129) on Windows 10 64bit.
Status: RESOLVED → VERIFIED
Flags: qe-verify+
Flags: in-qa-testsuite? → in-qa-testsuite+
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.
You need to log in before you can comment on or make changes to this bug.