Need access to Windows accent color via new system color extension

ASSIGNED
Assigned to

Status

()

Core
Widget: Win32
P1
normal
ASSIGNED
4 months ago
4 days ago

People

(Reporter: mconley, Assigned: jwatt)

Tracking

(Blocks: 2 bugs)

unspecified
Points:
---
Dependency tree / graph
Bug Flags:
qe-verify -

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [photon-visual][p1][tpi:+])

Attachments

(3 attachments)

(Reporter)

Description

4 months ago
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
(Reporter)

Updated

4 months ago
Blocks: 1344917

Updated

4 months ago
Priority: -- → P3
Whiteboard: tpi:+
(Reporter)

Comment 1

3 months ago
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?
Flags: needinfo?(dao+bmo)

Comment 2

3 months ago
(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.
Flags: needinfo?(dao+bmo)

Updated

3 months ago
Blocks: 1196266

Updated

3 months ago
No longer blocks: 1325171

Comment 3

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

Updated

3 months ago
Priority: P3 → P1
Whiteboard: tpi:+ → [tpi:+][photon]

Updated

3 months ago
Whiteboard: [tpi:+][photon] → [tpi:+][photon-visual]

Updated

3 months ago
Flags: qe-verify-
Priority: P1 → P2

Comment 4

2 months ago
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
Flags: needinfo?(dao+bmo)

Updated

2 months ago
Whiteboard: [tpi:+][photon-visual] → [photon-visual][p1][tpi:+]

Comment 5

2 months ago
(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.
Flags: needinfo?(dao+bmo)

Comment 6

2 months ago
(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.
(Assignee)

Updated

19 days ago
Assignee: nobody → jwatt

Updated

19 days ago
Status: NEW → ASSIGNED
Priority: P2 → P1
(Assignee)

Comment 7

7 days ago
Created attachment 8879338 [details] [diff] [review]
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.
(Assignee)

Comment 8

6 days ago
Created attachment 8879393 [details] [diff] [review]
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.)
Attachment #8879393 - Flags: review?(jmathies)
(Assignee)

Comment 9

6 days ago
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.
(Assignee)

Comment 10

6 days ago
Created 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

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.
Attachment #8879400 - Flags: review?(jmathies)
(Assignee)

Comment 11

6 days ago
I'm still wrapping up a patch to add a -moz-win-accentcolor-applies media query.
(Assignee)

Comment 12

6 days ago
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 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.
Attachment #8879393 - Flags: review?(jmathies) → review+
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|?
Attachment #8879400 - Flags: review?(jmathies) → review+

Comment 15

5 days ago
Pushed by jwatt@jwatt.org:
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
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
Flags: needinfo?(jwatt)

Comment 17

5 days ago
Backout by cbook@mozilla.com:
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
(Assignee)

Comment 18

5 days ago
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.
Flags: needinfo?(jwatt)
You need to log in before you can comment on or make changes to this bug.