924388 - There should be small separators between the sub-edit controls and sub-zoom controls when each controls is placed in a toolbar

Status: RESOLVED FIXED

(Firefox :: Theme, defect)

Comment 1

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

Attachment: Patch

After this patch is landed, we can update the OS X styles to use this class. (or just do that in this patch)

Comment 2

[:Gijs (out for now; he/him)]

Comment on attachment 814415 [details] [diff] [review]
Patch

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

::: browser/themes/windows/browser.css
@@ +440,5 @@
>  }
>  
>  #nav-bar .toolbarbutton-1:not(:hover):not(:active):not([open]) > .toolbarbutton-menubutton-dropmarker::before,
> +#nav-bar .toolbaritem-with-separator > .toolbarbutton-1:not(:active):not([open]) + .toolbarbutton-1:not(:hover):not(:active):not([open])::before,
> +#nav-bar .toolbaritem-with-separator > .toolbarbutton-1[disabled]:not(:active):not([open]) + .toolbarbutton-1[disabled]:not(:active):not([open])::before {

Why the separate [disabled] rule?

Comment 3

[:Gijs (out for now; he/him)]

Comment on attachment 814415 [details] [diff] [review]
Patch

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

r=me, but we should probably make sure that we deal with OS X and Linux as well.

Comment 4

[Dão Gottwald [:dao]]

Comment on attachment 814415 [details] [diff] [review]
Patch

"toolbaritem-with-separator" suggests that there will be one separator in total, but there can be multiple separators, the exact number being dependent on the number of buttons. Please come up with a better class name.

Comment 5

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

(In reply to :Gijs Kruitbosch from comment #2)
> Comment on attachment 814415 [details] [diff] [review]
> Patch
> 
> Review of attachment 814415 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: browser/themes/windows/browser.css
> @@ +440,5 @@
> >  }
> >  
> >  #nav-bar .toolbarbutton-1:not(:hover):not(:active):not([open]) > .toolbarbutton-menubutton-dropmarker::before,
> > +#nav-bar .toolbaritem-with-separator > .toolbarbutton-1:not(:active):not([open]) + .toolbarbutton-1:not(:hover):not(:active):not([open])::before,
> > +#nav-bar .toolbaritem-with-separator > .toolbarbutton-1[disabled]:not(:active):not([open]) + .toolbarbutton-1[disabled]:not(:active):not([open])::before {
> 
> Why the separate [disabled] rule?

This is because we still want to show the separator in the case that the buttons are disabled (since we aren't going to be showing the button outline in this case). I forgot to remove the :active from that rule too, will do it with the next patch.

(In reply to Dão Gottwald [:dao] from comment #4)
> Comment on attachment 814415 [details] [diff] [review]
> Patch
> 
> "toolbaritem-with-separator" suggests that there will be one separator in
> total, but there can be multiple separators, the exact number being
> dependent on the number of buttons. Please come up with a better class name.

Talked over IRC:
<jaws> dao: what do you think of "toolbaritem-combined-buttons" instead of "toolbaritem-with-separator" ?
<dao> jaws: that sounds better

Comment 6

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

https://hg.mozilla.org/projects/ux/rev/859ff4a1af98

(In reply to :Gijs Kruitbosch from comment #3)
> Comment on attachment 814415 [details] [diff] [review]
> Patch
> 
> Review of attachment 814415 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> r=me, but we should probably make sure that we deal with OS X and Linux as
> well.

Updated the OSX styling. Since Linux doesn't have the combined toolbar button styling yet there weren't any changes to make for Linux. That work is tracked in bug 875479.

Comment 7

[:Gijs (out for now; he/him)]

https://hg.mozilla.org/mozilla-central/rev/859ff4a1af98