Closed Bug 1293967 Opened 3 years ago Closed 3 years ago

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

Categories

(Firefox :: Theme, defect, P3)

defect

Tracking

()

RESOLVED FIXED
Firefox 51
Tracking Status
firefox51 --- fixed

People

(Reporter: dao, Assigned: dao)

References

(Blocks 1 open bug)

Details

Attachments

(1 file)

Attached patch patchSplinter Review
No description provided.
Attachment #8779691 - Flags: review?(paolo.mozmail)
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.
Attachment #8779691 - Flags: review?(paolo.mozmail) → review?(jaws)
(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 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.
Attachment #8779691 - Flags: review?(jaws) → review+
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.
Attachment #8779691 - Flags: review+ → review?(jaws)
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.
Attachment #8779691 - Flags: review?(jaws) → review+
Pushed by dgottwald@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/386aceaf3cc1
Use --panel-separator-color instead of hsla(210,4%,10%,...) more consistently. r=jaws
https://hg.mozilla.org/mozilla-central/rev/386aceaf3cc1
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 51
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.
Flags: needinfo?(dao+bmo)
Depends on: 1295929
Flags: needinfo?(dao+bmo)
Depends on: 1297091
You need to log in before you can comment on or make changes to this bug.