Bug 150550 - [GTK] Everything broken in GTK+ 3.19
Summary: [GTK] Everything broken in GTK+ 3.19
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKitGTK (show other bugs)
Version: Other
Hardware: PC Linux
: P2 Blocker
Assignee: Michael Catanzaro
URL:
Keywords:
Depends on: 151520 152508
Blocks: 152273 152271
  Show dependency treegraph
 
Reported: 2015-10-25 21:04 PDT by Michael Catanzaro
Modified: 2015-12-22 15:15 PST (History)
8 users (show)

See Also:


Attachments
Screenshot showing most everything broken (82.68 KB, image/png)
2015-11-20 06:57 PST, Michael Catanzaro
no flags Details
Patch (29.69 KB, patch)
2015-12-20 10:36 PST, Michael Catanzaro
no flags Details | Formatted Diff | Diff
Patch (35.60 KB, patch)
2015-12-20 13:49 PST, Michael Catanzaro
no flags Details | Formatted Diff | Diff
Patch (36.53 KB, patch)
2015-12-21 18:27 PST, Michael Catanzaro
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Michael Catanzaro 2015-10-25 21:04:58 PDT
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 <mclasen@redhat.com>
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?).
Comment 1 Michael Catanzaro 2015-10-25 22:40:17 PDT
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.
Comment 2 Michael Catanzaro 2015-10-25 22:51:57 PDT
I guess the invisible selected text is a separate bug.
Comment 3 Michael Catanzaro 2015-10-26 14:07:16 PDT
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.
Comment 4 Michael Catanzaro 2015-10-26 18:55:47 PDT
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.)
Comment 5 Michael Catanzaro 2015-10-26 19:32:27 PDT
(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.
Comment 6 Michael Catanzaro 2015-11-20 06:55:44 PST
We can no longer render buttons or scrollbars.
Comment 7 Michael Catanzaro 2015-11-20 06:57:19 PST
Created attachment 265953 [details]
Screenshot showing most everything broken
Comment 8 Benjamin Otte 2015-11-20 07:03:16 PST
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.
Comment 9 Michael Catanzaro 2015-11-20 17:03:56 PST
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.
Comment 10 Michael Catanzaro 2015-11-21 08:49:55 PST
(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....
Comment 11 Michael Catanzaro 2015-12-20 10:36:59 PST
Created attachment 267721 [details]
Patch
Comment 12 Michael Catanzaro 2015-12-20 10:39:55 PST
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....
Comment 13 Michael Catanzaro 2015-12-20 10:42:44 PST
(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.
Comment 14 Darin Adler 2015-12-20 13:07:33 PST
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.
Comment 15 Michael Catanzaro 2015-12-20 13:44:23 PST
(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+.
Comment 16 Michael Catanzaro 2015-12-20 13:49:45 PST
Created attachment 267726 [details]
Patch
Comment 17 Michael Catanzaro 2015-12-20 16:30:59 PST
Comment on attachment 267726 [details]
Patch

It broke scrollbars and progress bars with GTK+ 3.18....
Comment 18 Michael Catanzaro 2015-12-20 16:39:55 PST
(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....
Comment 19 Michael Catanzaro 2015-12-21 07:55:24 PST
(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.
Comment 20 Michael Catanzaro 2015-12-21 18:27:45 PST
Created attachment 267775 [details]
Patch
Comment 21 Michael Catanzaro 2015-12-21 18:38:40 PST
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.
Comment 22 Carlos Garcia Campos 2015-12-22 01:36:14 PST
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?
Comment 23 Michael Catanzaro 2015-12-22 05:59:14 PST
(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.
Comment 24 WebKit Commit Bot 2015-12-22 06:22:29 PST
Comment on attachment 267775 [details]
Patch

Clearing flags on attachment: 267775

Committed r194362: <http://trac.webkit.org/changeset/194362>
Comment 25 WebKit Commit Bot 2015-12-22 06:22:33 PST
All reviewed patches have been landed.  Closing bug.
Comment 26 WebKit Commit Bot 2015-12-22 12:34:48 PST
Re-opened since this is blocked by bug 152508
Comment 27 Michael Catanzaro 2015-12-22 14:24:17 PST
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);
}
Comment 28 Michael Catanzaro 2015-12-22 15:13:00 PST
Martin noticed I forgot to #include "GRefPtrGtk.h". I suppose it was being pulled in somehow only in debug builds.
Comment 29 Michael Catanzaro 2015-12-22 15:15:53 PST
Committed r194377: <http://trac.webkit.org/changeset/194377>