Bug 196685

BTW this feature was requested by Epiphany's primary distributor (elementary).

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.

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.

Created attachment 368222 [details] Screenshot

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

Committed r244766: <https://trac.webkit.org/changeset/244766>

Neat!

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 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?

(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?)

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.

(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 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; }

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 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 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.

This doesn't seems resolved. The prefers-color-scheme media query always return light on epiphany :/ Maybe it's an epiphany-related bug ? Sorry if it's the case

(In reply to account from comment #17) > This doesn't seems resolved. The prefers-color-scheme media query always > return light on epiphany :/ Maybe it's an epiphany-related bug ? Sorry if > it's the case Does the name of your GTK theme end in "-dark" or "-Dark"?

Nope, but it contains -dark (the full name is "WhiteSur-dark-purple"). Other softwares like Firefox manage to detect that the color scheme should be dark 🤔

Well that's why. If you want it to work, rename it to *end* in "-dark" or -"Dark".

Okay ! Could you consider implementing a feature so that theme with the nomenclature themeName-dark/light-accentColor are well detected ?

I think what we've implemented matches what GTK does itself when apps use the gtk-application-prefer-dark-theme setting.