[webkit-reviews] review denied: [Bug 126907] [GTK] Hardcoded text color in input fields : [Attachment 341088] Patch

[bugzilla-daemon at webkit.org]

Michael Catanzaro <mcatanzaro at igalia.com> has denied Carlos Eduardo Ramalho
<cadubentzen at gmail.com>'s request for review:
Bug 126907: [GTK] Hardcoded text color in input fields
https://bugs.webkit.org/show_bug.cgi?id=126907

Attachment 341088: Patch

https://bugs.webkit.org/attachment.cgi?id=341088&action=review




--- Comment #40 from Michael Catanzaro <mcatanzaro at igalia.com> ---
Comment on attachment 341088
  --> https://bugs.webkit.org/attachment.cgi?id=341088
Patch

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

I think Carlos Garcia should review the RenderThemeGtk changes, but I can
review the RenderThemeGadget bits. You're on the right track, but this isn't
quite what I had in mind.

> Source/WebCore/platform/gtk/RenderThemeGadget.cpp:64
> +    // Class to be used by GTK themers if they want custom styling inside
webviews.
> +    gtk_style_context_add_class(context.get(), "webview");

Hm, style classes have fallen out of favor, so let's not add a new one now. It
would be better to modify the widget path instead.

> Source/WebCore/platform/gtk/RenderThemeGadget.cpp:245
> +    // This workaround is for some themes like HighConstrast lacking "color"
gtk-css attribute in entries.
> +    // When the widgets are drawn by GTK they go to this fallback theme.
However here they were getting the default color.
> +    // As a result, for example, HighConstrast theme was drawing white text
in white background in text inputs.
> +    if (m_context)
> +	   gtk_style_context_add_class(m_context.get(), "background");

It's not a workaround at all! It's just the right thing to do. We're trying to
put together a widget path that mimics a real GTK+ app.

This isn't the right place, though, because the new widget path should be used
for all gadgets, not just text fields. It could fix more than just text color
in input fields.

Also, the goal is to adjust the widget path, not add a style class. I'm kinda
surprised this worked, actually.

Look at my comment #32. """So the place to do that would be in the
RenderThemeGadget constructor. There are two places we currently use
gtk_widget_path_new(). Those would need to instead use a default base widget
path. "window.background" is what Company thinks would fix the text color. And
then, optionally, "webview" to allow theme developers to provide custom styling
for WebKit...."""

See here:

    GRefPtr<GtkWidgetPath> path = parent ?
adoptGRef(gtk_widget_path_copy(gtk_style_context_get_path(parent->context())))
: adoptGRef(gtk_widget_path_new());
    if (!siblings.isEmpty()) {
	GRefPtr<GtkWidgetPath> siblingsPath = adoptGRef(gtk_widget_path_new());

Instead of passing gtk_widget_path_new(), we want to create a custom widget
path to pass there instead. So let's add a createWidgetPath() function to use
there instead. Should look something like:

static GRefPtr<GtkWidgetPath> createWidgetPath()
{
    GRefPtr<GtkWidgetPath> path = adoptGRef(gtk_widget_path_new());

    gtk_widget_path_append_type(path, GTK_TYPE_WINDOW);
    gtk_widget_path_iter_add_class (path, -1, GTK_STYLE_CLASS_BACKGROUND);

    gtk_widget_path_append_type(path, WEBKIT_TYPE_WEB_VIEW);
    gtk_widget_path_iter_set_object_name(path, -1, "webview");

    return path;
}

Then we should have a sane widget path for everything that we draw. I copied
the first part of that from
https://gitlab.gnome.org/GNOME/gtk/blob/gtk-3-24/testsuite/gtk/firefox-stylecon
text.c, which seems sufficiently-authoritative to me. Now, will it work? That
is the question....

> Source/WebCore/rendering/RenderThemeGtk.cpp:502
> +    switch (part) {
> +    default:
> +	   ASSERT_NOT_REACHED();

default at the top is really weird, is that copied from elsewhere in this file?
It's idiomatic to put the default case at the bottom of the switch.