1400024 - Photon Hamburger menu has inconsistent closing behavior based on button clicked

Status: VERIFIED FIXED

(Firefox :: Menus, defect, P1)

Description

[u554753]

Steps to Reproduce
1. Open the photon hamburger window
2. Click the word "Zoom" in the menu
3. Click the word "Edit" in the menu

Expected Results:
Either the menu is supposed to disappear when the user clicks the word or the menu is supposed to stay.

Actual Results:
When clicking "Zoom", the menu disappears. When clicking "Edit", the menu stays.

Comment 1

[Drew Willcoxon :adw]

Attachment: Bug 1400024 - Clicks on Zoom label in Photon app menu shouldn't close the menu.

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

Comment 2

[Drew Willcoxon :adw]

It's because closemenu=none is on the edit toolbarbutton but not the zoom button.  Although maybe there's a reason for that since every button but the full-screen button has it.  I added closemenu=auto to the full-screen button for good measure, but it doesn't seem to be necessary at least on macOS.

Comment 3

[:Gijs (out for now; he/him)]

Comment on attachment 8908852 [details]
Bug 1400024 - Clicks on Zoom label in Photon app menu shouldn't close the menu.

https://reviewboard.mozilla.org/r/180472/#review185708

r=me with a hack for the fullscreen button added.

::: browser/components/customizableui/content/panelUI.inc.xul:583
(Diff revision 1)
>            <toolbarseparator orient="vertical"/>
>            <toolbarbutton id="appMenu-fullscreen-button"
>                           class="subviewbutton subviewbutton-iconic"
>                           observes="View:FullScreen"
>                           type="checkbox"
> +                         closemenu="auto"

While this does close the panel eventually, at least on OS X it happens after the fullscreen transition and in the meantime the panel gets moved and/or squashed and looks terrible. In my testing, adding the following markup seems to avoid this:

```                         onclick="this.closest('panel').hidePopup()"
```

This is pretty hacky, but hey, it works!

Comment 4

[:Gijs (out for now; he/him)]

(In reply to :Gijs from comment #3)
> Comment on attachment 8908852 [details]
> Bug 1400024 - Clicks on Zoom label in Photon app menu shouldn't close the
> menu.
> 
> https://reviewboard.mozilla.org/r/180472/#review185708
> 
> r=me with a hack for the fullscreen button added.
> 
> ::: browser/components/customizableui/content/panelUI.inc.xul:583
> (Diff revision 1)
> >            <toolbarseparator orient="vertical"/>
> >            <toolbarbutton id="appMenu-fullscreen-button"
> >                           class="subviewbutton subviewbutton-iconic"
> >                           observes="View:FullScreen"
> >                           type="checkbox"
> > +                         closemenu="auto"
> 
> While this does close the panel eventually, at least on OS X it happens
> after the fullscreen transition and in the meantime the panel gets moved
> and/or squashed and looks terrible. In my testing, adding the following
> markup seems to avoid this:
> 
> ```                         onclick="this.closest('panel').hidePopup()"
> ```
> 
> This is pretty hacky, but hey, it works!

Err, and this should probably check if (event.button == 0).

Comment 5

[Drew Willcoxon :adw]

That happens currently too (without the patch) if I'm seeing the same thing as you fwiw.  But it's worth fixing regardless, I agree.

Comment 6

[:Gijs (out for now; he/him)]

(In reply to Drew Willcoxon :adw from comment #5)
> That happens currently too (without the patch) if I'm seeing the same thing
> as you fwiw.  But it's worth fixing regardless, I agree.

Err, yes, I should have been more clear, sorry. I just figured we could fix it while we were fixing this. :-)

Comment 7

[Drew Willcoxon :adw]

Comment on attachment 8908852 [details]
Bug 1400024 - Clicks on Zoom label in Photon app menu shouldn't close the menu.

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

Comment 8

[Drew Willcoxon :adw]

https://hg.mozilla.org/integration/autoland/rev/206b2ca14b91

Comment 9

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

https://hg.mozilla.org/mozilla-central/rev/206b2ca14b91

Comment 10

[Md. Majedul islam]

I have reproduced this bug with Nightly 57.0a1 (2017-09-14) on Ubuntu 16.04, 64 bit!

The fix is now verified on Latest Nightly 57.0a1 .

Build ID 	20170919100405
User Agent 	Mozilla/5.0 (X11; Linux x86_64; rv:57.0) Gecko/20100101 Firefox/57.0