1407591 - Hamburger Menu breaks if closing it during mid-transition

Status: RESOLVED WORKSFORME

(Firefox :: Toolbars and Customization, defect)

Description

[Emil Ghitta, QA [:emilghitta]]

[Affected versions]:
Firefox 57.0b6 
Firefox 58.0a1 

[Affected platforms]:
- Ubuntu 16.04 x64

[Unaffected platforms]:
- Windows 10 64bit
- macOS 10.11.6

[Steps to reproduce]:
1. Start Firefox.
2. Open the Hamburger menu.
3. Click the "Help" button.
4. Press the "Esc" keyboard button right after the subview starts sliding in.
5. Reopen the hamburger menu.
6. Repeat steps 2-5 once more.

[Expected result]:
The Hamburger menu opens without issues.

[Actual result]:
The Hamburger menu is displayed white with no elements inside the panel.

[Regression range]:
This is a recent regression:
Last good revision: af23e7b6eb9395aecf2a255316da782ca618750a
First bad revision: 35acf942df34d075d1a181db6feeec0a9904f27b

Pushlog:
https://hg.mozilla.org/integration/autoland/pushloghtml?fromchange=af23e7b6eb9395aecf2a255316da782ca618750a&tochange=35acf942df34d075d1a181db6feeec0a9904f27b

Looks like the following bug has the  changes which introduced the regression:
https://bugzilla.mozilla.org/show_bug.cgi?id=1401991

[Additional notes]:
For more information regarding this issue please observe the following screencast: https://drive.google.com/open?id=0BzOkHBmdnAoaNEpNaVpKdGE3Vjg

It is possible that you may have to repeat steps 2-5 a couple of times in order to reproduce this issue.

Comment 1

[Emil Ghitta, QA [:emilghitta]]

Hi Mike!

It seems that the fix for Bug 1401991 may have cause this regression as well.

Can you have a look into this?

Thanks!

Comment 2

[Mike de Boer [:mikedeboer]]

Yes, on it.

Comment 3

[Mike de Boer [:mikedeboer]]

This basically tells me something must be eerily wrong with panels on Linux and might be really hard to figure out...

Comment 4

[Mike de Boer [:mikedeboer]]

Attachment: Bug 1407591 - Listen for the 'transitioncancel' event as well to ensure that a panel transition is also cleaned up properly in this rare case.

Review commit: https://reviewboard.mozilla.org/r/188346/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/188346/

Comment 5

[Mike de Boer [:mikedeboer]]

Comment on attachment 8917339 [details]
Bug 1407591 - Listen for the 'transitioncancel' event as well to ensure that a panel transition is also cleaned up properly in this rare case.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/188346/diff/1-2/

Comment 6

[:Paolo Amadini]

Comment on attachment 8917339 [details]
Bug 1407591 - Listen for the 'transitioncancel' event as well to ensure that a panel transition is also cleaned up properly in this rare case.

https://reviewboard.mozilla.org/r/188346/#review193568

::: browser/components/customizableui/PanelMultiView.jsm:745
(Diff revision 2)
>            return;
> -        this._viewContainer.removeEventListener("transitionend", details.listener);
> -        delete details.listener;
>          resolve();
>        });
> +      this._viewContainer.addEventListener("transitioncancel", resolve);

We probably still have to check the target of the event, right?

Comment 7

[Mike de Boer [:mikedeboer]]

(In reply to :Paolo Amadini from comment #6)
> We probably still have to check the target of the event, right?

We can do that, but transitions are canceled under rare conditions, so to resolve & cleanup right away seems right to me.

Comment 8

[Mike de Boer [:mikedeboer]]

n-i regarding my reply in comment 7.

Comment 9

[:Paolo Amadini]

(In reply to Mike de Boer [:mikedeboer] from comment #7)
> (In reply to :Paolo Amadini from comment #6)
> > We probably still have to check the target of the event, right?
> 
> We can do that, but transitions are canceled under rare conditions, so to
> resolve & cleanup right away seems right to me.

I think extensions can show Photon subviews, and they can raise arbitrary events that would likely bubble up, so checking the target would probably be more correct.

That said, I admit that it may be a very rare case, so if you think this should still respond to the event from any subview element, please add a comment to that effect so people reading the code can figure out why things are done this way, and maybe figure out what's going on if some new code triggers a "transitioncancel" event on another element inside the menu while it is opening.

Comment 10

[:Paolo Amadini]

Comment on attachment 8917339 [details]
Bug 1407591 - Listen for the 'transitioncancel' event as well to ensure that a panel transition is also cleaned up properly in this rare case.

https://reviewboard.mozilla.org/r/188346/#review193986

This looks good with comment 9 addressed, although I'm not familiar with the latest changes to this code. I'd have recommended a second review from Gijs if he wasn't away, but as things stand, just make sure all the panels and not only the main one get tested extensively in this scenario, if you plan to uplift the fix.

Comment 11

[Mike de Boer [:mikedeboer]]

Comment on attachment 8917339 [details]
Bug 1407591 - Listen for the 'transitioncancel' event as well to ensure that a panel transition is also cleaned up properly in this rare case.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/188346/diff/2-3/

Comment 12

[Mike de Boer [:mikedeboer]]

I ended up following your advice and added a separate listener with a target check. Thanks!

Comment 13

[Mike de Boer [:mikedeboer]]

Comment on attachment 8917339 [details]
Bug 1407591 - Listen for the 'transitioncancel' event as well to ensure that a panel transition is also cleaned up properly in this rare case.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/188346/diff/3-4/

Comment 14

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/44d7a43c481a
Listen for the 'transitioncancel' event as well to ensure that a panel transition is also cleaned up properly in this rare case. r=Paolo

Comment 15

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/44d7a43c481a

Comment 16

[Emil Ghitta, QA [:emilghitta]]

Hi Mike!

Unfortunately this issue is still reproducible on Firefox 58.0a1 (BuildId:20171015220106) using Ubuntu 16.04 64bit.

Comment 17

[Mike de Boer [:mikedeboer]]

Well, in that case, feel free to reopen!

Comment 18

[Jim Mathies [:jimm]]

P1, assigned to you Mike, and tracking 57. Should we continue to track?

Comment 19

[Mike de Boer [:mikedeboer]]

Well, this is rare and hard enough to trigger, that it's OK to not have a fix for this in 57. So 57 = wontfix.

Comment 20

[Mike de Boer [:mikedeboer]]

In its current shape, I can't reproduce getting the Hamburger menu to break when closing it mid-transition.
I think I might have to try it on my desktop system using a different keyboard. Or something.

Comment 21

[:Gijs (he/him)]

Is this still reproducible on current trunk? I expect this may have gotten fixed via bug 1400259.

Comment 22

[Emil Ghitta, QA [:emilghitta]]

I can still reproduce this issue on Fx 58.0a1 (BuildId:20171108221316).

Comment 23

[Emil Ghitta, QA [:emilghitta]]

Ups.

It seems that I may have checked this with the wrong build.

This issue is not reproducible anymore on the latest Fx 58 on Ubuntu 16.04 64bit.

Sorry for the confusions created. My bad.

Comment 24

[:Gijs (he/him)]

Closing this out per comment #23. Thanks!