Closed Bug 1383473 Opened 7 years ago Closed 7 years ago

Some minor styling issues in the hamburger menu

Categories

(Firefox :: Toolbars and Customization, defect, P1)

56 Branch
defect

Tracking

()

VERIFIED FIXED
Firefox 57
Iteration:
57.1 - Aug 15
Tracking Status
firefox57 --- verified

People

(Reporter: bruce.bugz, Assigned: ewright)

References

Details

(Whiteboard: [photon-structure])

Attachments

(1 file)

User Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:56.0) Gecko/20100101 Firefox/56.0
Build ID: 20170722072631

Steps to reproduce:

(Please ignore the 'steps to reproduce,' 'actual results,' 'expected results' headings, if any, on this bug)

1) The `>` arrow indicating subviews on items like Library, Web Developer etc. isn't vertically centred.
Moving the arrow down by 1px (or even 2px) visually centres it and looks noticeably better. Please see gif for a before-after: http://i.imgur.com/Apnr5FV.gif.
The style currently in use is:
`.PanelUI-subView .subviewbutton-nav:-moz-locale-dir(ltr)::after { transform: scaleX(-1) }`
What vertically centres it is:
`.PanelUI-subView .subviewbutton-nav:-moz-locale-dir(ltr)::after { transform: scaleX(-1) translateY(1px) }`

2) The 'Sign into Sync' and 'Exit' buttons have unequal spacing on their top and bottom, and compared to all the other items in the menu.
Changing the margins on `photonpanelmultiview .panel-subview-body` from `4px 0` (currently in use) to `6px 0` fixes this. As per the Invision spec (https://mozilla.invisionapp.com/share/ZKBC94BPQ#/screens/229252106), the separators have spacings of 6px on top and bottom, so the spacing before the first item and after the last item on the hamburger menu should be 6px as well. It looks better too - here's a before-after: http://i.imgur.com/C38dtlg.gif.

3) The `+` and `full screen` buttons have smaller right and left spacing, respectively, than top and bottom. The two buttons have 6px spacing with the top and bottom separators, so they should have, at least, this same spacing with the vertical separator. See gif: http://i.imgur.com/H0gdn9u.gif. Current margins are 4px and 5px; setting them to 6px and 7px looks better. Note that the spec actually says 10px spacing (https://mozilla.invisionapp.com/share/ZKBC94BPQ#/screens/230516723).

4) I really dislike how tiny the clickable areas are on the cut-copy-paste buttons, especially when compared to how all the other entries (well, except zoom) are much easier-to-hit targets. There is so much empty, non-clickable space on the Edit row. Can't this be improved? I think Chrome uses the space much better - here's a comparison: http://i.imgur.com/8Y3M1Ps.png.
Component: Untriaged → Toolbars and Customization
Whiteboard: [photon-structure][triage]
Flags: qe-verify+
Priority: -- → P2
QA Contact: gwimberly
Whiteboard: [photon-structure][triage] → [photon-structure]
#1,#2, and #3 are on point, let's do those. 

#4 we'll have to revisit at another time. it's worth doing, but it's not a blocker ATM.
Assignee: nobody → ewright
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
Priority: P2 → P1
Comment on attachment 8893086 [details]
Bug 1383473 - Minor Styling fixes in the hamburger menu.

https://reviewboard.mozilla.org/r/164086/#review169808

::: browser/themes/shared/customizableui/panelUI.inc.css:365
(Diff revision 1)
>  #appMenu-popup[touchmode] panelview {
>    min-width: calc(@menuPanelWidth@ + 24px);
>  }
>  
>  photonpanelmultiview .panel-subview-body {
> -  margin: 4px 0;
> +  margin: 6px 0;

I'm traveling right now and no good data, so I can't check invision because it takes about 42gb to load, but does this also match for panels with footers, like the overflow panel, or the bookmarks subview of the library panel?

I'm asking because I know that we changed this for the bookmarks subview in the library panel in bug 1354159, and I would want to be sure we match the spec there still after this change, too - otherwise we might have to add (more) negative top margin to the footer classes to reduce the space between the bottom item (which might be in a scrollable view) and the footer.

::: browser/themes/shared/customizableui/panelUI.inc.css:1254
(Diff revision 1)
>    fill: GrayText;
>    float: right;
>  }
>  
>  .PanelUI-subView .subviewbutton-nav:-moz-locale-dir(ltr)::after {
> -  transform: scaleX(-1);
> +  transform: scaleX(-1) translateY(1px);

I don't think transform'ing this is the right solution. Can we just ensure the ::after element is aligned better, maybe just by giving it margin-top: 1px?

This also looks like it doesn't work right for RTL, because the selector is specific to LTR.
Attachment #8893086 - Flags: review?(gijskruitbosch+bugs)
Comment on attachment 8893086 [details]
Bug 1383473 - Minor Styling fixes in the hamburger menu.

https://reviewboard.mozilla.org/r/164086/#review169808

> I'm traveling right now and no good data, so I can't check invision because it takes about 42gb to load, but does this also match for panels with footers, like the overflow panel, or the bookmarks subview of the library panel?
> 
> I'm asking because I know that we changed this for the bookmarks subview in the library panel in bug 1354159, and I would want to be sure we match the spec there still after this change, too - otherwise we might have to add (more) negative top margin to the footer classes to reduce the space between the bottom item (which might be in a scrollable view) and the footer.

It still looks good with a footer and header ie the bookmarks menu. I looked at it with overflowing content, and I believ the 4px there currently isn't desireable either - for overflow cases I think what we want is 0px. I looked at invision, and can't actually find the specific measurements for `photonpanelmultiview .panel-subview-body` so i'm very-likely misunderstanding something

> I don't think transform'ing this is the right solution. Can we just ensure the ::after element is aligned better, maybe just by giving it margin-top: 1px?
> 
> This also looks like it doesn't work right for RTL, because the selector is specific to LTR.

ah, I missed the ltr - thanks. As a side note, is it more efficient to `transform: scaleX(-1);` rather than reference the forward chevron. Or, since most of our languages are ltr, doing the transform in the other direction when rtl?
Comment on attachment 8893086 [details]
Bug 1383473 - Minor Styling fixes in the hamburger menu.

https://reviewboard.mozilla.org/r/164086/#review170012

::: browser/themes/shared/customizableui/panelUI.inc.css:1438
(Diff revision 2)
>    border-color: var(--panel-separator-color);
>    box-shadow: 0 1px 0 hsla(210,4%,10%,.03) inset;
>  }
>  
>  .subviewbutton.panel-subview-footer {
> -  margin: 4px -4px -4px;
> +  margin: 0;

this fixes the footer in the overflow menu, it was unbalanced top and bottom.
Comment on attachment 8893086 [details]
Bug 1383473 - Minor Styling fixes in the hamburger menu.

https://reviewboard.mozilla.org/r/164086/#review170324

::: browser/themes/shared/customizableui/panelUI.inc.css:1251
(Diff revision 2)
>  .PanelUI-subView .subviewbutton-nav::after {
>    -moz-context-properties: fill;
>    content: url(chrome://browser/skin/back-12.svg);
>    fill: GrayText;
>    float: right;
> +  margin-top: 1px;

As discussed on slack, please switch back to using a translate to move the icon. You can set transform: translateY(1px); here, and then apply both transforms in 1 property in the ltr style so the arrow continues to point the right way.


You're right that ideally we should just have a separate glyph for LTR/RTL, but we can take care of that in a separate bug.

::: browser/themes/shared/customizableui/panelUI.inc.css:1438
(Diff revision 2)
>    border-color: var(--panel-separator-color);
>    box-shadow: 0 1px 0 hsla(210,4%,10%,.03) inset;
>  }
>  
>  .subviewbutton.panel-subview-footer {
> -  margin: 4px -4px -4px;
> +  margin: 0;

Good catch!
Attachment #8893086 - Flags: review?(gijskruitbosch+bugs) → review+
Blocks: 1387512
Comment on attachment 8893086 [details]
Bug 1383473 - Minor Styling fixes in the hamburger menu.

https://reviewboard.mozilla.org/r/164086/#review170370

::: browser/themes/shared/customizableui/panelUI.inc.css:1251
(Diff revision 3)
>  .PanelUI-subView .subviewbutton-nav::after {
>    -moz-context-properties: fill;
>    content: url(chrome://browser/skin/back-12.svg);
>    fill: GrayText;
>    float: right;
> +  transform: translate(1px);

translateY, right?

::: browser/themes/shared/customizableui/panelUI.inc.css:1255
(Diff revision 3)
>    float: right;
> +  transform: translate(1px);
>  }
>  
>  .PanelUI-subView .subviewbutton-nav:-moz-locale-dir(ltr)::after {
> -  transform: scaleX(-1);
> +  transform: scaleX(-1) translate(1px);

Ditto. :-)
Pushed by gijskruitbosch@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/ebaccd4031d2
Minor Styling fixes in the hamburger menu. r=Gijs
https://hg.mozilla.org/mozilla-central/rev/ebaccd4031d2
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 57
Iteration: --- → 57.1 - Aug 15
Any suggestions on how to test this with a RTL locale for 57?
Flags: needinfo?(gijskruitbosch+bugs)
(In reply to Grover Wimberly IV [:Grover-QA] from comment #13)
> Any suggestions on how to test this with a RTL locale for 57?

I don't think any of the issues in this bug are specific to RTL? Am I missing something?

In any case, you can set the intl.uidirection pref to 1 (and restart) to test RTL with English.
Flags: needinfo?(gijskruitbosch+bugs) → needinfo?(gwimberly)
I just wanted to see if the spacing looked OK on said locale. In any case, good to know for the future. 

Verified on Win/Mac/Ubuntu.
Status: RESOLVED → VERIFIED
Flags: qe-verify+
Flags: needinfo?(gwimberly)
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: