Closed Bug 1465337 Opened 6 years ago Closed 6 years ago

Tracking protection toggle and sync button should have their separators aligned

Categories

(Firefox :: Menus, enhancement, P1)

Product:
Firefox
Component:
Menus
Type:
enhancement

Tracking

()

Status:
VERIFIED FIXED
Milestone:
Firefox 63
Tracking Flags:
Tracking Status
firefox62 --- verified
firefox63 --- verified

People

(Reporter: johannh, Assigned: johannh)

References

(Blocks 1 open bug)

Details

Attachments

(1 file, 1 obsolete file)

Bug 1465337 - Adjust tracking protection toggle and other right-hand-side buttons in the main menu to align their separators.
59 bytes, text/x-review-board-request
dao
: review+
lizzard
: approval-mozilla-beta+
Details 
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
The zoom|fullscreen separator has to be realigned with them too (in case you move the sync button separator).
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 {
Attachment #8982494 - Flags: review?(dao+bmo)
(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?
Flags: needinfo?(dao+bmo)
(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.
Flags: needinfo?(dao+bmo)
(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.
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.
(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.
(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.
(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
Ok, so, I'll just make this padding larger then. That sounds reasonable to me.
Attachment #8982494 - Attachment is obsolete: true
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?
Attachment #8986932 - Flags: review?(dao+bmo)
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 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
Attachment #8986932 - Flags: review?(dao+bmo) → review+
Pushed by dgottwald@mozilla.com:
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
https://hg.mozilla.org/mozilla-central/rev/4cf608b3c77a
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
status-firefox63: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 63
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
Attachment #8986932 - Flags: approval-mozilla-beta?
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.
Attachment #8986932 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
QA Contact: cristian.comorasu
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.
Status: RESOLVED → VERIFIED
status-firefox62: fixed → verified
status-firefox63: fixed → verified
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: