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

RESOLVED FIXED in Firefox 28

Status

()

RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: jaws, Assigned: jaws)

Tracking

Trunk
Firefox 28
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [Australis:M9][Australis:P5])

Attachments

(1 attachment, 4 obsolete attachments)

Created attachment 814911 [details] [diff] [review]
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.
Attachment #814911 - Flags: review?(mdeboer)
Blocks: 870032
Whiteboard: [Australis:M?][Australis:P5]
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?
Attachment #814911 - Flags: review-
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!
Attachment #814911 - Flags: review?(mdeboer)
Created attachment 814923 [details] [diff] [review]
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.
Attachment #814911 - Attachment is obsolete: true
Attachment #814923 - Flags: review?(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,
(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.
Created attachment 814999 [details] [diff] [review]
Patch v1.2

Now showing the zoom reset button on the toolbar.
Attachment #814923 - Attachment is obsolete: true
Attachment #814923 - Flags: review?(dao)
Attachment #814999 - Flags: review?(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.
(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.
(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.
Created attachment 815441 [details] [diff] [review]
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.
Attachment #814999 - Attachment is obsolete: true
Attachment #814999 - Flags: review?(dao)
Attachment #815441 - Flags: review?(dao)
Attachment #815441 - Flags: review?(gijskruitbosch+bugs)

Comment 12

5 years ago
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?
Attachment #815441 - Flags: review?(gijskruitbosch+bugs) → feedback+
(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.
(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.
Created attachment 819166 [details] [diff] [review]
Patch v1.4
Attachment #815441 - Attachment is obsolete: true
Attachment #815441 - Flags: review?(dao)
Attachment #819166 - Flags: review?(gijskruitbosch+bugs)
Attachment #819166 - Flags: review?(dao)

Comment 16

5 years ago
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.
Attachment #819166 - Flags: review?(gijskruitbosch+bugs) → review+
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.
Whiteboard: [Australis:M?][Australis:P5] → [Australis:M9][Australis:P5][fixed-in-ux]

Comment 18

5 years ago
https://hg.mozilla.org/mozilla-central/rev/54be7ecbc773
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Whiteboard: [Australis:M9][Australis:P5][fixed-in-ux] → [Australis:M9][Australis:P5]
Target Milestone: --- → Firefox 28
You need to log in before you can comment on or make changes to this bug.