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?

(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.

(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.

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.

(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.

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