(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.

Happens on Win XP too.

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 on attachment 8654663 [details] [diff] [review] Rev 1 - Event handler changed to mousedown Review of attachment 8654663 [details] [diff] [review]: ----------------------------------------------------------------- Forwarding.

Comment on attachment 8654663 [details] [diff] [review] Rev 1 - Event handler changed to mousedown This looks good. Thank you for the patch.

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

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 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".

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).

(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

> 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.

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?

(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...

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 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.

> 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 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 on attachment 8655745 [details] [diff] [review] Rev 3 - Pointless check for event target removed It works as described. ui-r+

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

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

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

Reopening, see bug 1202268.

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.

(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.

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.

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.

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.

(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.

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.

Thanks!

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