Closed Bug 1215093 Opened 5 years ago Closed 5 years ago

For lightweight themes, make toolbar buttons semitransparent white with a black border on :hover/:active

Categories

(Firefox :: Theme, defect)

Unspecified
Windows
defect
Not set

Tracking

()

VERIFIED FIXED
Firefox 44
Tracking Status
firefox42 --- unaffected
firefox43 --- verified
firefox44 --- verified

People

(Reporter: dao, Assigned: dao)

References

Details

Attachments

(3 files, 2 obsolete files)

Attached patch patch (obsolete) — Splinter Review
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.
Attachment #8674208 - Flags: review?(gijskruitbosch+bugs)
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 ?
Attachment #8674208 - Flags: review?(gijskruitbosch+bugs) → review+
Attached image before-after.png (obsolete) —
I'm not sure this is an improvement for the light lightweight themes.
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.
(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).
(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.
Attached patch patch v2Splinter Review
(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.
Attachment #8674208 - Attachment is obsolete: true
Attachment #8674310 - Attachment is obsolete: true
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.
Attached patch followupSplinter Review
... aaand there was another typo
Flags: qe-verify+
QA Contact: cornel.ionce
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.
Status: RESOLVED → VERIFIED
Attached patch patch for upliftSplinter Review
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]:
Attachment #8676129 - Flags: approval-mozilla-aurora?
Marking 43 as affected since this change appears to have happened then.
Comment on attachment 8676129 [details] [diff] [review]
patch for uplift

Theme color fix for better contrast, ok to uplift to aurora.
Attachment #8676129 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Depends on: 1218083
I've also verified this issue on Firefox 43 beta 1 (build ID: 20151103023037) on the platforms mentioned in comment 12.
Depends on: 1229694
You need to log in before you can comment on or make changes to this bug.