1465337 - Tracking protection toggle and sync button should have their separators aligned

Status: VERIFIED FIXED

(Firefox :: Menus, enhancement, P1)

Description

[Johann Hofmann [:johannh]]

This is defined in the spec and I agree it looks a bit odd when they aren't aligned:

https://mozilla.invisionapp.com/share/RSIY1B8GMC2#/screens/297824891

Comment 1

[Fanolian]

The zoom|fullscreen separator has to be realigned with them too (in case you move the sync button separator).

Comment 2

[Johann Hofmann [:johannh]]

Attachment: Bug 1465337 - Horizontally align the TP toggle in the main menu with other subviewbuttons.

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

Comment 3

[Dão Gottwald [:dao]]

Comment on attachment 8982494 [details]
Bug 1465337 - Horizontally align the TP toggle in the main menu with other subviewbuttons.

https://reviewboard.mozilla.org/r/248470/#review254732

::: browser/themes/shared/customizableui/panelUI.inc.css:1037
(Diff revision 1)
>  
>  .toolbaritem-combined-buttons > toolbarseparator[orient="vertical"] + .subviewbutton,
>  #appMenu-zoom-controls > toolbarseparator[orient="vertical"] + .subviewbutton {
> -  margin-inline-start: 0;
> +  /* Add margin to offset the 1px border of the TP toggle */
> +  margin-inline-start: 1px;
> +  margin-inline-end: 1px;

While the comment helps some, trying to maintain this is going to be annoying. Can we make the toggle work within the available space, without changing other buttons?

Note that the 1px border actually looks pretty broken over here on Linux, because --panel-separator-color doesn't exactly work the way you expected. Do we even need to change the border on hover?

I played around with some changes on top of your patch and this seems to work pretty well for me:

diff --git a/browser/themes/shared/customizableui/panelUI.inc.css b/browser/themes/shared/customizableui/panelUI.inc.css
--- a/browser/themes/shared/customizableui/panelUI.inc.css
+++ b/browser/themes/shared/customizableui/panelUI.inc.css
@@ -585,50 +585,46 @@ toolbarbutton[constrain-size="true"][cui
   -moz-context-properties: fill;
   fill: currentColor;
   list-style-image: url(chrome://browser/skin/tracking-protection-16.svg#enabled);
   -moz-box-flex: 1;
 }
 
 #appMenu-tp-toggle {
   -moz-appearance: none;
-  box-sizing: border-box;
-  min-width: 26px;
+  min-width: 24px;
   height: 10px;
   border-radius: 10px;
   background-color: var(--arrowpanel-dimmed-even-further);
-  border: 1px solid transparent;
   margin-top: 4px;
   margin-bottom: 4px;
-  margin-inline-start: 1px;
-  margin-inline-end: 7px;
+  margin-inline-start: 0;
+  margin-inline-end: 8px;
   padding: 2px;
   transition: padding .2s ease;
 }
 
 #appMenu-tp-toggle::before {
   position: relative;
   display: block;
   content: "";
   width: 10px;
   height: 10px;
   border-radius: 10px;
   background: white;
 }
 
 #appMenu-tp-toggle[enabled=true] {
   background-color: #0a84ff;
-  border: 1px solid #0a84ff;
   /* Push the toggle to the right. */
   padding-inline-start: 12px;
 }
 
-#appMenu-tp-toggle:hover,
 #appMenu-tp-toggle:-moz-focusring {
-  border: 1px solid var(--panel-separator-color);
+  outline: 1px dotted;
 }
 
 #appMenu-tp-toggle[enabled=true]:hover,
 #appMenu-tp-toggle[enabled=true]:-moz-focusring {

Comment 5

[Johann Hofmann [:johannh]]

(In reply to Dão Gottwald [::dao] from comment #3)
> Comment on attachment 8982494 [details]
> Bug 1465337 - Horizontally align the TP toggle in the main menu with other
> subviewbuttons.
> 
> https://reviewboard.mozilla.org/r/248470/#review254732
> 
> ::: browser/themes/shared/customizableui/panelUI.inc.css:1037
> (Diff revision 1)
> >  
> >  .toolbaritem-combined-buttons > toolbarseparator[orient="vertical"] + .subviewbutton,
> >  #appMenu-zoom-controls > toolbarseparator[orient="vertical"] + .subviewbutton {
> > -  margin-inline-start: 0;
> > +  /* Add margin to offset the 1px border of the TP toggle */
> > +  margin-inline-start: 1px;
> > +  margin-inline-end: 1px;
> 
> While the comment helps some, trying to maintain this is going to be
> annoying. Can we make the toggle work within the available space, without
> changing other buttons?

I understand your point but this is an easy solution and I don't think this code will need to change a lot, so I don't really think it is a big issue.

> Note that the 1px border actually looks pretty broken over here on Linux,
> because --panel-separator-color doesn't exactly work the way you expected.
> Do we even need to change the border on hover?

Yes. I'd like to keep the border. It looks really nice on Windows and OSX. Can you please file a separate bug about the broken border on Linux (I presume you're referring to the fact that it doesn't show the blue border on active)?

> I played around with some changes on top of your patch and this seems to
> work pretty well for me:

Your patch does not actually make the toggle separator align with the other separators on OSX for me.

More importantly, showing a dotted border on active is okay, but it really looks broken on hover, especially compared to the subtle border effect.

I really don't think there's an alternative to keeping the border, so maybe you can reconsider a review here?

Comment 6

[Dão Gottwald [:dao]]

(In reply to Johann Hofmann [:johannh] from comment #5)
> > Note that the 1px border actually looks pretty broken over here on Linux,
> > because --panel-separator-color doesn't exactly work the way you expected.
> > Do we even need to change the border on hover?
> 
> Yes. I'd like to keep the border. It looks really nice on Windows and OSX.
> Can you please file a separate bug about the broken border on Linux (I
> presume you're referring to the fact that it doesn't show the blue border on
> active)?

It will also look wrong on Windows depending on the OS theme. That's because --panel-separator-color can be a platform color and then it's opaque. (I haven't checked but suspect it might also look wrong in the dark popup case when we set that color to rgba(249,249,250,.1).) Basically it's the wrong thing to use here even if you do want a border.

> Your patch does not actually make the toggle separator align with the other
> separators on OSX for me.

Curious. Was this on Mac only? I didn't invent anything new in my patch, just dropped the border and adjusted the min-width and margins. Are you sure dimensions are consistent across platforms without my patch?

> More importantly, showing a dotted border on active is okay, but it really
> looks broken on hover, especially compared to the subtle border effect.

Ahem, that's a focus ring for keyboard interaction, it's not supposed to show on active nor on hover. I think we need something like this as the border change is too subtle to effectively work as a focus indicator.

Comment 7

[Johann Hofmann [:johannh]]

(In reply to Dão Gottwald [::dao] from comment #6)
> (In reply to Johann Hofmann [:johannh] from comment #5)
> > Yes. I'd like to keep the border. It looks really nice on Windows and OSX.
> > Can you please file a separate bug about the broken border on Linux (I
> > presume you're referring to the fact that it doesn't show the blue border on
> > active)?
> 
> It will also look wrong on Windows depending on the OS theme. That's because
> --panel-separator-color can be a platform color and then it's opaque. (I
> haven't checked but suspect it might also look wrong in the dark popup case
> when we set that color to rgba(249,249,250,.1).) Basically it's the wrong
> thing to use here even if you do want a border.

Okay, makes total sense, I just went with what the appMenu-zoomReset-button uses:

https://searchfox.org/mozilla-central/rev/d0a41d2e7770fc00df7844d5f840067cc35ba26f/browser/themes/shared/customizableui/panelUI.inc.css#1055

We should probably fix both, then, but please in a separate bug.

> > Your patch does not actually make the toggle separator align with the other
> > separators on OSX for me.
> 
> Curious. Was this on Mac only? I didn't invent anything new in my patch,
> just dropped the border and adjusted the min-width and margins. Are you sure
> dimensions are consistent across platforms without my patch?

I've only tested yours on OSX but I tested my patch on Windows and OSX and there it's consistent, so, I guess?

> > More importantly, showing a dotted border on active is okay, but it really
> > looks broken on hover, especially compared to the subtle border effect.
> 
> Ahem, that's a focus ring for keyboard interaction, it's not supposed to
> show on active nor on hover. I think we need something like this as the
> border change is too subtle to effectively work as a focus indicator.

Ok, please take that to Bryan Bell and/or file a bug on it. This is all coordinated with several people in UX. I've not heard any negative feedback about this toggle so far, on the contrary.

All these things are irrelevant for this patch. This patch is about aligning the separators. Your original question was:

> Can we make the toggle work within the available space, without changing other buttons?

and the answer is no. This is how our design is supposed to look. I actually already made it a bit smaller. Even if I didn't like the UX spec it seems absurd to change it because we're unwilling to introduce two lines of dependency (with a comment!) in our CSS.

I appreciate your effort and the suggestions you're making but this is not something we should do in this bug. So if you do not have a suggestion for fixing this while maintaining the design we have, I don't see any reason to further stall on this patch.

I would be very happy to work on some of the perfectly valid but unrelated issues you found in follow-up bugs.

Comment 8

[Johann Hofmann [:johannh]]

This discussion makes me realize I should probably amend the comment to clarify that the margin is not necessarily offsetting the border but just generally the larger TP toggle.

Comment 9

[Dão Gottwald [:dao]]

(In reply to Johann Hofmann [:johannh] from comment #7)
> > It will also look wrong on Windows depending on the OS theme. That's because
> > --panel-separator-color can be a platform color and then it's opaque. (I
> > haven't checked but suspect it might also look wrong in the dark popup case
> > when we set that color to rgba(249,249,250,.1).) Basically it's the wrong
> > thing to use here even if you do want a border.
> 
> Okay, makes total sense, I just went with what the appMenu-zoomReset-button
> uses:
> 
> https://searchfox.org/mozilla-central/rev/
> d0a41d2e7770fc00df7844d5f840067cc35ba26f/browser/themes/shared/
> customizableui/panelUI.inc.css#1055
> 
> We should probably fix both, then, but please in a separate bug.

There's nothing to fix for appMenu-zoomReset-button as it doesn't have the blue background that --panel-separator-color isn't designed to be combined with.

> All these things are irrelevant for this patch. This patch is about aligning
> the separators. Your original question was:
> 
> > Can we make the toggle work within the available space, without changing other buttons?
> 
> and the answer is no. This is how our design is supposed to look. I actually
> already made it a bit smaller. Even if I didn't like the UX spec it seems
> absurd to change it because we're unwilling to introduce two lines of
> dependency (with a comment!) in our CSS.
> 
> I appreciate your effort and the suggestions you're making but this is not
> something we should do in this bug. So if you do not have a suggestion for
> fixing this while maintaining the design we have, I don't see any reason to
> further stall on this patch.

You're selling the comment as an upside but it's part of the problem. I'm unwilling to add a "match the TP toggle" dependency that makes people chase down mostly unrelated code. The way I want people to approach this row is "I have x pixels of horizontal space for my widget" rather than "my widget can use whatever space the TP toggle uses". The jargon doesn't help either, as it's not immediately obvious what a TP toggle is.

I'm trying to find a practical solution, hence looking at the bigger picture, as cleaning up the styling could automatically fix this bug without affecting other buttons. But if it was decided that these buttons should now be, say, 26px wide instead of 24px, let's just do that in a way that doesn't smell like a hack (which means you probably want to change the padding rather than introducing a margin) and doesn't make it sound like this toggle is now and forever going be the reference for other widgets.

Comment 10

[Johann Hofmann [:johannh]]

(In reply to Dão Gottwald [::dao] from comment #9)
> doesn't make it sound like this toggle
> is now and forever going be the reference for other widgets.

If someone ever ends up making a bigger separated element on the right hand side they increase the number after noticing that the alignment is off. That doesn't sound so bad to maintain to me.

The alternative would be to increase the spacing on all elements and make it negative on the TP toggle, which sounds okay, but it also raises the question "why do these elements have this spacing?" and the answer is "because some other element made it necessary by being wider".

Explicitly making it clear that the margin is increased because we're trying to match the biggest element still sounds like the best way to do it for me, because that's the entire truth. We can try to hide around the fact, but these elements wouldn't have a bigger margin if not for the TP button.

Comment 11

[Dão Gottwald [:dao]]

(In reply to Johann Hofmann [:johannh] from comment #10)
> (In reply to Dão Gottwald [::dao] from comment #9)
> > doesn't make it sound like this toggle
> > is now and forever going be the reference for other widgets.
> 
> If someone ever ends up making a bigger separated element on the right hand
> side they increase the number after noticing that the alignment is off. That
> doesn't sound so bad to maintain to me.

No, the standard approach should be to make your custom widget work in the available space rather than changing all others.

Similarly, if somebody breaks the TP toggle to consume more or less space, they should probably undo that rather than changing the others because they thought the TP toggle was the reference and free to define whatever width it wants.

The bottom line is that the column width is basically a magic number, and the reason why it was changed isn't all that interesting going forward.

> The alternative would be to increase the spacing on all elements and make it
> negative on the TP toggle, which sounds okay, but it also raises the
> question "why do these elements have this spacing?" and the answer is
> "because some other element made it necessary by being wider".

This is what I think you want to change in order to make these buttons wider: https://searchfox.org/mozilla-central/rev/d0a41d2e7770fc00df7844d5f840067cc35ba26f/browser/themes/shared/customizableui/panelUI.inc.css#1045

Comment 12

[Johann Hofmann [:johannh]]

Ok, so, I'll just make this padding larger then. That sounds reasonable to me.

Comment 13

[Johann Hofmann [:johannh]]

Attachment: Bug 1465337 - Adjust tracking protection toggle and other right-hand-side buttons in the main menu to align their separators.

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

Comment 14

[Dão Gottwald [:dao]]

Comment on attachment 8986932 [details]
Bug 1465337 - Adjust tracking protection toggle and other right-hand-side buttons in the main menu to align their separators.

https://reviewboard.mozilla.org/r/252160/#review258720

::: browser/themes/shared/customizableui/panelUI.inc.css:1041
(Diff revision 1)
>  .PanelUI-subView .toolbaritem-combined-buttons > .subviewbutton {
>    -moz-box-flex: 0;
>    height: auto;
>    margin-inline-start: 18px;
>    min-width: auto;
> -  padding: 4px;
> +  padding: 7px;

We probably want to change only the horizontal padding and keep the vertical padding as-is.

Your first patch reduced #appMenu-tp-toggle's horizontal margin. Can we still do this so we don't have to increase the padding here by as much?

Comment 15

[Johann Hofmann [:johannh]]

Comment on attachment 8986932 [details]
Bug 1465337 - Adjust tracking protection toggle and other right-hand-side buttons in the main menu to align their separators.

https://reviewboard.mozilla.org/r/252160/#review258720

> We probably want to change only the horizontal padding and keep the vertical padding as-is.
> 
> Your first patch reduced #appMenu-tp-toggle's horizontal margin. Can we still do this so we don't have to increase the padding here by as much?

Hah, right, I had made that change in the browser toolbox and not copied it correctly...

Sounds like a good idea.

Comment 16

[Johann Hofmann [:johannh]]

Comment on attachment 8986932 [details]
Bug 1465337 - Adjust tracking protection toggle and other right-hand-side buttons in the main menu to align their separators.

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

Comment 17

[Dão Gottwald [:dao]]

Comment on attachment 8986932 [details]
Bug 1465337 - Adjust tracking protection toggle and other right-hand-side buttons in the main menu to align their separators.

https://reviewboard.mozilla.org/r/252160/#review259372

Comment 18

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/4cf608b3c77a
Adjust tracking protection toggle and other right-hand-side buttons in the main menu to align their separators. r=dao

Comment 19

[Eliza Balazs [:ebalazs_]]

https://hg.mozilla.org/mozilla-central/rev/4cf608b3c77a

Comment 20

[Johann Hofmann [:johannh]]

Comment on attachment 8986932 [details]
Bug 1465337 - Adjust tracking protection toggle and other right-hand-side buttons in the main menu to align their separators.

Approval Request Comment
[Feature/Bug causing the regression]: Bug 1462468
[User impact if declined]: The main ("hamburger") menu looks a bit ugly because separators are not aligned.
[Is this code covered by automated tests?]: No, visual issue only
[Has the fix been verified in Nightly?]: I can see it fixed in Nightly
[Needs manual test from QE? If yes, steps to reproduce]: Not really
[List of other uplifts needed for the feature/fix]: None
[Is the change risky?]: No
[Why is the change risky/not risky?]: A few lines of CSS
[String changes made/needed]: None

Comment 21

[Liz Henry (:lizzard) (relman/hg->git project)]

Comment on attachment 8986932 [details]
Bug 1465337 - Adjust tracking protection toggle and other right-hand-side buttons in the main menu to align their separators.

Polish for a new addition to the menu in 62, low risk, let's put it in beta 4.

Comment 22

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

https://hg.mozilla.org/releases/mozilla-beta/rev/93c4df61e1d1

Comment 23

[Cristian Comorasu, QA [:ccomorasu], Releae Desktop QA - Please contact [e-mail]]

I can confirm this issue is fixed, I verified using Fx 63.0a1, build ID: 20180723100101 and Fx 62.0b10, on Windows 10 x64, Ubuntu 14.04 LTS and mac OS 10.13.5 beta.