940393 - Win8: Text color on inactive tabs makes text very hard to read

Status: VERIFIED FIXED

(Firefox :: Theme, defect)

Description

[Chris Lord [:cwiiis]]

Attachment: Screenshot 2013-11-19 09.02.51.png

Since updating to Australis, I get black text for inactive tabs, even though my window colour is also quite dark. Screenshot attached.

Comment 1

[Mike de Boer [:mikedeboer]]

How about we add a white dropshadow when text color is Black? Wouldn't hurt, methinks.

Comment 2

[Radek 'sysKin' Czyz]

For me the biggest issue with such low-contrast background tabs is that there is very little visual feedback when opening a new tab. Technically the animation is there, but without any tab background, it's not enough to catch the eye.
IMHO some bright current-tab background (comparable with current hover effect) would be great, even if I appreciate current aesthetics.

Comment 3

[Radek 'sysKin' Czyz]

s/bright current-tab background/bright background-tab background.
Apologies for the spam.

Comment 4

[:Gijs (he/him)]

Do the window border/titlebar colors get exposed to us by win8? (XP modern doesn't do a super-great job at this, hence my asking) If so, could we do the same thing we do on XP luna blue and use the titlebar color for the background tabs?

Chris, could you open http://www.iangraham.org/books/xhtml1/appd/update-23feb2000.html and check if CaptionText matches the text color usually used by the titlebar? (presumably not the black we're using)

Comment 6

[Chris Lord [:cwiiis]]

Attachment: System colours screenshot

(In reply to :Gijs Kruitbosch from comment #4)
> Do the window border/titlebar colors get exposed to us by win8? (XP modern
> doesn't do a super-great job at this, hence my asking) If so, could we do
> the same thing we do on XP luna blue and use the titlebar color for the
> background tabs?
> 
> Chris, could you open
> http://www.iangraham.org/books/xhtml1/appd/update-23feb2000.html and check
> if CaptionText matches the text color usually used by the titlebar?
> (presumably not the black we're using)

They match, screenshot attached.

Comment 7

[Chris Lord [:cwiiis]]

From talking with Gijs on IRC and testing with the provided link, it seems all the system colours in Windows 8 are constants. We should also note that Windows 8 always draws the titlebar text in black, regardless of the titlebar colour (and it can go darker than in the screenshots provided), so even if they weren't, it wouldn't help us in this scenario.

Searching, I came across this, which might be of interest: http://blog.quppa.net/2013/01/02/retrieving-windows-8-theme-colours/

I think fixing this will require changes to widget code - perhaps the colours could be retrieved via a read-back on Windows 8? (I've no idea if this is possible or not)

Comment 8

[(Currently slow to respond) Philipp Sackl [:phlsa] (Firefox UX) please use needinfo]

We should also make sure to switch the color of icons displayed in the title bar along with the text. Not sure if this should be a separate bug.

Comment 9

[Marilyn Munster]

Attachment: Chrome Tabs

Comment 10

[Marilyn Munster]

Attachment: easy read Old Theme Tabs

Comment 11

[Marilyn Munster]

Attachment: hard To read Bacground Tabs on 8/8.1 & Linux

Comment 12

[Marilyn Munster]

Attachment: oldtheme Text

Comment 13

[Marilyn Munster]

Attachment: newtheme Text

Comment 14

[Marilyn Munster]

Added more Screenshots OF FF 29 on win 8/8.1 & Linux(different linux theme)

Comment 15

[Marilyn Munster]

Attachment: Need Fix.jpg

Comment 16

[Marilyn Munster]

Attachment: background_window.JPG

Comment 17

[Marilyn Munster]

Attachment: New_foreground_tabs_win8 (1).jpg

Comment 18

[Marilyn Munster]

Attachment: New_foreground_tabs_win8 (2).jpg

Comment 19

[Marilyn Munster]

Attachment: New_foreground_tabs_win8 (3).jpg

[Approval Request Comment]
Bug caused by (feature/regressing bug #): 
User impact if declined: 
Testing completed (on m-c, etc.): 
Risk to taking this patch (and alternatives if risky): 
String or IDL/UUID changes made by this patch:

Comment 20

[Mike de Boer [:mikedeboer]]

Marilyn, while I appreciate your ferocity in trying to explain the issue here, but I think the original screenshot is already quite sufficient in explaining what the issue is.

I know you're new to Bugzilla, so please allow me to point you to the Bugzilla Etiquette: https://bugzilla.mozilla.org/page.cgi?id=etiquette.html
I suggest you read it and if you have any questions about it, please drop me a line via email or join us on IRC!

Thanks for understanding.

Comment 21

[Marilyn Munster]

Thanks 
& read it
sorry

When will this be Fixed
P.S- english not my tongue

Comment 22

[:Gijs (he/him)]

So... I have a suggestion for a super-hacky workaround... we could use drawWindow on a canvas, and read the pixel data. Because win8 styling is non-gradienty (as far as I can tell?) and drawWindow lets you specify exactly which bit of the window you care about, I should hope that this should be reasonably fast... We might even be able to use a canvas operation to invert the color for us, instead of doing the math ourselves. There might be a split second where we've not added our custom rule and so the color might not be 'correct' yet, but I think for an intermediate solution while bug 578780 isn't fixed, it might work.

Jared, I don't have win8 set up yet (working on it)... do you think this approach is worth investigating, and if so, do you think you could take it?

Comment 23

[Jared Wein [:jaws] (please needinfo? me)]

Attachment: WIP Patch

Gijs, what do you think of something in this direction? With a grey titlebar, I'm getting back rgba(0,0,0,255) from the imageData, so something isn't 100% here, but at a high level is this what you were thinking?

Comment 24

[Matthew N. [:MattN]]

Comment on attachment 8392658 [details] [diff] [review]
WIP Patch

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

::: browser/modules/Windows8WindowFrameColor.jsm
@@ +18,5 @@
> +      return this._windowFrameColor;
> +
> +    let canvas = aWindow.document.createElementNS("http://www.w3.org/1999/xhtml", "canvas");
> +    let ctx = canvas.getContext("2d");
> +    ctx.drawWindow(aWindow, 0, 0, 1, 1, "rgb(0,0,0)", ctx.DRAWWINDOW_DO_NOT_FLUSH);

You may need DRAWWINDOW_USE_WIDGET_LAYERS and should probably try a different coordinate in case 0,0 actually is transparent.

Comment 25

[Matthew N. [:MattN]]

(In reply to Jared Wein [:jaws] (please needinfo? me) from comment #23)
> Gijs, what do you think of something in this direction? With a grey
> titlebar, I'm getting back rgba(0,0,0,255) from the imageData, so something
> isn't 100% here, but at a high level is this what you were thinking?

Perhaps that's a black window outline since the code seems to be using 0,0? Or did you test at different points?

Comment 26

[:Gijs (he/him)]

Comment on attachment 8392658 [details] [diff] [review]
WIP Patch

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

Yes! This is the right idea. Matt's comments make sense. I've left some other tidbits below.

::: browser/base/content/browser.js
@@ +1198,5 @@
> +#ifdef XP_WIN
> +    let windowsVersion = Cc["@mozilla.org/system-info;1"]
> +                           .getService(Ci.nsIPropertyBag2)
> +                           .getProperty("version");
> +    if (parseFloat(windowsVersion) >= 6.2) {

Hrm. Do we have CSS selectors specifically for these themes so we don't mess up in case Windows 9 does something crazy? We can use them with matchMedia.

Also, I'm somewhat scared of the startup perf hit this will cause. Can we think of a way to alleviate that? Perhaps we can document.persist this in a custom attribute and reuse that, and then setTimeout() an update for it in case you changed windows themes between startups? Or do you think that's too much complexity? Not sure how slow this is in practice...

@@ +1202,5 @@
> +    if (parseFloat(windowsVersion) >= 6.2) {
> +      let windowFrameColor = Windows8WindowFrameColor.get(window);
> +      let brightness = Math.sqrt(.241 * windowFrameColor[0] * windowFrameColor[0] +
> +                                 .691 * windowFrameColor[1] * windowFrameColor[1] +
> +                                 .068 * windowFrameColor[2] * windowFrameColor[2]);

Probably worth commenting where these magical numbers come from.

@@ +1206,5 @@
> +                                 .068 * windowFrameColor[2] * windowFrameColor[2]);
> +      if (brightness < 128) {
> +        let styleElement = document.createElementNS("http://www.w3.org/1999/xhtml", "style");
> +        styleElement.textContent = ".tabbrowser-tab:not([selected=true]) { color: #fff; }";
> +        document.documentElement.appendChild(styleElement);

Might be cleaner to use document.styleSheets like the lightweight theme code does, or maybe not...

(alternative, this could be an XML Processing instruction that lists a stylesheet, with a data URI for an href, but that's probably more work than this...)

::: browser/modules/Windows8WindowFrameColor.jsm
@@ +23,5 @@
> +    let imageData = ctx.getImageData(0, 0, 1, 1);
> +    let pixelR = imageData.data[0];
> +    let pixelG = imageData.data[1];
> +    let pixelB = imageData.data[2];
> +    let pixelA = imageData.data[3];

nit: can you use a destructuring assignment here?

let [pixelR, pixelG, pixelB, pixelA] = imageData.data; ?

Comment 27

[:Gijs (he/him)]

ctx.drawWindow(window, 3, 3, 1, 1, "transparent", 0x1a) works for me in the console, fwiw.

Note that the window needs to be focused for this to work, however... we might need a check to deal with this. AFAICT unfocused windows are always a pale grey in Win8, and dealing with that is less problematic, color-wise.

Comment 28

[Jared Wein [:jaws] (please needinfo? me)]

Attachment: WIP Patch v2

An update on the patch. It still isn't getting the color for the window frame. I also need to check to see if the window is foreground/focused, not in private browsing mode or with a lw-theme applied.

I need to get the window frame color working before pushing on the other issues though.

Comment 29

[:Gijs (he/him)]

So, IRC comments from Jim Mathies indicate my idea is unlikely to work because on win8 the non-client areas will just be transparent. :-(

Comment 30

[Jared Wein [:jaws] (please needinfo? me)]

Unassigning for now but I'll keep thinking about any possible way to fix this without platform support.

Comment 31

[Jared Wein [:jaws] (please needinfo? me)]

It may be possible to read this value from the Registry at HKEY_CURRENT_USER\Control Panel\Colors. I'll see if I can do something similar like we do at http://mxr.mozilla.org/mozilla-central/source/browser/components/migration/src/IEProfileMigrator.js#130 to get the color.

Comment 32

[Jared Wein [:jaws] (please needinfo? me)]

Attachment: Patch

This works pretty well :)

Comment 33

[Dão Gottwald [:dao]]

Comment on attachment 8393592 [details] [diff] [review]
Patch

Instead of messing with the stylesheet, please set an attribute on the window element or something and let browser-aero.css set the color depending on that attribute:
http://hg.mozilla.org/mozilla-central/annotate/3bc3b9e2cd99/browser/themes/windows/browser-aero.css#l131

Comment 34

[Dão Gottwald [:dao]]

Comment on attachment 8393592 [details] [diff] [review]
Patch

>+XPCOMUtils.defineLazyModuleGetter(this, "Windows8WindowFrameColor",
>+                                  "resource:///modules/Windows8WindowFrameColor.jsm");

This should be ifdef'd as well, or you should import the module in the code block where you use it; you don't really need it to be global.

Comment 35

[Jared Wein [:jaws] (please needinfo? me)]

Attachment: Patch v2

Comment 36

[:Gijs (he/him)]

Comment on attachment 8393628 [details] [diff] [review]
Patch v2

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

::: browser/base/content/browser.js
@@ +1193,5 @@
>        document.getElementById("key_delete").setAttribute("disabled", true);
>      }
>  
> +#ifdef XP_WIN
> +    if (window.matchMedia("-moz-os-version: windows-win8")) {

I'm concerned that we'll be doing this even for custom or high contrast themes which might not reflect this registry value. Can we detect that the user is using the default theme?

@@ +1195,5 @@
>  
> +#ifdef XP_WIN
> +    if (window.matchMedia("-moz-os-version: windows-win8")) {
> +      let windows8WindowFrameColor = Cu.import("resource:///modules/Windows8WindowFrameColor.jsm", {}).Windows8WindowFrameColor;
> +      let windowFrameColor = windows8WindowFrameColor.get(window);

Nit: don't need to pass window here

@@ +1208,5 @@
> +      let fY = 0; // Default to black for foreground text.
> +      let brightnessDifference = Math.abs(bY - fY);
> +      let colorDifference = windowFrameColor[0] + windowFrameColor[1] + windowFrameColor[2];
> +
> +      if (brightnessDifference < brightnessThreshold && colorDifference < colorThreshold) {

Add a comment here that the range for the brightness difference is 255 and so if the brightness difference with black is less than this, the difference with white is guaranteed to exceed the threshold.

::: browser/modules/Windows8WindowFrameColor.jsm
@@ +13,5 @@
> +
> +const Windows8WindowFrameColor = {
> +  _windowFrameColor: null,
> +
> +  get: function(aWindow) {

Nit: You don't use aWindow.

@@ +21,5 @@
> +    let windowFrameColor = WindowsRegistry.readRegKey(Ci.nsIWindowsRegKey.ROOT_KEY_CURRENT_USER,
> +                                                      "Software\\Microsoft\\Windows\\DWM",
> +                                                      "ColorizationColor");
> +    // The color returned from the Registry is in decimal form.
> +    let windowFrameColorHex = windowFrameColor.toString(16);

Please note here that the alpha value is fixed to be < 16, and is the highest 16 bits, which means this string is guaranteed to have 8 characters (4 * 2) which is why the regex works. If you are paranoid like me, please add code that zero-pads this string up to 8 so that the code below here doesn't error out if that expectation breaks because some custom theme or other breaks stuff.

@@ +22,5 @@
> +                                                      "Software\\Microsoft\\Windows\\DWM",
> +                                                      "ColorizationColor");
> +    // The color returned from the Registry is in decimal form.
> +    let windowFrameColorHex = windowFrameColor.toString(16);
> +    let windowFrameColorArray = windowFrameColorHex.match(/(.{1,2})/g);

Uhhh. .match(/../g) will do. :-)

@@ +23,5 @@
> +                                                      "ColorizationColor");
> +    // The color returned from the Registry is in decimal form.
> +    let windowFrameColorHex = windowFrameColor.toString(16);
> +    let windowFrameColorArray = windowFrameColorHex.match(/(.{1,2})/g);
> +    let [pixelA, pixelR, pixelG, pixelB] = windowFrameColorArray.map(function(val) parseInt(val, 16));

Generally, part of me thinks there's a simpler (bit shift) way to do this while keeping this numeric (instead of the string conversions and regex) but I guess this is probably more readable and less errorprone.

::: browser/modules/WindowsRegistry.jsm
@@ +29,5 @@
> +      if (registry.hasValue(aKey)) {
> +        let type = registry.getValueType(aKey);
> +        switch (type) {
> +          case kRegMultiSz:
> +            // nsIWindowsRegKey doesn't support REG_MULTI_SZ type out of the box.

Can you be awesome and file a followup bug for this if there isn't one? This is dumb. :-)

::: browser/themes/windows/browser-aero.css
@@ +137,5 @@
>    }
>  
> +  #main-window[darkwindowframe="true"] .tabbrowser-tab:not([selected=true]):not(:-moz-lwtheme) {
> +    color: white;
> +  }

Hmm, no. Can you:

1) make this set the color on the #TabsToolbar ? The tabs have color:inherit so they should be OK that way, as should other widgets.
2) make the tabstoolbar and menubar also use Toolbar-inverted in this case, while we're here? I believe there should be an existing rule for the menubar in here where you can add a selector. It's a bit of a pity that we'll need to update other things (like the sync thing you just reviewed) as well.

Comment 37

[Jared Wein [:jaws] (please needinfo? me)]

Attachment: Patch v3

Addressed review feedback, will wait to file the bug until this is finished.

Comment 38

[:Gijs (he/him)]

Comment on attachment 8394256 [details] [diff] [review]
Patch v3

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

So this is all great, except that now because I landed bug 985267 already, this needs to do two more things (apart from the notes below):

- update the comment for the color: inherit block added in https://hg.mozilla.org/mozilla-central/rev/63cf491f2032 to note that on glass/win8 we only use this for inactive windows
- add a rule after the block that adds the fog in http://mxr.mozilla.org/mozilla-central/source/browser/themes/windows/browser-aero.css#196:

#main-menubar > menu:not(:-moz-lwtheme):not(:-moz-window-inactive) {
  color: black
}

I believe this should fix the color issue for the menubar.

::: browser/modules/Windows8WindowFrameColor.jsm
@@ +23,5 @@
> +                                                      "ColorizationColor");
> +    // The color returned from the Registry is in decimal form.
> +    let windowFrameColorHex = windowFrameColor.toString(16);
> +    // Zero-pad the number just to make sure that it is 8 digits.
> +    windowFrameColorHex = (windowFrameColorHex + "00000000").substr(0, 8); 

Errr. You want to left-pad, not right pad.

Nit: trailing whitespace

::: browser/themes/windows/browser-aero.css
@@ +151,5 @@
> +  #main-window[darkwindowframe="true"] :-moz-any(#toolbar-menubar, #TabsToolbar) > toolbarpaletteitem > #new-tab-button:not(:-moz-lwtheme):not(:-moz-window-inactive) {
> +    list-style-image: url(chrome://browser/skin/tabbrowser/newtab-inverted.png);
> +  }
> +
> +  #main-window[darkwindowframe="true"] .tab-close-button:not(:hover):not([selected="true"]):not(:-moz-lwtheme):not(:-moz-window-inactive) {

Nit: Generally, when the number of :nots >=3, I prefer :not(:-moz-any(...)) which does the same.

Comment 39

[Dão Gottwald [:dao]]

(In reply to :Gijs Kruitbosch from comment #38)
> - update the comment for the color: inherit block added in
> https://hg.mozilla.org/mozilla-central/rev/63cf491f2032 to note that on
> glass/win8 we only use this for inactive windows
> - add a rule after the block that adds the fog in
> http://mxr.mozilla.org/mozilla-central/source/browser/themes/windows/browser-
> aero.css#196:
> 
> #main-menubar > menu:not(:-moz-lwtheme):not(:-moz-window-inactive) {
>   color: black
> }

The color should be set to black on #main-menubar where we also add the white background.

Comment 40

[:Gijs (he/him)]

(In reply to Dão Gottwald [:dao] from comment #39)
> (In reply to :Gijs Kruitbosch from comment #38)
> > - update the comment for the color: inherit block added in
> > https://hg.mozilla.org/mozilla-central/rev/63cf491f2032 to note that on
> > glass/win8 we only use this for inactive windows
> > - add a rule after the block that adds the fog in
> > http://mxr.mozilla.org/mozilla-central/source/browser/themes/windows/browser-
> > aero.css#196:
> > 
> > #main-menubar > menu:not(:-moz-lwtheme):not(:-moz-window-inactive) {
> >   color: black
> > }
> 
> The color should be set to black on #main-menubar where we also add the
> white background.

Err, yes, I went back and forth on an approach and didn't think through what I ended up saying in the end fully. This is correct, and we can keep the rule inside the existing rule that adds the white background.

Comment 41

[:Gijs (he/him)]

FWIW, I landed the "active sync icon" patch (bug 940844) and so this should now ideally add an "inverted" rule for that icon (which is sadly not in the sprite because it's animated), too. :-(

Comment 42

[Matthew N. [:MattN]]

Comment on attachment 8394256 [details] [diff] [review]
Patch v3

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

::: browser/base/content/browser.js
@@ +1198,5 @@
> +        window.matchMedia("-moz-windows-default-theme")) {
> +      let windows8WindowFrameColor = Cu.import("resource:///modules/Windows8WindowFrameColor.jsm", {}).Windows8WindowFrameColor;
> +      let windowFrameColor = windows8WindowFrameColor.get();
> +
> +      // Formula from W3C Techniques For Accessibility Evaluation And

A comment explaining what the code below is actually doing would be nice. Moving this to a method (see below) would make that less necessary with a good method name.

This would be a good addition to colorUtils.jsm as it already implements another AERT algorithm. That would mean refactoring it to remove the metro specific event stuff and moving it out of browser/metro/ though so I'm not sure you want to do that in this bug.

ColorAnalyzer.js is in places since it's primary usage is for favicons (which are stored in places). I think this would be better in colorUtils.jsm if you're looking for somewhere better to put it.

Why don't you put this algorithm in a method in Windows8WindowFrameColor.jsm for now to avoid adding more to browser.js?

::: browser/modules/WindowsRegistry.jsm
@@ +1,1 @@
> +/* This Source Code Form is subject to the terms of the Mozilla Public

Why not put this in toolkit/modules/ since it's not browser-specific?

Comment 43

[Jared Wein [:jaws] (please needinfo? me)]

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

(In reply to :Gijs Kruitbosch from comment #41)
> FWIW, I landed the "active sync icon" patch (bug 940844) and so this should
> now ideally add an "inverted" rule for that icon (which is sadly not in the
> sprite because it's animated), too. :-(

The push that you are referencing got backed out. Please include a fix for that when you reland.

(In reply to Matthew N. [:MattN] from comment #42)
> Comment on attachment 8394256 [details] [diff] [review]
> Patch v3
> 
> Review of attachment 8394256 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: browser/base/content/browser.js
> @@ +1198,5 @@
> > +        window.matchMedia("-moz-windows-default-theme")) {
> > +      let windows8WindowFrameColor = Cu.import("resource:///modules/Windows8WindowFrameColor.jsm", {}).Windows8WindowFrameColor;
> > +      let windowFrameColor = windows8WindowFrameColor.get();
> > +
> > +      // Formula from W3C Techniques For Accessibility Evaluation And
> 
> A comment explaining what the code below is actually doing would be nice.
> Moving this to a method (see below) would make that less necessary with a
> good method name.
> 
> This would be a good addition to colorUtils.jsm as it already implements
> another AERT algorithm. That would mean refactoring it to remove the metro
> specific event stuff and moving it out of browser/metro/ though so I'm not
> sure you want to do that in this bug.

Yeah, I don't want to do that in this bug, because I'd like to uplift the same patch that gets landed on m-c ideally. We can handle the other code movements in a follow-up that doesn't get uplifted.

> ColorAnalyzer.js is in places since it's primary usage is for favicons
> (which are stored in places). I think this would be better in colorUtils.jsm
> if you're looking for somewhere better to put it.
> 
> Why don't you put this algorithm in a method in Windows8WindowFrameColor.jsm
> for now to avoid adding more to browser.js?

Windows8WindowFrameColor.jsm isn't really a better place for it either. As you've said colorUtils.jsm or ColorAnalyzer.js are better places for it.

> ::: browser/modules/WindowsRegistry.jsm
> @@ +1,1 @@
> > +/* This Source Code Form is subject to the terms of the Mozilla Public
> 
> Why not put this in toolkit/modules/ since it's not browser-specific?

I don't want to hold this patch up much longer because I'd like to get more eyes on the results of it. Moving that to /toolkit/ will require another review pass and it's somewhat orthogonal to the goal of this bug. I'd prefer to handle that in a follow-up.

Comment 44

[Phil Ringnalda (:philor)]

Backed out in https://hg.mozilla.org/integration/fx-team/rev/c1f9dc63a495 for breaking WinXP in browser-chrome startup and a rather odd assortment of mochitest-chrome and mochitest-a11y tests, all of them complaining about the undefined windowFrameColor in a jsm they had no business loading or running in the first place.

Comment 45

[Matthew N. [:MattN]]

(In reply to Jared Wein [:jaws] (please needinfo? me) from comment #43)
> > ::: browser/modules/WindowsRegistry.jsm
> > Why not put this in toolkit/modules/ since it's not browser-specific?
> 
> I don't want to hold this patch up much longer because I'd like to get more
> eyes on the results of it. Moving that to /toolkit/ will require another
> review pass and it's somewhat orthogonal to the goal of this bug. I'd prefer
> to handle that in a follow-up.

You wouldn't have needed another review pass just to move the file and update the URI for the JSM.

Comment 46

[:Gijs (he/him)]

(In reply to Phil Ringnalda (:philor) from comment #44)
> Backed out in https://hg.mozilla.org/integration/fx-team/rev/c1f9dc63a495
> for breaking WinXP in browser-chrome startup and a rather odd assortment of
> mochitest-chrome and mochitest-a11y tests, all of them complaining about the
> undefined windowFrameColor in a jsm they had no business loading or running
> in the first place.

I should have caught this in review. window.matchMedia returns an object. The if condition you're using should check its "matches" property.

Comment 47

[:Gijs (he/him)]

Relanded with the correct window.matchMedia call, the sync icon fix (because I relanded sooner after we were both backed out), and the JSM moved to toolkit:

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

Comment 48

[:Gijs (he/him)]

(filed bug 986483 about all those braces in the media query... weirdness)

Comment 49

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

https://hg.mozilla.org/mozilla-central/rev/9863ccc87573

Comment 50

[Jared Wein [:jaws] (please needinfo? me)]

Clearing needinfo flags, they were set to make sure that we were going to uplift this to aurora/beta. That is in process and will be requested tomorrow.

Comment 51

[Sylvestre Ledru [:Sylvestre]]

Tracking to make sure that they are uplifted at some point.

Comment 52

[Jared Wein [:jaws] (please needinfo? me)]

Attachment: Patch for Aurora

[Approval Request Comment]
Bug caused by (feature/regressing bug #): supporting a new Windows platform
User impact if declined: impossible to read tab titles if user switched to a dark Windows theme
Testing completed (on m-c, etc.): landed on m-c, present for a couple days now
Risk to taking this patch (and alternatives if risky): none expected
String or IDL/UUID changes made by this patch: none

Comment 53

[Mike de Boer [:mikedeboer]]

Aurora: https://hg.mozilla.org/releases/mozilla-aurora/rev/88b686a5cb0a

Comment 54

[Mike de Boer [:mikedeboer]]

Beta: https://hg.mozilla.org/releases/mozilla-beta/rev/12cc25892a24

Comment 55

[Mike Conley (:mconley) (:⚙️)]

Are we sure this is 100% closed? Just saw this on Reddit:

http://i.imgur.com/M35Poqb.png

via http://www.reddit.com/r/firefox/comments/249i2t/if_you_dont_like_someallany_aspects_of_the_new/ch4zw7o

Comment 56

[Jared Wein [:jaws] (please needinfo? me)]

(In reply to Mike Conley (:mconley) from comment #55)
> Are we sure this is 100% closed? Just saw this on Reddit:

There is a P2 bug on file for a case that we one user ran into with the opposite happening (white background with white text): bug 998231. This is probably the same bug.

Comment 57

[Chris Lord [:cwiiis]]

I can't reproduce this, but note that a Firefox restart is required before it picks up the colour settings (so technically I can reproduce it, but it's transient across browser restarts).

Comment 58

[[e-mail]]

I am running Windows 8.1 with the latest update and Firefox 29 and I am still getting black text on whatever background I choose.

I have taken care to restart the browser but nope, same issue.

Comment 59

[Andrei Vaida [:avaida]]

I was able to confirm the fix for this issue on Firefox 29.0 (Build ID: 20140421221237), the latest Beta (Build ID: 20140505140302) and Aurora 31 2014-05-05, using:
  * Windows 8 32-bit [1],
  * Windows 8.1 64-bit [2].

Strangely enough, this issue is *still reproducible* on Firefox 29.0 with Windows 8.1 64-bit - checked on two different machines. Jared, any thoughts on this?


[1] Mozilla/5.0 (Windows NT 6.2; rv:30.0) Gecko/20100101 Firefox/30.0
[2] Mozilla/5.0 (Windows NT 6.3; WOW64; rv:30.0) Gecko/20100101 Firefox/30.0

Comment 60

[:Gijs (he/him)]

(In reply to Andrei Vaida, QA [:avaida] from comment #59)
> I was able to confirm the fix for this issue on Firefox 29.0 (Build ID:
> 20140421221237), the latest Beta (Build ID: 20140505140302) and Aurora 31
> 2014-05-05, using:
>   * Windows 8 32-bit [1],
>   * Windows 8.1 64-bit [2].
> 
> Strangely enough, this issue is *still reproducible* on Firefox 29.0 with
> Windows 8.1 64-bit - checked on two different machines. Jared, any thoughts
> on this?


This is because of bug 907373. The 29.0.1 builds (if we do decide to do a followup stable release) should be fixed: http://ftp.mozilla.org/pub/mozilla.org/firefox/tinderbox-builds/mozilla-release-win32/1399325500/

Comment 61

[Tim Nguyen :ntim]

I see 3 issues with the current implementation :
- Not future proof (Windows 9 will be affected :/)
- Requires a restart
- Not everything is inverted (tab scrollbox, dropdown icons);

Comment 62

[:Gijs (he/him)]

(In reply to Tim Nguyen [:ntim] from comment #61)
> I see 3 issues with the current implementation :
> - Not future proof (Windows 9 will be affected :/)

It can't be, because 9 will likely be different again and MS doesn't expose a good API for this.

> - Requires a restart

As does everything to do with changing Windows themes. Even Windows Explorer doesn't handle this correctly (try switching between glass and classic on Win7 and look at the input fields at the top of the window)

> - Not everything is inverted (tab scrollbox, dropdown icons);

This is a generic issue with inverting (also happens with lwthemes) and we already have a bug on file for it.



If you see any other issues or think any of my replies are wrong, please file a new bug. :-)