Dão Gottwald [:dao] Assignee
	Description • 9 years ago

In bug 1173743, we made toolbar buttons semitransparent black with a black border or semitransparent white with a white border, respectively. This works well for lightweight themes with simple backgrounds, but not so much for e.g. dark themes with brighter portions. Hence I think we should go back to using a white background with a black border.

	:Gijs (he/him)
	Comment 1 • 9 years ago

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

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

::: browser/themes/windows/browser.css
@@ +65,5 @@
> +  --toolbarbutton-active-background: rgba(255,255,255,.5);
> +
> +  --toolbarbutton-active-background: rgba(70%,70%,70%,.25);
> +  --toolbarbutton-active-bordercolor: rgba(0,0,0,.7);
> +  --toolbarbutton-active-boxshadow: 0 0 2px rgba(0,0,0,.6) inset;

I am not a visual designer, but I find that the box-shadow looks a little off on light themes (e.g. Linen Light, which we ship) for the active state. On dark themes I essentially don't/can't see it (which makes some amount of sense given the color...). Do you feel we really need this and/or was there a particular reason to add it? Could we just keep it as none ?

	Tim Nguyen :ntim
	Comment 2 • 9 years ago

I'm not sure this is an improvement for the light lightweight themes.

	Tim Nguyen :ntim
	Comment 3 • 9 years ago

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

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

::: browser/themes/windows/browser.css
@@ +63,5 @@
> +#nav-bar:-moz-lwtheme {
> +  --toolbarbutton-hover-bordercolor: rgba(0,0,0,.5);
> +  --toolbarbutton-active-background: rgba(255,255,255,.5);
> +
> +  --toolbarbutton-active-background: rgba(70%,70%,70%,.25);

--toolbar-active-background is being defined twice.

	Tim Nguyen :ntim
	Comment 4 • 9 years ago

(In reply to Dão Gottwald [:dao] from comment #0)
> Created attachment 8674208 [details] [diff] [review]
> patch
> 
> In bug 1173743, we made toolbar buttons semitransparent black with a black
> border or semitransparent white with a white border, respectively. This
> works well for lightweight themes with simple backgrounds, but not so much
> for e.g. dark themes with brighter portions.
I think a better fix would be increasing the navbar background opacity rather than this. Like this, there will be less contrast differences on dark themes with brighter portions.
Either way, the spec seems to have a higher navbar opacity: https://people.mozilla.org/~shorlander/mockups/Windows-10/Windows-10.html

> Hence I think we should go back to using a white background with a black border.
I don't think we ever did that (on Windows at least).

	Dão Gottwald [:dao] Assignee
	Comment 5 • 9 years ago

(In reply to Tim Nguyen [:ntim] from comment #4)
> I think a better fix would be increasing the navbar background opacity
> rather than this. Like this, there will be less contrast differences on dark
> themes with brighter portions.

This wouldn't help at all for dark themes. The background would get lighter across the board, which would decrease contrast with toolbar buttons; exactly what we don't want here. However, if we made nav bar bright enough, we could just use black as the toolbar button color regardless of the theme colors. This doesn't necessarily sound like a better solution to me, though.

> > Hence I think we should go back to using a white background with a black border.
> I don't think we ever did that (on Windows at least).

Not exactly semitransparent white + black, but we used a light background with a darker border since about forever.

	Dão Gottwald [:dao] Assignee
	Comment 6 • 9 years ago

(In reply to Tim Nguyen [:ntim] from comment #3)
> Comment on attachment 8674208 [details] [diff] [review]
> patch
> 
> Review of attachment 8674208 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: browser/themes/windows/browser.css
> @@ +63,5 @@
> > +#nav-bar:-moz-lwtheme {
> > +  --toolbarbutton-hover-bordercolor: rgba(0,0,0,.5);
> > +  --toolbarbutton-active-background: rgba(255,255,255,.5);
> > +
> > +  --toolbarbutton-active-background: rgba(70%,70%,70%,.25);
> 
> --toolbar-active-background is being defined twice.

Oops, I missed that comment and just discovered the same problem myself. The first one should have been --toolbarbutton-hover-background. I fixed this and also tweaked the border colors based on that.

	Dão Gottwald [:dao] Assignee
	Comment 7 • 9 years ago

I did keep the box-shadow for :active since otherwise the buttons don't really look depressed. For the non-lwt case we use a box-shadow without any blur making it look like a 2px border, but that doesn't really work here due to the border being rendered next to the background and the box-shadow being rendered on top of it.

So we shouldn't remove the shadow but we might want to tweak it. I couldn't seem to reproduce the issues from comment 1, though.

	Pulsebot
	Comment 8 • 9 years ago

https://hg.mozilla.org/integration/fx-team/rev/398514bdc83f

	Dão Gottwald [:dao] Assignee
	Comment 9 • 9 years ago

... aaand there was another typo

	Pulsebot
	Comment 10 • 9 years ago

https://hg.mozilla.org/integration/fx-team/rev/1a46c1fd048b

	Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)
	Comment 11 • 9 years ago

https://hg.mozilla.org/mozilla-central/rev/398514bdc83f
https://hg.mozilla.org/mozilla-central/rev/1a46c1fd048b

	Cornel Ionce [:noni] [Hubs QA]
	Comment 12 • 9 years ago

Confirming the fix using several lightweight themes on latest Nightly, buildID 20151018030250.
Tested on Windows 10 32-bit, Windows 8.1 64-bit and Windows 7 64-bit.

	Dão Gottwald [:dao] Assignee
	Comment 13 • 9 years ago

Approval Request Comment
[Feature/regressing bug #]: bug 1173743
[User impact if declined]: see comment 0
[Describe test coverage new/current, TreeHerder]:
[Risks and why]: very straightforward change, low risk
[String/UUID change made/needed]:

	Liz Henry (:lizzard Please n-i to RyanVM, jcristau, or pascal)
	Comment 14 • 9 years ago

Marking 43 as affected since this change appears to have happened then.

	Liz Henry (:lizzard Please n-i to RyanVM, jcristau, or pascal)
	Comment 15 • 9 years ago

Comment on attachment 8676129 [details] [diff] [review]
patch for uplift

Theme color fix for better contrast, ok to uplift to aurora.

	Carsten Book [:Tomcat]
	Comment 16 • 9 years ago

https://hg.mozilla.org/releases/mozilla-aurora/rev/51cfbff86756
https://hg.mozilla.org/releases/mozilla-aurora/rev/6219f59cbfbf

	Cornel Ionce [:noni] [Hubs QA]
	Comment 17 • 9 years ago

I've also verified this issue on Firefox 43 beta 1 (build ID: 20151103023037) on the platforms mentioned in comment 12.