Closed Bug 1344910 Opened 8 years ago Closed 7 years ago

Need access to Windows accent color via new system color extension

Categories

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

enhancement

Tracking

()

RESOLVED FIXED
mozilla56
Iteration:
56.2 - Jul 10
Tracking Status
firefox56 --- fixed

People

(Reporter: mconley, Assigned: jwatt)

References

Details

(Keywords: dev-doc-complete, Whiteboard: [photon-visual][p1][tpi:+])

Attachments

(3 files, 2 obsolete files)

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
Blocks: 1344917
Priority: -- → P3
Whiteboard: tpi:+
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)
(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)
Blocks: 1196266
No longer blocks: photon-visual
(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
Priority: P3 → P1
Whiteboard: tpi:+ → [tpi:+][photon]
Whiteboard: [tpi:+][photon] → [tpi:+][photon-visual]
Flags: qe-verify-
Priority: P1 → P2
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)
Whiteboard: [tpi:+][photon-visual] → [photon-visual][p1][tpi:+]
(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)
(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: nobody → jwatt
Status: NEW → ASSIGNED
Priority: P2 → P1
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.
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)
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.
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)
I'm still wrapping up a patch to add a -moz-win-accentcolor-applies media query.
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+
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
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
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)
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 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
Attachment #8881621 - Flags: review?(jmathies) → review+
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
Attachment #8881622 - Flags: review?(jmathies) → review+
Attachment #8879393 - Attachment is obsolete: true
Attachment #8879400 - Attachment is obsolete: true
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)
Pushed by ecoal95@gmail.com: 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
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla56
Iteration: --- → 56.2 - Jul 10
Blocks: 1366405
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) { ... } ??
(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?
Flags: needinfo?(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!
Flags: needinfo?(cmills)
Depends on: 1514715
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: