1409377 - Scrollbar overlaps the keyboard shortcuts in the Hamburger menu

Status: VERIFIED FIXED

(Firefox :: Menus, defect, P1)

Description

[Emil Ghitta, QA [:emilghitta]]

Attachment: Mac.png

[Affected versions]:
Firefox 57.0b9 (BuildId:20171016185129)
Firefox 58.0a1 (BuildId:20171016220427)

[Affected platforms]:
- macOS 10.11.6

[Unaffected platforms]:
- Windows 10 64bit
- Ubuntu 16.04 x64

[Steps to reproduce]:
1. Start Firefox.
2. Drag Firefox window to the bottom of the screen.
3. Click the "Hamburger" menu.

[Expected result]:
The Hamburger menu displays without any issues.

[Actual result]:
The scrollbar overlaps the keyboard shortcuts.

[Regression range]:
This is a regression:
Last good revision: 3b7add087f2b2fe1c489350af9c3ffe1fb21fb2a
First bad revision: c33c05b5d47d306a2a7eeeca4a17aa48aa14f35f

Pushlog:
https://hg.mozilla.org/integration/autoland/pushloghtml?fromchange=3b7add087f2b2fe1c489350af9c3ffe1fb21fb2a&tochange=c33c05b5d47d306a2a7eeeca4a17aa48aa14f35f

It seems that Bug 1397754 may have caused this regression.

[Additional notes]:
For more information regarding this issue please observe the attached screenshot.

Comment 1

[Emil Ghitta, QA [:emilghitta]]

Hi Mike!

It seems that Bug 1397754 may have caused this regression.

Can you please take a look into this?

Thanks!

Comment 2

[Mike de Boer [:mikedeboer]]

I'm having quite a bit of trouble to reproduce this, or better: I can't at all :(

Emil, is this still reproducible for you? If so, do you have a more elaborate STR so that I can try more options locally? Thanks!

Comment 3

[Emil Ghitta, QA [:emilghitta]]

Sure thing.

Basically you need to drag the Firefox window to the bottom of the screen until the scrollbar becomes visible on the Hamburger Menu.

For further details, I created a screencast: https://drive.google.com/open?id=0BzOkHBmdnAoaU0p5ZlNuWUVHLTg

I hope this helps.

Comment 4

[Mike de Boer [:mikedeboer]]

But... those scrollbars look weird... Which version of OSX are you using and do you have a special setting wrt scrollbars?

Comment 5

[Mike de Boer [:mikedeboer]]

(Go to 'System Preferences > General')

Comment 6

[Emil Ghitta, QA [:emilghitta]]

Attachment: general.png

I can reproduce this with 10.11.6, 10.12.1 and 10.12.6 as well. 

I don't have any special settings applied for scrollbars.

Comment 7

[Mike de Boer [:mikedeboer]]

Weird. I can only reproduce this in 10.12.6 when I set 'Show scroll bars: Always'... Markus, are you the right person to ask about this kind of issue? Why isn't the content pushed aside to make room for the appMenu contents?

Comment 8

[Albert Scheiner [:alberts]]

Attachment: Screen Shot 2017-10-23 at 22.48.22.png

Mike, I can reproduce this with 10.11.6 if I move a really tiny window to the bottom of the screen.

Comment 9

[Jared Wein [:jaws] (please needinfo? me)]

A follow-up patch for bug 1387808 just landed on autoland (https://bugzilla.mozilla.org/show_bug.cgi?id=1387808#c38). Perhaps you could test with a build from that push?

Comment 10

[Drew Willcoxon :adw]

It's still reproducable with bug 1387808's fix unfortunately, which doesn't seem to have changed anything for this bug, based on the attached screenshots here.

An interesting thing is that neither the library panel nor the page action panel have this problem.  Their content get shifted to the left to make room for the scrollbar.

Comment 11

[Drew Willcoxon :adw]

It must have something to do with #appMenu-zoom-controls.  When I comment it out, the scrollbar appears like in the other panels.

Comment 12

[Drew Willcoxon :adw]

It's the margin-inline-start on `.toolbaritem-combined-buttons > .subviewbutton` and `#appMenu-zoom-controls > .subviewbutton` that's the problem looks like.

Comment 13

[Drew Willcoxon :adw]

Or #appMenu-zoomReset-button...

Comment 14

[Drew Willcoxon :adw]

Attachment: Bug 1409377 - Prevent the app menu's scrollbar from overlapping its content.

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

Comment 15

[Drew Willcoxon :adw]

The problem is just that the panel is too narrow to accommodate all the buttons in the zoom item.  The zoom item has more buttons than the edit item, which is why the problem doesn't happen when I remove only the zoom item and leave the edit item.  The panel just doesn't seem to get wider when the scrollbar appears.  Maybe panels aren't designed to do that.

This patch just increases menuPanelWidth from 24.35em to 26.35em.  24.35 is an oddly specific number, so I don't know if I'm breaking anything by changing it.

Comment 16

[Drew Willcoxon :adw]

I'll leave the needinfo on Markus but clear it on Albert since I was able to answer Jared's question to him.

Comment 17

[Mike de Boer [:mikedeboer]]

Drew, this looks awfully similar to bug 1387808... Can you still reproduce this bug with that patch applied or on latest Nightly?

Comment 18

[Drew Willcoxon :adw]

Yes I can.

Comment 19

[Mike de Boer [:mikedeboer]]

OK, so then my next question is: is increasing the width here a solution we can maintain? What about other locales that have longer labels for 'Zoom'?
On OSX the panels have more start-padding because it needs to make room for an eventual checkbox that might appear for type="check" items. On Windows and Linux we don't have this. So it might make sense to make the menu 2em wider on OSX only.
(The AppMenu on other OSes does appear to become wider and place the scrollbar adjacent to the items.)

Comment 20

[Mike de Boer [:mikedeboer]]

Also, menuPanelWidth is used as the _min-width_ for the main view, not a width constraining property. The max-width is in fact 30em.

In other words: the panel should have enough space left to show a scrollbar; therefore I think this is a platform/ widget issue.

Comment 21

[Drew Willcoxon :adw]

I guess so... but what do you think about this, at least as a fix for now: I can look at all the fullZoom.label strings in the l10n repo to see how long the longest "Zoom" label is.  We make the panel wide enough for it.  Also, we add crop=end to the label so that in the worst case, where this bug currently happens, we instead crop the label (with an ellipsis) so that the scrollbar doesn't overlap the content.  (This works, I tested it.)

Comment 22

[Mike de Boer [:mikedeboer]]

Sounds good to me. But please make the min-width change OSX only. The crop=end for the label should be good regardless.

Comment 23

[Mike de Boer [:mikedeboer]]

(Something like `calc(@menuPanelWidth@ + 20px); // On OSX, the scrollbar width may need to be added.`

Comment 24

[Drew Willcoxon :adw]

Mike, this problem is reproducable on Windows too, if you make the zoom label long enough.

Comment 25

[Drew Willcoxon :adw]

Comment on attachment 8921284 [details]
Bug 1409377 - Prevent the app menu's scrollbar from overlapping its content.

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

Comment 26

[Drew Willcoxon :adw]

I looked at all the localized strings for the "Zoom" label.  There are some long ones.  Making the panel wide enough for the longest string destroys its svelteness in locales with short strings (like English locales!).  I don't think that's what we want.

I played around with it some more, trying to get the content, not the panel, to "just work," to just resize itself to make room for the scrollbar.  I found that using a spacer after the label seems to do the trick.  So now we have spacers before and after the label.  This is really a continuation of bug 1387808 it seems.

I couldn't get the cropping to work right.  "Zoom" got cropped to "Zo...", so I dropped that idea.  Instead, to better accommodate large strings, I increased the max-width of the panelview a little.

I tested this on Windows 10 and macOS with large strings and small, and it works.

Comment 27

[Markus Stange [:mstange]]

Sorry I haven't gotten a chance to look into this yet. Has the cause been found in the meantime?

Comment 28

[Mike de Boer [:mikedeboer]]

(In reply to Drew Willcoxon :adw from comment #26)
> I looked at all the localized strings for the "Zoom" label.  There are some
> long ones.  Making the panel wide enough for the longest string destroys its
> svelteness in locales with short strings (like English locales!).  I don't
> think that's what we want.

Absolutely not, I agree!

> I played around with it some more, trying to get the content, not the panel,
> to "just work," to just resize itself to make room for the scrollbar.  I
> found that using a spacer after the label seems to do the trick.  So now we
> have spacers before and after the label.  This is really a continuation of
> bug 1387808 it seems.

No way! Argh, let's just go with that, then!

I do think that we're moving into the realm of 'definitely not for 57'.

> I couldn't get the cropping to work right.  "Zoom" got cropped to "Zo...",
> so I dropped that idea.  Instead, to better accommodate large strings, I
> increased the max-width of the panelview a little.

That's OK. Let's drop the cropping idea and just stick with what you've got. I think that this is now an all-round solution.

(In reply to Markus Stange [:mstange] from comment #27)
> Sorry I haven't gotten a chance to look into this yet. Has the cause been
> found in the meantime?

Myeah, I think Drew just found a way to work around it. I'm also thinking now that this is a bug in XUL layout that we're dealing with, similar to bug 1293242.
In other words; probably not something you'd be able or - more probable - even _want_ to help out with. ;-P

Comment 29

[Mike de Boer [:mikedeboer]]

Comment on attachment 8921284 [details]
Bug 1409377 - Prevent the app menu's scrollbar from overlapping its content.

https://reviewboard.mozilla.org/r/192304/#review198156

::: browser/components/customizableui/content/panelUI.inc.xul:547
(Diff revision 2)
>          <toolbarseparator/>
>          <toolbaritem id="appMenu-zoom-controls" class="toolbaritem-combined-buttons" closemenu="none">
>            <!-- Use a spacer, because panel sizing code gets confused when using CSS methods. -->
> -          <spacer/>
> +          <spacer class="before-label"/>
>            <label value="&fullZoom.label;"/>
> +          <!-- This spacer keeps the scrollbar from overlapping the view. -->

nit: please repeat this comment below as well.

Comment 30

[Drew Willcoxon :adw]

Comment on attachment 8921284 [details]
Bug 1409377 - Prevent the app menu's scrollbar from overlapping its content.

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

Comment 31

[Drew Willcoxon :adw]

Thanks Mike!

Comment 32

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/89cc8d814aa9
Prevent the app menu's scrollbar from overlapping its content. r=mikedeboer

Comment 33

[Attila Craciun [:arny]]

https://hg.mozilla.org/mozilla-central/rev/89cc8d814aa9

Comment 34

[u554753]

Verified on 58.0b4 with Windows 10, Ubuntu 16.04, and Mac OS X 10.13