963083 - Background tabs should inherit the text color

Status: RESOLVED FIXED

(Firefox :: Theme, defect)

Description

[Dão Gottwald [::dao]]

Since background tabs are transparent, it makes sense for them to just inherit the text color rather than having it set explicitly to the right value. On Windows and Linux, we already set the right text colors (e.g. CaptionText, -moz-menubartext) on #TabsToolbar.

Comment 1

[Dão Gottwald [::dao]]

Attachment: patch

Comment 2

[Matthew N. [:MattN] (PM me if requests are blocking you)]

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

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

This made non-LWT tabs have white text on OS X inherited from the global skin. See the screenshot to follow.

Comment 3

[Matthew N. [:MattN] (PM me if requests are blocking you)]

Attachment: attachment 8364340 screenshot on OS X non-LWT

Comment 4

[Dão Gottwald [::dao]]

Attachment: patch v2

Comment 5

[Matthew N. [:MattN] (PM me if requests are blocking you)]

Comment on attachment 8368473 [details] [diff] [review]
patch v2

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

::: browser/themes/windows/browser.css
@@ -1532,5 @@
>    -moz-image-region: rect(0, 64px, 16px, 48px) !important;
>  }
>  
> -#main-window[tabsintitlebar]:not([inFullscreen]) .tabbrowser-tab:not([selected]):not(:-moz-lwtheme) {
> -  color: CaptionText;

AFAICT, this would be a change in behaviour for Aero Glass and Win8 because CaptionText isn't set on #TabsToolbar for -moz-windows-compositor. Are you sure we want to do this?

https://mxr.mozilla.org/mozilla-central/source/browser/themes/windows/browser.css?rev=b1d99c879787&mark=85-91#78

Comment 6

[Dão Gottwald [::dao]]

Comment on attachment 8368473 [details] [diff] [review]
patch v2

(In reply to Matthew N. [:MattN] from comment #5)
> Comment on attachment 8368473 [details] [diff] [review]
> patch v2
> 
> Review of attachment 8368473 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: browser/themes/windows/browser.css
> @@ -1532,5 @@
> >    -moz-image-region: rect(0, 64px, 16px, 48px) !important;
> >  }
> >  
> > -#main-window[tabsintitlebar]:not([inFullscreen]) .tabbrowser-tab:not([selected]):not(:-moz-lwtheme) {
> > -  color: CaptionText;
> 
> AFAICT, this would be a change in behaviour for Aero Glass and Win8 because
> CaptionText isn't set on #TabsToolbar for -moz-windows-compositor. Are you
> sure we want to do this?

Yes, the color for that text was supposed to be set here: http://hg.mozilla.org/mozilla-central/annotate/f550b112a19b/browser/themes/windows/browser-aero.css#l132

Comment 7

[Matthew N. [:MattN] (PM me if requests are blocking you)]

Comment on attachment 8368473 [details] [diff] [review]
patch v2

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

OK, if you're sure this is what the UI team wants, then it's fine with me. If you're unsure, please check with them.

Comment 8

[Dão Gottwald [::dao]]

It doesn't even matter practically, as CaptionText is just black with the compositor enabled...

Comment 9

[Dão Gottwald [::dao]]

https://hg.mozilla.org/integration/fx-team/rev/124eee46ccb6

Comment 10

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/124eee46ccb6

Comment 11

[:Gijs (he/him)]

This broke text-shadow on selected tabs on OSX without lwthemes - they now have a dark text shadow, which looks awful. Backing out 124eee46ccb6 locally fixes it.

Comment 12

[Dão Gottwald [::dao]]

(In reply to :Gijs Kruitbosch from comment #11)
> This broke text-shadow on selected tabs on OSX without lwthemes - they now
> have a dark text shadow, which looks awful. Backing out 124eee46ccb6 locally
> fixes it.

attachment 8364489 [details] already showed this. I meant to fix this in the latest revision of my patch here, but somehow missed it. Filed bug 968442.

Comment 13

[:Gijs (he/him)]

Should this + bug 968442 be uplifted or are we OK leaving this as-is for Aurora? I'm not sure if this fixed any actual user-experienced bugs or just made the CSS more straightforward.

Comment 14

[Dão Gottwald [::dao]]

This is just cleanup. Uplifting it might make sense to reduce the risk of differences between aurora and central affecting other uplifted patches.

Comment 15

[:Gijs (he/him)]

(In reply to Dão Gottwald [:dao] from comment #14)
> This is just cleanup. Uplifting it might make sense to reduce the risk of
> differences between aurora and central affecting other uplifted patches.

Ah, right. Perhaps, what with the win8 styling and all the other 'tweaking' we're still doing, we should consider doing that... Would you mind putting up a combined patch and requesting approval? Thanks!

Comment 16

[Dão Gottwald [::dao]]

Attachment: aurora patch

[Approval Request Comment]
Bug caused by (feature/regressing bug #): Australis tab styling
User impact if declined: no user impact, see comment 14
Testing completed (on m-c, etc.): landed on mozilla-central some weeks ago
Risk to taking this patch (and alternatives if risky): low
String or IDL/UUID changes made by this patch: none

Comment 17

[Dão Gottwald [::dao]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/2d1f9c800085