Bug 196685 - [GTK] Support prefers-color-scheme media query
Summary: [GTK] Support prefers-color-scheme media query
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKitGTK (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Linux
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on: 126907
Blocks:
  Show dependency treegraph
 
Reported: 2019-04-07 15:22 PDT by Ian Brunelli
Modified: 2019-05-16 08:05 PDT (History)
6 users (show)

See Also:


Attachments
Patch (18.17 KB, patch)
2019-04-25 02:13 PDT, Carlos Garcia Campos
mcatanzaro: review+
Details | Formatted Diff | Diff
Screenshot (100.77 KB, image/png)
2019-04-25 02:14 PDT, Carlos Garcia Campos
no flags Details

Note You need to log in before you can comment on or make changes to this bug.
Description Ian Brunelli 2019-04-07 15:22:39 PDT
The prefers-color-scheme media query currently exposes dark mode only in macOS Mojave [1].

GTK should also have this feature by using the gtk-application-prefer-dark-theme [2] property.

1: https://bugs.webkit.org/show_bug.cgi?id=190499
2: https://developer.gnome.org/gtk3/stable/GtkSettings.html#GtkSettings--gtk-application-prefer-dark-theme
Comment 1 Michael Catanzaro 2019-04-24 14:04:23 PDT
BTW this feature was requested by Epiphany's primary distributor (elementary).
Comment 2 Carlos Garcia Campos 2019-04-25 01:47:03 PDT
The initial basic support for this is easy now after r244635. What I'm not sure we will be able to support is the supported color schemes feature. That means that if we are in dark mode and page claims to only support light, we would render in light mode. And if we are in light mode and page claims to only support dark, we would render in dark mode. That requires to change the theme a lot of times, which is very slow in GTK.
Comment 3 Carlos Garcia Campos 2019-04-25 02:13:59 PDT
Created attachment 368221 [details]
Patch

This can be tested with the inspector that has a really nice dark mode using prefers-color-scheme: dark. I'll submit a screenshot.
Comment 4 Carlos Garcia Campos 2019-04-25 02:14:17 PDT
Created attachment 368222 [details]
Screenshot
Comment 5 Michael Catanzaro 2019-04-25 06:18:50 PDT
Comment on attachment 368221 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=368221&action=review

(In reply to Carlos Garcia Campos from comment #2)
> What I'm not
> sure we will be able to support is the supported color schemes feature. That
> means that if we are in dark mode and page claims to only support light, we
> would render in light mode. And if we are in light mode and page claims to
> only support dark, we would render in dark mode.

From conversations with Timothy, I believe the behavior on macOS is subtly different than that. The intent is for all pages to be light unless the webpage specifically says it supports dark. That's how macOS avoids all the web compat problems we've suffered with pages that just don't expect to be displayed in dark mode.

For GTK, I think it's OK to do whatever we want that seems to work, including not supporting this at all if it's really slow, but matching the macOS behavior would of course be safest.

> LayoutTests/ChangeLog:8
> +        Unskip css-dark-mode tests and add platform specific results for some of the tests using the suported color

supported
Comment 6 Carlos Garcia Campos 2019-04-30 02:06:30 PDT
Committed r244766: <https://trac.webkit.org/changeset/244766>
Comment 7 Timothy Hatcher 2019-04-30 13:24:14 PDT
Neat!
Comment 8 Michael Catanzaro 2019-05-15 17:36:37 PDT
Comment on attachment 368221 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=368221&action=review

> Source/WebCore/testing/InternalSettings.cpp:531
> +        gboolean preferDarkTheme;
> +        g_object_get(settings, "gtk-application-prefer-dark-theme", &preferDarkTheme, nullptr);
> +        if (preferDarkTheme != useDarkAppearance)
> +            g_object_set(settings, "gtk-application-prefer-dark-theme", useDarkAppearance, nullptr);

Honestly, after talking with Cassidy today, I think we did this wrong. Cassidy actually liked your approach, but I don't anymore. Problem is gtk-application-prefer-dark-theme is used by apps that always want to be dark, e.g. Photos or Videos, to force dark theme. For Epiphany, it's guaranteed to be false, because Epiphany doesn't want to force dark theme.

But the theme could still be dark. And if the theme is dark, then we need this media query to reflect that, or websites will draw dark text on dark background. If the media query doesn't actually reflect whether the theme is dark, then we will wind up with unreadable text and no hope of web compat.

So your implementation is surely not going to be web-compatible. Instead, we should use a heuristic to guess whether the current GTK theme is dark or not, e.g. "does the current theme name end in the string 'dark'?" This is the only way dark themes will ever be usable without breaking the web. Cassidy points out there's no way to make that work for themes named, say, "Midnight." But there's not really anything we can do about that.

I know just fixing the media query is not enough. We need to support the color-schemes property too, the one formerly supported-color-schemes, which I know you said we might not be able to do. But anyway, this bug is about the media query, and what we have here just isn't going to work well.
Comment 9 Michael Catanzaro 2019-05-15 17:44:30 PDT
Comment on attachment 368221 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=368221&action=review

>> Source/WebCore/testing/InternalSettings.cpp:531
>> +            g_object_set(settings, "gtk-application-prefer-dark-theme", useDarkAppearance, nullptr);
> 
> Honestly, after talking with Cassidy today, I think we did this wrong. Cassidy actually liked your approach, but I don't anymore. Problem is gtk-application-prefer-dark-theme is used by apps that always want to be dark, e.g. Photos or Videos, to force dark theme. For Epiphany, it's guaranteed to be false, because Epiphany doesn't want to force dark theme.
> 
> But the theme could still be dark. And if the theme is dark, then we need this media query to reflect that, or websites will draw dark text on dark background. If the media query doesn't actually reflect whether the theme is dark, then we will wind up with unreadable text and no hope of web compat.
> 
> So your implementation is surely not going to be web-compatible. Instead, we should use a heuristic to guess whether the current GTK theme is dark or not, e.g. "does the current theme name end in the string 'dark'?" This is the only way dark themes will ever be usable without breaking the web. Cassidy points out there's no way to make that work for themes named, say, "Midnight." But there's not really anything we can do about that.
> 
> I know just fixing the media query is not enough. We need to support the color-schemes property too, the one formerly supported-color-schemes, which I know you said we might not be able to do. But anyway, this bug is about the media query, and what we have here just isn't going to work well.

Er, yeah I really misread this patch, sorry. I see this is only testing code.

What, then, decides whether the media query is true or false? Where is the logic that decides whether prefers-color-scheme is true or false? How can you have enabled this without any platform implementation for it?
Comment 10 Michael Catanzaro 2019-05-15 17:46:14 PDT
(To be clear: I realize I was wrong in comment #8, and that your use of gtk-application-prefer-dark-theme in the testing code does make sense. But how does this work outside tests?)
Comment 11 Cassidy James Blaede 2019-05-15 17:50:52 PDT
I still think this is correct from how you've explained it to me, Michael. 

The fact is, theme variants exist in GTK to handle dark themes, and if a theme is always-dark, it's a broken theme imho. What needs to exist in GNOME or GTK is a system-wide preference of "the user would like apps to use a dark style *if the app supports it*," like we now see in Android Q, macOS, Windows, and soon in iOS. This is separate and distinct from "the user is using a custom theme which happens to use a dark background for everything," and I don't think that case should be supported—again, because that's what the dark theme variant is for.

Instead, browsers like Epiphany could look at that system-wide dark preference and in response, set their own `gtk-application-prefer-dark-theme` based on the system-wide preference. Then, if `gtk-application-prefer-dark-theme` is true, WebKit should use the dark variant and sites that hook off that media query should also use their dark styles. That last part is what I understand this patch does—if not, I'm sorry that I've misunderstood!

I guess the one edge case that I'm not sure of is what happens when the browser has `gtk-application-prefer-dark-theme` as true, but the site does not claim to support a dark style. In *that* case, WebKit should continue to use light/default form styles to prevent color conflicts. I believe this is also how macOS Safari handles it.
Comment 12 Michael Catanzaro 2019-05-15 17:55:03 PDT
(In reply to Cassidy James Blaede from comment #11)
> That last part is what I understand this patch does—if not, I'm
> sorry that I've misunderstood!

gtk-application-prefer-dark-theme is only used in the testing code, that's what I missed. So I don't know how it could possibly work currently.

> I guess the one edge case that I'm not sure of is what happens when the
> browser has `gtk-application-prefer-dark-theme` as true, but the site does
> not claim to support a dark style. In *that* case, WebKit should continue to
> use light/default form styles to prevent color conflicts. I believe this is
> also how macOS Safari handles it.

Yes, that's even more important actually, but we don't do it currently.
Comment 13 Carlos Garcia Campos 2019-05-16 02:25:22 PDT
Comment on attachment 368221 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=368221&action=review

>>> Source/WebCore/testing/InternalSettings.cpp:531
>>> +            g_object_set(settings, "gtk-application-prefer-dark-theme", useDarkAppearance, nullptr);
>> 
>> Honestly, after talking with Cassidy today, I think we did this wrong. Cassidy actually liked your approach, but I don't anymore. Problem is gtk-application-prefer-dark-theme is used by apps that always want to be dark, e.g. Photos or Videos, to force dark theme. For Epiphany, it's guaranteed to be false, because Epiphany doesn't want to force dark theme.
>> 
>> But the theme could still be dark. And if the theme is dark, then we need this media query to reflect that, or websites will draw dark text on dark background. If the media query doesn't actually reflect whether the theme is dark, then we will wind up with unreadable text and no hope of web compat.
>> 
>> So your implementation is surely not going to be web-compatible. Instead, we should use a heuristic to guess whether the current GTK theme is dark or not, e.g. "does the current theme name end in the string 'dark'?" This is the only way dark themes will ever be usable without breaking the web. Cassidy points out there's no way to make that work for themes named, say, "Midnight." But there's not really anything we can do about that.
>> 
>> I know just fixing the media query is not enough. We need to support the color-schemes property too, the one formerly supported-color-schemes, which I know you said we might not be able to do. But anyway, this bug is about the media query, and what we have here just isn't going to work well.
> 
> Er, yeah I really misread this patch, sorry. I see this is only testing code.
> 
> What, then, decides whether the media query is true or false? Where is the logic that decides whether prefers-color-scheme is true or false? How can you have enabled this without any platform implementation for it?

It was done in r244635. What we do there is ensuring dark mode is enabled when the current theme is dark. We use heuristics indeed, see:

bool PageClientImpl::effectiveAppearanceIsDark() const
{
    auto* settings = gtk_widget_get_settings(m_viewWidget);
    gboolean preferDarkTheme;
    g_object_get(settings, "gtk-application-prefer-dark-theme", &preferDarkTheme, nullptr);
    if (preferDarkTheme)
        return true;

    GUniqueOutPtr<char> themeName;
    g_object_get(settings, "gtk-theme-name", &themeName.outPtr(), nullptr);
    if (g_str_has_suffix(themeName.get(), "-dark"))
        return true;

    if (auto* themeNameEnv = g_getenv("GTK_THEME"))
        return g_str_has_suffix(themeNameEnv, ":dark");

    return false;
}
Comment 14 Michael Catanzaro 2019-05-16 06:49:05 PDT
OK, I think that's fine for now.

If GTK ever gets a proper dark mode option, like Cassidy wants, then we can revisit.
Comment 15 Timothy Hatcher 2019-05-16 07:48:54 PDT
Comment on attachment 368221 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=368221&action=review

>>>> Source/WebCore/testing/InternalSettings.cpp:531
>>>> +            g_object_set(settings, "gtk-application-prefer-dark-theme", useDarkAppearance, nullptr);
>>> 
>>> Honestly, after talking with Cassidy today, I think we did this wrong. Cassidy actually liked your approach, but I don't anymore. Problem is gtk-application-prefer-dark-theme is used by apps that always want to be dark, e.g. Photos or Videos, to force dark theme. For Epiphany, it's guaranteed to be false, because Epiphany doesn't want to force dark theme.
>>> 
>>> But the theme could still be dark. And if the theme is dark, then we need this media query to reflect that, or websites will draw dark text on dark background. If the media query doesn't actually reflect whether the theme is dark, then we will wind up with unreadable text and no hope of web compat.
>>> 
>>> So your implementation is surely not going to be web-compatible. Instead, we should use a heuristic to guess whether the current GTK theme is dark or not, e.g. "does the current theme name end in the string 'dark'?" This is the only way dark themes will ever be usable without breaking the web. Cassidy points out there's no way to make that work for themes named, say, "Midnight." But there's not really anything we can do about that.
>>> 
>>> I know just fixing the media query is not enough. We need to support the color-schemes property too, the one formerly supported-color-schemes, which I know you said we might not be able to do. But anyway, this bug is about the media query, and what we have here just isn't going to work well.
>> 
>> Er, yeah I really misread this patch, sorry. I see this is only testing code.
>> 
>> What, then, decides whether the media query is true or false? Where is the logic that decides whether prefers-color-scheme is true or false? How can you have enabled this without any platform implementation for it?
> 
> It was done in r244635. What we do there is ensuring dark mode is enabled when the current theme is dark. We use heuristics indeed, see:
> 
> bool PageClientImpl::effectiveAppearanceIsDark() const
> {
>     auto* settings = gtk_widget_get_settings(m_viewWidget);
>     gboolean preferDarkTheme;
>     g_object_get(settings, "gtk-application-prefer-dark-theme", &preferDarkTheme, nullptr);
>     if (preferDarkTheme)
>         return true;
> 
>     GUniqueOutPtr<char> themeName;
>     g_object_get(settings, "gtk-theme-name", &themeName.outPtr(), nullptr);
>     if (g_str_has_suffix(themeName.get(), "-dark"))
>         return true;
> 
>     if (auto* themeNameEnv = g_getenv("GTK_THEME"))
>         return g_str_has_suffix(themeNameEnv, ":dark");
> 
>     return false;
> }

Couldn't you check the color value for text and/or a background for light or dark? That would catch more cases like Midnight, etc.
Comment 16 Carlos Garcia Campos 2019-05-16 08:05:52 PDT
Comment on attachment 368221 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=368221&action=review

>>>>> Source/WebCore/testing/InternalSettings.cpp:531
>>>>> +            g_object_set(settings, "gtk-application-prefer-dark-theme", useDarkAppearance, nullptr);
>>>> 
>>>> Honestly, after talking with Cassidy today, I think we did this wrong. Cassidy actually liked your approach, but I don't anymore. Problem is gtk-application-prefer-dark-theme is used by apps that always want to be dark, e.g. Photos or Videos, to force dark theme. For Epiphany, it's guaranteed to be false, because Epiphany doesn't want to force dark theme.
>>>> 
>>>> But the theme could still be dark. And if the theme is dark, then we need this media query to reflect that, or websites will draw dark text on dark background. If the media query doesn't actually reflect whether the theme is dark, then we will wind up with unreadable text and no hope of web compat.
>>>> 
>>>> So your implementation is surely not going to be web-compatible. Instead, we should use a heuristic to guess whether the current GTK theme is dark or not, e.g. "does the current theme name end in the string 'dark'?" This is the only way dark themes will ever be usable without breaking the web. Cassidy points out there's no way to make that work for themes named, say, "Midnight." But there's not really anything we can do about that.
>>>> 
>>>> I know just fixing the media query is not enough. We need to support the color-schemes property too, the one formerly supported-color-schemes, which I know you said we might not be able to do. But anyway, this bug is about the media query, and what we have here just isn't going to work well.
>>> 
>>> Er, yeah I really misread this patch, sorry. I see this is only testing code.
>>> 
>>> What, then, decides whether the media query is true or false? Where is the logic that decides whether prefers-color-scheme is true or false? How can you have enabled this without any platform implementation for it?
>> 
>> It was done in r244635. What we do there is ensuring dark mode is enabled when the current theme is dark. We use heuristics indeed, see:
>> 
>> bool PageClientImpl::effectiveAppearanceIsDark() const
>> {
>>     auto* settings = gtk_widget_get_settings(m_viewWidget);
>>     gboolean preferDarkTheme;
>>     g_object_get(settings, "gtk-application-prefer-dark-theme", &preferDarkTheme, nullptr);
>>     if (preferDarkTheme)
>>         return true;
>> 
>>     GUniqueOutPtr<char> themeName;
>>     g_object_get(settings, "gtk-theme-name", &themeName.outPtr(), nullptr);
>>     if (g_str_has_suffix(themeName.get(), "-dark"))
>>         return true;
>> 
>>     if (auto* themeNameEnv = g_getenv("GTK_THEME"))
>>         return g_str_has_suffix(themeNameEnv, ":dark");
>> 
>>     return false;
>> }
> 
> Couldn't you check the color value for text and/or a background for light or dark? That would catch more cases like Midnight, etc.

Yes, we could ask for the widget bg color and guess if it's light or dark.