Scrollbar overlaps the keyboard shortcuts in the Hamburger menu

VERIFIED FIXED in Firefox 58

Status

()

defect
P1
normal
VERIFIED FIXED
2 years ago
a year ago

People

(Reporter: emilghitta, Assigned: adw)

Tracking

(Blocks 1 bug, {regression})

Trunk
Firefox 58
All
macOS
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox-esr52 unaffected, firefox56 unaffected, firefox57 wontfix, firefox58 verified)

Details

(Whiteboard: [reserve-photon-structure])

Attachments

(4 attachments)

Posted image 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.
Reporter

Comment 1

2 years ago
Hi Mike!

It seems that Bug 1397754 may have caused this regression.

Can you please take a look into this?

Thanks!
Flags: needinfo?(mdeboer)
Flags: needinfo?(mdeboer)
Whiteboard: [photon-structure][triage]
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!
Flags: needinfo?(emil.ghitta)
Reporter

Comment 3

2 years ago
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.
Flags: needinfo?(emil.ghitta)
But... those scrollbars look weird... Which version of OSX are you using and do you have a special setting wrt scrollbars?
Flags: needinfo?(emil.ghitta)
(Go to 'System Preferences > General')
Reporter

Comment 6

2 years ago
Posted image 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.
Flags: needinfo?(emil.ghitta)
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?
Flags: needinfo?(mstange)
Mike, I can reproduce this with 10.11.6 if I move a really tiny window to the bottom of the screen.
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?
Flags: needinfo?(albert)
Assignee

Comment 10

2 years ago
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.
Assignee

Comment 11

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

Comment 12

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

Comment 13

2 years ago
Or #appMenu-zoomReset-button...
Comment hidden (mozreview-request)
Assignee

Comment 15

2 years ago
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.
Assignee

Comment 16

2 years ago
I'll leave the needinfo on Markus but clear it on Albert since I was able to answer Jared's question to him.
Flags: needinfo?(albert)
Assignee: nobody → adw
Status: NEW → ASSIGNED
Priority: -- → P1
Whiteboard: [photon-structure][triage] → [reserve-photon-structure]
Drew, this looks awfully similar to bug 1387808... Can you still reproduce this bug with that patch applied or on latest Nightly?
Flags: needinfo?(adw)
Attachment #8921284 - Flags: review?(mdeboer)
Assignee

Comment 18

2 years ago
Yes I can.
Flags: needinfo?(adw)
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.)
Flags: needinfo?(adw)
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.
Assignee

Comment 21

2 years ago
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.)
Flags: needinfo?(adw)
Sounds good to me. But please make the min-width change OSX only. The crop=end for the label should be good regardless.
(Something like `calc(@menuPanelWidth@ + 20px); // On OSX, the scrollbar width may need to be added.`
Assignee

Comment 24

2 years ago
Mike, this problem is reproducable on Windows too, if you make the zoom label long enough.
Comment hidden (mozreview-request)
Assignee

Comment 26

2 years ago
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.
Sorry I haven't gotten a chance to look into this yet. Has the cause been found in the meantime?
(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
Flags: needinfo?(mstange)

Comment 29

2 years ago
mozreview-review
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.
Attachment #8921284 - Flags: review?(mdeboer) → review+
Comment hidden (mozreview-request)
Assignee

Comment 31

2 years ago
Thanks Mike!

Comment 32

2 years ago
Pushed by dwillcoxon@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/89cc8d814aa9
Prevent the app menu's scrollbar from overlapping its content. r=mikedeboer

Comment 33

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/89cc8d814aa9
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 58

Comment 34

2 years ago
Verified on 58.0b4 with Windows 10, Ubuntu 16.04, and Mac OS X 10.13
Status: RESOLVED → VERIFIED

Updated

a year ago
Depends on: 1454607
You need to log in before you can comment on or make changes to this bug.