924933 - Follow-up to fix the separator styling for the zoom controls

Status: RESOLVED FIXED

(Firefox :: Theme, defect)

Description

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

Attachment: sepHoverLogic.patch

The zoom-controls have the zoom-reset button between the zoom-out and zoom-in buttons, meaning that our styling for the separator is broken in this case because we are using styling rules that depend on the two visible buttons being adjacent siblings.

This patch rearranges the zoom controls buttons to put the zoom-reset button as the last child, and then uses -moz-box-ordinal-group to display them in the correct visible order when the zoom-controls are in the panel.

I also fixed some of the separator rules while in here while working on the patch as I noticed it didn't handle all the cases.

Comment 1

[Dão Gottwald [:dao]]

Comment on attachment 814911 [details] [diff] [review]
sepHoverLogic.patch

>+#nav-bar .toolbaritem-combined-buttons > .toolbarbutton-1:not(:hover):not(:active):not([open]) + .toolbarbutton-1:not(:hover):not(:active):not([open])::before,

:not(:active) is wrong here (try clicking on the button and moving the mouse pointer away without releasing the mouse button)

>+#nav-bar .toolbaritem-combined-buttons > .toolbarbutton-1:not(:hover):not(:active):not([open]) + .toolbarbutton-1[disabled]:hover:not([open])::before,

A disabled button cannot be open. Also, what is :hover doing there?

Comment 2

[Mike de Boer [:mikedeboer]]

Comment on attachment 814911 [details] [diff] [review]
sepHoverLogic.patch

I think Dão provided some great feedback already, before I finished building (he's like road-runner!).

Looking forward to your next patch!

Comment 3

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

Attachment: Patch v1.1

(In reply to Dão Gottwald [:dao] from comment #1)
> Comment on attachment 814911 [details] [diff] [review]
> sepHoverLogic.patch
> 
> >+#nav-bar .toolbaritem-combined-buttons > .toolbarbutton-1:not(:hover):not(:active):not([open]) + .toolbarbutton-1:not(:hover):not(:active):not([open])::before,
> 
> :not(:active) is wrong here (try clicking on the button and moving the mouse
> pointer away without releasing the mouse button)

Fixed.

> >+#nav-bar .toolbaritem-combined-buttons > .toolbarbutton-1:not(:hover):not(:active):not([open]) + .toolbarbutton-1[disabled]:hover:not([open])::before,
> 
> A disabled button cannot be open. Also, what is :hover doing there?

Fixed. I was thinking I that :hover was needed in this case but it's not.

Comment 4

[Dão Gottwald [:dao]]

Comment on attachment 814923 [details] [diff] [review]
Patch v1.1

>+#zoom-controls@inAnyPanel@ > #zoom-in-button {
>+  -moz-box-ordinal-group: 2;
>+}

I'm not a fan of this, but I have no better recommendation other than using the same layout regardless of where the zoom controls are. It's somewhat illogical that moving the controls to a more prominent place (meaning that the user cares about them) results in buttons being hidden. Or from the opposite perspective, if the Reset button isn't important enough to appear on the toolbar, we should just get rid of it in the panels as well.

>+#nav-bar .toolbaritem-combined-buttons > .toolbarbutton-1:not(:hover):not([open]) + .toolbarbutton-1:not(:hover):not([open])::before,
>+#nav-bar .toolbaritem-combined-buttons > .toolbarbutton-1:not(:hover):not([open]) + .toolbarbutton-1[disabled]::before,
>+#nav-bar .toolbaritem-combined-buttons > .toolbarbutton-1[disabled] + .toolbarbutton-1:not(:hover):not([open])::before {

Looks like this could be reduced to:

> #nav-bar .toolbaritem-combined-buttons > .toolbarbutton-1:-moz-any(:not(:hover):not([open]),[disabled]) + .toolbarbutton-1:-moz-any(:not(:hover):not([open]),[disabled])::before,

Comment 5

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

(In reply to Dão Gottwald [:dao] from comment #4)
> Comment on attachment 814923 [details] [diff] [review]
> Patch v1.1
> 
> >+#zoom-controls@inAnyPanel@ > #zoom-in-button {
> >+  -moz-box-ordinal-group: 2;
> >+}
> 
> I'm not a fan of this, but I have no better recommendation other than using
> the same layout regardless of where the zoom controls are. It's somewhat
> illogical that moving the controls to a more prominent place (meaning that
> the user cares about them) results in buttons being hidden. Or from the
> opposite perspective, if the Reset button isn't important enough to appear
> on the toolbar, we should just get rid of it in the panels as well.

I think that makes a good point to making the reset button visible on the toolbar. With the separators visible it will make the relationship between the three buttons more prominent too.

Comment 6

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

Attachment: Patch v1.2

Now showing the zoom reset button on the toolbar.

Comment 7

[Dão Gottwald [:dao]]

Comment on attachment 814999 [details] [diff] [review]
Patch v1.2

>+#zoom-controls[customizableui-areatype="toolbar"] > #zoom-reset-button {
>+  min-width: 45px;

Where does this number come from?

>+#zoom-controls[customizableui-areatype="toolbar"]:not(.overflowedItem) > #zoom-reset-button > .toolbarbutton-text {
>+  display: -moz-box;
> }

Looks like this should be in a content stylesheet.

Comment 8

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

(In reply to Dão Gottwald [:dao] from comment #7)
> Comment on attachment 814999 [details] [diff] [review]
> Patch v1.2
> 
> >+#zoom-controls[customizableui-areatype="toolbar"] > #zoom-reset-button {
> >+  min-width: 45px;
> 
> Where does this number come from?

I tried various values until I found one that looked good.

> >+#zoom-controls[customizableui-areatype="toolbar"]:not(.overflowedItem) > #zoom-reset-button > .toolbarbutton-text {
> >+  display: -moz-box;
> > }
> 
> Looks like this should be in a content stylesheet.

Sure, I can move this to browser/base/content/browser.css.

Comment 9

[Dão Gottwald [:dao]]

(In reply to Jared Wein [:jaws] from comment #8)
> (In reply to Dão Gottwald [:dao] from comment #7)
> > Comment on attachment 814999 [details] [diff] [review]
> > Patch v1.2
> > 
> > >+#zoom-controls[customizableui-areatype="toolbar"] > #zoom-reset-button {
> > >+  min-width: 45px;
> > 
> > Where does this number come from?
> 
> I tried various values until I found one that looked good.

Does this depend on the text size? If so, it shouldn't be a px value.

Comment 10

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

Attachment: Patch v1.3

I moved the min-width rule to be cross platform since having a set min-width helps here to make sure that the button doesn't get narrower when the zoom value gets below 100%. Tested on OS X and Windows.

Comment 12

[:Gijs (he/him)]

Comment on attachment 815441 [details] [diff] [review]
Patch v1.3

Review of attachment 815441 [details] [diff] [review]:
-----------------------------------------------------------------

::: browser/themes/shared/customizableui/panelUIOverlay.inc.css
@@ +381,5 @@
>    display: none;
>  }
>  
> +#zoom-controls[customizableui-areatype="toolbar"] > #zoom-reset-button {
> +  min-width: 7ch;

This doesn't make sense to me. The zoom text is 2-4 characters. Presumably this should be either calc(4ch + 20px) or something similar, if we're going for padding, or if the box model allows it (I forget), then the padding should be set separately and fixed. Unless we think the padding should be proportional to the text size... which might make sense? Or not? I'm not sure either way. If this is intentionally proportional, I guess it can stay like this.

::: browser/themes/windows/browser.css
@@ +390,5 @@
>    -moz-box-align: center;
>  }
>  
>  #nav-bar .toolbarbutton-1 > .toolbarbutton-icon,
> +#nav-bar .toolbarbutton-1 > .toolbarbutton-text,

I'm assuming that for this not to break stuff, the toolbarbutton text is generally display: none in the navbar anyway.

Note also, this is navbar specific - it'll break if you move the button outside the navbar, say, to the bookmarks toolbar or some other toolbar. Many if not all of the other styles are general for all toolbars, so this probably should be, too. Which raises the question about whether the other rules should be more general, too.

@@ +482,5 @@
>  
>  #nav-bar .toolbarbutton-1 > .toolbarbutton-menubutton-button:not([disabled]):not([open]):not(:active):hover > .toolbarbutton-icon,
>  #nav-bar .toolbarbutton-1:not([buttonover]):not([open]):not(:active):hover > .toolbarbutton-menubutton-dropmarker:not([disabled]) > .dropmarker-icon,
> +@conditionalForwardWithUrlbar@ > #forward-button:not([open]):not(:active):not([disabled]):hover > .toolbarbutton-icon,
> +@conditionalForwardWithUrlbar@ > #forward-button:not([open]):not(:active):not([disabled]):hover > .toolbarbutton-text {

Isn't the toolbarbutton-text for the forward button display: none anyway?

Comment 13

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

(In reply to :Gijs Kruitbosch from comment #12)
> Comment on attachment 815441 [details] [diff] [review]
> Patch v1.3
> 
> Review of attachment 815441 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: browser/themes/shared/customizableui/panelUIOverlay.inc.css
> @@ +381,5 @@
> >    display: none;
> >  }
> >  
> > +#zoom-controls[customizableui-areatype="toolbar"] > #zoom-reset-button {
> > +  min-width: 7ch;
> 
> This doesn't make sense to me. The zoom text is 2-4 characters. Presumably
> this should be either calc(4ch + 20px) or something similar, if we're going
> for padding, or if the box model allows it (I forget), then the padding
> should be set separately and fixed. Unless we think the padding should be
> proportional to the text size... which might make sense? Or not? I'm not
> sure either way. If this is intentionally proportional, I guess it can stay
> like this.

One 'ch' unit has the width of the '0' character in the element's font-size. The '1' character may be slightly narrower than the '0' character, but the '%' character is normally wider than the '0' character, sometimes twice the width.

Setting a min-width of 4ch with a 12px Segoe UI font on the #zoom-reset-button results in a 42px width. A 7ch min-width also results in 42px in this case. calc(4ch + 20px) results in 44px width. cacl(7ch + 20px) results in 62px.

Setting a max-width of 1ch in this case computes a 6px width. Oddly enough, setting a min-width to 7ch and is too small for a textContents of '00000' (it stops contracting when textContents is '0000' or shorter).

We could use 4em here, that seems to work fine on my Windows machine. I'll just need to test it with OS X.

Comment 14

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

(In reply to :Gijs Kruitbosch from comment #12)
> >  #nav-bar .toolbarbutton-1 > .toolbarbutton-menubutton-button:not([disabled]):not([open]):not(:active):hover > .toolbarbutton-icon,
> >  #nav-bar .toolbarbutton-1:not([buttonover]):not([open]):not(:active):hover > .toolbarbutton-menubutton-dropmarker:not([disabled]) > .dropmarker-icon,
> > +@conditionalForwardWithUrlbar@ > #forward-button:not([open]):not(:active):not([disabled]):hover > .toolbarbutton-icon,
> > +@conditionalForwardWithUrlbar@ > #forward-button:not([open]):not(:active):not([disabled]):hover > .toolbarbutton-text {
> 
> Isn't the toolbarbutton-text for the forward button display: none anyway?

I don't know why this was here. I've removed it.

Comment 15

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

Attachment: Patch v1.4

Comment 16

[:Gijs (he/him)]

Comment on attachment 819166 [details] [diff] [review]
Patch v1.4

Review of attachment 819166 [details] [diff] [review]:
-----------------------------------------------------------------

LGTM. Please can you point me to, or file if we don't have one, a bug about the navbar-specificity of some of this styling? I would have assumed the button should look the same if I dragged it to, say, the bookmarks toolbar.

Comment 17

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

https://hg.mozilla.org/projects/ux/rev/54be7ecbc773

Bug 734326 is on file to track getting the styling of these buttons on the bookmarks toolbar and other places.

Comment 18

[:Gijs (he/him)]

https://hg.mozilla.org/mozilla-central/rev/54be7ecbc773