[Webkit-unassigned] [Bug 197947] [GTK] Should use light theme unless website declares support for dark themes in color-schemes property

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Tue Jan 28 09:27:39 PST 2020


https://bugs.webkit.org/show_bug.cgi?id=197947

--- Comment #20 from Michael Catanzaro <mcatanzaro at gnome.org> ---
Comment on attachment 389004
  --> https://bugs.webkit.org/attachment.cgi?id=389004
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.

-- 
You are receiving this mail because:
You are the assignee for the bug.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.webkit.org/pipermail/webkit-unassigned/attachments/20200128/7bdcdc07/attachment.htm>


More information about the webkit-unassigned mailing list