Closed
Bug 1484275
Opened 6 years ago
Closed 6 years ago
Hamburger menu breaks after force opening it during a bookmark animation
Categories
(Firefox :: Toolbars and Customization, defect, P1)
Tracking
()
VERIFIED
FIXED
Firefox 63
Tracking | Status | |
---|---|---|
firefox-esr52 | --- | unaffected |
firefox-esr60 | --- | unaffected |
firefox61 | --- | unaffected |
firefox62 | + | wontfix |
firefox63 | + | verified |
People
(Reporter: emilghitta, Assigned: Paolo)
References
(Blocks 2 open bugs)
Details
(Keywords: regression)
Attachments
(2 files)
[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.
Updated•6 years ago
|
Component: Bookmarks & History → Toolbars and Customization
Comment 1•6 years ago
|
||
Can you reproduce this with the Copy Link confirmation from the page action menu too?
Flags: needinfo?(emil.ghitta)
Reporter | ||
Comment 2•6 years ago
|
||
I didn't managed to reproduce this issue with the Copy Link confirmation.
Flags: needinfo?(emil.ghitta)
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.
Flags: needinfo?(paolo.mozmail)
Priority: -- → P1
Comment 4•6 years ago
|
||
(Setting to P1 since alternative and simpler STR are currently unknown, but the resulting symptom is pretty awful)
Updated•6 years ago
|
Severity: normal → major
Flags: in-qa-testsuite?
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.
Assignee: nobody → paolo.mozmail
Status: NEW → ASSIGNED
Flags: needinfo?(paolo.mozmail)
Assignee | ||
Comment 6•6 years ago
|
||
Assignee | ||
Comment 8•6 years ago
|
||
Note that I tried both "PanelUI-button" and "PanelUI-menu-button" and also with only "mousedown" type events.
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?
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?
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...
Updated•6 years ago
|
tracking-firefox62:
--- → ?
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.
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.
Attachment #9003136 -
Flags: review+
Comment 15•6 years ago
|
||
Thanks for digging into this so quickly, Paolo!
Updated•6 years ago
|
Assignee | ||
Comment 16•6 years ago
|
||
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.
Blocks: 1438507
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
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
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
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
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 63
Updated•6 years ago
|
Flags: qe-verify+
Updated•6 years ago
|
Flags: in-testsuite+
Reporter | ||
Comment 22•6 years ago
|
||
This issue is verified fixed using Fx 63.0a1 (BuildId:20180827100129) on Windows 10 64bit.
Updated•6 years ago
|
Flags: in-qa-testsuite? → in-qa-testsuite+
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.
You need to log in
before you can comment on or make changes to this bug.
Description
•