1344910 - Need access to Windows accent color via new system color extension

Status: RESOLVED FIXED

(Core :: Widget: Win32, enhancement, P1)

Description

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

We want Photon to look fantastic on Windows 10, which means integrating nicely, and using the Accent Color when applicable. We also need to know when that accent color changes.

We need to add a mechanism, probably usable within CSS (so perhaps a @media selector of some kind?) that can be true if the accent color is being used, and then a color extension[1] to refer to that accent color.

We also need to know when that color changes. Windows 10 (and perhaps lower) has a setting that allows the accent color to change if the background image changes (and there's a slideshow mode for the background, so it can change at any time, arbitrarily).

[1]: Like these https://developer.mozilla.org/en/docs/Web/CSS/color_value#Mozilla_System_Color_Extensions

Comment 1

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

I'm looking for someone to own this, because it's blocking Photon.

Hey dao, am I correct in comment 0 that a @media selector for knowing the state of accent colours, and a color extension would be the API you'd want to use here?

Comment 2

[Dão Gottwald [:dao]]

(In reply to Mike Conley (:mconley) (High latency until March 31) from comment #1)
> I'm looking for someone to own this, because it's blocking Photon.
> 
> Hey dao, am I correct in comment 0 that a @media selector for knowing the
> state of accent colours, and a color extension would be the API you'd want
> to use here?

That's sounds right, yes.

Comment 3

[Dão Gottwald [:dao]]

(In reply to Mike Conley (:mconley) (Very high latency until April 3rd) from comment #1)
> I'm looking for someone to own this, because it's blocking Photon.
> 
> Hey dao, am I correct in comment 0 that a @media selector for knowing the
> state of accent colours, and a color extension would be the API you'd want
> to use here?

Looking at attachment 8766535 [details], a corresponding color extension for the text color would be handy too. I guess Windows doesn't provide a text color for this, so widget code would have to pick one itself. Could be just black / white based on the accent color, much like this: http://searchfox.org/mozilla-central/rev/72fe012899a1b27d34838ab463ad1ae5b116d76b/toolkit/modules/LightweightThemeConsumer.jsm#114-116

Comment 4

[Jim Mathies [:jimm]]

Hey Dao, is there any way we can do what we want to do here using some changes to the Windows8WindowFrameColor.jsm [1] module? The accent colors you're interested in are stored in the same location in the registry.

Alternatively, maybe it's time we move this front-end color extraction code down into widget and add color extensions for the win8 colorization colors as well?

[1] https://dxr.mozilla.org/mozilla-central/source/browser/modules/Windows8WindowFrameColor.jsm

Comment 5

[Dão Gottwald [:dao]]

(In reply to Jim Mathies [:jimm] from comment #4)
> Hey Dao, is there any way we can do what we want to do here using some
> changes to the Windows8WindowFrameColor.jsm [1] module? The accent colors
> you're interested in are stored in the same location in the registry.

Hmm, enabling accent colors in title bars and running this code on Windows 10:

new Color(...Cu.import("resource:///modules/Windows8WindowFrameColor.jsm", {}).Windows8WindowFrameColor.get())

Gives me:

Object { r: -7000796454, g: -3478923282, b: -1589137678 }

So something isn't working here.

> Alternatively, maybe it's time we move this front-end color extraction code
> down into widget and add color extensions for the win8 colorization colors
> as well?

I think this would be preferable. Windows8WindowFrameColor.jsm always seemed like a hack. In fact we already have ActiveCaption, CaptionText, InactiveCaption and InactiveCaptionText but they don't currently seem to give us useful values.

Comment 6

[Jim Mathies [:jimm]]

(In reply to Dão Gottwald [::dao] from comment #5)
> (In reply to Jim Mathies [:jimm] from comment #4)
> > Hey Dao, is there any way we can do what we want to do here using some
> > changes to the Windows8WindowFrameColor.jsm [1] module? The accent colors
> > you're interested in are stored in the same location in the registry.
> 
> Hmm, enabling accent colors in title bars and running this code on Windows
> 10:
> 
> new Color(...Cu.import("resource:///modules/Windows8WindowFrameColor.jsm",
> {}).Windows8WindowFrameColor.get())
> 
> Gives me:
> 
> Object { r: -7000796454, g: -3478923282, b: -1589137678 }

Different key names for the accent color info, but they are in there.

> So something isn't working here.
> 
> > Alternatively, maybe it's time we move this front-end color extraction code
> > down into widget and add color extensions for the win8 colorization colors
> > as well?
> 
> I think this would be preferable. Windows8WindowFrameColor.jsm always seemed
> like a hack. In fact we already have ActiveCaption, CaptionText,
> InactiveCaption and InactiveCaptionText but they don't currently seem to
> give us useful values.

Sounds good.

Comment 7

[Jonathan Watt [:jwatt]]

Attachment: For posterity: patch to add -moz-win-accentcolor keyword using GetImmersiveColorFromColorSetExPtr

Given the issue with getting the accent color from the registry reported in comment 5 (and elsewhere on the Web) I went looking for alternative ways to get the accent color. Unfortunately there is no official method to get this value. The closest I could find to an alternative was a document on Microsoft's website detailing how some uxtheme.dll entry points could be used:

https://code.msdn.microsoft.com/windowsdesktop/How-to-get-Accent-brush-6652403f

Specifically to use the undocumented functions:

  GetImmersiveColorFromColorSetEx
  GetImmersiveColorTypeFromName
  GetImmersiveUserColorSetPreference

and the undocumented ImmersiveColors type name:

  ImmersiveStartSelectionBackground

(See https://github.com/ermau/HueMove/blob/master/HueMove/Color/ImmersiveColors.cs for a list of type names that appear to have became available in Windows 8.)

I talked this approach over with jimm and we felt using unofficial API that has some documents discussing its use in addition to other people apparently using it was probably preferable to using undocumented registry entries. This patch, which works, implements that approach.

Unfortunately jimm and I are not comfortable with some of the details as things have turned out. Specifically, it turns out that GetImmersiveColorTypeFromName only has an export ordinal, and is not exported by name like the other two functions. This patch fetches it by export ordinal, but MS's documents explicitly warn against doing that when you don't control the DLLs ordinal values yourself. If the ordinal were to change we could end up crashing in arbitrary ways.

We could sidestep this issue by hardcode into the source the value returned by GetImmersiveColorTypeFromName for the type name "ImmersiveStartSelectionBackground" (the value 16), but it's also unclear how stable that number is, or even what this type name really refers to.

So we're leaning back towards the registry again, and hopefully we can figure out and work around any buggyness with that approach.

Comment 8

[Jonathan Watt [:jwatt]]

Attachment: part 1 - Add a -moz-win-accentcolor color keyword to expose the Win10 accent color

This keyword is live to Windows 10 accent color changes when used in the UI (not if it's used in content). It turned out there is no WM_SYSCOLORCHANGE, WM_THEMECHANGED or even WM_STYLECHANGED event dispatched for accent color changes as you might expect. It seems to be WM_SETTINGCHANGE that we have to listen for. (Since this implementation gets the accent color from the registry, an alternative would be to monitor changes to the DWM key. However our nsIWindowsRegKey implementation doesn't allow us to do that without polling its hasChanged() method.)

Comment 9

[Jonathan Watt [:jwatt]]

The hardcoded fallback values come from what we're currently falling back to in:

https://dxr.mozilla.org/mozilla-central/source/browser/modules/Windows8WindowFrameColor.jsm

We shouldn't be using -moz-win-accentcolor when the accent color is not available or applied to windows though, so I'm not sure how much we should care about what value we use.

Comment 10

[Jonathan Watt [:jwatt]]

Attachment: part 2 - Add a -moz-win-accentcolortext color keyword to color text that will be drawn over an accent color background

This provides the color black or white based on what will best display text drawn over a background that has the current accent color, as per comment 3, and as per what Windows 10 itself seems to do.

Comment 11

[Jonathan Watt [:jwatt]]

I'm still wrapping up a patch to add a -moz-win-accentcolor-applies media query.

Comment 12

[Jonathan Watt [:jwatt]]

Dao points out on IRC that bug 1344917 is about adding a media query, so I'll take that bug and attach the media query patch over there.

Comment 13

[Jim Mathies [:jimm]]

Comment on attachment 8879393 [details] [diff] [review]
part 1 - Add a -moz-win-accentcolor color keyword to expose the Win10 accent color

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

::: widget/windows/nsLookAndFeel.cpp
@@ +264,5 @@
>        break;
> +    case eColorID__moz_win_accentcolor:
> +      res = GetAccentColor(aColor);
> +      if (NS_SUCCEEDED(res))
> +        return res;

nit - paren here plz

@@ +768,5 @@
> +                       NS_LITERAL_STRING("SOFTWARE\\Microsoft\\Windows\\DWM"),
> +                       nsIWindowsRegKey::ACCESS_QUERY_VALUE);
> +    if (NS_WARN_IF(NS_FAILED(rv))) {
> +      mDwmKey = nullptr;
> +      return rv;

If this fails for some reason, we end up creating this registry service over and over again, failing each time. Lets remove this nullptr assignment, close the open key after we're done below, and reopen each time checking the result. Opening reg keys shouldn't be a big deal perf wise.

@@ +783,5 @@
> +      colorPrevalence != 1) {
> +    return NS_ERROR_NOT_AVAILABLE;
> +  }
> +
> +  aColor = accentColor;

did you check this? I'm surprised win registry values match our nscolor values.

Comment 14

[Jim Mathies [:jimm]]

Comment on attachment 8879400 [details] [diff] [review]
part 2 - Add a -moz-win-accentcolortext color keyword to color text that will be drawn over an accent color background

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

::: widget/windows/nsLookAndFeel.cpp
@@ +797,5 @@
> +/* static */ nsresult
> +nsLookAndFeel::GetAccentColorText(nscolor& aColor)
> +{
> +  nscolor accentColor;
> +  nsresult rv = GetAccentColor(accentColor);

Ah! ok, so lets add a comment to GetAccentColor detailing what kind of value it returns. Same for GetAccentColorText.

@@ +802,5 @@
> +  if (NS_WARN_IF(NS_FAILED(rv))) {
> +    return rv;
> +  }
> +
> +  float luminance = 0.2125f * accentColor.r + 0.7154f * accentColor.g + 0.0721f * accentColor.b;

nit - comment me plz

@@ +804,5 @@
> +  }
> +
> +  float luminance = 0.2125f * accentColor.r + 0.7154f * accentColor.g + 0.0721f * accentColor.b;
> +
> +  aColor = (luminance <= 110) ? NS_RGB(255, 255, 255) : NS_RGB(0, 0, 0);

ditto here. why do we check |luminance <= 110|?

Comment 15

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/31a3362f641c
part 1 - Add a '-moz-win-accentcolor' color keyword to expose the Win10 accent color. r=jimm
https://hg.mozilla.org/integration/mozilla-inbound/rev/94f08c38200b
part 2 - Add a '-moz-win-accentcolortext' color keyword to color text that will be drawn over an accent color background. r=jimm

Comment 16

[Carsten Book [:Tomcat]]

sorry had to back this out in https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=71444e81ac24f74179aed226fcbd90f463460c9b&filter-classifiedState=unclassified&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception since one of the changes caused crashes like https://treeherder.mozilla.org/logviewer.html#?job_id=108844802&repo=mozilla-inbound

Comment 17

[Pulsebot]

Backout by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/3878dd93fa71
Backed out changeset 94f08c38200b 
https://hg.mozilla.org/integration/mozilla-inbound/rev/8282a7374309
Backed out changeset 31a3362f641c for crashes in  marionette.py | application crashed

Comment 18

[Jonathan Watt [:jwatt]]

That crash wasn't caused by the patches here (it was caused by the patch for bug 1369508). That said, this bug did cause tier 2 stylo failures. Simon has written a stylo patch to fix those issues at https://github.com/servo/servo/pull/17449/ and once that has the necessary reviews we can reland this patch and that one together.

Comment 19

[Jonathan Watt [:jwatt]]

Because these changes need to be synchronized with Servo changes it seems like I'll need to use MozReview so that I can land using Autoland. I'll put up MozReview patches and r+ them ready for landing shortly.

Comment 20

[Jonathan Watt [:jwatt]]

Attachment: Bug 1344910, part 1 - Add a '-moz-win-accentcolor' color keyword to expose the Win10 accent color.

Review commit: https://reviewboard.mozilla.org/r/152778/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/152778/

Comment 21

[Jonathan Watt [:jwatt]]

Attachment: Bug 1344910, part 2 - Add a '-moz-win-accentcolortext' color keyword to color text that will be drawn over an accent color background.

Review commit: https://reviewboard.mozilla.org/r/152780/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/152780/

Comment 22

[Jim Mathies [:jimm]]

Comment on attachment 8881621 [details]
Bug 1344910, part 1 - Add a '-moz-win-accentcolor' color keyword to expose the Win10 accent color.

https://reviewboard.mozilla.org/r/152778/#review157926

Comment 23

[Jim Mathies [:jimm]]

Comment on attachment 8881622 [details]
Bug 1344910, part 2 - Add a '-moz-win-accentcolortext' color keyword to color text that will be drawn over an accent color background.

https://reviewboard.mozilla.org/r/152780/#review157928

Comment 24

[Mozilla Autoland]

We're sorry, Autoland could not rebase your commits for you automatically. Please manually rebase your commits and try again.

hg error in cmd: hg rebase -s 4d8180fe95f8 -d 990cbc0aaded: rebasing 404718:4d8180fe95f8 "Bug 1344910, part 1 - Add a '-moz-win-accentcolor' color keyword to expose the Win10 accent color. r=jimm"
merging servo/components/style/properties/longhand/color.mako.rs
warning: conflicts while merging servo/components/style/properties/longhand/color.mako.rs! (edit, then use 'hg resolve --mark')
unresolved conflicts (see hg resolve, then hg rebase --continue)

Comment 25

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/963efb32d822
part 1 - Add a '-moz-win-accentcolor' color keyword to expose the Win10 accent color. r=jimm
https://hg.mozilla.org/integration/autoland/rev/a4d028c197ef
part 2 - Add a '-moz-win-accentcolortext' color keyword to color text that will be drawn over an accent color background. r=jimm

Comment 26

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/963efb32d822
https://hg.mozilla.org/mozilla-central/rev/a4d028c197ef

Comment 27

[Chris Mills [:cmills]]

I've documented this:

https://developer.mozilla.org/en-US/docs/Web/CSS/color_value#Mozilla_System_Color_Extensions
https://developer.mozilla.org/en-US/Firefox/Releases/56#CSS
https://developer.mozilla.org/en-US/docs/Web/CSS/Media_Queries/Using_media_queries#-moz-windows-accent-color-in-titlebar

Can you let me know if these descriptions look OK?

Also, it seemed to me that the media query was applying when I had its value set to 0, rather than 1. Is this correct?

e.g. @media(-moz-windows-accent-color-in-titlebar: 0) { ... }

??

Comment 28

[Dão Gottwald [:dao]]

(In reply to Chris Mills (Mozilla, MDN editor) [:cmills] from comment #27)
> I've documented this:
> 
> https://developer.mozilla.org/en-US/docs/Web/CSS/
> color_value#Mozilla_System_Color_Extensions

-moz-win-accentcolor-text should be -moz-win-accentcolortext. I'd fix it myself but MDN asks me to log in with a github account and I don't have one.

> Also, it seemed to me that the media query was applying when I had its value
> set to 0, rather than 1. Is this correct?
> 
> e.g. @media(-moz-windows-accent-color-in-titlebar: 0) { ... }
> 
> ??

Maybe you're seeing bug 1379269?

Comment 29

[Chris Mills [:cmills]]

(In reply to Dão Gottwald [::dao] from comment #28)
> (In reply to Chris Mills (Mozilla, MDN editor) [:cmills] from comment #27)
> > I've documented this:
> > 
> > https://developer.mozilla.org/en-US/docs/Web/CSS/
> > color_value#Mozilla_System_Color_Extensions
> 
> -moz-win-accentcolor-text should be -moz-win-accentcolortext. I'd fix it
> myself but MDN asks me to log in with a github account and I don't have one.

No worries, I've fixed the typo.

Yeah, it is sad that we require a GitHub login these days, but we had to do something with Persona going away, and GitHub login seemed like the best option fore the target audience, given the developer resources we had available at the time.

> 
> > Also, it seemed to me that the media query was applying when I had its value
> > set to 0, rather than 1. Is this correct?
> > 
> > e.g. @media(-moz-windows-accent-color-in-titlebar: 0) { ... }
> > 
> > ??
> 
> Maybe you're seeing bug 1379269?

Yes, I think so — on restart, the example works as expected. Thanks for your help!