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

RESOLVED FIXED in Firefox 51

Status

()

P3
normal
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: dao, Assigned: dao)

Tracking

(Blocks: 1 bug)

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

Firefox Tracking Flags

(firefox51 fixed)

Details

Attachments

(1 attachment)

(Assignee)

Description

2 years ago
Created attachment 8779691 [details] [diff] [review]
patch
Attachment #8779691 - Flags: review?(paolo.mozmail)

Comment 1

2 years ago
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)
(Assignee)

Comment 2

2 years ago
(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+
(Assignee)

Comment 4

2 years ago
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+

Comment 6

2 years ago
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

Comment 7

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/386aceaf3cc1
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
status-firefox51: affected → fixed
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)
(Assignee)

Updated

2 years ago
Depends on: 1295929
(Assignee)

Updated

2 years ago
Flags: needinfo?(dao+bmo)
(Assignee)

Updated

2 years ago
Depends on: 1297091
You need to log in before you can comment on or make changes to this bug.