Closed
Bug 963083
Opened 7 years ago
Closed 7 years ago
Background tabs should inherit the text color
Categories
(Firefox :: Theme, defect)
Firefox
Theme
Tracking
()
RESOLVED
FIXED
Firefox 30
People
(Reporter: dao, Assigned: dao)
References
Details
(Whiteboard: [Australis:P5])
Attachments
(3 files, 1 obsolete file)
347.48 KB,
image/png
|
Details | |
3.68 KB,
patch
|
MattN
:
review+
MattN
:
feedback+
|
Details | Diff | Splinter Review |
3.57 KB,
patch
|
Sylvestre
:
approval-mozilla-aurora+
|
Details | Diff | Splinter Review |
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.
Assignee | ||
Comment 1•7 years ago
|
||
Attachment #8364340 -
Flags: review?(MattN+bmo)
Comment 2•7 years ago
|
||
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-
Comment 3•7 years ago
|
||
Assignee | ||
Comment 4•7 years ago
|
||
Attachment #8364340 -
Attachment is obsolete: true
Attachment #8368473 -
Flags: review?(MattN+bmo)
Updated•7 years ago
|
Whiteboard: [Australis:P5]
Comment 5•7 years ago
|
||
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+
Assignee | ||
Comment 6•7 years ago
|
||
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 7•7 years ago
|
||
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+
Assignee | ||
Comment 8•7 years ago
|
||
It doesn't even matter practically, as CaptionText is just black with the compositor enabled...
Assignee | ||
Comment 9•7 years ago
|
||
https://hg.mozilla.org/integration/fx-team/rev/124eee46ccb6
Comment 10•7 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/124eee46ccb6
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 30
Comment 11•7 years ago
|
||
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.
Assignee | ||
Comment 12•7 years ago
|
||
(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.
Updated•7 years ago
|
status-firefox29:
--- → fixed
status-firefox30:
--- → affected
Updated•7 years ago
|
Comment 13•7 years ago
|
||
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)
Assignee | ||
Comment 14•7 years ago
|
||
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)
Comment 15•7 years ago
|
||
(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!
Assignee | ||
Comment 16•7 years ago
|
||
[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?
Updated•7 years ago
|
Attachment #8381341 -
Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Assignee | ||
Comment 17•7 years ago
|
||
https://hg.mozilla.org/releases/mozilla-aurora/rev/2d1f9c800085
Updated•7 years ago
|
QA Whiteboard: [qa-]
You need to log in
before you can comment on or make changes to this bug.
Description
•