Last Comment Bug 890332 - Hamburger menu button doesn't open the menu on mouse down on first use
: Hamburger menu button doesn't open the menu on mouse down on first use
Status: RESOLVED FIXED
:
Product: Thunderbird
Classification: Client Software
Component: Toolbars and Tabs (show other bugs)
: Trunk
: All All
-- minor (vote)
: Thunderbird 44.0
Assigned To: Giulio Portioli [:gportioli]
:
:
Mentors:
Depends on: 1202268
Blocks:
  Show dependency treegraph
 
Reported: 2013-07-04 09:07 PDT by Jesse Ruderman
Modified: 2015-09-21 15:03 PDT (History)
5 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
Rev 1 - Event handler changed to mousedown (1.43 KB, patch)
2015-08-30 15:17 PDT, Giulio Portioli [:gportioli]
richard.marti: review+
Details | Diff | Splinter Review
Rev 2 - Appending of menu moved to constructor (1.80 KB, patch)
2015-09-01 15:59 PDT, Giulio Portioli [:gportioli]
no flags Details | Diff | Splinter Review
Rev 3 - Pointless check for event target removed (1.75 KB, patch)
2015-09-01 17:28 PDT, Giulio Portioli [:gportioli]
mkmelin+mozilla: review+
richard.marti: ui‑review+
Details | Diff | Splinter Review
Rev 4 - reverted to rev 1 with additional comment (1.87 KB, patch)
2015-09-15 09:43 PDT, Giulio Portioli [:gportioli]
mkmelin+mozilla: review+
Details | Diff | Splinter Review

Description User image Jesse Ruderman 2013-07-04 09:07:17 PDT
(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 User image :aceman 2013-07-08 02:37:54 PDT
Happens on Win XP too.
Comment 2 User image Giulio Portioli [:gportioli] 2015-08-30 15:17:45 PDT
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.
Comment 3 User image aleth [:aleth] 2015-08-30 15:36:36 PDT
Comment on attachment 8654663 [details] [diff] [review]
Rev 1 - Event handler changed to mousedown

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

Forwarding.
Comment 4 User image Richard Marti (:Paenglab) 2015-08-31 09:22:12 PDT
Comment on attachment 8654663 [details] [diff] [review]
Rev 1 - Event handler changed to mousedown

This looks good.

Thank you for the patch.
Comment 5 User image Richard Marti (:Paenglab) 2015-08-31 09:23:45 PDT
I set checkin-needed to let it land by a good ghost after the tree is reopened.
Comment 6 User image Giulio Portioli [:gportioli] 2015-08-31 09:52:53 PDT
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 User image Richard Marti (:Paenglab) 2015-08-31 10:31:02 PDT
I'm sorry, I'm not so experienced in such things. Aleth, aceman or Magnus, can you reply?
Comment 8 User image aleth [:aleth] 2015-08-31 10:39:54 PDT
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 User image Giulio Portioli [:gportioli] 2015-08-31 12:34:11 PDT
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 User image aleth [:aleth] 2015-08-31 14:56:22 PDT
(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 User image Giulio Portioli [:gportioli] 2015-08-31 15:19:17 PDT
> 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 User image aleth [:aleth] 2015-08-31 15:24:05 PDT
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 User image aleth [:aleth] 2015-08-31 15:39:12 PDT
(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 User image Giulio Portioli [:gportioli] 2015-09-01 15:59:13 PDT
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?
Comment 15 User image aleth [:aleth] 2015-09-01 16:02:54 PDT
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 User image Giulio Portioli [:gportioli] 2015-09-01 17:28:31 PDT
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.
Comment 17 User image Richard Marti (:Paenglab) 2015-09-02 00:59:04 PDT
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 User image Richard Marti (:Paenglab) 2015-09-02 10:16:51 PDT
Comment on attachment 8655745 [details] [diff] [review]
Rev 3 - Pointless check for event target removed

It works as described. ui-r+
Comment 19 User image Magnus Melin 2015-09-02 13:11:37 PDT
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 User image aleth [:aleth] 2015-09-04 13:09:00 PDT
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 User image aleth [:aleth] 2015-09-06 11:07:41 PDT
https://hg.mozilla.org/comm-central/rev/802cf75f479b67dd3d3db3dcf01811367dc3966e
Backed out 559c6baa1039 (bug 890332) for causing a Calendar regression (bug 1202268). a=aleth
Comment 22 User image aleth [:aleth] 2015-09-06 11:08:29 PDT
Reopening, see bug 1202268.
Comment 23 User image aleth [:aleth] 2015-09-06 11:09:59 PDT
Please investigate what additional changes are needed.
Comment 24 User image Richard Marti (:Paenglab) 2015-09-06 11:11:25 PDT
(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 User image Giulio Portioli [:gportioli] 2015-09-07 02:36:03 PDT
(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 User image Richard Marti (:Paenglab) 2015-09-07 03:04:19 PDT
Calendar and Chat are using the appmenu-popup class. Maybe this is the problem.
Comment 27 User image Giulio Portioli [:gportioli] 2015-09-13 14:55:55 PDT
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 User image aleth [:aleth] 2015-09-14 08:36:08 PDT
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 User image aleth [:aleth] 2015-09-14 08:42:55 PDT
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 User image aleth [:aleth] 2015-09-14 09:10:16 PDT
(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 User image Giulio Portioli [:gportioli] 2015-09-15 09:43:31 PDT
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.
Comment 32 User image aleth [:aleth] 2015-09-20 13:41:54 PDT
Thanks!
Comment 33 User image aleth [:aleth] 2015-09-21 15:01:43 PDT
https://hg.mozilla.org/comm-central/rev/d1fc3d4e18648ee21fd51cd0ef5545ba3e95aed3
Bug 890332 - Ensure appmenu always opens correctly on mousedown. r=mkmelin

Note You need to log in before you can comment on or make changes to this bug.