Closed Bug 963083 Opened 6 years ago Closed 6 years ago

Background tabs should inherit the text color

Categories

(Firefox :: Theme, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
Firefox 30
Tracking Status
firefox29 --- fixed
firefox30 --- fixed

People

(Reporter: dao, Assigned: dao)

References

Details

(Whiteboard: [Australis:P5])

Attachments

(3 files, 1 obsolete file)

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.
Attached patch patch (obsolete) — Splinter Review
Attachment #8364340 - Flags: review?(MattN+bmo)
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.
Attachment #8364340 - Flags: review?(MattN+bmo) → review-
Attached patch patch v2Splinter Review
Attachment #8364340 - Attachment is obsolete: true
Attachment #8368473 - Flags: review?(MattN+bmo)
Whiteboard: [Australis:P5]
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
Attachment #8368473 - Flags: review?(MattN+bmo) → feedback+
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
Attachment #8368473 - Flags: review?(MattN+bmo)
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.
Attachment #8368473 - Flags: review?(MattN+bmo) → review+
It doesn't even matter practically, as CaptionText is just black with the compositor enabled...
https://hg.mozilla.org/mozilla-central/rev/124eee46ccb6
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 30
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.
(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.
Depends on: 968442
See Also: → 968782
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.
Flags: needinfo?(dao)
This is just cleanup. Uplifting it might make sense to reduce the risk of differences between aurora and central affecting other uplifted patches.
Flags: needinfo?(dao)
(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!
Attached patch aurora patchSplinter Review
[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
Attachment #8381341 - Flags: approval-mozilla-aurora?
Attachment #8381341 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
QA Whiteboard: [qa-]
You need to log in before you can comment on or make changes to this bug.