The default bug view has changed. See this FAQ.

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

RESOLVED FIXED in Thunderbird 44.0

Status

Thunderbird
Toolbars and Tabs
--
minor
RESOLVED FIXED
4 years ago
2 years ago

People

(Reporter: Jesse Ruderman, Assigned: gportioli)

Tracking

Trunk
Thunderbird 44.0

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 3 obsolete attachments)

(Reporter)

Description

4 years ago
(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

4 years ago
Happens on Win XP too.
OS: Mac OS X → All
Hardware: x86_64 → All
Version: unspecified → Trunk
(Assignee)

Comment 2

2 years ago
Created attachment 8654663 [details] [diff] [review]
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.
Attachment #8654663 - Flags: review?(aleth)

Comment 3

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

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

Forwarding.
Attachment #8654663 - Flags: review?(aleth) → review?(richard.marti)

Updated

2 years ago
Assignee: nobody → mozilla
Status: NEW → ASSIGNED
Comment on attachment 8654663 [details] [diff] [review]
Rev 1 - Event handler changed to mousedown

This looks good.

Thank you for the patch.
Attachment #8654663 - Flags: review?(richard.marti) → review+
I set checkin-needed to let it land by a good ghost after the tree is reopened.
Keywords: checkin-needed
(Assignee)

Comment 6

2 years ago
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.
I'm sorry, I'm not so experienced in such things. Aleth, aceman or Magnus, can you reply?

Comment 8

2 years ago
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".
(Assignee)

Comment 9

2 years ago
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

2 years ago
(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
(Assignee)

Comment 11

2 years ago
> 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.
Keywords: checkin-needed

Comment 12

2 years ago
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

2 years ago
(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...
(Assignee)

Comment 14

2 years ago
Created attachment 8655695 [details] [diff] [review]
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?
Attachment #8654663 - Attachment is obsolete: true
Attachment #8655695 - Flags: review?(aleth)

Comment 15

2 years ago
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.
Attachment #8655695 - Flags: review?(aleth) → ui-review?(richard.marti)
(Assignee)

Comment 16

2 years ago
Created attachment 8655745 [details] [diff] [review]
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.
Attachment #8655695 - Attachment is obsolete: true
Attachment #8655695 - Flags: ui-review?(richard.marti)
Attachment #8655745 - Flags: review?(richard.marti)
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.
Attachment #8655745 - Flags: ui-review?(richard.marti)
Attachment #8655745 - Flags: review?(richard.marti)
Attachment #8655745 - Flags: review?(mkmelin+mozilla)
Comment on attachment 8655745 [details] [diff] [review]
Rev 3 - Pointless check for event target removed

It works as described. ui-r+
Attachment #8655745 - Flags: ui-review?(richard.marti) → ui-review+

Comment 19

2 years ago
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
Attachment #8655745 - Flags: review?(mkmelin+mozilla) → review+

Updated

2 years ago
Keywords: checkin-needed

Comment 20

2 years ago
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

Updated

2 years ago
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 43.0
Depends on: 1202268

Comment 21

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

Comment 22

2 years ago
Reopening, see bug 1202268.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---

Comment 23

2 years ago
Please investigate what additional changes are needed.
(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.

Updated

2 years ago
Summary: Menu Button only opens the menu onmouseup on first use → Hamburger menu button doesn't open the menu on mouse down on first use
(Assignee)

Comment 25

2 years ago
(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.
Calendar and Chat are using the appmenu-popup class. Maybe this is the problem.
(Assignee)

Comment 27

2 years ago
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

2 years ago
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

2 years ago
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

2 years ago
(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.
(Assignee)

Comment 31

2 years ago
Created attachment 8661325 [details] [diff] [review]
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.
Attachment #8655745 - Attachment is obsolete: true
Attachment #8661325 - Flags: review?(mkmelin+mozilla)

Updated

2 years ago
Attachment #8661325 - Flags: review?(mkmelin+mozilla) → review+

Comment 32

2 years ago
Thanks!
Keywords: checkin-needed

Comment 33

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

Updated

2 years ago
Status: REOPENED → RESOLVED
Last Resolved: 2 years ago2 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: Thunderbird 43.0 → Thunderbird 44.0
You need to log in before you can comment on or make changes to this bug.