[Webkit-unassigned] [Bug 126907] [GTK] Hardcoded text color in input fields

[bugzilla-daemon at webkit.org]

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

--- Comment #44 from Carlos Eduardo Ramalho <cadubentzen at gmail.com> ---
(In reply to Michael Catanzaro from comment #40)

Thanks for the review!

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

Hmm really? Ok. I started out trying that and things didn't work so I went the other way. I must've done something wrong and will check it then.

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

I was doing it for all gadgets but some weird background colors were showing up in other gadgets like buttons so I opted to place it only for input fields. As I put it specifically for input fields, that's why I called it a workaround.

However, as the whole approach by setting additional classes might be wrong, then maybe if I use the widget path approach correctly it will work when setting it for all gadgets.

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

It seems good :) I'll do so following Benjamin Otte correction and see what I get!

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

Indeed it is copied from another function in the file! I thought if it was like that in there then it was the convention followed. I'll fix that.

(In reply to Michael Catanzaro from comment #43)
> (In reply to Benjamin Otte from comment #42)
> > But you absolutely want to create real RenderThemeGadgets for those two
> > elements. Otherwise the color inheritance is not going to work because you
> > have no parent GtkStyleContext to inherit from.
> 
> OK, that sounds more complicated... Carlos Eduardo, do you want to
> investigate that? I don't know how hard it would be.

I'll look into that.

-- 
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/20180523/660f8cea/attachment.html>