1387808 - Make the Firefox menu (hamburger button application menu) a bit wider in cases where the scrollbar is needed

Status: VERIFIED FIXED

(Firefox :: Menus, defect, P1)

Description

[Itiel]

Attachment: Scrollbar hides parts of the menu

Environment:
Windows 10 x86
Latest Nightly (57.0a1 2017-08-05)

STR:
1. Relocate Nightly's window close to the taskbar (in a way that Nightly's menu wouldn't have enough space)
2. Click the hamburger menu

AR:
The scrollbar hides parts of the menu (some icons and shortcuts)

ER:
It shouldn't. The menu's width should probably get a bit wider in cases where the scrollbar is needed

See attached screenshot.

Comment 1

[Itiel]

This issue is relevant also for LTR builds.

Comment 2

[(Away until May 24) Jared Wein [:jaws] (please needinfo? me)]

I can't reproduce this on Windows 10 57.0a1 (2017-09-13) (64-bit) en-US build. I didn't test with an RTL language due to comment #1. Can you still reproduce this? Please reopen if so.

Comment 3

[Itiel]

I can confirm this works correctly on en-US build (even if you change the intl.uidirection value in it so the Nightly would be RTL), but on a Hebrew build this doesn't work even if you change the mentioned value to change Nightly UI to LTR.

When I posted comment 1, I probably checked it in the Hebrew build with changing the intl.uidirection to change the UI to LTR.

So yeah, still reproducible.

Comment 5

[(Away until May 24) Jared Wein [:jaws] (please needinfo? me)]

[Tracking Requested - why for this release]: visual regression seen on some devices/locales.

Mike, do you know why this might be happening? See also bug 1402947 which reproduced this on an en-US build.

Comment 7

[(Away until May 24) Jared Wein [:jaws] (please needinfo? me)]

Itiel, are you able to use mozregression to find when this started happening? If we can find what caused this we are much more likely to fix this.

http://mozilla.github.io/mozregression/

Comment 8

[Itiel]

(In reply to Jared Wein [:jaws] (please needinfo? me) from comment #7)
> Itiel, are you able to use mozregression to find when this started
> happening? If we can find what caused this we are much more likely to fix
> this.
> 
> http://mozilla.github.io/mozregression/

Not sure how to check this one, on my system I can't reproduce with an en-US build (with or without changing the intl.uidirection pref value), and to my knowledge (I'd be glad to be proven wrong!) there's no way to bisect in mozregression using a pure Hebrew build.
Additionaly, I'm unable to reproduce the steps from bug 1387426, and I'm not sure what are the required steps for reproduction in bug 1402947.

Comment 9

[Julien Cristau [:jcristau]]

tracking as new visual regression

Comment 10

[Mike de Boer [:mikedeboer]]

(In reply to Itiel from comment #8)
> Not sure how to check this one, on my system I can't reproduce with an en-US
> build (with or without changing the intl.uidirection pref value), and to my
> knowledge (I'd be glad to be proven wrong!) there's no way to bisect in
> mozregression using a pure Hebrew build.
> Additionaly, I'm unable to reproduce the steps from bug 1387426, and I'm not
> sure what are the required steps for reproduction in bug 1402947.

Well, it is the case that the menu panel gets a max-width set after the first (main) view was shown. Perhaps what should be done here is to _not_ set the max-width whenever the first (main) view is shown any second time and assume its contents can change its dimensions dynamically over time.
This'd be a rather simple patch, but since this isn't reproducible on an en-US build, I might need help from Itiel to verify the patch using build that I'd supply.

Comment 11

[Mike de Boer [:mikedeboer]]

As per IRC:

[15:11:04] <johannh> Gijs: just add touchmode=true to that panel
[15:11:08] <johannh> or wait
[15:11:19] <johannh> yeah
[15:11:21] <johannh> that's the name
[15:11:31] <Gijs> mikedeboer: ^^ I can trivially repro https://bugzilla.mozilla.org/show_bug.cgi?id=1387808 on my touch-enabled device by opening the panel via touch if the window is in the middle of the screen
[15:11:33] <firebot> Bug 1387808 — NEW, [e-mail] — Make the Firefox menu (hamburger button application menu) a bit wider in cases where the scrollbar i
[15:11:35] <Gijs> it makes the hamburger menu way too tall
[15:11:38] <johannh> https://searchfox.org/mozilla-central/source/browser/themes/windows/browser.css#1111
[15:11:38] <Gijs> so it gets a scrollbar
[15:11:45] <Gijs> and it breaks as described in the screenshots

Comment 13

[Mike de Boer [:mikedeboer]]

Attachment: Bug 1387808 - Don't ever fixate the width and height of the main view, but leave it flexible.

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

Comment 14

[Mike de Boer [:mikedeboer]]

This patch is the least invasive method I can think of. The other solutions - solving it for all panels immediately - are:

1. In `MenuTouchModeObserver`, dispatch a custom 'TouchModeChanged' event on the target node next to changing the 'touchmode' attribute, add a listener for the event in PanelMultiView.jsm
2. Or add a MutationObserver that listens to changes of the 'touchmode' attribute in PanelMultiView.jsm
3. Whenever we enter or leave touch mode, recalculate the dimensions of the mainView offscreen, cache 'em and set that as the constraints for the subviews.

I think the attached patch is still valid and the steps above are an optional addition.

Comment 15

[Luke Usherwood]

Hi, while this is about widening a panel to account for the scrollbar, I think some tickets closed as dups may be making an independent request: not to require a scrollbar in the first place, when activated by touch (maybe #1387426 or #1402727)

Or at least, that's my Beta feedback :-D

I feel the main menu should fit by default in a maximised FF window, even if touch-activated. With ~2000px of vertical space? (4k UHD @ 250% scaling here.) It only goes over by about 1 row.

Perhaps by making the rows expand less for touch? (I calculate 15% smaller would match the size of a touch-activated context menu in MS Outlook 2016, for example.) Or alternately by allowing them to squash down a bit, if they're "nearly" going to fit. (For some definition of "nearly".)

Should a ticket be reopened? Would you like me to spin out a new one? Or has it been considered already somewhere?

Comment 16

[Tortino]

Hi, I'm the reporter of bug 1402727, and I definitely prefer no scrollbar at all, if that's possibile.
thanks.

Comment 17

[Calvin Walton]

Attachment: firefox-menu-scrollbar-limited-height.png

As far as it goes, a small adjustment to height would only hide the problem for some users. This ss is from my 8" Windows touchscreen tablet, the menu is about half again as tall as the screen.

The previous customizable tile menu *did* fit (or at least almost fit), the new menu is just too vertical.
It might be helpful to move more commonly used items "above the fold", so to speak, or even have a fixed panel that doesn't scroll above or below the lesser used entries.

Chrome's menu does fit on my screen, but it has fewer entries and is not using touch-friendly spacing.

Comment 18

[:Gijs (he/him)]

(In reply to Luke Usherwood from comment #15)
> Should a ticket be reopened? Would you like me to spin out a new one? Or has
> it been considered already somewhere?

I've reopened bug 1402727 and copied some of the comments there. But we still need to make the scrollbar situation not horrible, so we'll want to take a patch here, because as some of the comments here have noted, there will always be situations where things don't completely fit, depending on the positioning of the Firefox window, the screen size, etc.

Comment 19

[:Gijs (he/him)]

Comment on attachment 8915549 [details]
Bug 1387808 - Don't ever fixate the width and height of the main view, but leave it flexible.

https://reviewboard.mozilla.org/r/186754/#review192672

Somewhat nervous about how this interacts with webextension popup sizing and their tests. Did you trypush?

Comment 20

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/88f59765d8cb
Don't ever fixate the width and height of the main view, but leave it flexible. r=Gijs

Comment 21

[Sebastian Hengst [:aryx] (needinfo on intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/88f59765d8cb

Comment 22

[Mike de Boer [:mikedeboer]]

Comment on attachment 8915549 [details]
Bug 1387808 - Don't ever fixate the width and height of the main view, but leave it flexible.

Approval Request Comment
[Feature/Bug causing the regression]: New Hamburger menu/ App Menu in combination with the new Touch Mode.
[User impact if declined]: When touch mode is enabled in the main menu, it will not adjust its size accordingly, potentially causing scrollbars to appear and the end side to be cut off partly.
[Is this code covered by automated tests?]: No.
[Has the fix been verified in Nightly?]: Not yet.
[Needs manual test from QE? If yes, steps to reproduce]:  Yes, STR in comment 0.
[List of other uplifts needed for the feature/fix]: n/a.
[Is the change risky?]: Minor.
[Why is the change risky/not risky?]: Because with this patch we are not enforcing the mainview to its own width and height anymore, which was unnecessary in the first place.
[String changes made/needed]: n/a.

Comment 23

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Comment on attachment 8915549 [details]
Bug 1387808 - Don't ever fixate the width and height of the main view, but leave it flexible.

Photon polish, beta57+

Comment 24

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/da0c1e20a0cc

Comment 25

[Itiel]

The main issue described in comment 0, and seen in attachment 8894187 [details], is still there in latest Nightly.
Using the Hebrew build.

Comment 26

[Liz Henry (:lizzard Please n-i to RyanVM, jcristau, or pascal)]

Itiel, is it still there in 57 beta as well?  If you have time to check, that will help!

Comment 27

[Itiel]

(In reply to Liz Henry (:lizzard) (needinfo? me) from comment #26)
> Itiel, is it still there in 57 beta as well?  If you have time to check,
> that will help!

Yes, that's still reproducible in 57 latest beta.

Comment 28

[Liz Henry (:lizzard Please n-i to RyanVM, jcristau, or pascal)]

Mike, want to take a look?

Comment 29

[Itiel]

Note that I'm using the normal density setting, no touch mode.

Comment 30

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

(In reply to Itiel from comment #27)
> (In reply to Liz Henry (:lizzard) (needinfo? me) from comment #26)
> > Itiel, is it still there in 57 beta as well?  If you have time to check,
> > that will help!
> 
> Yes, that's still reproducible in 57 latest beta.

This fix will be in 57.0b8 which will be made available to beta users tomorrow. So the beta you tested against did not have the fix in it. Could you please retest on 57.0b8 tomm/weekend/Monday? Thanks!

Comment 31

[Itiel]

(In reply to Ritu Kothari (:ritu) from comment #30)
> This fix will be in 57.0b8 which will be made available to beta users
> tomorrow. So the beta you tested against did not have the fix in it. Could
> you please retest on 57.0b8 tomm/weekend/Monday? Thanks!

Does it really matter which beta do I test it against?
AKAIK the fix should already be in 58 Nightly (comment 21) 3 days ago, and it is reproducible in latest Nightly.

Comment 32

[Itiel]

FWIW, the issue can be reproduced also on 57.0b8.
I think the root of the issue is that the scrollbar appears on the left side of the menu in RTL builds (which is fine), but Nightly doesn't account also for that.

Comment 33

[Mike de Boer [:mikedeboer]]

This is caused by the 'Zoom' label being too long and not overflowing to an ellipsis... I'll take a look what I need to do to make the label overflow correctly.
Note, however, that this is an issue that is locale-specific and so far is only reported for Hebrew. This is not dependent on the text direction.

Comment 34

[Mike de Boer [:mikedeboer]]

Attachment: Patch 2: Switch to using XUL spacer elements for the zoom and edit controls' leading space in the app menu to fix auto-sizing issues

ReviewBoard doesn't like follow-ups and I don't want to scare Itiel (our friendly bug reporter) away to another bug - do you mind reviewing it this way, Jared?

Comment 35

[(Away until May 24) Jared Wein [:jaws] (please needinfo? me)]

Comment on attachment 8920105 [details] [diff] [review]
Patch 2: Switch to using XUL spacer elements for the zoom and edit controls' leading space in the app menu to fix auto-sizing issues

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

Thanks for the write-up in the patch commit message. I wonder if some part of that should go in as a comment in the markup.

Comment 36

[Mike de Boer [:mikedeboer]]

Attachment: Patch 2: Switch to using XUL spacer elements for the zoom and edit controls' leading space in the app menu to fix auto-sizing issues

Patch for checkin, carrying over r=jaws.

Comment 37

[Mike de Boer [:mikedeboer]]

Comment on attachment 8921024 [details] [diff] [review]
Patch 2: Switch to using XUL spacer elements for the zoom and edit controls' leading space in the app menu to fix auto-sizing issues

Ryan, can you push this to autoland or inbound for me? Thanks!

Comment 38

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 8921024 [details] [diff] [review]
Patch 2: Switch to using XUL spacer elements for the zoom and edit controls' leading space in the app menu to fix auto-sizing issues

https://hg.mozilla.org/integration/autoland/rev/62617c6d5534053189a8497fa4705d5b0466e771

Comment 39

[Sebastian Hengst [:aryx] (needinfo on intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/62617c6d5534

Comment 40

[Ryan VanderMeulen [:RyanVM]]

I assume Part 2 needs and uplift request now too.

Comment 41

[Mike de Boer [:mikedeboer]]

Comment on attachment 8921024 [details] [diff] [review]
Patch 2: Switch to using XUL spacer elements for the zoom and edit controls' leading space in the app menu to fix auto-sizing issues

Approval Request Comment
[Feature/Bug causing the regression]: Photon Panel UI design.
[User impact if declined]: User will experience a cut-off App Menu when the list of items becomes scrollable. This only happens in locales where the 'Zoom' or 'Edit' label becomes larger than the empty space before them. Yes, layouting quirk that we had to work around. 
[Is this code covered by automated tests?]: No.
[Has the fix been verified in Nightly?]: No, not yet.
[Needs manual test from QE? If yes, steps to reproduce]: Yes, see the STR in comment 0 - use a Hebrew version of Nightly to test, or manually add characters to the 'Zoom' label in the App Menu using the Browser Toolbox.
[List of other uplifts needed for the feature/fix]: n/a.
[Is the change risky?]: Minor.
[Why is the change risky/not risky?]: We switched to using fixed-width XUL spacer elements instead of using CSS padding here, which is quite harmless.
[String changes made/needed]: n/a.

Comment 42

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/6495434fe826

Comment 43

[Itiel]

Does it makes sense that it's still not fixed in latest Nightly? Using 58.0a1 2017-10-25.

Comment 44

[Mike de Boer [:mikedeboer]]

What's left to do is bug 1409377, which is the remaining part of sizing issues caused by the zoom and edit controls.

Comment 45

[Itiel]

Working great on latest Nightly now, marking as verified.

Comment 46

[Ciprian Georgiu [:ciprian_georgiu], Release Desktop QA]

Removing the qe+ flag since this is already verified in 58 Nightly and the remaining patch from bug 1409377 is not going to be uplifted in 57 Beta.