946595 - Win 8 high contrast themes are also considered to match -moz-windows-default-theme

Status: VERIFIED FIXED

(Core :: Widget: Win32, defect)

Description

[hgk]

Attachment: thunderbird2411negativepicture.jpg

User Agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:25.0) Gecko/20100101 Firefox/25.0 (Beta/Release)
Build ID: 20131112160018

Steps to reproduce:

use windows settings black contrast
start email client
start writting an email


Actual results:

window for writting opens but parts of the window are white on white


Expected results:

window should open with all parts are black in the background and white text color.

this is espacially a problem for people who are visually handicapped. would be nice when you can change that. I an older version that problem didn't exist.

Comment 1

[Richard Marti (:Paenglab)]

On Windows 7 I'm seeing the background black and correct contrasting text color. Are you sure this screenshot is taken from Windows 7? And are you sure you have no theme installed which changes the background.

Please check to run TB in safe mode (Hilfe/Mit deaktivierten Add-ons neu starten...).

Comment 2

[hgk]

Hello!
I'm sorry that was my fault. The operating system is windows 8 (not 8.1).
I changed the operation system in the bug above. On a windows 7 machine I had a team view window to a window 8 machine open. That's why I wrote the wrong system. Sorry.
Would be nice when you can look for it again.
Thanks!

Comment 3

[hgk]

In version 17.0.3 everything was fine with the contrast.

Comment 4

[Richard Marti (:Paenglab)]

Ah yes, I see this on Win 8 too. Also Firefox shows the light blue background with the high contrast themes.

The rules in @media (-moz-windows-default-theme) are also used with HC themes -> toolkit issue.

Comment 5

[Richard Marti (:Paenglab)]

Jim, do you know whats wrong?

Comment 6

[Richard Marti (:Paenglab)]

Attachment: Firefox on Win8 and white high contrast

The interesting is, I tried attachment 8377497 [details] on Win8 with white high contrast, and "Default Theme" isn't red. I also changed the testcase to use @media (-moz-windows-default-theme) instead of -moz-system-metric(windows-default-theme) and it was still not red.

But the toolbars where still light blue (with some toolbar-buttons on them white).

Inspector shows http://mxr.mozilla.org/mozilla-central/source/browser/themes/windows/browser-aero.css#25 ff. is still used.

Comment 7

[Jim Mathies [:jimm]]

(In reply to Richard Marti (:Paenglab) from comment #5)
> Jim, do you know whats wrong?

not really no, but from your test doesn't seem like this bug should be filed in widget.

Comment 8

[Richard Marti (:Paenglab)]

And where should it be filed?

Comment 9

[Jim Mathies [:jimm]]

Dao, any thoughts on this? It looks like the media queries are working, but per comment 6 css that shouldn't be in use is.

I just checked on win7, while not perfect the blue background in the chrome area does switch to a fade black.

Comment 10

[Richard Marti (:Paenglab)]

On Win7 it works. This issue is only on Win8.1. I have no Win8 at hand now but I remember it worked then.

Comment 11

[Dão Gottwald [:dao]]

(In reply to Jim Mathies [:jimm] from comment #9)
> Dao, any thoughts on this? It looks like the media queries are working, but
> per comment 6 css that shouldn't be in use is.

The CSS that shouldn't be in use is behind -moz-windows-default-theme, so the media query does not seem to be working as expected.

Comment 12

[Jim Mathies [:jimm]]

That gets updated here - 

http://mxr.mozilla.org/mozilla-central/source/widget/windows/nsUXThemeData.cpp#320

which implies these high contrast themes are still being identified as a default themes.

Comment 13

[:Gijs (he/him)]

(In reply to Jim Mathies [:jimm] (away 4/1-4/21) from comment #12)
> That gets updated here - 
> 
> http://mxr.mozilla.org/mozilla-central/source/widget/windows/nsUXThemeData.
> cpp#320
> 
> which implies these high contrast themes are still being identified as a
> default themes.

I just checked in VS's debugger, it returns AeroLite.msstyles as the theme path (above that section). This seems to match MS documentation which suggests that high contrast styles are now implemented on top of "normal" Windows theming styles, whereas before they were using classic. styles and the path lookup (GetCurrentThemeName) would fail.

Comment 14

[:Gijs (he/him)]

Attachment: high contrast themes shouldn't be considered the default theme in CSS,

This seems to fix it. Jim, does this look right to you?

Comment 15

[Jim Mathies [:jimm]]

Comment on attachment 8398072 [details] [diff] [review]
high contrast themes shouldn't be considered the default theme in CSS,

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

::: widget/windows/nsUXThemeData.cpp
@@ +332,5 @@
>    if (theme == WINTHEME_UNRECOGNIZED)
>      return;
>  
> +  if ((!IsWin8OrLater() || !sIsHighContrastOn) &&
> +	  (theme == WINTHEME_AERO || theme == WINTHEME_AERO_LITE || theme == WINTHEME_LUNA)) {

Yes, this does. Please add a comment above this explaining why we do these checks.

Also, nit, on the second line you've introduced a tab.

Comment 16

[Mike de Boer [:mikedeboer]]

General question, should we start tracking this for Australis?

Gijs, awesome that you're taking this! :)

Comment 17

[:Gijs (he/him)]

(In reply to Mike de Boer [:mikedeboer] from comment #16)
> General question, should we start tracking this for Australis?

I'm not sure. This doesn't magically fix all our high contrast bugs - not even close. It's probably helpful to have the behaviour match between different Windows versions, though.

AFAICS this fix is relatively contained, so I wouldn't feel bad asking for uplift to Aurora and Beta.

To ensure that this stays on uplift radars, I've just set it as P5.

Comment 18

[:Gijs (he/him)]

Attachment: high contrast themes shouldn't be considered the default theme in CSS,

Trees are closed and I need sleep. This is the patch w/ comments addressed.

Comment 19

[Matthew N. [:MattN]]

Try push: https://tbpl.mozilla.org/?tree=Try&rev=48745c03564a

Comment 20

[Matthew N. [:MattN]]

Oops, forgot to qpush. Here is a valid try push:
https://tbpl.mozilla.org/?tree=Try&rev=5af93d8b9c19

Comment 21

[Matthew N. [:MattN]]

Comment on attachment 8398188 [details] [diff] [review]
high contrast themes shouldn't be considered the default theme in CSS,

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

Comment 22

[:Gijs (he/him)]

(In reply to Matthew N. [:MattN] from comment #21)
> Comment on attachment 8398188 [details] [diff] [review]
> high contrast themes shouldn't be considered the default theme in CSS,
> 
> https://hg.mozilla.org/integration/fx-team/rev/d8a36b893b2b

Thanks!

Comment 23

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

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

Comment 24

[Sylvestre Ledru [:Sylvestre]]

Matthew, could you fill the uplift request? thanks

Comment 25

[Matthew N. [:MattN]]

Comment on attachment 8398188 [details] [diff] [review]
high contrast themes shouldn't be considered the default theme in CSS,

[Approval Request Comment]
Bug caused by (feature/regressing bug #): Windows 8 High Contrast Implementation
User impact if declined: Users in High Contrast (HC) mode on Windows 8+, won't get the proper styles for that mode (Other CSS will apply which shouldn't and may greatly reduce the contrast thus defeating the point of HC mode). It will help fix a few other HC bugs on Win8 due to the Win8 change.
Testing completed (on m-c, etc.): m-c although HC mode isn't tested in automation AFAIK.
Risk to taking this patch (and alternatives if risky): Tiny refactoring with a different code path for Win8+. It seems pretty straightforward so likely low risk.
String or IDL/UUID changes made by this patch: None

Comment 26

[Matthew N. [:MattN]]

P3- since it blocks a P3-

Comment 27

[Mike de Boer [:mikedeboer]]

Aurora: https://hg.mozilla.org/releases/mozilla-aurora/rev/e89c8841b036
Beta: https://hg.mozilla.org/releases/mozilla-beta/rev/250d63775815

Comment 28

[Andrei Vaida [:avaida]]

Some elements from the High Contrast Black theme, such as:
- the location bar's placeholder text,
- the google search bar's placeholder text,
- the help sub-menu,
are still not displayed properly, screenshots here: http://bit.ly/1lN3seb 

I'm not sure if the issues above should have been fixed by this as well, please confirm.

Verified on Windows 8 Pro 64-bit [1], using High Contrast Black and High Contrast White themes with:
- the latest Beta (Build ID: 20140407135746)
- the latest Aurora 2014-04-07 (Build ID: 20140407004002),
- the latest Nightly 2014-04-07 (Build ID: 20140407030203).

1. Mozilla/5.0 (Windows NT 6.2; WOW64; rv:29.0) Gecko/20100101 Firefox/29.0

Comment 29

[:Gijs (he/him)]

(In reply to Andrei Vaida, QA [:avaida] from comment #28)
> Some elements from the High Contrast Black theme, such as:
> - the location bar's placeholder text,
> - the google search bar's placeholder text,
> - the help sub-menu,
> are still not displayed properly, screenshots here: http://bit.ly/1lN3seb 
> 
> I'm not sure if the issues above should have been fixed by this as well,
> please confirm.
> 
> Verified on Windows 8 Pro 64-bit [1], using High Contrast Black and High
> Contrast White themes with:
> - the latest Beta (Build ID: 20140407135746)
> - the latest Aurora 2014-04-07 (Build ID: 20140407004002),
> - the latest Nightly 2014-04-07 (Build ID: 20140407030203).
> 
> 1. Mozilla/5.0 (Windows NT 6.2; WOW64; rv:29.0) Gecko/20100101 Firefox/29.0

I don't know anything about the graphical consequences of this change. It was a technical change; the easiest way to test is to evaluate the following in the browser console on Windows 8:

window.matchMedia("(-moz-windows-default-theme)").matches


It should be false for the Windows 8 high contrast themes, and true for the default "flat" Windows 8 theme.

Comment 30

[Andrei Vaida [:avaida]]

(In reply to :Gijs Kruitbosch from comment #29)
> I don't know anything about the graphical consequences of this change. It
> was a technical change; the easiest way to test is to evaluate the following
> in the browser console on Windows 8:
> 
> window.matchMedia("(-moz-windows-default-theme)").matches
> 
> 
> It should be false for the Windows 8 high contrast themes, and true for the
> default "flat" Windows 8 theme.

I was able to confirm the fix for this issue on the environment mentioned in Comment 28 and in accordance with Gijs' instruction (Thanks Gijs!).

Comment 32

[Dão Gottwald [:dao]]

(In reply to Andrei Vaida, QA [:avaida] from comment #28)
> Some elements from the High Contrast Black theme, such as:
> - the location bar's placeholder text,
> - the google search bar's placeholder text,

This is bug 986324.

> - the help sub-menu,
> are still not displayed properly, screenshots here: http://bit.ly/1lN3seb 
> 
> I'm not sure if the issues above should have been fixed by this as well,
> please confirm.