890332 - Hamburger menu button doesn't open the menu on mouse down on first use

Status: RESOLVED FIXED

(Thunderbird :: Toolbars and Tabs, defect)

Description

[Jesse Ruderman]

(Thunderbird Beta 23)

1. Hold down mouse button on the "hamburger" menu button (next to the search bar)

Expected: menu appears immediately (cf bug 890137 for firefox)

Result: depends on whether I've opened the menu before in this session.  On the first use, it only appears onmouseup.  If I've opened it before, it behaves properly.

Comment 1

[:aceman]

Happens on Win XP too.

Comment 2

[Giulio Portioli [:gportioli]]

Attachment: Rev 1 - Event handler changed to mousedown

Hi there, I'm happy to take this bug. Can someone please assign it to me?

The attached solution seems to do the trick. In summary, I've changed the event handler for the 'Hamburger button'/appmenu-vertical binding from a 'click' to a 'mousedown', and also removed the instruction

   this.open = true;

which seems wholly redundant, and indeed seems to interfere with the mousedown event, making the appmenu blink on mouse-up.

Now the appmenu correctly appears on the first mousedown after launching Thunderbird, and on all subsequent ones too, as before.

Comment 3

[aleth [:aleth]]

Comment on attachment 8654663 [details] [diff] [review]
Rev 1 - Event handler changed to mousedown

Review of attachment 8654663 [details] [diff] [review]:
-----------------------------------------------------------------

Forwarding.

Comment 4

[Richard Marti (:Paenglab)]

Comment on attachment 8654663 [details] [diff] [review]
Rev 1 - Event handler changed to mousedown

This looks good.

Thank you for the patch.

Comment 5

[Richard Marti (:Paenglab)]

I set checkin-needed to let it land by a good ghost after the tree is reopened.

Comment 6

[Giulio Portioli [:gportioli]]

Out of interest, since the Hamburger button was already responding to mousedown events (only not on the first mousedown after program launch), it must already have a mousedown event handler defined somewhere, but where? Is it done in JavaScript or C++? I searched long and hard, before realising that it didn't matter anyway for this bug; I still wonder though... Thanks for any clarification.

Comment 7

[Richard Marti (:Paenglab)]

I'm sorry, I'm not so experienced in such things. Aleth, aceman or Magnus, can you reply?

Comment 8

[aleth [:aleth]]

Comment on attachment 8654663 [details] [diff] [review]
Rev 1 - Event handler changed to mousedown

Review of attachment 8654663 [details] [diff] [review]:
-----------------------------------------------------------------

::: mail/base/content/mailWidgets.xml
@@ -2800,5 @@
>                let appmenuPopup = document.getElementById("appmenu-popup");
>                if (this.lastChild != appmenuPopup) {
>                  this.appendChild(appmenuPopup);
>                }
> -              this.open = true;

Are you sure this is redundant? I suppose this refers to
https://developer.mozilla.org/en-US/docs/Mozilla/Tech/XUL/Attribute/open
cf
https://developer.mozilla.org/en-US/docs/Mozilla/Tech/XUL/toolbarbutton#Attributes

This change may be the actual fix here, I don't see why you have to change "click" to "mousedown".

Comment 9

[Giulio Portioli [:gportioli]]

I've tested in isolation the two changes in the patch file:

1) 'this.open = true' removed but event handler left to 'click'. Result: regression - The menu does not appear at all on first click, not even on mouseup (currently at least it opens on mouseup, on first click). Subsequent clicks behave correctly: the menu opens on mousedown.

2) 'this.open = true' left there but event handler changed to 'mousedown'. Result: regression - The menu always flashes briefly on mousedown but never actually opens, with no difference between first click and subsequent ones.

So it looks as though it's the combination of the two changes that fixes this bug without causing regression (although I'm still not sure how it could work before, with the button responding to mousedown events despite having a 'click' event handler, see my comment #6).

Comment 10

[aleth [:aleth]]

(In reply to Giulio Portioli [:gportioli] from comment #9)
> I've tested in isolation the two changes in the patch file:
> 
> 1) 'this.open = true' removed but event handler left to 'click'. Result:
> regression - The menu does not appear at all on first click, not even on
> mouseup (currently at least it opens on mouseup, on first click). Subsequent
> clicks behave correctly: the menu opens on mousedown.
> 
> 2) 'this.open = true' left there but event handler changed to 'mousedown'.
> Result: regression - The menu always flashes briefly on mousedown but never
> actually opens, with no difference between first click and subsequent ones.
> 
> So it looks as though it's the combination of the two changes that fixes
> this bug without causing regression (although I'm still not sure how it
> could work before, with the button responding to mousedown events despite
> having a 'click' event handler, see my comment #6).

Thanks for the explanation.

<binding id="appmenu-vertical" extends="chrome://global/content/bindings/toolbarbutton.xml#menu-vertical"> means this XBL element extends toolbarbuttons. That means they inherit everything that isn't explicitly overridden, including a click handler. I think that answers your question?

Well, to be precise the toolbarbutton extends button, see https://dxr.mozilla.org/mozilla-central/source/toolkit/content/widgets/toolbarbutton.xml, and the button has https://dxr.mozilla.org/mozilla-central/source/toolkit/content/widgets/button.xml#144.

You could similarly dig down and look at what actually happens when open is set to true, etc.

It seems to me that an initializing method like _setupAppmenu shouldn't have to run on *every* click or keypress. 1) suggests "click" is "too late" to be able to initialize the menu before it is shown. Is there a good reason _setupAppmenu isn't just put inside a constructor? 
https://developer.mozilla.org/en-US/docs/Mozilla/Tech/XUL/Tutorial/Adding_Methods_to_XBL-defined_Elements#Constructors_and_Destructors

Comment 11

[Giulio Portioli [:gportioli]]

> an initializing method like _setupAppmenu shouldn't have to run on *every* click [...]
> Is there a good reason _setupAppmenu isn't just put inside a constructor?

That's a good point; I bet there isn't. I've cleared the checkin-needed flag, while I look into the matter.

Comment 12

[aleth [:aleth]]

Another simple possibility might be to move the appmenu element (that gets added as a childnode dynamically) inside mailWindowOverlay.xml from the popupset to being a child of the toolbar button from the start. Not sure if that breaks something else?

Comment 13

[aleth [:aleth]]

(In reply to aleth [:aleth] from comment #12)
> Another simple possibility might be to move the appmenu element (that gets
> added as a childnode dynamically) inside mailWindowOverlay.xml from the
> popupset to being a child of the toolbar button from the start. Not sure if
> that breaks something else?

Someone who knows this code better than me should check this. Or maybe it was done just so as to keep all the menupopups together...

Comment 14

[Giulio Portioli [:gportioli]]

Attachment: Rev 2 - Appending of menu moved to constructor

I've tried with this minimalistic <binding> and it seems to work just fine. Basically I've stripped the binding of method and handlers, leaving only a constructor that appends the menu to the Hamburger button. Testing it, the menu opens on first and subsequent mousedowns, with no apparent regression.
Maybe I'm throwing away some baby with the bath water?

Not sure how to test for response to key presses (assuming the previous handler was doing anything); is there a way to command the Hamburger button via keyboard?

Comment 15

[aleth [:aleth]]

Comment on attachment 8655695 [details] [diff] [review]
Rev 2 - Appending of menu moved to constructor

Review of attachment 8655695 [details] [diff] [review]:
-----------------------------------------------------------------

This looks much better.

Paenglab should know how to test the UX aspects (like keyboard interactions) better than me!

::: mail/base/content/mailWidgets.xml
@@ +2795,5 @@
>      <implementation>
> +      <constructor>
> +        <![CDATA[
> +          let appmenuPopup = document.getElementById("appmenu-popup");
> +          if (this.lastChild != appmenuPopup) {

Do you even need this check? Constructors only get called once.

Comment 16

[Giulio Portioli [:gportioli]]

Attachment: Rev 3 - Pointless check for event target removed

> Do you even need this check? Constructors only get called once.

The answer seems to be no. The attached Rev 3, stripped of that checked, works just as well as Rev 2, from what I can see.

Comment 17

[Richard Marti (:Paenglab)]

Comment on attachment 8655745 [details] [diff] [review]
Rev 3 - Pointless check for event target removed

I'll better let someone review this who knows this things better.
ui-r me to check the functioning.

Comment 18

[Richard Marti (:Paenglab)]

Comment on attachment 8655745 [details] [diff] [review]
Rev 3 - Pointless check for event target removed

It works as described. ui-r+

Comment 19

[Magnus Melin [:mkmelin]]

Comment on attachment 8655745 [details] [diff] [review]
Rev 3 - Pointless check for event target removed

Review of attachment 8655745 [details] [diff] [review]:
-----------------------------------------------------------------

Thx for the patch Giulio! r=mkmelin

Comment 20

[aleth [:aleth]]

https://hg.mozilla.org/comm-central/rev/559c6baa10398afe1e9078cd96d8b353596f8ee6
Bug 890332 - Appending of app menu to Hamburger button moved to binding's constructor. r=aleth,mkmelin

Comment 21

[aleth [:aleth]]

https://hg.mozilla.org/comm-central/rev/802cf75f479b67dd3d3db3dcf01811367dc3966e
Backed out 559c6baa1039 (bug 890332) for causing a Calendar regression (bug 1202268). a=aleth

Comment 22

[aleth [:aleth]]

Reopening, see bug 1202268.

Comment 23

[aleth [:aleth]]

Please investigate what additional changes are needed.

Comment 24

[Richard Marti (:Paenglab)]

(In reply to aleth [:aleth] from comment #21)
> Backed out 559c6baa1039 (bug 890332) for causing a Calendar regression (bug
> 1202268). a=aleth

It's not only regressing Calendar, Chat App menu too.

Comment 25

[Giulio Portioli [:gportioli]]

(In reply to aleth [:aleth] from comment #23)
> Please investigate what additional changes are needed.

OK, I'll have a look at what's going on.

Comment 26

[Richard Marti (:Paenglab)]

Calendar and Chat are using the appmenu-popup class. Maybe this is the problem.

Comment 27

[Giulio Portioli [:gportioli]]

As it turns out, Rev 1 of my patch (change of event from click to mousedown and deletion of redundant 'this.open = true' instruction) fixes the bug without breaking any other Hamburger button, despite leaving that somehow convoluted logic as it is. I'm tempted to resubmit it as Rev 4, unless there are other suggestions.

To further specify the bug itself, when other tabs are open (Calendar, Tasks or Chat), the bug actually occurs every time the tab is activated, not only the first time after launching Thunderbird, i.e:

- Launch Thunderbird.
- On the Mail tab, the first click on the Hamburger button (HB) doesn't work properly, as described in the original bug report.
- Open the Calendar tab.
- The HB behaves exactly the same as in the Mail tab (first click screwed, subsequent OK).
- Go back to the Mail tab.
- The bug occurs again: first click on the HB is screwed, subsequent are OK.
- Repeat at will, also with the Tasks and Chat tabs.

With Rev 1 patch, all of the above seem fixed: first and subsequent clicks on the HB on all tabs (and the message window too) work as expected, i.e. on mousedown.

Comment 28

[aleth [:aleth]]

It seems worth figuring out what is going on here before choosing the fix. If the current code is a bit hacky, adding another hack may just add more problems in the end.

Comment 29

[aleth [:aleth]]

Adding a dump() call to the constructor shows that it is called correctly for each button. Since the appmenu-popup element is unique, the last call "wins" and that button gets the menu.

Comment 30

[aleth [:aleth]]

(In reply to aleth [:aleth] from comment #29)
> Adding a dump() call to the constructor shows that it is called correctly
> for each button. Since the appmenu-popup element is unique, the last call
> "wins" and that button gets the menu.

If mkmelin agrees, I think that means we can go with your rev 1 patch, as long as you add a comment explaining why it is the way it is, to avoid confusion in the future.

Comment 31

[Giulio Portioli [:gportioli]]

Attachment: Rev 4 - reverted to rev 1 with additional comment

Please see attached Rev 4 patch, which is functionally identical to Rev 1, but I've expanded the comment at the bottom with my understanding of the rationale.

Comment 32

[aleth [:aleth]]

Thanks!

Comment 33

[aleth [:aleth]]

https://hg.mozilla.org/comm-central/rev/d1fc3d4e18648ee21fd51cd0ef5545ba3e95aed3
Bug 890332 - Ensure appmenu always opens correctly on mousedown. r=mkmelin