The default bug view has changed. See this FAQ.

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

VERIFIED FIXED in Firefox 29

Status

()

Firefox
Theme
VERIFIED FIXED
3 years ago
8 months ago

People

(Reporter: cwiiis, Assigned: jaws)

Tracking

(Blocks: 1 bug)

Trunk
Firefox 31
x86_64
Windows 8.1
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox29+ fixed, firefox30+ verified, firefox31 verified)

Details

(Whiteboard: [Australis:P3])

Attachments

(4 attachments, 14 obsolete attachments)

162.94 KB, image/png
Details
84.99 KB, image/png
Details
15.36 KB, patch
Gijs
: review+
Details | Diff | Splinter Review
14.79 KB, patch
Details | Diff | Splinter Review
(Reporter)

Description

3 years ago
Created attachment 8334525 [details]
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.

Updated

3 years ago
Blocks: 939862
Depends on: 859751

Updated

3 years ago
Blocks: 738491
How about we add a white dropshadow when text color is Black? Wouldn't hurt, methinks.
Flags: needinfo?(shorlander)
Whiteboard: [Australis:P3]

Comment 2

3 years ago
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

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

Comment 4

3 years ago
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)
Flags: needinfo?(chrislord.net)

Updated

3 years ago
Summary: Text color on inactive tabs makes text very hard to read → Win8: Text color on inactive tabs makes text very hard to read

Updated

3 years ago
Duplicate of this bug: 943530

Updated

3 years ago
Depends on: 578780
(Reporter)

Comment 6

3 years ago
Created attachment 8339895 [details]
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.
Flags: needinfo?(chrislord.net)
(Reporter)

Comment 7

3 years ago
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)
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

3 years ago
Created attachment 8355881 [details]
Chrome Tabs

Comment 10

3 years ago
Created attachment 8355882 [details]
easy read Old Theme Tabs

Comment 11

3 years ago
Created attachment 8355883 [details]
hard To read Bacground Tabs on 8/8.1 & Linux

Comment 12

3 years ago
Created attachment 8355884 [details]
oldtheme Text

Comment 13

3 years ago
Created attachment 8355885 [details]
newtheme Text

Comment 14

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

Comment 15

3 years ago
Created attachment 8355888 [details]
Need Fix.jpg
Depends on: 960517
Whiteboard: [Australis:P3] → [Australis:P3][Australis:M-]

Comment 16

3 years ago
Created attachment 8362087 [details]
background_window.JPG

Comment 17

3 years ago
Created attachment 8362088 [details]
New_foreground_tabs_win8 (1).jpg

Comment 18

3 years ago
Created attachment 8362089 [details]
New_foreground_tabs_win8 (2).jpg

Comment 19

3 years ago
Created attachment 8362090 [details]
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:
Attachment #8362090 - Flags: ui-review+
Attachment #8362090 - Flags: review+
Attachment #8362090 - Flags: feedback+
Attachment #8362090 - Flags: checkin+
Attachment #8362090 - Flags: approval-mozilla-aurora?
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.
Attachment #8362090 - Attachment is obsolete: true
Attachment #8362090 - Flags: ui-review+
Attachment #8362090 - Flags: review+
Attachment #8362090 - Flags: feedback+
Attachment #8362090 - Flags: approval-mozilla-aurora?
Attachment #8362089 - Attachment is obsolete: true
Attachment #8362088 - Attachment is obsolete: true
Attachment #8362087 - Attachment is obsolete: true
Attachment #8355888 - Attachment is obsolete: true
Attachment #8355885 - Attachment is obsolete: true
Attachment #8355884 - Attachment is obsolete: true
Attachment #8355883 - Attachment is obsolete: true
Attachment #8355882 - Attachment is obsolete: true
Attachment #8355881 - Attachment is obsolete: true

Comment 21

3 years ago
Thanks 
& read it
sorry

When will this be Fixed
P.S- english not my tongue
See Also: → bug 940024

Comment 22

3 years ago
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?
Flags: needinfo?(jaws)
Created attachment 8392658 [details] [diff] [review]
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?
Assignee: nobody → jaws
Status: NEW → ASSIGNED
Attachment #8392658 - Flags: feedback?(gijskruitbosch+bugs)
Flags: needinfo?(jaws)
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.
(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

3 years ago
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; ?
Attachment #8392658 - Flags: feedback?(gijskruitbosch+bugs) → feedback+

Comment 27

3 years ago
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.
Created attachment 8393069 [details] [diff] [review]
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.
Attachment #8392658 - Attachment is obsolete: true

Comment 29

3 years ago
So, IRC comments from Jim Mathies indicate my idea is unlikely to work because on win8 the non-client areas will just be transparent. :-(
Unassigning for now but I'll keep thinking about any possible way to fix this without platform support.
Assignee: jaws → nobody
Status: ASSIGNED → NEW
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.
Assignee: nobody → jaws
Status: NEW → ASSIGNED
Created attachment 8393592 [details] [diff] [review]
Patch

This works pretty well :)
Attachment #8393069 - Attachment is obsolete: true
Attachment #8393592 - Flags: review?(gijskruitbosch+bugs)
Flags: needinfo?(shorlander)
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
Attachment #8393592 - Flags: review-
Attachment #8393592 - Flags: feedback+
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.
Created attachment 8393628 [details] [diff] [review]
Patch v2
Attachment #8393592 - Attachment is obsolete: true
Attachment #8393592 - Flags: review?(gijskruitbosch+bugs)
Attachment #8393628 - Flags: review?(dao)
Attachment #8393628 - Flags: review?(dao) → review?(gijskruitbosch+bugs)

Comment 36

3 years ago
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.
Attachment #8393628 - Flags: review?(gijskruitbosch+bugs) → feedback+

Updated

3 years ago
Whiteboard: [Australis:P3][Australis:M-] → [Australis:P3]
Created attachment 8394256 [details] [diff] [review]
Patch v3

Addressed review feedback, will wait to file the bug until this is finished.
Attachment #8393628 - Attachment is obsolete: true
Attachment #8394256 - Flags: review?(gijskruitbosch+bugs)

Comment 38

3 years ago
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.
Attachment #8394256 - Flags: review?(gijskruitbosch+bugs) → review+
(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

3 years ago
(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

3 years ago
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 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?
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.
status-firefox29: --- → affected
status-firefox30: --- → affected
status-firefox31: --- → fixed
Whiteboard: [Australis:P3] → [Australis:P3][fixed-in-fx-team]
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.
status-firefox31: fixed → affected
Whiteboard: [Australis:P3][fixed-in-fx-team] → [Australis:P3]
(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

3 years ago
(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

3 years ago
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
Whiteboard: [Australis:P3] → [Australis:P3][fixed-in-fx-team]

Comment 48

3 years ago
(filed bug 986483 about all those braces in the media query... weirdness)
https://hg.mozilla.org/mozilla-central/rev/9863ccc87573
Status: ASSIGNED → RESOLVED
Last Resolved: 3 years ago
Resolution: --- → FIXED
Whiteboard: [Australis:P3][fixed-in-fx-team] → [Australis:P3]
Target Milestone: --- → Firefox 31

Updated

3 years ago
status-firefox31: affected → fixed
tracking-firefox29: --- → ?
tracking-firefox30: --- → ?
Flags: needinfo?(jaws)
Flags: needinfo?(gijskruitbosch+bugs)
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.
Flags: needinfo?(jaws)
Flags: needinfo?(gijskruitbosch+bugs)
Tracking to make sure that they are uplifted at some point.
tracking-firefox29: ? → +
tracking-firefox30: ? → +
Created attachment 8395965 [details] [diff] [review]
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
Attachment #8395965 - Flags: approval-mozilla-aurora?
Depends on: 988191

Updated

3 years ago
No longer depends on: 988191

Updated

3 years ago
Depends on: 988191
Attachment #8395965 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Aurora: https://hg.mozilla.org/releases/mozilla-aurora/rev/88b686a5cb0a
status-firefox30: affected → fixed
Beta: https://hg.mozilla.org/releases/mozilla-beta/rev/12cc25892a24
status-firefox29: affected → fixed
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
Flags: needinfo?(chrislord.net)
Flags: needinfo?(chrislord.net) → needinfo?(jaws)
(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.
Flags: needinfo?(jaws)
(Reporter)

Comment 57

3 years ago
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).
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.
Keywords: verifyme
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
Status: RESOLVED → VERIFIED
status-firefox30: fixed → verified
status-firefox31: fixed → verified
Flags: needinfo?(jaws)
Keywords: verifyme

Comment 60

3 years ago
(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/
Flags: needinfo?(jaws)

Comment 61

3 years ago
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

3 years ago
(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. :-)

Updated

3 years ago
Depends on: 1008134
See Also: → bug 1013654

Updated

3 years ago
Duplicate of this bug: 578780

Updated

3 years ago
Depends on: 1065998
Depends on: 1292997
You need to log in before you can comment on or make changes to this bug.