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

RESOLVED FIXED in Firefox 51

Status

()

Firefox
Theme
P3
normal
RESOLVED FIXED
a year ago
a year ago

People

(Reporter: dao, Assigned: dao)

Tracking

(Blocks: 1 bug, {access})

Trunk
Firefox 51
access
Points:
---

Firefox Tracking Flags

(firefox51 fixed)

Details

Attachments

(1 attachment, 1 obsolete attachment)

10.60 KB, patch
florian
: review+
Details | Diff | Splinter Review
(Assignee)

Description

a year ago
Created attachment 8785670 [details] [diff] [review]
patch
Attachment #8785670 - Flags: review?(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?
Attachment #8785670 - Flags: review?(florian) → review-
(Assignee)

Comment 2

a year ago
(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.
(Assignee)

Comment 3

a year ago
Created attachment 8786766 [details] [diff] [review]
patch
Attachment #8785670 - Attachment is obsolete: true
Attachment #8786766 - Flags: review?(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.
(Assignee)

Comment 5

a year ago
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.
(Assignee)

Comment 6

a year ago
(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 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.
Attachment #8786766 - Flags: review?(florian) → review+

Comment 8

a year ago
Pushed by dgottwald@mozilla.com:
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

a year ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/9c52e335a783
Status: ASSIGNED → RESOLVED
Last Resolved: a year ago
status-firefox51: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 51
You need to log in before you can comment on or make changes to this bug.