+++ This bug was initially created as a clone of Bug #1004576 +++ Renewed STR: 0. Ensure tabsintitlebar is enabled 1. Select "High Contrast White" in Windows settings 2. Open Firefox ER: White titlebar text AR: Dark titlebar text :-(

So is the registry color hack not working here?

We only run Windows8WindowFrameColor.jsm when -moz-windows-default-theme is true. Secondly, the registry values for High Contrast White are: ColorizationColor: 0x7f000000 ColorizationColorBalance: 0x32 (50) ColorizationGlassAttribute: 0x2 Through online tutorials, people mention using ColorizationGlassAttribute of 0x2 to enable transparent window titles in Windows 8, so there may be some people who would trigger this code path if we started using that key to determine if we should disregard ColorizationColorBalance. I'll upload a patch here shortly.

On today's Nightly, I get bad results on High Contrast #1, High Contrast Black, and High Contrast White. (HC Black is technically legible for _me_ (black on a darkish purple), but these themes exist primarily for accessibility reasons for people who need that contrast for anything to be legible.) Tagging as a11y/sec508, as we should ensure this gets addressed before 31 ESR, since this will be an major issue for big deployers required to comply with sec508.

The JSM is loaded in the if-condition block so as to not load the JSM if the host OS is not Windows 8.

Comment on attachment 8418877 [details] [diff] [review] Patch Going to try to use ActiveCaption for high contrast themes since it seems reliable for them.

Umm, why are we not already using ActiveCaption? This should be the default. We are using it on Windows <8, are we not? If so, what part of the Win8 adjustments broke it?

ActiveCaption doesn't provide the correct color on Windows 8 (non-High Contrast themes): http://screencast.com/t/UArnGmKA

Comment on attachment 8418922 [details] [diff] [review] Patch v2 I actually meant CaptionText when I said ActiveCaption, because I wasn't even thinking about Windows8WindowFrameColor.jsm there. We should use CaptionText by default (no script needed) and override it for the Win8 default theme. Apparently the CSS setting CaptionText is here: http://hg.mozilla.org/mozilla-central/annotate/fef1a56f0237/browser/themes/windows/browser.css#l78 It's disabled for -moz-windows-compositor, but -moz-windows-compositor is always true on Windows 8.

(In reply to Dão Gottwald [:dao] from comment #9) > Comment on attachment 8418922 [details] [diff] [review] > Patch v2 > > I actually meant CaptionText when I said ActiveCaption, because I wasn't > even thinking about Windows8WindowFrameColor.jsm there. We should use > CaptionText by default (no script needed) and override it for the Win8 > default theme. > > Apparently the CSS setting CaptionText is here: > http://hg.mozilla.org/mozilla-central/annotate/fef1a56f0237/browser/themes/ > windows/browser.css#l78 > > It's disabled for -moz-windows-compositor, but -moz-windows-compositor is > always true on Windows 8. We can't just use CSS because we also need to invert the icons based on the color of the window frame.

That seems orthogonal to this bug, as we'd want to invert the icons on Windows <8 as well when needed.

Comment on attachment 8418922 [details] [diff] [review] Patch v2 (In reply to Dão Gottwald [:dao] from comment #11) > That seems orthogonal to this bug, as we'd want to invert the icons on > Windows <8 as well when needed. As this bug is tracking Firefox30, I'd like to keep the changes self-contained and then fix the CaptionText changes in a follow-up bug that can get more testing before going out. http://mxr.mozilla.org/mozilla-central/source/browser/themes/windows/browser.css#111 shows how changing the way we apply the color here may have unintended changes in other places.

Comment on attachment 8418922 [details] [diff] [review] Patch v2 (In reply to Jared Wein [:jaws] (please needinfo? me) from comment #12) > As this bug is tracking Firefox30, I'd like to keep the changes > self-contained and then fix the CaptionText changes in a follow-up bug that > can get more testing before going out. > http://mxr.mozilla.org/mozilla-central/source/browser/themes/windows/browser. > css#111 shows how changing the way we apply the color here may have > unintended changes in other places. This can be fixed entirely in browser.css and browser-aero.css. Don't think this is less self-contained or more complicated than your browser.js and Windows8WindowFrameColor.jsm changes. I can prepare a patch if you like, although I'd need you or Gijs or someone else to test it since my shiny new notebook came with Windows 7 installed.

Since bug 946595 was fixed, don't we have the `-moz-windows-default-theme` media query at our disposal to use alternative, say uglier, styles when a High Contrast theme is selected?

untested :( This is mostly just moving stuff around, hardly any new code.

Comment on attachment 8419251 [details] [diff] [review] CSS patch This patch works with the following High-contrast themes on Win8: - High Contrast #1 - High Contrast Black. It does NOT work with - High Contrast #2 - High Contrast White. This makes sense, because these themes use a light colored titlebar. *sigh* In other words, I'm afraid that -moz-windows-default-theme is not enough OR something's wrong on the platform level, not providing the correct value for CaptionText.

IOW, this patch fixes 50% of the issue.

(In reply to Mike de Boer [:mikedeboer] from comment #16) > In other words, I'm afraid that -moz-windows-default-theme is not enough OR > something's wrong on the platform level, not providing the correct value for > CaptionText. What values do you get for CaptionText and ActiveCaption when using a white high-contrast theme? data:text/html,<html style="background:captiontext"> data:text/html,<html style="background:activecaption">

Default theme: - captiontext: #000 - activecaption: #99b4d1 High Contrast #1: - captiontext: #000 - activecaption: #000 High Contrast #2: - captiontext: #000 - activecaption: #000 High Contrast Black - captiontext: #fff - activecaption: #fff It's pretty clear, I think, that `captiontext` and `activecaption` are not used for text labels in a tab button.

Desired text colors: Default theme: #000 High Contrast #1: #fff High Contrast #2: #000 High Contrast Black: #fff High Contrast White: #fff IOW, when we draw in the titlebar, the tab caption color should be the same as the titlebar text color for background tabs.

(In reply to Mike de Boer [:mikedeboer] from comment #20) > IOW, when we draw in the titlebar, the tab caption color should be the same > as the titlebar text color for background tabs. What does this sentence mean? I don't understand it. It sounds like "X should be the same as X". What is the difference between "tab caption color" and "titlebar text color for background tabs" ?

(In reply to :Gijs Kruitbosch from comment #21) > What does this sentence mean? I don't understand it. It sounds like "X > should be the same as X". What is the difference between "tab caption color" > and "titlebar text color for background tabs" ? Yeah, that sentence was a bit condensed, sorry. What I meant was that when you have the titlebar enabled (like you do with the button in Customization Mode) and check the color of the text in the title bar (the caption of the window), it'll show up in the right color, i.e. with the correct contrast level to be readable on the titlebar background. When we draw tabs in the titlebar (the titlebar itself hidden), the tabs in the background will need to have a text color with the same, correct contrast level as the caption of the window had with the titlebar visible. So, the solution would be to make sure that inactive tabs, when drawing in the titlebar, get the same text color as the window caption text would have if the titlebar were visible.

(In reply to Mike de Boer [:mikedeboer] from comment #22) > So, the solution would be to make sure that inactive tabs, when drawing in > the titlebar, get the same text color as the window caption text would have > if the titlebar were visible. Yes, that's what Dão's patch attempted to accomplish. "CaptionText" should reflect the titlebar ("Caption") text color, "ActiveCaption" the background of the titlebar. Note that this doesn't work in the default theme because the titlebar text color is always black, even if you set a black background for the titlebar, which is what bug 1004576 et al. were fixing. I don't know why Dão's patch isn't good enough (waiting for a build). However: (In reply to Mike de Boer [:mikedeboer] from comment #20) (In reply to Mike de Boer [:mikedeboer] from comment #19) > Desired text colors: > High Contrast #1: #fff > - captiontext: #000 > High Contrast #2: #000 > - captiontext: #000 (but activecaption is also #000? That is strange) > High Contrast Black: #fff > - captiontext: #fff > High Contrast White: #fff - captiontext: ???? sounds like a pure CSS solution doesn't work, seeing as captiontext is wrong for HC1, if your comments are both correct...

(In reply to :Gijs Kruitbosch from comment #23) > (In reply to Mike de Boer [:mikedeboer] from comment #20) > (In reply to Mike de Boer [:mikedeboer] from comment #19) > > Desired text colors: > > High Contrast #1: #fff > > - captiontext: #000 This is even more odd because you specified that HC1 worked in comment 16...

(In reply to :Gijs Kruitbosch from comment #24) > This is even more odd because you specified that HC1 worked in comment 16... I know, just reporting what I see...

Posting a screenshot of the curious situation. This is with Dão's patch applied. So whatever color name we're using that supposedly should be the titlebar caption, in practice it isn't. In Windows themes there is a mention of 'ActiveTitle' and 'InActiveTitle', which we don't expose. Perhaps we're meant to use them and then we'd have to get to work in gfx.

(In reply to Mike de Boer [:mikedeboer] from comment #26) > Created attachment 8419390 [details] > Color names confusion > > Posting a screenshot of the curious situation. This is with Dão's patch > applied. > So whatever color name we're using that supposedly should be the titlebar > caption, in practice it isn't. It shouldn't, because that screenshot shows the window with the title bar enabled. This means that the styles using [tabsintitlebar] don't apply.

(In reply to Dão Gottwald [:dao] from comment #27) > It shouldn't, because that screenshot shows the window with the title bar > enabled. This means that the styles using [tabsintitlebar] don't apply. Err, of course. Technically, I had it applied, but it indeed has no effect with the titlebar enabled. I just wanted to show that darn Win 8/ 8.1 shows a different font color than we can get to.

(In reply to Mike de Boer [:mikedeboer] from comment #28) > (In reply to Dão Gottwald [:dao] from comment #27) > > It shouldn't, because that screenshot shows the window with the title bar > > enabled. This means that the styles using [tabsintitlebar] don't apply. > > Err, of course. Technically, I had it applied, but it indeed has no effect > with the titlebar enabled. I just wanted to show that darn Win 8/ 8.1 shows > a different font color than we can get to. Your screenshot shows data:text/html,<html style="background:captiontext"> loaded and it's as black as the native caption text. All things considered, there's nothing wrong in that screenshot. That's not to say that there aren't problems under conditions not shown in that screenshot.

What is the background in these screenshots? The url bar is cropped and so I can't tell for sure, but it seems to start "in" or "ir" which isn't either "captiontext" or "activeacption" so I'm confused.

Alright, the colors for High Contrast themes are apparently correctly listed in `HKCU/Control Panel/Colors` (not `HKCU/Control Panel/Desktop/Colors`, which seems to be obsolete). When I change the `TitleText` key there, it works and changes the active titlebar text color. This is the text color we need to use for inactive tab captions when drawing in the titlebar. Sorry for the spam, I'm just simply investigating.

(In reply to :Gijs Kruitbosch from comment #34) > What is the background in these screenshots? The url bar is cropped and so I > can't tell for sure, but it seems to start "in" or "ir" which isn't either > "captiontext" or "activeacption" so I'm confused. Please disregard the background color. They are like I said comment 19.

So you're saying for High Contrast #2, CaptionText is black and attachment 8419413 [details] confirms this (in the content area), yet in attachment 8419413 [details] background tabs don't get black text, which means that they don't use CaptionText. Can you use DOM Inspector to check if the rule that's supposed to set CaptionText is applied at all?

(In reply to Dão Gottwald [:dao] from comment #37) > So you're saying for High Contrast #2, CaptionText is black and attachment 8419413 [details] > confirms this I meant attachment 8419390 [details] here.

(In reply to Dão Gottwald [:dao] from comment #13) > although I'd need you or Gijs or someone else to test it since my shiny new > notebook came with Windows 7 installed. Upgraded to Windows 8.1 now... (In reply to Dão Gottwald [:dao] from comment #37) > Can you use DOM Inspector to check if the rule > that's supposed to set CaptionText is applied at all? The browser.css styles are not applied. There's a stupid typo in the media query.

Comment on attachment 8420038 [details] [diff] [review] CSS patch, typo fixed Review of attachment 8420038 [details] [diff] [review]: ----------------------------------------------------------------- This fixes the high contrast situation, but creates an issue with the menu bar on win8.1's default themes. I'd like to keep the fog as-is for that case, and followup anything you feel needs to change there. ::: browser/themes/windows/browser-aero.css @@ -140,5 @@ > - #TabsToolbar:not(:-moz-lwtheme) { > - background-color: transparent !important; > - color: black; > - border-left-style: none !important; > - border-right-style: none !important; Why get rid of these border styles? Are they just dead code now? @@ +276,5 @@ > + #toolbar-menubar:not(:-moz-lwtheme) { > + text-shadow: 0 0 .5em white, 0 0 .5em white, 0 1px 0 rgba(255,255,255,.4); > + } > + > + #main-menubar:not(:-moz-lwtheme):not(:-moz-window-inactive) { This moving to glass means that we get no menu fog on Win8, which means the default blue highlights for menus look like ... In particular, blue on my orange/yellow-y background just turns into something resembling green/purplish? It's not pretty. Equally, I imagine that on a darker theme color, this will either be hard to distinguish, or will make the text illegible (as that'll be white). I think I'd prefer to keep the fog on Win8 default themes for now, and file a followup to consider how to deal with that. Does that make sense?

(In reply to :Gijs Kruitbosch from comment #41) > > - #TabsToolbar:not(:-moz-lwtheme) { > > - background-color: transparent !important; > > - color: black; > > - border-left-style: none !important; > > - border-right-style: none !important; > > Why get rid of these border styles? Are they just dead code now? Have been dead for a long time, I think. Surely dead now. > This moving to glass means that we get no menu fog on Win8, which means the > default blue highlights for menus look like ... > > In particular, blue on my orange/yellow-y background just turns into > something resembling green/purplish? It's not pretty. That fog was intended for aero glass. It made sense to keep it on Windows 8 as long as we couldn't adjust the text color depending on the frame color, but now not so much. It feels super alien on Windows 8. The menu highlight blending in the background seems minor to me. > Equally, I imagine > that on a darker theme color, this will either be hard to distinguish, or > will make the text illegible (as that'll be white). Why would it make the text illegible? The text color is the same we use for background tabs. When a menu is active, the text color stays the same while the highlight is transparent enough that it shouldn't render the text illegible.

I would however create a reduced patch for aurora and beta. Are you fine with landing the patch as is on central?

(In reply to Dão Gottwald [:dao] from comment #43) > I would however create a reduced patch for aurora and beta. Are you fine > with landing the patch as is on central? I would be more comfortable with landing the same patch on nightly that we're landing on Aurora and beta, and landing the cleanup changes you want to see only on nightly as a separate followup, also because I just realized we should then change the patch some more (the ThreeDShadow rules for aero/glass should be in the same selectors as the fog bits).

(In reply to :Gijs Kruitbosch from comment #44) > (In reply to Dão Gottwald [:dao] from comment #43) > > I would however create a reduced patch for aurora and beta. Are you fine > > with landing the patch as is on central? > > I would be more comfortable with landing the same patch on nightly that > we're landing on Aurora and beta, and landing the cleanup changes you want > to see only on nightly as a separate followup, also because I just realized > we should then change the patch some more (the ThreeDShadow rules for > aero/glass should be in the same selectors as the fog bits). Err, media queries, not selectors, sorry - basically, in the same place in the file and so on.

(In reply to :Gijs Kruitbosch from comment #44) > (In reply to Dão Gottwald [:dao] from comment #43) > > I would however create a reduced patch for aurora and beta. Are you fine > > with landing the patch as is on central? > > I would be more comfortable with landing the same patch on nightly that > we're landing on Aurora and beta, and landing the cleanup changes you want > to see only on nightly as a separate followup, How does that make a difference? > also because I just realized > we should then change the patch some more (the ThreeDShadow rules for > aero/glass should be in the same selectors as the fog bits). That's for Windows 8. It explicitly excludes Vista and 7.

(In reply to Dão Gottwald [:dao] from comment #46) > (In reply to :Gijs Kruitbosch from comment #44) > > (In reply to Dão Gottwald [:dao] from comment #43) > > > I would however create a reduced patch for aurora and beta. Are you fine > > > with landing the patch as is on central? > > > > I would be more comfortable with landing the same patch on nightly that > > we're landing on Aurora and beta, and landing the cleanup changes you want > > to see only on nightly as a separate followup, > > How does that make a difference? It means we test and review what we ship on Aurora and beta separately from the changes to the menubar color, and don't get confused by the otherwise unrelated menu bar changes (as I apparently just did when it came to the grey text stuff!). We have a history of issues with these colors and how they're inverted, fogged or not. All I'm trying to do is be cautious. If you were going to make the patch anyway, surely it's not that much work to just upload that here? You might be able to use qrefresh --interactive to split your patch up as necessary (depending on whether it would be a strict subset of this patch, or whether you're actually going to be creating a different patch).

Central diverges from aurora and beta either way. Even if I'd spin off a second patch for central, I'd like to not artificially delay landing that. Alternatively, we could uplift everything such that everybody tests and dogfoods more or less the same thing.

(In reply to Dão Gottwald [:dao] from comment #48) > Central diverges from aurora and beta either way. Right, but I would like to actually have a nightly or two with what we ship on aurora and beta before landing it there, rather than a nightly or two with something else, and then land a non-equal patch on aurora and beta. > Even if I'd spin off a > second patch for central, I'd like to not artificially delay landing that. I would like that too, for the purposes of nightly moving forward, but I don't think it's the right thing to do here as I'm more worried about aurora and beta. Furthermore, we have a lot of cycle left, and so in terms of release users, a day or two of delays before changing the menu bar color here isn't going to hurt very much. > > Alternatively, we could uplift everything such that everybody tests and > dogfoods more or less the same thing. I would be OK with uplifting the separate patch, too, after more dogfooding, yes. But the second part is riskier, IMO, hence my caution.

Comment on attachment 8420135 [details] [diff] [review] more conservative patch Review of attachment 8420135 [details] [diff] [review]: ----------------------------------------------------------------- Looks good. Thanks, Dão.

Flagging for verification.

Thanks Dão! I just tested this on my Win 8.1 box and looks a-ok!

Comment on attachment 8420135 [details] [diff] [review] more conservative patch [Approval Request Comment] Bug caused by (feature/regressing bug #): Australis tabs User impact if declined: unreadable tab titles on Windows 8 with high-contrast theme Testing completed (on m-c, etc.): on m-c Risk to taking this patch (and alternatives if risky): medium (can't really make it worse for high-contras themes on Win8, though) String or IDL/UUID changes made by this patch: none

https://hg.mozilla.org/releases/mozilla-aurora/rev/378a31e9d368 https://hg.mozilla.org/releases/mozilla-beta/rev/ee16a6a74667

Mozilla/5.0 (Windows NT 6.3; rv:32.0) Gecko/20100101 Firefox/32.0 Mozilla/5.0 (Windows NT 6.3; WOW64; rv:32.0) Gecko/20100101 Firefox/32.0 Verified fixed on latest Nightly, build ID: 20140513030201. The background tabs text can be properly read using High Contrast Themes: - High Contrast #1 - High Contrast #2 - High Contrast Black - High Contrast White

Reproduced on Firefox 29 beta 8 under Win 8 64-bit, Ultrabook. Verified as fixed with all High Contrast themes using latest Aurora 31.0a2 2014-05-14. On Firefox 30 beta 4 the issue still reproduces, the fix will be verified on next beta later this week.

Mozilla/5.0 (Windows NT 6.3; rv:30.0) Gecko/20100101 Firefox/30.0 Mozilla/5.0 (Windows NT 6.2; WOW64; rv:30.0) Gecko/20100101 Firefox/30.0 Verified as fixed on Firefox 30 beta 5, build ID: 20140515140857.

(In reply to Jared Wein [:jaws] (please needinfo? me) from comment #10) > We can't just use CSS because we also need to invert the icons based on the > color of the window frame. (In reply to Dão Gottwald [:dao] from comment #11) > That seems orthogonal to this bug, as we'd want to invert the icons on > Windows <8 as well when needed. If there's a bug filed on this (I couldn't find one), please make it depend on bug 1012629 which I just filed.