1293967 - Use --panel-separator-color instead of hsla(210,4%,10%,...) more consistently

Status: RESOLVED FIXED

(Firefox :: Theme, defect, P3)

Description

[Dão Gottwald [:dao]]

Attachment: patch

Comment 1

[:Paolo Amadini]

Comment on attachment 8779691 [details] [diff] [review]
patch

Thanks! One notable feature of this patch on Windows and Linux is that it removes the transparency of the separators entirely, while previously the same separators had only a 10-15% effect, and the remaining 85-90% was the underlying color.

As far as I know this might be good and even make things better for the high contrast case, but given that the change is substantial I think someone with more experience on the theme like Jared or Gijs should review the patch.

Comment 2

[Dão Gottwald [:dao]]

(In reply to :Paolo Amadini from comment #1)
> Comment on attachment 8779691 [details] [diff] [review]
> patch
> 
> Thanks! One notable feature of this patch on Windows and Linux is that it
> removes the transparency of the separators entirely, while previously the
> same separators had only a 10-15% effect, and the remaining 85-90% was the
> underlying color.

10-15% of a very dark color, that is. --panel-separator-color (ThreeDShadow on Linux, ThreeDLightShadow on Windows) is substantially lighter than that color. Also, the background color is always white or some shade of gray with default OS themes. So the net result is roughly the same.

Comment 3

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

Comment on attachment 8779691 [details] [diff] [review]
patch

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

r=me if you agree with me and use --separator-color. Otherwise please request review again.

::: browser/themes/windows/browser.css
@@ +1362,5 @@
>    padding: 2px 2px;
>  }
>  
>  #urlbar-search-footer {
> +  border-top: 1px solid var(--panel-separator-color);

This variable should have a different name. It doesn't make sense reading the code that the panel-separator-color is used for the urlbar-search-footer.

Is this really just --separator-color ? That name would be fine by me.

Comment 4

[Dão Gottwald [:dao]]

Comment on attachment 8779691 [details] [diff] [review]
patch

(In reply to Jared Wein [:jaws] (please needinfo? me) from comment #3)
> >  #urlbar-search-footer {
> > +  border-top: 1px solid var(--panel-separator-color);
> 
> This variable should have a different name. It doesn't make sense reading
> the code that the panel-separator-color is used for the urlbar-search-footer.

Why does it not make sense? It's the footer of the urlbar's autocomplete /panel/. I believe Paolo also wants to consolidate this styling further across different panels.

> Is this really just --separator-color ? That name would be fine by me.

--separator-color is too generic. E.g. we wouldn't want to use this for tab separators.

Comment 5

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

Comment on attachment 8779691 [details] [diff] [review]
patch

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

Okay, I didn't think about it that way. I was only thinking about "panel-separator" in terms of the subview left-border separator.

Comment 6

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/386aceaf3cc1
Use --panel-separator-color instead of hsla(210,4%,10%,...) more consistently. r=jaws

Comment 7

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/386aceaf3cc1

Comment 8

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

This separator color really is too light for the app menu in the normal case of non-high contrast etc. The darker lines were better for creating the edge of the subview, and now with the subview open there is not as much of a clear distinction between the subview footer and the main view footer, http://screencast.com/t/5PMCbwJNj

I think we should only apply ThreeDLightLightShadow when not -moz-windows-default-theme.