Currently the web breaks pretty badly when users use a dark GTK theme. We should follow macOS in restricting dark theme usage to websites that have declared support for it using the color-scheme CSS property, as documented in https://webkit.org/blog/8840/dark-mode-support-in-webkit/. This is not possible to do perfectly because GTK does not have any concept of dark vs. light themes, so we'll have to use heuristics. We can attempt to switch from a dark theme to light theme by default, e.g. by pruning -dark or :dark off the name of the GTK theme and hoping for the best. Then we can switch back to the dark theme variant only if used on websites that really support dark mode. If we cannot switch like this due to performance considerations, then it's better to always try to prune the -dark portion of the theme name and use a light theme, because dark themes will never be web-compatible except for websites that opt-in.
This is indeed not so easy. Our theming code always uses the values from current theme, and the only way to use a different theme is by actually changing the theme. That's indeed what mac does, but in GTK loading a new theme takes some time, and we would need to do that for every single call to RenderTheme API. I implemented it, and it was unusable.
OK. (In reply to Carlos Garcia Campos from comment #1) > and we would need to do that for every single call to RenderTheme API. Naively, we'd like to change the theme just once per webpage. Nowadays we're guaranteed to have a separate process per web view, except for related views. The problem becomes related views and frames. The page displayed in a parent frame might opt-in to dark theme, but not the page in a child frame, even if both frames share the same security origin (and therefore the same process, under PSON). So at a minimum, we would need to change the theme at least as often as we switch between rendering related views or different frames in the same process, but only to the extent the pages rendered in those views/frames want different themes. An alternative approach would be to attempt to force light theme in GTK 3, punt dark theme to GTK 4, and see if we can convince Company to support multiple themes in the same process. It could be doable if GTK were to grow a library context object so that multiple instances of GTK could run in the same process. That might be nuts, though.
Something that might simplify this: it looks like almost* all other engines on other OSes use the light styles always, regardless of the site declaring support in CSS. I presume this is because browsers have largely had light-styled defaults for so long that it's less breaking to keep the light defaults, and assume that developers adopting the *new* feature of `prefers-color-scheme` are going to provide styles that work in the majority of browsers shipping today. I've put together a little demo site to show the current behaviors of Gecko, Blink, WebKit, and WebKitGTK across OSes: https://cassidyjames.com/dark-demo/ https://cassidyjames.com/dark-demo/mixed.html https://cassidyjames.com/dark-demo/dark.html It looks like the two outliers are Firefox on Linux (due to using the dark GTK theme for inputs) and WebKitGTK due to using a dark default stylesheet *and* dark styled inputs. I think WebKit GTK could safely always use the light GTK variant and be more in line with the other browsers.
(In reply to Cassidy James Blaede from comment #3) > It looks like the two outliers are Firefox on Linux (due to using the dark > GTK theme for inputs) and WebKitGTK due to using a dark default stylesheet > *and* dark styled inputs. I think WebKit GTK could safely always use the > light GTK variant and be more in line with the other browsers. It is not technically possible to do with GTK, per Carlos's comments above. We seem to have reached a consensus that we need to remove support for GTK themes in order to fix this bug, because the GTK developers are not interested in changing GTK to make this solvable. Fixing this bug is more important than keeping GTK theme support, and GTK developers don't want us using GTK theme on the web anyway. A large number of other dark mode bugs will disappear as soon as we remove the GTK support. But that requires writing a cross-platform theme to replace the GTK theme. Safari might be doing this soon too, to get away from macOS platform theme, so we will wait and see what happens there.
(In reply to Michael Catanzaro from comment #4) > We seem to have reached a consensus that we need to remove support for GTK > themes in order to fix this bug, because the GTK developers are not > interested in changing GTK to make this solvable. Fixing this bug is more > important than keeping GTK theme support, and GTK developers don't want us > using GTK theme on the web anyway. A large number of other dark mode bugs > will disappear as soon as we remove the GTK support. That does sound reasonable for a long-term goal, and is in line with what Chrome has done across platforms as far as I can tell. > But that requires writing a cross-platform theme to replace the GTK theme. > Safari might be doing this soon too, to get away from macOS platform theme, > so we will wait and see what happens there. The one major issue I see with waiting is that the behavior in WebKitGTK changed recently, and is badly breaking websites. See specifically the case where a developer defines a foreground color but not a background color, which is not uncommon: https://cassidyjames.com/dark-demo/mixed.html A real-world example of this is Startpage.com search results and the GNOME Wiki (https://wiki.gnome.org/Apps/Web). Websites are completely broken with the new dark default stylesheet when using a dark GTK theme. If that's a standalone bug, I am fine to file it, but it's my biggest concern right now with WebKitGTK and will make WebKitGTK unusable for elementary OS in its current state.
(In reply to Michael Catanzaro from comment #4) > But that requires writing a cross-platform theme to replace the GTK theme. > Safari might be doing this soon too, to get away from macOS platform theme, > so we will wait and see what happens there. Would it be unreasonable to bundle a light-only Adwaita with WebKitGTK, at least in the meantime? Adwaita is a very neutral style and removing its dark variant would not be difficult.
Sadly dark themes have *never* been well-supported by WebKitGTK (as evidenced by many, many bugs in this bugtracker) so I strongly recommend you ensure it doesn't get used until this changes... somehow. If it's possible to change theme using environment variables (not sure), then you could run Epiphany with an envvar to force light theme, for instance. If there's some particular commit that has made the situation worse recently, and you could identify it, then we could consider reverting. (In reply to Cassidy James Blaede from comment #6) > Would it be unreasonable to bundle a light-only Adwaita with WebKitGTK, at > least in the meantime? Adwaita is a very neutral style and removing its dark > variant would not be difficult. Maybe?
(In reply to Cassidy James Blaede from comment #3) > I've put together a little demo site to show the current behaviors of Gecko, > Blink, WebKit, and WebKitGTK across OSes: > > https://cassidyjames.com/dark-demo/ > https://cassidyjames.com/dark-demo/mixed.html > https://cassidyjames.com/dark-demo/dark.html > > It looks like the two outliers are Firefox on Linux (due to using the dark > GTK theme for inputs) and WebKitGTK due to using a dark default stylesheet > *and* dark styled inputs. I think WebKit GTK could safely always use the > light GTK variant and be more in line with the other browsers. This is a really helpful demo BTW. I agree the WebKitGTK behavior is clearly broken.
(In reply to Michael Catanzaro from comment #7) > Sadly dark themes have *never* been well-supported by WebKitGTK (as > evidenced by many, many bugs in this bugtracker) so I strongly recommend you > ensure it doesn't get used until this changes... somehow. If it's possible > to change theme using environment variables (not sure), then you could run > Epiphany with an envvar to force light theme, for instance. The GTK_THEME environment variable works for all GTK+ applications: % GTK_THEME=Adwaita epiphany It should also be possible to use the GtkSettings::theme-name property to set a theme programatically. That being said, forcing its value for any application that uses WebKitGTK would be to blatantly ignore the user's settings: instead, I would only do it for the Web process, where we do handling of the widgets *from Web pages*, so only those would be affected, and the chrome of the applications using WebKitGTK (like Epiphany) would still follow the user settings. > If there's some particular commit that has made the situation worse > recently, and you could identify it, then we could consider reverting. > > (In reply to Cassidy James Blaede from comment #6) > > Would it be unreasonable to bundle a light-only Adwaita with WebKitGTK, at > > least in the meantime? Adwaita is a very neutral style and removing its dark > > variant would not be difficult. > > Maybe? If that is the way we want things to go (forcing a non-dark theme) there is no need to bundle it. We can use GtkSettings::theme-name or GTK_THEME (as outlined above) to force using Adwaita in the Web process as soon as it starts. I would rather not bundle a GTK theme with WebKit which is expected to be installed anyway — if we do, it will soon get desynced from upstream Adwaita.
(In reply to Adrian Perez from comment #9) > > [...] > > If that is the way we want things to go (forcing a non-dark theme) > there is no need to bundle it. We can use GtkSettings::theme-name > or GTK_THEME (as outlined above) to force using Adwaita in the Web > process as soon as it starts. Actually, I would try and be more fine grained here: if the current value of GtkSettings::theme-name ends in “-dark”, remove the suffix; in any other case set it to “Adwaita”. If the theme name resulting from removing the “-dark” suffix is not installed, GTK will default to Adwaita anyway. Rationaly: If I have installed themes “Arc” and “Arc-dark”, and my settings have “Arc-dark” configured, I would rather have controls rendered in Web pages using “Arc” than “Adwaita”. (Yes, I am one of those people in the intersection of “subjects who don't like Adwaita much” and “subjects who have the slight amount of OCD to care that widgets in Web pages get rendered at least in a somewhat coherent way with the rest of the system theme”).
(In reply to Adrian Perez from comment #10) > Actually, I would try and be more fine grained here: if the current > value of GtkSettings::theme-name ends in “-dark”, remove the suffix; +1, let's do this for now, and close all the dark theme bugs. Themes named "midnight" will just stay broken for now. In the long term, after removing support for GTK themes, then we can have some proper dark mode support. But until then, forcing light is the way to go. > in any other case set it to “Adwaita”. This must be a typo, because then we'd change "Arc" to "Adwaita" and you don't want that.
(In reply to Michael Catanzaro from comment #11) > (In reply to Adrian Perez from comment #10) > > Actually, I would try and be more fine grained here: if the current > > value of GtkSettings::theme-name ends in “-dark”, remove the suffix; > > +1, let's do this for now, and close all the dark theme bugs. Themes named > "midnight" will just stay broken for now. > > In the long term, after removing support for GTK themes, then we can have > some proper dark mode support. But until then, forcing light is the way to > go. I can try to have a patch ready in the next days. > > in any other case set it to “Adwaita”. > > This must be a typo, because then we'd change "Arc" to "Adwaita" and you > don't want that. True, I wrote too fast. Let's try again, this time writing down things properly: * If the theme name is “<name>-dark”: - Remove the “-dark” suffix, use “<name>” as the theme. - If the theme “<name>” does not exist, GTK automatically fall-back to Adwaita. * Otherwise: - Keep the current theme name, assuming that it is light. This matches the on-going discussion about the proposal started around the last GUADEC where dark themes should be required to have a “-dark” suffix.
(In reply to Cassidy James Blaede from comment #3) > I've put together a little demo site to show the current behaviors of Gecko, > Blink, WebKit, and WebKitGTK across OSes: > > https://cassidyjames.com/dark-demo/ > https://cassidyjames.com/dark-demo/mixed.html > https://cassidyjames.com/dark-demo/dark.html > > It looks like the two outliers are Firefox on Linux (due to using the dark > GTK theme for inputs) and WebKitGTK due to using a dark default stylesheet > *and* dark styled inputs. I think WebKit GTK could safely always use the > light GTK variant and be more in line with the other browsers. BTW Emilio: this is what I had mentioned to you a couple weeks ago. You can see in https://cassidyjames.com/dark-demo/ or https://cassidyjames.com/dark-demo/mixed.html that Firefox on Linux displays a clearly undesirable result. With https://cassidyjames.com/dark-demo/dark.html the page content looks OK but is still different than all the other browsers.
I can see that in Firefox 67 (just a build that I had handy), but seems fixed in Firefox 70 (this is the "force light theme" hack that I mentioned at the hackfest)
Is there any update on fixing this quite critical bug? For now the only way for users is to launch WebKitGTK applications with a light variant of the theme, which breaks integration for the whole application (rather than just web, which is doomed to not be) and usage for non-advanced users which would still prefer a dark variant for their desktop.
I agree with Adrian that this is a good idea: (In reply to Adrian Perez from comment #12) > * If the theme name is “<name>-dark”: > - Remove the “-dark” suffix, use “<name>” as the theme. > - If the theme “<name>” does not exist, GTK automatically > fall-back to Adwaita. > * Otherwise: > - Keep the current theme name, assuming that it is light. > > This matches the on-going discussion about the proposal started around > the last GUADEC where dark themes should be required to have a “-dark” > suffix. Then we need to revert the change from bug #196685 and have PageClientImpl::effectiveAppearanceIsDark always return false. And then we can close this bug, and all related dark mode bugs. In the future, it would be nice to actually support dark mode, but it's clear at this point we can't do that in GTK, so it must be preconditioned on removal of GTK theme support and implementation of a cross-platform theme that doesn't use GTK.
(In reply to Michael Catanzaro from comment #16) > Then we need to revert the change from bug #196685 and have > PageClientImpl::effectiveAppearanceIsDark always return false. And then we > can close this bug, and all related dark mode bugs. Carlos, are you OK with this proposal? It requires rolling out r244766. (In reply to Adrian Perez from comment #12) > * If the theme name is “<name>-dark”: > - Remove the “-dark” suffix, use “<name>” as the theme. > - If the theme “<name>” does not exist, GTK automatically > fall-back to Adwaita. > * Otherwise: > - Keep the current theme name, assuming that it is light. Let's do this too. I guess the RenderTheme::singleton() constructor in RenderThemeGtk.cpp would be a good place. Only other place I can think of that might be appropriate would be WebProcessMainGtk.cpp.
Created attachment 389004 [details] Patch
Thanks for the patch. If this patch contains new public API please make sure it follows the guidelines for new WebKit2 GTK+ API. See http://trac.webkit.org/wiki/WebKitGTK/AddingNewWebKit2API
Comment on attachment 389004 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=389004&action=review Thanks Carlos, this was a huge help. I'll test it soon to verify it fixes older unpatched Evolution, and to check out its behavior in Cassidy's test cases. > LayoutTests/platform/gtk/css-dark-mode/default-colors-expected.txt:9 > -PASS Body text color is white > -FAIL View base background color is a dark grey assert_equals: expected "rgb(30, 30, 30)" but got "rgb(51, 57, 59)" > +FAIL Body text color is white assert_equals: expected "rgb(255, 255, 255)" but got "rgb(0, 0, 0)" > +FAIL View base background color is a dark grey assert_equals: expected "rgb(30, 30, 30)" but got "rgb(255, 255, 255)" I have a dream that one day, we can make this pass. One day.... > Source/WebCore/ChangeLog:10 > + guads should be enough. guards > Source/WebKit/ChangeLog:11 > + Handle the theme changes in the UI process, converting dark variant to the light one before sending the theme > + name to the web process. The web process is still notified when a dark theme is in use, so that if website > + prefers a dark color scheme it will be used, but the gtk theme that will be used for controls styling will > + always be light. I wrote a very long comment on why you should not do this, but checking Cassidy's example, maybe you're right and this is actually safe and amazing. I had failed to realize that other browser's dark modes are actually using light form controls (except Firefox on Linux, at least in Cassidy's old screenshots; I think Emilio mentioned that needs to be fixed). So maybe this is perfect. I will test. > Source/WebKit/UIProcess/API/gtk/PageClientImpl.cpp:598 > + if (name.endsWith("-dark")) Why aren't you also checking for ":dark" here? Mistake? > Source/WebKit/UIProcess/API/gtk/PageClientImpl.cpp:601 > +String PageClientImpl::themeName() const > +{ > + if (auto* themeNameEnv = g_getenv("GTK_THEME")) { > + String name = String::fromUTF8(themeNameEnv); > + if (name.endsWith("-dark") || name.endsWith(":dark")) > + return name.substring(0, name.length() - 5); > + return name; > + } > + > + GUniqueOutPtr<char> themeNameSetting; > + g_object_get(gtk_widget_get_settings(m_viewWidget), "gtk-theme-name", &themeNameSetting.outPtr(), nullptr); > + String name = String::fromUTF8(themeNameSetting.get()); > + if (name.endsWith("-dark")) > + return name.substring(0, name.length() - 5); > + return name; > +} Why does this have to be done in UI process instead of the web process? I was thinking we would do it in the RenderThemeGtk singleton function, so it takes effect exactly once per process, before rendering anything. Then you wouldn't have to remove the web process theme monitor. Is there a problem with that? I'm OK with removing the web process theme monitor if this really needs to move to the UI process, I just don't see why it does. > Source/WebKit/WebProcess/gtk/WebProcessMainGtk.cpp:58 > + // Ignore the GTK_THEME environment variable, the theme is always set by the UI process now. > + g_unsetenv("GTK_THEME"); You can't mutate the environment after XInitThreads(). It's not safe. Prior to this point, we have the g_usleep() -- which probably doesn't initialize threads, but we don't know for sure that will never change -- and the AuxiliaryProcessMainBase constructor, and libgcrypt initialization in Source/WebKit/WebProcess/EntryPoint/unix/WebProcessMain.cpp. I would move this to the top of WebProcessMain.cpp. Even if doing it at the top of this constructor is totally safe -- I think it is, because our platformInitialize() is currently called before InitializeWebKit2(), putting it there would be dangerous because any of that could change in the future. Mutating the environment is so exceptionally dangerous that it's just not worth messing around with this. The top of main() in WebProcessMain.cpp is always going to be safe, so definitely makes sense to do it there. > Source/WebKit/WebProcess/gtk/WebProcessMainGtk.cpp:60 > gtk_init(nullptr, nullptr); GTK actually uses a library constructor to unset environment variables, to avoid crashes that could occur if it were to do so in gtk_init(). On Windows, where library constructors aren't available, it risks crashing.
(In reply to Michael Catanzaro from comment #20) > Thanks Carlos, this was a huge help. I'll test it soon to verify it fixes > older unpatched Evolution, and to check out its behavior in Cassidy's test > cases. OK, it fixes the Evolution/Geary regression as expected. Great! It also fixes Cassidy's third test in comment #3, dark style, which was previously broken. It badly fails Cassidy's first two tests though (neither of which passed before). Test #1 (no style) is now worse than before because it now uses black text (as desired) instead of light text (not desired) on a dark background (not desired). Test #2 (some style) is a little better than before, but still pretty bad. We need to pass these tests to be web-compatible. The problem is that you've now fixed the text color and all the form controls, but the background color is still wrong because the web process background is transparent, so the web content is displayed over the UI process's dark background instead of the light web process background. That is, everything is now perfect except for the background, and the background is only wrong because it is still drawn using the UI process theme! I'm going to give r+ because your patch moves us far in the desired direction. Everything except the background now works as desired, which is much better than before. We'll probably get new bug reports from users once this lands though, because dark mode users are going to see more dark text on dark background than they did before this patch. So after this patch lands, then a full, complete fix for all our dark mode problems would simply require rendering a default web view background in the web process, instead of allowing the UI process background color to peek through. So I plan to close all existing dark mode bugs and open one new bug for the background. Do you agree? And: is the background something you'd have time to work on?
(In reply to Michael Catanzaro from comment #20) > Comment on attachment 389004 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=389004&action=review > > Thanks Carlos, this was a huge help. I'll test it soon to verify it fixes > older unpatched Evolution, and to check out its behavior in Cassidy's test > cases. > > > LayoutTests/platform/gtk/css-dark-mode/default-colors-expected.txt:9 > > -PASS Body text color is white > > -FAIL View base background color is a dark grey assert_equals: expected "rgb(30, 30, 30)" but got "rgb(51, 57, 59)" > > +FAIL Body text color is white assert_equals: expected "rgb(255, 255, 255)" but got "rgb(0, 0, 0)" > > +FAIL View base background color is a dark grey assert_equals: expected "rgb(30, 30, 30)" but got "rgb(255, 255, 255)" > > I have a dream that one day, we can make this pass. One day.... Once we stop using GTK theme, I guess. > > Source/WebCore/ChangeLog:10 > > + guads should be enough. > > guards Ooops. > > Source/WebKit/ChangeLog:11 > > + Handle the theme changes in the UI process, converting dark variant to the light one before sending the theme > > + name to the web process. The web process is still notified when a dark theme is in use, so that if website > > + prefers a dark color scheme it will be used, but the gtk theme that will be used for controls styling will > > + always be light. > > I wrote a very long comment on why you should not do this, but checking > Cassidy's example, maybe you're right and this is actually safe and amazing. This seems to be what other browsers do, according to the screenshots in the demos. When it's properly implemented it looks great, like the web inspector in dark mode. > I had failed to realize that other browser's dark modes are actually using > light form controls (except Firefox on Linux, at least in Cassidy's old > screenshots; I think Emilio mentioned that needs to be fixed). So maybe this > is perfect. I will test. Looks consistent and fixes the problem of unreadable form controls. > > Source/WebKit/UIProcess/API/gtk/PageClientImpl.cpp:598 > > + if (name.endsWith("-dark")) > > Why aren't you also checking for ":dark" here? Mistake? That's what we currently do, I think :dark is only used in the env var, but not in theme name settings. > > Source/WebKit/UIProcess/API/gtk/PageClientImpl.cpp:601 > > +String PageClientImpl::themeName() const > > +{ > > + if (auto* themeNameEnv = g_getenv("GTK_THEME")) { > > + String name = String::fromUTF8(themeNameEnv); > > + if (name.endsWith("-dark") || name.endsWith(":dark")) > > + return name.substring(0, name.length() - 5); > > + return name; > > + } > > + > > + GUniqueOutPtr<char> themeNameSetting; > > + g_object_get(gtk_widget_get_settings(m_viewWidget), "gtk-theme-name", &themeNameSetting.outPtr(), nullptr); > > + String name = String::fromUTF8(themeNameSetting.get()); > > + if (name.endsWith("-dark")) > > + return name.substring(0, name.length() - 5); > > + return name; > > +} > > Why does this have to be done in UI process instead of the web process? We need to override the theme to force a light variant in the web process. Once you have overriden the theme by setting the gtk-theme-name setting, you don't get notifications when the theme is changed in the system. So, the theme monitor in the web process does nothing. > I > was thinking we would do it in the RenderThemeGtk singleton function, so it > takes effect exactly once per process, before rendering anything. Then you > wouldn't have to remove the web process theme monitor. Is there a problem > with that? Yes, it doesn't work after setting the theme manually. > I'm OK with removing the web process theme monitor if this really needs to > move to the UI process, I just don't see why it does. > > > Source/WebKit/WebProcess/gtk/WebProcessMainGtk.cpp:58 > > + // Ignore the GTK_THEME environment variable, the theme is always set by the UI process now. > > + g_unsetenv("GTK_THEME"); > > You can't mutate the environment after XInitThreads(). It's not safe. > > Prior to this point, we have the g_usleep() -- which probably doesn't > initialize threads, but we don't know for sure that will never change -- and > the AuxiliaryProcessMainBase constructor, and libgcrypt initialization in > Source/WebKit/WebProcess/EntryPoint/unix/WebProcessMain.cpp. I would move > this to the top of WebProcessMain.cpp. Even if doing it at the top of this > constructor is totally safe -- I think it is, because our > platformInitialize() is currently called before InitializeWebKit2(), putting > it there would be dangerous because any of that could change in the future. > Mutating the environment is so exceptionally dangerous that it's just not > worth messing around with this. The top of main() in WebProcessMain.cpp is > always going to be safe, so definitely makes sense to do it there. I'll move it. > > Source/WebKit/WebProcess/gtk/WebProcessMainGtk.cpp:60 > > gtk_init(nullptr, nullptr); > > GTK actually uses a library constructor to unset environment variables, to > avoid crashes that could occur if it were to do so in gtk_init(). On > Windows, where library constructors aren't available, it risks crashing. I don't understand this.
(In reply to Michael Catanzaro from comment #21) > (In reply to Michael Catanzaro from comment #20) > > Thanks Carlos, this was a huge help. I'll test it soon to verify it fixes > > older unpatched Evolution, and to check out its behavior in Cassidy's test > > cases. > > OK, it fixes the Evolution/Geary regression as expected. Great! \o/ > It also fixes Cassidy's third test in comment #3, dark style, which was > previously broken. > > It badly fails Cassidy's first two tests though (neither of which passed > before). They work fine here, I'm getting the same results than the screenshots (expect the WebKitGTK and Firefox on Linux, of course). > Test #1 (no style) is now worse than before because it now uses > black text (as desired) instead of light text (not desired) on a dark > background (not desired). Background shouldn't be dark, it's white here. > Test #2 (some style) is a little better than > before, but still pretty bad. We need to pass these tests to be > web-compatible. The problem is that you've now fixed the text color and all > the form controls, but the background color is still wrong because the web > process background is transparent, so the web content is displayed over the > UI process's dark background instead of the light web process background. > That is, everything is now perfect except for the background, and the > background is only wrong because it is still drawn using the UI process > theme! #if HAVE(OS_DARK_MODE_SUPPORT) -#if PLATFORM(COCOA) - static const auto cssValueControlBackground = CSSValueAppleSystemControlBackground; -#else - static const auto cssValueControlBackground = CSSValueWindow; -#endif - Color baseBackgroundColor = backgroundColor.valueOr(RenderTheme::singleton().systemColor(cssValueControlBackground, styleColorOptions())); + Color baseBackgroundColor = backgroundColor.valueOr(RenderTheme::singleton().systemColor(CSSValueAppleSystemControlBackground, styleColorOptions())); #else Color baseBackgroundColor = backgroundColor.valueOr(Color::white); #endif This reverts changes made in r244635, so now we always use white. Please, check OS_DARK_MODE_SUPPORT is not defined in your build. > I'm going to give r+ because your patch moves us far in the desired > direction. Everything except the background now works as desired, which is > much better than before. We'll probably get new bug reports from users once > this lands though, because dark mode users are going to see more dark text > on dark background than they did before this patch. So after this patch > lands, then a full, complete fix for all our dark mode problems would simply > require rendering a default web view background in the web process, instead > of allowing the UI process background color to peek through. So I plan to > close all existing dark mode bugs and open one new bug for the background. > Do you agree? And: is the background something you'd have time to work on? Yes, I can work on it if I can reproduce it.
Committed r255342: <https://trac.webkit.org/changeset/255342>
(In reply to Carlos Garcia Campos from comment #22) > > > Source/WebKit/WebProcess/gtk/WebProcessMainGtk.cpp:60 > > > gtk_init(nullptr, nullptr); > > > > GTK actually uses a library constructor to unset environment variables, to > > avoid crashes that could occur if it were to do so in gtk_init(). On > > Windows, where library constructors aren't available, it risks crashing. > > I don't understand this. It's simple! gtk_init() is a library call, so it has no way to guarantee that it will be called before the application creates a secondary thread. And you know that you cannot mutate the environment after threads are created, because setenv() and unsetenv() are "MT-Unsafe const:env" (meaning: cannot be used in multithreaded programs unless you guarantee no other threads ever use getenv(), which is wildly unrealistic and never possible for a library to guarantee). I'm sure you're well aware of that, because we discussed it many times in the past. :) So by using a library constructor, GTK guarantees it can call unsetenv() before application code ever runs. The only way this fails is if another library uses a library constructor that decides to initialize threads.
It occurs to me that there is one thing Cassidy's examples don't check: the color-scheme CSS property (formerly supported-color-schemes). Now, we can't support that until the distant future arrives, once we have an HTML theme that doesn't depend on GTK. If that is set by the website, then we should draw form controls using the OS's dark theme. I think. Cassidy, any chance you have time to investigate this further, in a new bug report? It would be helpful to have a fourth test case that makes use of color-schemes and does not use prefers-color-scheme at all, and maybe a fifth that uses both. Does that make sense? (In reply to Carlos Garcia Campos from comment #23) > This reverts changes made in r244635, so now we always use white. Please, > check OS_DARK_MODE_SUPPORT is not defined in your build. I will check. It doesn't seem likely, but I don't know how else to explain hte difference in behavior we are seeing.
(In reply to Michael Catanzaro from comment #26) > It occurs to me that there is one thing Cassidy's examples don't check: the > color-scheme CSS property (formerly supported-color-schemes). Now, we can't > support that until the distant future arrives, once we have an HTML theme > that doesn't depend on GTK. BTW, it's currently only implemented by Safari: https://caniuse.com/#search=color-scheme. Other browsers only have prefers-color-scheme. So it's not a big deal that it will be a long time before we can make it work.
(In reply to Carlos Garcia Campos from comment #23) > #if HAVE(OS_DARK_MODE_SUPPORT) > -#if PLATFORM(COCOA) > - static const auto cssValueControlBackground = > CSSValueAppleSystemControlBackground; > -#else > - static const auto cssValueControlBackground = CSSValueWindow; > -#endif > - Color baseBackgroundColor = > backgroundColor.valueOr(RenderTheme::singleton(). > systemColor(cssValueControlBackground, styleColorOptions())); > + Color baseBackgroundColor = > backgroundColor.valueOr(RenderTheme::singleton(). > systemColor(CSSValueAppleSystemControlBackground, styleColorOptions())); > #else > Color baseBackgroundColor = backgroundColor.valueOr(Color::white); > #endif > > This reverts changes made in r244635, so now we always use white. Please, > check OS_DARK_MODE_SUPPORT is not defined in your build. Just to confirm, you are testing Cassidy's example https://cassidyjames.com/dark-demo/ with Adwaita Dark and you and you are seeing a white background on that page? Testing again with a clean build, it's definitely dark here. It doesn't make any sense why we would see different behavior in a basic test like this. And it doesn't matter whether my build defines OS_DARK_MODE_SUPPORT because after your change, that is only ever used in *.mm files.
I decided not to report a new bug for the color-schemes property. We can deal with it later if desired, or not. (In reply to Michael Catanzaro from comment #28) > And it doesn't matter whether my build defines OS_DARK_MODE_SUPPORT because > after your change, that is only ever used in *.mm files. So I write, immediately after quoting a code example that uses it in FrameView.cpp. I'm hopeless. I was grepping under Source/WebKit instead of toplevel Source/. Whoops. I did a rebuild using #error to be 100% certain it's not somehow defined. It's not. Let's continue in bug #206953.
(In reply to Michael Catanzaro from comment #29) > Let's continue in bug #206953. It was caused by Ephy calling webkit_web_view_set_background_color(). :) Seems we are done here. Carlos, you fixed a huge number of longstanding bugs all at once. Awesome!
BTW, at the risk of leaving seven Bugzilla comments in a row, we should have retitled the bug before landing. We landed this as: [GTK] Should use light theme unless website declares support for dark themes in color-schemes property But, in fact, we do not support color-schemes at all. Oh well.
(In reply to Michael Catanzaro from comment #29) > I decided not to report a new bug for the color-schemes property. We can > deal with it later if desired, or not. Bug #208204