Bug 150550 – [GTK] Everything broken in GTK+ 3.19

• Component: WebKitGTK
• Status: RESOLVED FIXED

[Michael Catanzaro]

Text entries (e.g. on this Bugzilla) aren't drawn anymore, since this GTK+ commit:

c6a7ceedc988f84ca6acf9d0dffa12e8490281fb is the first bad commit
commit c6a7ceedc988f84ca6acf9d0dffa12e8490281fb
Author: Matthias Clasen <[e-mail]>
Date:   Fri Oct 23 14:52:27 2015 -0400

    Adwaita: Update entry styling
    
    Use the new element name instead of the style class.
    There is some minor fallout for vertical spin buttons that
    will need a second look.

:040000 040000 6ec7ab073c88946e96647a5b90dbc38b3cab5d14 b13d9ebcffe6d514ef53cb3cca419ca5d7fd3951 M	gtk


Also, selected text (both in and outside text entries) is invisible (might be caused by a different upstream commit?).

[Michael Catanzaro]

OK, so seems GTK+ is removing a ton of style classes from widgets. 408920d438af56d8d83e6a78065608723057c325 begins the migration to CSS names instead; it just so happens that GtkEntry is the first one to be problematic for us. (The style classes are not actually removed from the API; they are just not set by the widgets anymore, nor used in Adwaita.)

I am not completely sure how to handle this, but looking at our now-broken code in RenderThemeGtk.cpp, I guess we have to stop passing GtkStyleContexts around and use GtkCssNodes instead. I expect this is going to be painful for us.

[Michael Catanzaro]

I guess the invisible selected text is a separate bug.

[Michael Catanzaro]

Company is planning to add some function that we can use to set a CSS name on a GtkStyleContext (or a GtkWidgetPath?).  GtkCssNode isn't public and hopefully we won't have to use it.

[Michael Catanzaro]

Company added gtk_widget_path_iter_set_object_name, but I'm having no luck with it.

I was hoping something like this would be all that's needed:

--- a/Source/WebCore/rendering/RenderThemeGtk.cpp
+++ b/Source/WebCore/rendering/RenderThemeGtk.cpp
@@ -170,7 +170,7 @@ static GtkStyleContext* getStyleContext(GType widgetType)
     gtk_widget_path_append_type(path, widgetType);
 
     if (widgetType == GTK_TYPE_ENTRY)
-        gtk_widget_path_iter_add_class(path, 0, GTK_STYLE_CLASS_ENTRY);
+        gtk_widget_path_iter_set_object_name(path, 0, "entry");
     else if (widgetType == GTK_TYPE_ARROW)
         gtk_widget_path_iter_add_class(path, 0, "arrow");
     else if (widgetType == GTK_TYPE_BUTTON) {

But that doesn't make any difference in rendering, unfortunately.

(There are a couple more uses of GTK_STYLE_CLASS_ENTRY in that file, but the first one just adds the style class to a GtkStyleContext already guaranteed to have it, and the next one just removes it from one guaranteed to not have it, so they were both already useless.)

[Michael Catanzaro]

(In reply to comment #2)
> I guess the invisible selected text is a separate bug.

Guessed wrong:

mcatanzaro:  mclasen: Also, any clue why selected text in WebKit is now invisible with latest GTK+? I don't spot any suspicious theme changes related to that, and I'll have to bisect it unless you have any guesses.
Company:  because you look at entries
Company:  and if your entries don't work when rendering entries, they don't work when rendering colors either
mclasennot sure he can parse that
Company:  the background color for selections in webkit is the selected entry background color
Company:  and webkit fails to match entries

(In reply to comment #4)
> Company added gtk_widget_path_iter_set_object_name, but I'm having no luck
> with it.

It works fine when I use it in Epiphany, though.

[Michael Catanzaro]

We can no longer render buttons or scrollbars.

[Michael Catanzaro]

Created attachment 265953 [details]
Screenshot showing most everything broken

[Benjamin Otte]

https://wiki.gnome.org/Projects/GTK%2B/StyleClasses lists the changes we made.

https://blogs.gnome.org/mclasen/2015/11/20/a-gtk-update/ for the motivations behind these changes.

[Michael Catanzaro]

I have text entries fixed locally, but I had to rip out the GtkStyleContext cache we have in RenderThemeGtk.cpp to make it work, see GNOME #758442.

[Michael Catanzaro]

(In reply to comment #9)
> I have text entries fixed locally, but I had to rip out the GtkStyleContext
> cache we have in RenderThemeGtk.cpp to make it work, see GNOME #758442.

OK, Benjamin fixed this bug (thanks), so now we should be good to go....

[Michael Catanzaro]

Created attachment 267721 [details]
Patch

[Michael Catanzaro]

OK, I fixed *almost* everything. Reviews greatly appreciated.

I am stumped by three remaining bugs; help on these would be appreciated:

1) We're rendering scrollbar steppers because the scrollbar style context now has set has-backward-stepper and has-forward-stepper. That's really weird, since Adwaita turns these off. (I guess we can pretend this is a feature, though.)

2) I can't figure out how to render the trough of the GtkProgressBar. Seems like it should be simple. I don't know why my code for this does not work. I did fix rendering of the progress, but that's not very useful if you can't see how wide the progress bar is. (Also, our progress bar is much wider than Adwaita's, but that's a preexisting issue.)

3) The slider in GtkScale is now too wide, which causes the thumb to stretch into an ugly oval shape. Probably we are miscomputing margins or something.

Since almost everything is broken without this patch, we should commit it despite those issues unless someone finds immediate fixes.

Question: Anyone know an example of a combo box separator? I didn't test that, since I'm frankly not sure what that is....

[Michael Catanzaro]

(In reply to comment #12)
> I did fix rendering of the progress, but that's not very useful if you can't
> see how wide the progress bar is. (Also, our progress bar is much wider than
> Adwaita's, but that's a preexisting issue.)

Hm, I used "width" to refer to two different dimensions. We can't see how long horizontally the progress bar is without the trough. Our progress bars are much taller vertically than Adwaita's, since Adwaita switched to drawing skinny little progress bars.

[Darin Adler]

Comment on attachment 267721 [details]
Patch

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

> Source/WebCore/platform/gtk/ScrollbarThemeGtk.cpp:173
> +    GRefPtr<GtkStyleContext> context = gtk_style_context_new();

Looks like this introduces a storage leak.

[Michael Catanzaro]

(In reply to comment #14)
> Looks like this introduces a storage leak.

Yup, thanks! You're awesome!

I've fixed the leak, and added a bunch of extra #ifdefs to avoid adding style classes that aren't used anymore... to make it easier to remove obsolete code whenever we next bump our GTK+ dependency (hopefully not before GTK+ 4) and to stay as close as possible to the style classes used by GTK+.

[Michael Catanzaro]

Created attachment 267726 [details]
Patch

[Michael Catanzaro]

Comment on attachment 267726 [details]
Patch

It broke scrollbars and progress bars with GTK+ 3.18....

[Michael Catanzaro]

(In reply to comment #12)
> Question: Anyone know an example of a combo box separator? I didn't test
> that, since I'm frankly not sure what that is....

Seems that context menu separators are broken....

[Michael Catanzaro]

(In reply to comment #12)
> 2) I can't figure out how to render the trough of the GtkProgressBar. Seems
> like it should be simple. I don't know why my code for this does not work. I
> did fix rendering of the progress, but that's not very useful if you can't
> see how wide the progress bar is. (Also, our progress bar is much wider than
> Adwaita's, but that's a preexisting issue.)

Fixed locally.

> 3) The slider in GtkScale is now too wide, which causes the thumb to stretch
> into an ugly oval shape. Probably we are miscomputing margins or something.

Ben and Lapo say it's broken in GTK+; probably nothing we can do at this point.

[Michael Catanzaro]

Created attachment 267775 [details]
Patch

[Michael Catanzaro]

Darin, I changed enough of the code that I have set r? again rather than put your name as reviewer on code you didn't see, but it's pretty similar to what I had before.

(In reply to comment #17)
> It broke scrollbars and progress bars with GTK+ 3.18....

Fixed.

(In reply to comment #18)
> Seems that context menu separators are broken....

This is the only thing still (I'm aware of) still broken. It's not a regression of this patch, and I'm not going to fix it in this patch.

[Carlos Garcia Campos]

Comment on attachment 267775 [details]
Patch

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

> Source/WebCore/rendering/RenderThemeGtk.cpp:164
> -        gtk_widget_path_iter_add_class(path, 0, "arrow");
> +        gtk_widget_path_iter_set_object_name(path.get(), 0, "button"); // Note: not a typo.

It's good to know this is not a typo, but wy do we use button name for arrows? is that a workaround for a bug in GTK+? Are all the object names documented somewhere? Doesn't GTK+ provide #defines for those?

[Michael Catanzaro]

(In reply to comment #22) 
> It's good to know this is not a typo, but wy do we use button name for
> arrows?

Because GtkArrow has been deprecated for ages, it doesn't have its own CSS name. Whether by design or luck, if we use the name "button" and call gtk_render_arrow, we get an arrow. Maybe it would work for any node name, but the original code only ever draws arrows with the "button" style context, so I stuck with "button."

> is that a workaround for a bug in GTK+?

No.

> Are all the object names
> documented somewhere? Doesn't GTK+ provide #defines for those?

They're documented in the GTK+ documentation, e.g. https://developer.gnome.org/gtk3/unstable/GtkMenu.html shows you can pick from the following subnodes:

menu
├── arrow.top
├── ...
╰── arrow.bottom

Since a GtkButton has no subnodes itself, there's no such node diagram for it, but there's still some discussion of valid style contexts.

There are no #defines. I guess for the same reason #defines are no longer provided for new style classes -- providing API makes people think the style classes are API, but they can be changed or removed at any time. I really hope that CSS nodes will be stable, but frankly I'm not sure what is planned.

P.S. Expect more pain this cycle; they've started removing the style properties we rely on for margins and displacements.

[WebKit Commit Bot]

Comment on attachment 267775 [details]
Patch

Clearing flags on attachment: 267775

Committed r194362: <http://trac.webkit.org/changeset/194362>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[WebKit Commit Bot]

Re-opened since this is blocked by bug 152508

[Michael Catanzaro]

The problem is that in release builds, ~GRefPtr<_GtkWidgetPath> gets generated using the default template:

// GRefPtr.cpp
template <typename T> inline void derefGPtr(T* ptr)
{
    if (ptr)
        g_object_unref(ptr);
}

Which is a crash because GtkWidgetPath is not a GObject.

In debug builds, there is no crash, so it must use the template that I intended:

// GRefPtrGtk.cpp
template <> void derefGPtr(GtkWidgetPath* ptr)
{
    if (ptr)
        gtk_widget_path_unref(ptr);
}

[Michael Catanzaro]

Martin noticed I forgot to #include "GRefPtrGtk.h". I suppose it was being pulled in somehow only in debug builds.

[Michael Catanzaro]

Committed r194377: <http://trac.webkit.org/changeset/194377>