1298659 - Remove hardcoded colors from searchbar.css and make it more consistent across platforms

Status: RESOLVED FIXED

(Firefox :: Theme, defect, P3)

Description

[Dão Gottwald [::dao]]

Attachment: patch

Comment 1

[Florian Quèze [:florian]]

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

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

Looks reasonable. r- because I think the hardcoded background-image issue needs to be addressed.

The .search-setting-button changes may be fine, but I'm not sure I understand them so I would like if you could explain.

::: browser/themes/osx/searchbar.css
@@ +162,1 @@
>    background-image: url('data:image/png;base64,iVBORw0KGgoAAAANSUhEUgAAAAEAAAAWCAYAAAABxvaqAAAABmJLR0QA/wD/AP+gvaeTAAAACXBIWXMAAAsTAAALEwEAmpwYAAAAB3RJTUUH3gofECQNNVW2/AAAABBJREFUGFdjOHPmzH8GehEA/KpKg9YTf4AAAAAASUVORK5CYII=');

Unless I'm misremembering, this image hardcodes the #ccc value, so you'll need to replace it with something else.

We may be able to replace it with:

background-image: linear-gradient(transparent 15%,var(--panel-separator-color) 15%,var(--panel-separator-color) 85%, transparent 85%)
background-position: calc(100% - 1px) center;

(I haven't tested this. It's just an idea based on Tim's comments in bug 1206709.)

@@ -282,5 @@
>  }
>  
>  .search-setting-button[selected] {
> -  background-color: #d3d3d3;
> -  border-top-color: #bdbebe;

Why is the border-top-color rule no longer needed?

::: browser/themes/windows/searchbar.css
@@ -290,5 @@
>    -moz-appearance: none;
> -  border-bottom: none;
> -  border-left: none;
> -  border-right: none;
> -  -moz-border-top-colors: none;

Are we sure this isn't needed?

Comment 2

[Dão Gottwald [::dao]]

(In reply to Florian Quèze [:florian] [:flo] from comment #1)
> >  .search-setting-button[selected] {
> > -  background-color: #d3d3d3;
> > -  border-top-color: #bdbebe;
> 
> Why is the border-top-color rule no longer needed?
> 
> ::: browser/themes/windows/searchbar.css
> @@ -290,5 @@
> >    -moz-appearance: none;
> > -  border-bottom: none;
> > -  border-left: none;
> > -  border-right: none;
> > -  -moz-border-top-colors: none;
> 
> Are we sure this isn't needed?

This element also has the search-panel-header class which takes care of that.

Comment 3

[Dão Gottwald [::dao]]

Attachment: patch

Comment 4

[Florian Quèze [:florian]]

(In reply to Dão Gottwald [:dao] from comment #2)
> (In reply to Florian Quèze [:florian] [:flo] from comment #1)
> > >  .search-setting-button[selected] {
> > > -  background-color: #d3d3d3;
> > > -  border-top-color: #bdbebe;
> > 
> > Why is the border-top-color rule no longer needed?
> > 
> > ::: browser/themes/windows/searchbar.css
> > @@ -290,5 @@
> > >    -moz-appearance: none;
> > > -  border-bottom: none;
> > > -  border-left: none;
> > > -  border-right: none;
> > > -  -moz-border-top-colors: none;
> > 
> > Are we sure this isn't needed?
> 
> This element also has the search-panel-header class which takes care of that.

I'm not sure the rules in search-panel-header are enough to no longer need the -moz-border-top-colors.
I vaguely recall that setting the border-top property wasn't enough to override the -moz-border-top-colors values from toolkit/themes/windows/global/button.css. It was several years ago though, so things may have changed.

When testing the patch, the separator are significantly lighter than without the patch, so I'm not sure --panel-separator-color is the right color. When comparing the colors with the hamburger panel, --panel-separator-color seems to be the color used for the borders of the zoom and copy/paste controls. I wonder if we would want instead to have the same colors as for the borders of the Help and Customize buttons.

Comment 5

[Dão Gottwald [::dao]]

I tested the patch on Windows, there didn't seem to be any issues with not explicitly removing -moz-border-top-colors. I think border:none does that automatically?

We use --panel-separator-color all over the place in various panels, not just for the zoom and edit controls.

Comment 6

[Dão Gottwald [::dao]]

(In reply to Dão Gottwald [:dao] from comment #5)
> I tested the patch on Windows, there didn't seem to be any issues with not
> explicitly removing -moz-border-top-colors. I think border:none does that
> automatically?

DOM Inspector confirms that this is the case.

> We use --panel-separator-color all over the place in various panels, not
> just for the zoom and edit controls.

Btw, this includes the Help and Customize buttons.

Comment 7

[Florian Quèze [:florian]]

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

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

(In reply to Dão Gottwald [:dao] from comment #6)

OK. I guess the difference in the visible color is due to the different background color and the transparency of the border color.

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/9c52e335a783
Remove hardcoded colors from searchbar.css and make it more consistent across platforms. r=florian

Comment 9

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/9c52e335a783