[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]

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

--- Comment #22 from Carlos Garcia Campos <cgarcia at igalia.com> ---
(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.

-- 
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/20200129/5e61344e/attachment.htm>