Last Comment Bug 946595 - Win 8 high contrast themes are also considered to match -moz-windows-default-theme
: Win 8 high contrast themes are also considered to match -moz-windows-default-...
Status: VERIFIED FIXED
[Australis:P3-]
:
Product: Core
Classification: Components
Component: Widget: Win32 (show other bugs)
: unspecified
: All Windows 8
-- normal (vote)
: mozilla31
Assigned To: :Gijs
:
: Jim Mathies [:jimm]
Mentors:
: 993370 1002863 (view as bug list)
Depends on: 1042625
Blocks: 963950 986324
  Show dependency treegraph
 
Reported: 2013-12-04 23:46 PST by hgk
Modified: 2014-07-23 04:58 PDT (History)
12 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
+
verified
+
verified
verified


Attachments
thunderbird2411negativepicture.jpg (63.83 KB, image/jpeg)
2013-12-04 23:46 PST, hgk
no flags Details
Firefox on Win8 and white high contrast (15.88 KB, image/png)
2014-02-18 10:37 PST, Richard Marti (:Paenglab)
no flags Details
high contrast themes shouldn't be considered the default theme in CSS, (5.42 KB, patch)
2014-03-27 12:53 PDT, :Gijs
jmathies: review+
Details | Diff | Splinter Review
high contrast themes shouldn't be considered the default theme in CSS, (5.76 KB, patch)
2014-03-27 16:02 PDT, :Gijs
gijskruitbosch+bugs: review+
sledru: approval‑mozilla‑aurora+
sledru: approval‑mozilla‑beta+
MattN+bmo: checkin+
Details | Diff | Splinter Review

Description User image hgk 2013-12-04 23:46:34 PST
Created attachment 8342912 [details]
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 User image Richard Marti (:Paenglab) 2013-12-05 11:42:09 PST
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 User image hgk 2013-12-08 10:23:55 PST
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 User image hgk 2013-12-08 10:48:15 PST
In version 17.0.3 everything was fine with the contrast.
Comment 4 User image Richard Marti (:Paenglab) 2013-12-08 11:03:09 PST
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 User image Richard Marti (:Paenglab) 2014-02-17 09:48:32 PST
Jim, do you know whats wrong?
Comment 6 User image Richard Marti (:Paenglab) 2014-02-18 10:37:45 PST
Created attachment 8377696 [details]
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 User image Jim Mathies [:jimm] 2014-02-20 03:25:08 PST
(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 User image Richard Marti (:Paenglab) 2014-02-20 03:27:52 PST
And where should it be filed?
Comment 9 User image Jim Mathies [:jimm] 2014-02-20 03:54:21 PST
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 User image Richard Marti (:Paenglab) 2014-02-20 03:57:58 PST
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 User image Dão Gottwald [:dao] 2014-02-20 04:31:42 PST
(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 User image Jim Mathies [:jimm] 2014-02-20 04:48:33 PST
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 User image :Gijs 2014-03-27 11:54:46 PDT
(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 User image :Gijs 2014-03-27 12:53:16 PDT
Created attachment 8398072 [details] [diff] [review]
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 User image Jim Mathies [:jimm] 2014-03-27 13:12:30 PDT
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 User image Mike de Boer [:mikedeboer] 2014-03-27 14:13:58 PDT
General question, should we start tracking this for Australis?

Gijs, awesome that you're taking this! :)
Comment 17 User image :Gijs 2014-03-27 15:12:29 PDT
(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 User image :Gijs 2014-03-27 16:02:44 PDT
Created attachment 8398188 [details] [diff] [review]
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 User image Matthew N. [:MattN] (PM if requests are blocking you) 2014-03-27 23:42:52 PDT Comment hidden (obsolete)
Comment 20 User image Matthew N. [:MattN] (PM if requests are blocking you) 2014-03-27 23:47:58 PDT
Oops, forgot to qpush. Here is a valid try push:
https://tbpl.mozilla.org/?tree=Try&rev=5af93d8b9c19
Comment 21 User image Matthew N. [:MattN] (PM if requests are blocking you) 2014-03-28 03:16:14 PDT
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 User image :Gijs 2014-03-28 04:01:55 PDT
(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 User image Wes Kocher (:KWierso) 2014-03-28 16:51:38 PDT
https://hg.mozilla.org/mozilla-central/rev/d8a36b893b2b
Comment 24 User image Sylvestre Ledru [:sylvestre] 2014-04-02 02:50:43 PDT
Matthew, could you fill the uplift request? thanks
Comment 25 User image Matthew N. [:MattN] (PM if requests are blocking you) 2014-04-02 03:02:16 PDT
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 User image Matthew N. [:MattN] (PM if requests are blocking you) 2014-04-02 03:03:20 PDT
P3- since it blocks a P3-
Comment 28 User image Andrei Vaida, QA [:avaida] – please ni? me 2014-04-08 01:37:53 PDT
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 User image :Gijs 2014-04-08 02:25:56 PDT
(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 User image Andrei Vaida, QA [:avaida] – please ni? me 2014-04-08 02:47:03 PDT
(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 31 User image Richard Marti (:Paenglab) 2014-04-08 05:26:35 PDT
*** Bug 993370 has been marked as a duplicate of this bug. ***
Comment 32 User image Dão Gottwald [:dao] 2014-04-08 08:27:16 PDT
(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.
Comment 33 User image Dão Gottwald [:dao] 2014-04-29 02:25:04 PDT
*** Bug 1002863 has been marked as a duplicate of this bug. ***

Note You need to log in before you can comment on or make changes to this bug.