1224713 - [dev theme] In devtools theme, text "Bookmarks" and "History" in sidebar is white-on-white

Status: VERIFIED FIXED

(Firefox :: Theme, defect)

Description

[arni2033]

Attachment: screenshot 1 - dev theme sidebar text white-on-white.png

STR:   (Win7_64, Nightly 45, 32bit, ID 20151113030248, new profile)
1. Open about:customizing , set devtools theme
2. Open sidebar (Ctrl+B or Ctrl+H)

Result:       Text in sidebar is white-on-white
Expectations: Text should be visible. I saw a screenshot on linux - it was white-on-black

Note: I found this issue long time ago, like most of bugs filed by me.

Comment 1

[:Gijs (he/him)]

Attachment: MozReview Request: Bug 1224713 - fix sidebar header text in devedition theme on linux and windows, r?bgrins

Bug 1224713 - fix sidebar header text in devedition theme, r?bgrins

Comment 2

[Brian Grinstead [:bgrins]]

Sorry for the delay, I'll take a look at this tomorrow

Comment 3

[Brian Grinstead [:bgrins]]

Comment on attachment 8688474 [details]
MozReview Request: Bug 1224713 - fix sidebar header text in devedition theme on linux and windows, r?bgrins

https://reviewboard.mozilla.org/r/25327/#review23217

I'm on board with the close icon fix, but take a look at this with a dark lightweight theme installed and you will see the same issue with the sidebar header.

::: browser/themes/windows/devedition.css:261
(Diff revision 1)
> +#sidebar-header {

This is a problem with dark lightweight themes as well, so I don't think the fix belongs here.  I think that this should be fixed at the source where the background color is set instead by hardcoding a color to be used there (or at least doing `color: initial`:  https://dxr.mozilla.org/mozilla-central/source/browser/themes/windows/browser-aero.css#55.

As an example, osx is setting `color: #535f6d;` on the title element (although I think this should probably be on the header object where the background color is set: https://dxr.mozilla.org/mozilla-central/source/browser/themes/osx/browser.css#2468

Comment 5

[:Gijs (he/him)]

(In reply to Brian Grinstead [:bgrins] from comment #3)
> Comment on attachment 8688474 [details]
> MozReview Request: Bug 1224713 - fix sidebar header text in devedition
> theme, r?bgrins
> 
> https://reviewboard.mozilla.org/r/25327/#review23217
> 
> I'm on board with the close icon fix, but take a look at this with a dark
> lightweight theme installed and you will see the same issue with the sidebar
> header.
> 
> ::: browser/themes/windows/devedition.css:261
> (Diff revision 1)
> > +#sidebar-header {
> 
> This is a problem with dark lightweight themes as well, so I don't think the
> fix belongs here.  I think that this should be fixed at the source where the
> background color is set instead by hardcoding a color to be used there (or
> at least doing `color: initial`: 
> https://dxr.mozilla.org/mozilla-central/source/browser/themes/windows/
> browser-aero.css#55.

So that would help on win7/vista, but on win8+ the header is still transparent. And that is deliberate, see: 

https://dxr.mozilla.org/mozilla-central/rev/22f51211915bf7daff076180847a7140d35aa353/toolkit/themes/windows/global/global.css#165

So instead I'm changing win7/vista to not apply that background in lwt mode. It seems the dark devedition theme doesn't get :-moz-lwtheme-brighttext, and so because of that and the win8/win10 issue, I'm still also fixing devedition.

I'm looking at linux next per the dupe that just got reported.

Comment 6

[:Gijs (he/him)]

Comment on attachment 8688474 [details]
MozReview Request: Bug 1224713 - fix sidebar header text in devedition theme on linux and windows, r?bgrins

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/25327/diff/1-2/

Comment 7

[:Gijs (he/him)]

Comment on attachment 8688474 [details]
MozReview Request: Bug 1224713 - fix sidebar header text in devedition theme on linux and windows, r?bgrins

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/25327/diff/2-3/

Comment 8

[:Gijs (he/him)]

For bonus points that last patch also fixes the findbar close icon on Linux with the dark devedition theme.

Comment 9

[:Gijs (he/him)]

https://reviewboard.mozilla.org/r/25327/#review26061

::: browser/themes/linux/devedition.css:81
(Diff revision 3)
> -/* Prevent devedition foreground color from seeping into the sidebar-box (since
> - * its background colors aren't affected by the devedition theme) */
> -#sidebar-box {
> +/* Fix the bad-looking text-shadow in the sidebar header: */
> +#sidebar-header {
> +  text-shadow: none;
> -  color: initial;

Note that the old color:initial here is from bug 1137089 which predated us moving to a lwtheme (bug 1148996), so now it does more harm than good (oops). I decided to just blanket rm the text-shadow because it looks bad and I don't see how removing that specifically in the devedition theme would do any harm. :-)

Comment 10

[Brian Grinstead [:bgrins]]

(In reply to :Gijs Kruitbosch from comment #5)
> So instead I'm changing win7/vista to not apply that background in lwt mode.
> It seems the dark devedition theme doesn't get :-moz-lwtheme-brighttext, and
> so because of that and the win8/win10 issue, I'm still also fixing
> devedition.

Ah right, because lwthemetextcolor is not set on the lightweight theme.  Toolbar button icons do the right thing because of the `toolbar[brighttext]` selector and https://dxr.mozilla.org/mozilla-central/rev/0771c5eab32f0cee4f7d12bc382298a81e0eabb2/browser/base/content/browser.js#7844.  I wonder if the :-moz-lwtheme-brighttext and :-moz-lwtheme-darktext pseudo classes could be removed and replaced with a similar mechanism plus an attribute on the root element - they don't seem very widely used and we could remove some code that way (not just in DE theme but also the DOM / CSS stuff that deals with those classes).

Comment 11

[Brian Grinstead [:bgrins]]

Comment on attachment 8688474 [details]
MozReview Request: Bug 1224713 - fix sidebar header text in devedition theme on linux and windows, r?bgrins

https://reviewboard.mozilla.org/r/25327/#review26251

This looks good - thank you very much for doing the investigation and fixing this!

Do we not need to worry about .sidebar-header here?  I see some existing windows selectors cover both .sidebar-header and #sidebar-header but the only element that it looks like this matches is the social-sidebar-header: https://dxr.mozilla.org/mozilla-central/source/browser/base/content/browser.xul#1072.  I'm not sure how to toggle that to test it.

Comment 12

[:Gijs (he/him)]

(In reply to Brian Grinstead [:bgrins] from comment #11)
> Comment on attachment 8688474 [details]
> MozReview Request: Bug 1224713 - fix sidebar header text in devedition theme
> on linux and windows, r?bgrins
> 
> https://reviewboard.mozilla.org/r/25327/#review26251
> 
> This looks good - thank you very much for doing the investigation and fixing
> this!
> 
> Do we not need to worry about .sidebar-header here?  I see some existing
> windows selectors cover both .sidebar-header and #sidebar-header but the
> only element that it looks like this matches is the social-sidebar-header:
> https://dxr.mozilla.org/mozilla-central/source/browser/base/content/browser.
> xul#1072.  I'm not sure how to toggle that to test it.

Seems like we can do that with https://activations.cdn.mozilla.net/en-US/goal.html .

Looking at if this suffers from the same bug or not at the minute.

Comment 13

[:Gijs (he/him)]

Landed with .sidebar-header(:-moz-lwtheme) added to linux/windows devedition selectors. The social things don't have close-icons so there was no need to adjust browser.css

Comment 14

[Pulsebot]

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

Comment 16

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/c4a3bf799a22

Comment 17

[Saheda Reza [:Antora]]

I have successfully reproduced this bug on Firefox nightly version 45.0a1 according to (2015-11-13) 

It's fixed and verified on Latest Developer Edition
Build ID 	20160222004010
User Agent 	Mozilla/5.0 (Windows NT 6.1; rv:45.0) Gecko/20100101 Firefox/45.0

Tested OS -- 32bit Windows 7

Comment 18

[Vasilica Tamas, QA [:vtamas]]

Thanks Saheda for your testing.
I've also verified this issue on Firefox 46.0a2 (2016-02-22) under Ubuntu 12.04 32-bit.

Based on Comment 17 and my testing, I am marking this bug as Verified Fixed.