Last Comment Bug 1494034 - Add support for CSS prefers-color-scheme media feature
: Add support for CSS prefers-color-scheme media feature
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: CSS Parsing and Computation (show other bugs)
: Trunk
: x86_64 All
P3 normal with 7 votes (vote)
: mozilla67
Assigned To: Mats Palmgren (:mats)
:
: Sean Voisen (:svoisen)
Mentors: Emilio Cobos Álvarez (:emilio)
Depends on: 1529323 1368808 1466335 1525775 1528960 1532850
Blocks: mediaqueries-5 1459504 1488384 1529972
  Show dependency treegraph
 
Reported: 2018-09-25 10:04 PDT by Merih Akar
Modified: 2019-03-21 06:30 PDT (History)
48 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
wontfix
wontfix
wontfix
fixed


Attachments
Bug 1494034 - Intial work for adding prefer-dark-theme media query. (47 bytes, text/x-phabricator-request)
2018-11-01 07:16 PDT, Jonathan Kingston [:jkt]
emilio: feedback+
Details | Review
fix+tests (13.24 KB, patch)
2019-02-08 18:47 PST, Mats Palmgren (:mats)
emilio: review+
Details | Diff | Splinter Review
fix+tests (16.13 KB, patch)
2019-02-09 10:30 PST, Mats Palmgren (:mats)
emilio: review+
Details | Diff | Splinter Review
fix+tests (17.65 KB, patch)
2019-02-14 21:42 PST, Mats Palmgren (:mats)
emilio: review+
Details | Diff | Splinter Review

Description User image Merih Akar 2018-09-25 10:04:24 PDT
A new media query is added to CSS Media Queries Level 5 to utilize the dark mode preference operating systems provide.

https://drafts.csswg.org/mediaqueries-5/#prefers-color-scheme
Comment 1 User image David Baron :dbaron: 🏴󠁵󠁳󠁣󠁡󠁿 ⌚UTC-7 (if account gets disabled due to email bounces, ask a bugzilla admin to reenable it) 2018-10-25 06:42:07 PDT
https://paulmillr.com/posts/using-dark-mode-in-css/ is a post about implementation of this feature in Safari; https://webkit.org/blog/8475/release-notes-for-safari-technology-preview-68/ documents its addition.

I think if we implement this we may want to provide an hidden (about:config) preference to express a preference on platforms where the OS doesn't have one.
Comment 2 User image Dão Gottwald [::dao] 2018-10-26 07:34:29 PDT
Bug 1466335 implemented a similar proprietary media query for Mac, bug 1368808 for Windows.

(In reply to David Baron :dbaron: 🇫🇷 ⌚UTC+2 from comment #1)
> https://paulmillr.com/posts/using-dark-mode-in-css/ is a post about
> implementation of this feature in Safari;
> https://webkit.org/blog/8475/release-notes-for-safari-technology-preview-68/
> documents its addition.
> 
> I think if we implement this we may want to provide an hidden (about:config)
> preference to express a preference on platforms where the OS doesn't have
> one.

I believe this also already works via ui.systemUsesDarkTheme.
Comment 3 User image Jonathan Kingston [:jkt] 2018-11-01 07:14:39 PDT
I was hacking this over my lunch and managed to get the following working:

* { background: yellow; } // Applies to anyone without the pref override

@media (prefers-color-scheme) {  // Applies to anyone with the pref override
  * { background: red; }
}

@media (prefers-color-scheme: dark) {  // Applies to anyone with the pref set to 1
  * { background: blue; }
}

@media (prefers-color-scheme: light) { // Applies to anyone with the pref set to 0
  * { background: pink; }
}

Is this the expected behavior?
Comment 4 User image Jonathan Kingston [:jkt] 2018-11-01 07:16:40 PDT
Created attachment 9021821 [details]
Bug 1494034 - Intial work for adding prefer-dark-theme media query.
Comment 5 User image Jonathan Kingston [:jkt] 2018-11-01 07:17:34 PDT
Comment on attachment 9021821 [details]
Bug 1494034 - Intial work for adding prefer-dark-theme media query.

With the right tests is this enough to get this supported? Should we not send the light mode ever?
Comment 6 User image Jonathan Kingston [:jkt] 2018-11-01 07:29:47 PDT
Taking for now, I'm just working on this in my own time though.
Comment 7 User image Emilio Cobos Álvarez (:emilio) 2018-11-01 07:51:04 PDT
Comment on attachment 9021821 [details]
Bug 1494034 - Intial work for adding prefer-dark-theme media query.

It does look reasonable to me.

We don't have a `prefers-light-theme` thing, maybe `(prefers-color-scheme: light)` should always evaluate to false? Worth checking what other browsers do.

Similarly for what `(prefers-color-scheme)` evaluates to. The spec doesn't seem to mention anything. We should file a spec issue.

----

I just took a look at WebKit's source, and it doesn't make sense to me:

  http://webkit.crisal.io/webkit/rev/6e4aafe0e39c8d49ad33a31b9b22a2dd65192c25/Source/WebCore/css/MediaQueryEvaluator.cpp#731

The semantics WebKit implements looks wrong. Safari seems to evaluate '(prefers-color-scheme: no-preference)' to true all the time, regardless of the actual user preference.

Similarly it seems to implement light: !dark, which also seems slightly wrong.

Finally, Safari seems to flat out reject `(prefers-color-scheme)` at parse time. That is:

  matchMedia("(prefers-color-scheme)").media == "not all"

That seems unfortunate. We could implement such a thing adding another ParsingRequirements thing, but I think that always evaluating to false instead but parsing is better, since we need to eventually implement bug 1469173 which would end up with such behavior anyway.

In any case, we should:

 * Implement what WebKit implements and file spec issues, or:
 * Implement saner things and file spec issues and potentially file WebKit bugs, or:
 * Only file spec issues and hold off on the implementation.

I'd be ok with any of these, though I'd prefer (2) or (3), provided we send a PSA to dev-platform@ and people agree.
Comment 8 User image Jonathan Kingston [:jkt] 2018-11-02 03:14:41 PDT
I raised a spec issue for some clarification: https://github.com/w3c/csswg-drafts/issues/3278

I would like to see this tied to the dark/light theme if it makes sense to do so for non supporting systems.
Comment 9 User image quasicomputational 2019-01-15 07:00:20 PST
Thinking aloud slightly, what should the media query match while printing? Light, because paper? Always false, because it's only meaningful on screens?
Comment 10 User image Emilio Cobos Álvarez (:emilio) 2019-01-15 07:38:13 PST
That's a good question. I think we shouldn't intermix the concept of printing and the user theme preference, but I could see the argument for the opposite.

Maybe worth filing an issue in https://github.com/w3c/csswg-drafts?

Also, Jonathan, are you actively working on this? Chromium is implemented, so it'd be great not to lag behind much given our spec questions were answered. Let me know if you want me (or someone else, quasicomputational? :)) to take over or what not (I'll make sure that the patch appears as co-authored by you of course).
Comment 11 User image quasicomputational 2019-01-15 08:39:58 PST
Went ahead and filed https://github.com/w3c/csswg-drafts/issues/3522.

I don't mind taking this on, but I do think there's still some spec work to be done. On the other hand, both Chromium and Safari are gearing up to ship, so who knows if that'll get done before the current version's ossified...
Comment 12 User image Jonathan Kingston [:jkt] 2019-01-17 05:58:15 PST
I don't have cycles to complete this right at the moment, so if people want to take it on that is fine with me :).

> but I do think there's still some spec work to be done.

Yeah I was kinda waiting for this to resolve before working on this further.

Thanks!
Comment 13 User image Emilio Cobos Álvarez (:emilio) 2019-01-17 06:00:14 PST
Ok, I'll clear the assignee for now :)
Comment 14 User image Mats Palmgren (:mats) 2019-02-08 10:12:23 PST
I agree with this comment:
https://github.com/w3c/csswg-drafts/issues/3278#issuecomment-452834498
In particular:
> @media (prefers-color-scheme)
> Simply means that the webbrowser at hand supports communicating it's users color-scheme preference.

so, I guess that means:
> None => return prefers_dark_theme

should be:
> None => return true

Other than that, is there anything else that needs fixing here?
Comment 15 User image Emilio Cobos Álvarez (:emilio) 2019-02-08 13:10:58 PST
Well, adding tests, but yeah.
Comment 16 User image Mats Palmgren (:mats) 2019-02-08 15:09:23 PST
OK, I made a few updates to reflect the latest spec issues, and I'll add some tests too...
Comment 17 User image Mats Palmgren (:mats) 2019-02-08 18:47:18 PST
Created attachment 9042706 [details] [diff] [review]
fix+tests

A few substantial changes based on feedback in the github issues:
* if presContext->IsPrintingOrPrintPreview() is true, then return "light"
* if we fail to retrieve the LookAndFeel::eIntID_SystemUsesDarkTheme value, then return "no-preference" (this is currently the case for GTK: bug 1525775)
* we already have an integer pref, ui.systemUsesDarkTheme, where 0 means "light" and 1 means "dark"; I'm extending that so that 2 means "no-preference".  This makes the name a slight misnomer, but I think it's more user friendly to continue to use the same name so that existing usage of this pref works as before rather than introducing a new (non-binary) name. It's a hidden pref anyway, so naming isn't that important.
* made "@media (prefers-color-scheme)" return true, since we now support this feature
* added reftests

Probably worth waiting a few more days until we have CSSWG resolutions on the github issues before we finalize this.  I've added Agenda+ on both issues so hopefully we'll have answers soon.  (Feel free to punt on the review until then if you wish.)

https://treeherder.mozilla.org/#/jobs?repo=try&revision=1841c5b4d825e8f5f16c8ac74795f1c35f6654c4
Comment 18 User image Emilio Cobos Álvarez (:emilio) 2019-02-08 19:10:47 PST
Comment on attachment 9042706 [details] [diff] [review]
fix+tests

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

Looks good with those addressed, assuming the CSSWG resolves as you expect, thanks!

Let me know if you want me to do another once-over after addressing the comments or what not.

Feel free to either wait for a CSSWG resolution or not, as long as we address any possible changes ASAP.

::: layout/style/GeckoBindings.h
@@ +802,5 @@
> +// Return values have the following meaning:
> +// 0: light
> +// 1: dark
> +// 2: no-preference
> +// anything else: the user has set the ui.systemUsesDarkTheme pref to an invalid value.

Can we instead return a StylePrefersColorScheme directly? You'd need to make the enum `pub` in rust for that to work. You can follow the pattern DisplayMode / StyleDisplayMode uses.

::: layout/style/nsMediaFeatures.cpp
@@ +243,5 @@
> +{
> +  if (nsContentUtils::ShouldResistFingerprinting(aDocument)) {
> +    return 0; // "light"
> +  }
> +  if (nsIDocShell* docShell = aDocument->GetDocShell()) {

Why doesn't just aDocument->GetPresContext() work?

::: servo/components/style/gecko/media_features.rs
@@ +364,5 @@
> +    };
> +
> +    let prefers_color_scheme =
> +        unsafe { bindings::Gecko_MediaFeatures_PrefersColorScheme(device.document()) };
> +    match query_value {

Then this will just become `query_value == prefers_color_scheme`.
Comment 19 User image Mats Palmgren (:mats) 2019-02-09 10:30:13 PST
Created attachment 9042753 [details] [diff] [review]
fix+tests
Comment 20 User image Mats Palmgren (:mats) 2019-02-09 18:24:41 PST
So it appears we now pass all the tests in
https://searchfox.org/mozilla-central/source/testing/web-platform/tests/css/mediaqueries/prefers-color-scheme.html
except the last one:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=3247feecfcb9c376928afa0e9ec66a5235552d6d&selectedJob=227451411
which appears to test that "(prefers-color-scheme)" is always
the exact opposite of "(prefers-color-scheme: no-preference)".

This looks like a bogus test to me, at least with the assumption
that "(prefers-color-scheme)" is always true in an UA that
supports this feature (like we do with this patch).
Comment 21 User image Emilio Cobos Álvarez (:emilio) 2019-02-09 19:36:05 PST
Comment on attachment 9042753 [details] [diff] [review]
fix+tests

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

Yeah, I'll make sure we address whatever changes as a result of https://github.com/w3c/csswg-drafts/issues/3278, or update the WPT test / file a chromium bug.
Comment 22 User image Mats Palmgren (:mats) 2019-02-14 21:42:43 PST
Created attachment 9044088 [details] [diff] [review]
fix+tests

Updated per clarifications from CSSWG.  Intradiff:

```plaintext
 fn eval_prefers_color_scheme(device: &Device, query_value: Option<PrefersColorScheme>) -> bool {
-    let query_value = match query_value {
-        Some(v) => v,
-        None => return true,
-    };
-
     let prefers_color_scheme =
         unsafe { bindings::Gecko_MediaFeatures_PrefersColorScheme(device.document()) };
-    query_value == prefers_color_scheme
+    match query_value {
+        Some(v) => prefers_color_scheme == v,
+        None => prefers_color_scheme != PrefersColorScheme::NoPreference,
+    }
 }

```
Comment 23 User image Emilio Cobos Álvarez (:emilio) 2019-02-14 21:48:31 PST
Comment on attachment 9044088 [details] [diff] [review]
fix+tests

I've only looked at the intradiff, but looks good, thanks!

Please send an intent email when landing :)
Comment 24 User image Emilio Cobos Álvarez (:emilio) 2019-02-14 21:49:20 PST
Well I guess you also need to remove the test expectations for the test mentioned in comment 20.
Comment 25 User image Emilio Cobos Álvarez (:emilio) 2019-02-14 21:49:56 PST
Oh, it's already in the patch, nvm :)
Comment 26 User image Mats Palmgren (:mats) 2019-02-15 12:39:02 PST
Intent-to-ship: https://groups.google.com/forum/#!topic/mozilla.dev.platform/VnJ49XN6flE
Comment 27 User image Pulsebot 2019-02-15 12:41:11 PST
Pushed by mpalmgren@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/4739353088fc
Add support for CSS prefers-color-scheme media feature.  r=emilio
Comment 28 User image Cristina Coroiu [:ccoroiu] 2019-02-16 01:39:47 PST
https://hg.mozilla.org/mozilla-central/rev/4739353088fc
Comment 29 User image felix.b 2019-02-22 04:43:29 PST
Just tested this on Nightly 67.0a1 (2019-02-22) on macOS. It detects the OS theme correctly, but it doesn't adapt to theme changes like Safari does, not even after reloading the page. You need to completely quit the browser and restart it for a new theme to be considered. `window.matchMedia('(prefers-color-scheme: dark)').addListener()` handlers are also not fired (they are in Safari).
Comment 30 User image Hiroyuki Ikezoe (:hiro) 2019-02-22 13:30:37 PST
(In reply to felix.b from comment #29)

> Just tested this on Nightly 67.0a1 (2019-02-22) on macOS. It detects the OS theme correctly, but it doesn't adapt to theme changes like Safari does, not even after reloading the page. You need to completely quit the browser and restart it for a new theme to be considered. `window.matchMedia('(prefers-color-scheme: dark)').addListener()` handlers are also not fired (they are in Safari).

That's bug 1528960.

Note You need to log in before you can comment on or make changes to this bug.