Gtk 3.20 brings new style model based on CSS Nodes (https://blogs.gnome.org/mclasen/2015/11/20/a-gtk-update/) Recent FF fails to draw scrollbars/text/combo/radio boxes. There's also a bug in cursor switch.

Comment on attachment 8700553 [details] [diff] [review] wip patch Karl, what do you think about this approach? It replaces single widget with structure with widget + styles (not sure we need the widgets any more when we keep styles). moz_gtk_style_create() extracts the style nodes and ensure_*_widget() is paired with remove_*_widget(). It also updates style state(s) but that's just a minor issue here and needs to be addressed further.

(In reply to Martin Stránský from comment #0) > Created attachment 8700553 [details] [diff] [review] > wip patch > There is an issue here. Although this works flawlessly under current Fedora rawhide, it does not work under Fedora 23 becasue of this error: libxul.so: undefined symbol: gtk_widget_path_iter_set_object_name

(In reply to Bill Gianopoulos [:WG9s] from comment #3) > (In reply to Martin Stránský from comment #0) > > Created attachment 8700553 [details] [diff] [review] > > wip patch > > > > There is an issue here. Although this works flawlessly under current Fedora > rawhide, it does not work under Fedora 23 becasue of this error: > > libxul.so: undefined symbol: gtk_widget_path_iter_set_object_name Just to make it clear this would require more than a compile time check. In fact the version that I tested that solved the issue under fedora rawhide was compiled under fedora 23. The problem is that Mozilla has this requirement that one build run form the current version of Linux back to the Centos 6 release distribution. We either need a way to duplicate what this missing API does in Mozilla code or somehow have e pre and post 3.20 path that can be taken at object time. I suppose we could just build the 32 bit version with gtk2 and the 64 bit version with gtke, but that is really not ideal and I am sure no one whans to build 4 versions for linux.

(In reply to Bill Gianopoulos [:WG9s] from comment #4) > version with gtke, but that is really not ideal and I am sure no one whans ^^^^ gtk3

> I suppose we could just build the 32 bit version with gtk2 and the 64 bit > version with gtk3, How does that help matters any? The 64 bit version will *still* encounter issues depending on what version of gtk3 the system has, and the 32 bit version will be stuck with gtk2 (does the build even support using gtk2 rather than 3 anymore)?

(In reply to Valdis Kletnieks from comment #6) > > I suppose we could just build the 32 bit version with gtk2 and the 64 bit > > version with gtk3, > > How does that help matters any? The 64 bit version will *still* encounter > issues depending on > what version of gtk3 the system has, and the 32 bit version will be stuck > with gtk2 (does the > build even support using gtk2 rather than 3 anymore)? well if mozilla decided to continue to support gtk2 for 32-bit and require gtk3 for 64-bit support it could, but if you actually read what I said before this was not anything I recommended. So what is your point?

(In reply to Bill Gianopoulos [:WG9s] from comment #7) > well if mozilla decided to continue to support gtk2 for 32-bit and require > gtk3 for 64-bit support it could, but if you actually read what I said > before this was not anything I recommended. So what is your point? Oh, I misread it - if what you intended was "at least provide a semi-functional gtk2 for the people still stuck on 32bit", you're right. It sounded like you thought the 32/64 bit distinction had some significance, such that gtk2/3 behaved differently on the two.... Note to self: defer reading bug reports until after caffeine. :)

(In reply to Bill Gianopoulos [:WG9s] from comment #3) > (In reply to Martin Stránský from comment #0) > > Created attachment 8700553 [details] [diff] [review] > > wip patch > > > > There is an issue here. Although this works flawlessly under current Fedora > rawhide, it does not work under Fedora 23 becasue of this error: > > libxul.so: undefined symbol: gtk_widget_path_iter_set_object_name That's because this patch is just a concept how to approach to the CSS Node change and it's for Gtk 3.20 only. It does not work with GTK < 3.20.

(In reply to Martin Stránský from comment #9) > (In reply to Bill Gianopoulos [:WG9s] from comment #3) > > (In reply to Martin Stránský from comment #0) > > > Created attachment 8700553 [details] [diff] [review] > > > wip patch > > > > > > > There is an issue here. Although this works flawlessly under current Fedora > > rawhide, it does not work under Fedora 23 becasue of this error: > > > > libxul.so: undefined symbol: gtk_widget_path_iter_set_object_name > > That's because this patch is just a concept how to approach to the CSS Node > change and it's for Gtk 3.20 only. It does not work with GTK < 3.20. OK as long as there is a non 3.20 way to do this as well as a concept it seems to work great!

(In reply to Martin Stránský from comment #1) > Comment on attachment 8700553 [details] [diff] [review] > wip patch With this patch applied, it is no longer possible to build using the cairo-gtk2 toolkit. Altering this area of the patch as shown, corrects this issue. @@ -459,13 +467,21 @@ gboolean moz_gtk_images_in_menus(void); gboolean moz_gtk_images_in_buttons(void); /** * Get a boolean which indicates whether the theme draws scrollbar buttons. * If TRUE, draw scrollbar buttons. */ gboolean moz_gtk_has_scrollbar_buttons(void); +#if (MOZ_WIDGET_GTK == 3) +/** + * Create style from parrent and Gtk CSS node data. + */ +GtkStyleContext * +moz_gtk_style_create(GtkCssNode *node, GtkStyleContext *parent); +#endif + #ifdef __cplusplus } #endif /* __cplusplus */

The attached patch doesn't apply to latest source from trunk.

I could try my patch it at least applied lat time i tried. the issue is that with this patch and latest gtk3 the bookmarks toolbar does not display correctly so i stopped doing builds with this included.

(In reply to Bill Gianopoulos [:WG9s] from comment #14) > Created attachment 8725532 [details] [diff] [review] > Same patch updated to tip OH and it also includes fix for gtk2 builds from comment 11.

(In reply to Bill Gianopoulos [:WG9s] from comment #15) > (In reply to Bill Gianopoulos [:WG9s] from comment #14) > > Created attachment 8725532 [details] [diff] [review] > > Same patch updated to tip > > OH and it also includes fix for gtk2 builds from comment 11. OK that patch applies fine and still fixes the gtk2 build issue but still leaves the bookmarks toolbar missing padding issue.

(In reply to Bill Gianopoulos [:WG9s] from comment #16) > (In reply to Bill Gianopoulos [:WG9s] from comment #15) > > (In reply to Bill Gianopoulos [:WG9s] from comment #14) > > > Created attachment 8725532 [details] [diff] [review] > > > Same patch updated to tip > > > > OH and it also includes fix for gtk2 builds from comment 11. > > OK that patch applies fine and still fixes the gtk2 build issue but still > leaves the bookmarks toolbar missing padding issue. And now the missing padding issue has been corrected in gtk3. SO this patch is good to go, in my opinion.

I should have also mentioned, I no longer see the issue I reported in comment #3.

It is easy enough to weakly depend on gtk_widget_class_set_css_name and gtk_widget_path_iter_set_object_name to avoid a hard dependency on gtk+ >= 3.19.1 Like https://git.launchpad.net/plank/tree/lib/gtk-compat.c

Comment on attachment 8700553 [details] [diff] [review] wip patch updated one is comming.

Hi Karl, I have a proposal for this one which involves a slight widget management rework. I believe we may need something like a widget cache which may be used by gtk3drawing, LookAndFeel and NativeTheme. It means to replace single ensure_*_widget() by get_gtk_widget(GtkThemeWidgetType widget) which may be used in the modules. It will unify the widget creation and setup across the modules and ensure we have the same widgets to get size, colors, render background and so on. It also allows to get different widget and styles for light/dark themes. The stored widget information may include created widget itself and widget child styles which will be compatible with old and new widget styles. Something like: typedef struct { GtkWidget* widget; union { struct { GtkStyleContext* style; GtkStyleContext* styleSelection; } entry; struct { GtkStyleContext* style; } button; struct { GtkStyleContext* style; GtkStyleContext* styleTrough; GtkStyleContext* styleSlider; } scroll; .... struct { GtkStyleContext* style[MOZ_WIDGET_STYLES]; } all; }; } MozGtkWidget; I also wonder if we can move gtk3drawing.c to a c++ file so the widget cache can be a c++ code. Thoughts?

(In reply to Martin Stránský from comment #21) > I have a proposal for this one which involves a slight widget management > rework. I believe we may need something like a widget cache which may be > used by gtk3drawing, LookAndFeel and NativeTheme. Sharing GtkWidgets between gtk3drawing and LookAndFeel sounds great, thanks. > It means to replace single ensure_*_widget() by > get_gtk_widget(GtkThemeWidgetType widget) which may be used in the modules. > It will unify the widget creation and setup across the modules and ensure we > have the same widgets to get size, colors, render background and so on. It > also allows to get different widget and styles for light/dark themes. Sounds good. The only exception may be if only nsLookAndFeel needs a widget to get something little like a color once and can then release the widget. > The stored widget information may include created widget itself and widget > child styles which will be compatible with old and new widget styles. > Something like: > > typedef struct { > GtkWidget* widget; > union { > struct { > GtkStyleContext* style; > GtkStyleContext* styleSelection; > } entry; > > struct { > GtkStyleContext* style; > } button; > > struct { > GtkStyleContext* style; > GtkStyleContext* styleTrough; > GtkStyleContext* styleSlider; > } scroll; > > .... > > struct { > GtkStyleContext* style[MOZ_WIDGET_STYLES]; > } all; > }; > } MozGtkWidget; I don't yet understand the need for this. Can you give me an overview please of what has changed in the style contexts in libgtk? I've seen https://blogs.gnome.org/mclasen/2015/11/20/a-gtk-update/ but that doesn't seem to have the details of what exactly has changed in how they are constructed. How has libgtk maintained backward compat with existing themes? Why do we need both widget and style? Is there a simple example in the libgtk code that would help me understand, perhaps? What in the libgtk code corresponds to the foreign drawing demo? Does libgtk keep copies of all the different style contexts? Where does GtkRange keep its contexts for trough and slider, for example? Given each widget seems to have its own struct, I wonder about having a separate function and named return value class for each widget we need, instead of a union. Or maybe a single function for getting any style context and we use a different or extended enum to select the right one? But I need to understand more. What problems are being solved by caching the style contexts? Is it so that nsLookAndFeel and gtk3drawing share the same code to set the style selectors? That's sounds sensible if there are a number of such situations. But I'm guessing only one context is required at a time, and so wonder about a function returning one style context at a time. In the past we've had trouble when creating style contexts from paths, and so instead used gtk_widget_get_style_context(), because this behaves differently from the style contexts created from paths. Can we keep doing that? If not, do you know what the problem was about, and if and when that has been fixed? > I also wonder if we can move gtk3drawing.c to a c++ file so the widget cache > can be a c++ code. Yes, please. Moving this to C++ would be great, thanks.

> > The stored widget information may include created widget itself and widget > > child styles which will be compatible with old and new widget styles. > > Something like: > > > > typedef struct { > > GtkWidget* widget; > > union { > > struct { > > GtkStyleContext* style; > > GtkStyleContext* styleSelection; > > } entry; > > > > struct { > > GtkStyleContext* style; > > } button; > > > > struct { > > GtkStyleContext* style; > > GtkStyleContext* styleTrough; > > GtkStyleContext* styleSlider; > > } scroll; > > > > .... > > > > struct { > > GtkStyleContext* style[MOZ_WIDGET_STYLES]; > > } all; > > }; > > } MozGtkWidget; I'll try to answer but Matthias promised to give a feedback here so he will correct me if I'm wrong. > I don't yet understand the need for this. > > Can you give me an overview please of what has changed in the style contexts > in libgtk? > I've seen https://blogs.gnome.org/mclasen/2015/11/20/a-gtk-update/ > but that doesn't seem to have the details of what exactly has changed in how > they are constructed. Widgets are divided to pieces which are described by CSS. The pieces (CSS nodes) are hierarchical and are derived from parent ones. See https://developer.gnome.org/gtk3/unstable/GtkScrollbar.html for example. The scrollbar itself has "scrollbar" style and "trough" and "slider" child styles are derived from it. You can see that in ensure_scrollbar_widget() in the WIP patch attached here. > How has libgtk maintained backward compat with existing themes? IMHO no, it's not compatible. Styles derived from widgets no longer works for some widgets, as the widgets are transferred to CSS nodes. My WIP patch uses dlsym() to look up the new gtk_widget_path_iter_set_object_name() symbol to keep compatibility. > Why do we need both widget and style? The widgets can be used for old gtk3 and gtk2 code. Styles can be used for gtk3.20 code. > Is there a simple example in the libgtk code that would help me understand, > perhaps? > What in the libgtk code corresponds to the foreign drawing demo? > > Does libgtk keep copies of all the different style contexts? > Where does GtkRange keep its contexts for trough and slider, for example? > > Given each widget seems to have its own struct, I wonder about having a > separate function and named return value class for each widget we need, > instead of a union. > Or maybe a single function for getting any style context and we use a > different or extended enum to select the right one? > But I need to understand more. > What problems are being solved by caching the style contexts? It solves the problem that you have to construct the final style from the parent styles. From the quick look it seems that LibreOffice does not cache the styles but creates them before rendering every time: http://opengrok.libreoffice.org/xref/core/vcl/unx/gtk3/gtk3salnativewidgets-gtk.cxx There's also a WebKit code for it: http://trac.webkit.org/browser/trunk/Source/WebCore/rendering/RenderThemeGtk.cpp It's really a question what needs to be cached and how heavily are the widgets used. > Is it so that nsLookAndFeel and gtk3drawing share the same code to set the > style selectors? That's sounds sensible if there are a number of such > situations. > But I'm guessing only one context is required at a time, and so wonder about > a function returning one style context at a time. > > In the past we've had trouble when creating style contexts from paths, and > so instead used gtk_widget_get_style_context(), because this behaves > differently > from the style contexts created from paths. Can we keep doing that? If > not, do you know what the problem was about, and if and when that has been > fixed? Styles derived from widgets no longer works in gtk3.20 and the only way is to create the style tree. There's a complete new approach and style/widget rewrite in gtk3.20.

There's additional feedback from Matthias here: > I don't yet understand the need for this. > > Can you give me an overview please of what has changed in the style > contexts in libgtk? > I've seen https://blogs.gnome.org/mclasen/2015/11/20/a-gtk-update/ > but that doesn't seem to have the details of what exactly has changed > in how they are constructed. GTK+ internally has moved from style contexts as the primary object holding style information to css nodes. Every widget has one or more of them, and they form a tree in the expected way. Each node has a name that is used in CSS matching. E.g. scrollbar trough will match the css node that GtkScrollbar uses for the trough (which is a subnode (with name "trough") of the its main css node (with name "scrollbar"). Now, we don't expose CSS nodes as public api (yet, at least). So, for foreign drawing purposes, you can't directly create a tree of nodes. The workaround that we've come up for that is to create multiple style contexts (since creating a style context also creates a backing css node), and that is the construction you see in foreigndrawing.c. > How has libgtk maintained backward compat with existing themes? In short, it hasn't. Themes need to be rewritten to update all their CSS selectors for the new names. On the positive side, the names and the node tree structure are now documented for all widgets, so theme authors have some actual information to work with. > Why do we need both widget and style? The problem with using just the style context that you get from a widget (e.g. a GtkScrollBar) is that it is backed by the widgets 'main' css node (ie the "scrollbar" node), so you can't use it to draw the trough.

I have more understanding now, thanks, though still filling in some gaps. I suspect much of the direct use of GtkWidget in Gecko is with gtk_widget_style_get(). Could this be changed to use gtk_style_context_get_style()? If that can be done, then I'd guess there are only a few exceptions requiring a GtkWidget, which can be handled through special case functions, or a separate getter for the GtkWidget? The rest can use only style contexts. Where GTK caches each of the style contexts for each part of a widget, doing the same in Gecko sounds fine to me. However, where GTK is building/modifying style contexts on the fly, then it would be nice for Gecko to do the same. The 3.4-3.18 way was to reuse the same main style context from the widget, modified for each part, so it would be nice if the API could support continuing to do that, without making several copies of each widget. Even 3.20 looks like it still uses save/restore sometimes. So I would counter-propose something like enum class WidgetNodeType; typedef unsigned StyleFlags; enum : StyleFlags { NO_STYLE_FLAGS, WHATEVER_MIGHT_BE_NEEDED = 1U << 0, } GtkStyleContext* GetStyleContext(WidgetNodeType aObjectType, GtkTextDirection aDirection, ObjectStyleFlags aFlags); Would that work for GTK3 versions? I expect the GTK2 code doesn't need to (and couldn't) be adjusted to use this method, but I'm keen to unify the GTK3.18/3.20 code if/where that makes sense. There is then the issue of restoring from any style context save that was required to return the appropriate context. "The matching call to gtk_style_context_restore() must be done before GTK returns to the main loop", so I guess we need a balancing release method. Perhaps ClaimStyleContext()/ReleaseStyleContext(). It looks like it may be possible to imitate save/restore through creating a new style context and parenting to the "saved" copy, which might permit caching the equivalent of saved style contexts, and make the ReleaseStyleContext() unnecessary. I'm very uneasy about this though. With CSS nodes having different parents to style contexts, it looks like we could easily get tripped. I'm also not certain that this parent/child model is desirable for save(). I'd at least want some sort of assurance from GTK maintainers that such an approach is suitable and the behaviour not going to change until GTK 4. Having the style context cache get the style context from the widget is still the appropriate approach when it can be done. (i.e. when it is the widget's "main" node.) That way the widget sets up some of the appropriate node names and/or style classes in the appropriate way for the version of libgtk in use. If we were to switch to constructing all paths and style contexts in Gecko, then *everything* will break next time libgtk reorganizes its theming. We were a little fortunate we didn't take this approach before the switch from classes to node names. If libgtk does not provide a way to get style contexts for the sub-main nodes, and it looks like it does not, then, with GTK 3.20, some style contexts (e.g. trough) will need to be created and set up in Gecko. They can be parented to style contexts from the widgets. Watch out that widgets in paths sometimes need siblings.

I suggest implementing the cache first for the scrollbar and/or togglebox nodes. Those are the ones currently rendering incorrectly with 3.20 IIUC, and those would be sufficient to develop an initial implementation. A GtkWidget* GetWidget(WidgetNodeType aNodeType) method on the cache would provide the widgets required for ensure_window_widget() and setup_widget_prototype(), while they still exist.

(In reply to Karl Tomlinson (ni?:karlt) from comment #27) > I suggest implementing the cache first for the scrollbar and/or togglebox > nodes. Those are the ones currently rendering incorrectly with 3.20 IIUC, > and those would be sufficient to develop an initial implementation. What do you expect the WidgetNodeType contains? A combination of widget and the requested node? For example there's MOZ_GTK_SCROLLBAR_THUMB_HORIZONTAL for the thumb which may work but needs to be extended for all other nodes of GtkScrollbar. It also means that the whole hierarchy has to be created when the leaf node is requested. That should be also cached to avoid repeated constructions when a different node from the same widget is requested. > A GtkWidget* GetWidget(WidgetNodeType aNodeType) method on the cache would > provide the widgets required for ensure_window_widget() and > setup_widget_prototype(), while they still exist. The GtkWidget* GetWidget(WidgetNodeType aNodeType) implementation can always return only the base widget - GtkScrollbar for MOZ_GTK_SCROLLBAR_THUMB_HORIZONTAL, because the thumb node does not have any widget itself, only style can be constructed for it. btw. would be better to avoid gtk_style_context_save/restore - it causes bugs because gtk_style_context_save() removes some state from saved context.

(In reply to Martin Stránský from comment #28) > (In reply to Karl Tomlinson (ni?:karlt) from comment #27) > > I suggest implementing the cache first for the scrollbar and/or togglebox > > nodes. Those are the ones currently rendering incorrectly with 3.20 IIUC, > > and those would be sufficient to develop an initial implementation. > > What do you expect the WidgetNodeType contains? A combination of widget and > the requested node? That sounds sensible. Something to uniquely identify the node for each style context that we'll need. > For example there's MOZ_GTK_SCROLLBAR_THUMB_HORIZONTAL > for the thumb which may work but needs to be extended for all other nodes of > GtkScrollbar. Extending that existing enum probably makes sense. I'm not clear exactly how much overlap there will be, but I guess the widget type often maps directly to a single node. > It also means that the whole hierarchy has to be created when the leaf node > is requested. That should be also cached to avoid repeated constructions > when a different node from the same widget is requested. Yes, at least the ancestors need to be constructed, and, in some cases, it probably makes sense to construct siblings at the same time. That's consistent with the existing widget construction, and I don't think there's a way to avoid constructing the associated parts of the hierarchy. > > A GtkWidget* GetWidget(WidgetNodeType aNodeType) method on the cache would > > provide the widgets required for ensure_window_widget() and > > setup_widget_prototype(), while they still exist. > > The GtkWidget* GetWidget(WidgetNodeType aNodeType) implementation can always > return only the base widget - GtkScrollbar for > MOZ_GTK_SCROLLBAR_THUMB_HORIZONTAL, because the thumb node does not have any > widget itself, only style can be constructed for it. I'd prefer that it return null, and probably assert not reached, if the node is not the main node for a widget, because then the widget is not a good representation of the node. I can reconsider if there is a need, but I think this method will only need to be implemented for some special cases of aNodeType. Usually only the style context will be required. No need to implement for all nodes, nor all widgets even. > btw. would be better to avoid gtk_style_context_save/restore - it causes > bugs because gtk_style_context_save() removes some state from saved context. gtk_style_context_save/restore changes the hierarchy [1], and so we should use it when and only when GTK uses it. I suspect many of our save/restore pairs are correct for older GTK versions, but there are likely some that are not right. I'm interested to hear if there are other bugs in save/restore, but the solution is still the same: do what GTK does. [1] https://bugzilla.gnome.org/show_bug.cgi?id=761870#c2

Thanks. There's a WIP of the patch, seems to be roughly working for both old and new GTK versions. It implements the scrollbars only for now. It moves the scrollbar widgets to the cache file but leaves rest in the original one. It may allow partial migration of the widgets. The main MozGtkWidgetStyle class creates basic style from widget (as for < 3.20) and can be overloaded for more complex widgets (like the scrollbars). It also provides style lock/unlock with automatic style save/restore for gtk < 3.20 which ensures that we provide only one active copy of the style, which is needed here. I reordered GtkThemeWidgetType to allow to index style nodes inside MozGtkWidgetStyle. StyleContextCache class contains complete style sets for the widgets. We can have multiple context caches (for light themes for instance) but there's only one right now. It also provides easy way to re-initialize all styles which can be used by gtk_init/shutdown, when system theme is changed and so on. The patch is missing implementation of GtkTextDirection and StyleFlags.

Comment on attachment 8737389 [details] [diff] [review] WIP v.2 I just realized that we need to also save/restore styles for gtk 3.20 because mStyle[0] is always owned by Gtk now. But that can be fixed in next patch iteration.

I tried this patch. There is an issue with the scrollbars. Under adwaita gtk+3.20.2, the scrollbars are not colored blue when selected (only when hovered). The behavior in native gtk3 apps is blue colored selected scrollbars. This also works in the gtk2 build so is a regression.

(In reply to Hussam Al-Tayeb from comment #32) > I tried this patch. There is an issue with the scrollbars. Under adwaita > gtk+3.20.2, the scrollbars are not colored blue when selected (only when > hovered). The behavior in native gtk3 apps is blue colored selected > scrollbars. I see the same with gtk3-3.16.7, so it's not 3.20 specific and should be targeted in a separate bug.

(In reply to Martin Stránský from comment #33) > (In reply to Hussam Al-Tayeb from comment #32) > > I tried this patch. There is an issue with the scrollbars. Under adwaita > > gtk+3.20.2, the scrollbars are not colored blue when selected (only when > > hovered). The behavior in native gtk3 apps is blue colored selected > > scrollbars. > > I see the same with gtk3-3.16.7, so it's not 3.20 specific and should be > targeted in a separate bug. Thanks. I filed a new bug https://bugzilla.mozilla.org/show_bug.cgi?id=1261576

For those playing along at home, I am including the pending patch for this bug in my Linux 64-bit Gtk3 nightly builds available here: http://www.wg9s.com/mozilla/firefox/

The scrollb(In reply to Bill Gianopoulos [:WG9s] from comment #36) > (In reply to Martin Stránský from comment #33) > > (In reply to Hussam Al-Tayeb from comment #32) > > > I tried this patch. There is an issue with the scrollbars. Under adwaita > > > gtk+3.20.2, the scrollbars are not colored blue when selected (only when > > > hovered). The behavior in native gtk3 apps is blue colored selected > > > scrollbars. > > > > I see the same with gtk3-3.16.7, so it's not 3.20 specific and should be > > targeted in a separate bug. > > OK, but a vanilla Firefox gtk3 build without the pending patch in this bug > exhibits correct scrollbar handle coloring under gtk3-3.16.7. Therefore, I > have marked the new bug as a regression caused by this one (even though the > code is still WIP and has not landed anywhere) As I mentioned in the other bug, the scrollbar handle coloring issue is in 3.18 as well without the patch. 3.18 was before the css themeing changes in gtk+

(In reply to Hussam Al-Tayeb from comment #38) > The scrollb(In reply to Bill Gianopoulos [:WG9s] from comment #36) > > (In reply to Martin Stránský from comment #33) > > > (In reply to Hussam Al-Tayeb from comment #32) > > > > I tried this patch. There is an issue with the scrollbars. Under adwaita > > > > gtk+3.20.2, the scrollbars are not colored blue when selected (only when > > > > hovered). The behavior in native gtk3 apps is blue colored selected > > > > scrollbars. > > > > > > I see the same with gtk3-3.16.7, so it's not 3.20 specific and should be > > > targeted in a separate bug. > > > > OK, but a vanilla Firefox gtk3 build without the pending patch in this bug > > exhibits correct scrollbar handle coloring under gtk3-3.16.7. Therefore, I > > have marked the new bug as a regression caused by this one (even though the > > code is still WIP and has not landed anywhere) > > As I mentioned in the other bug, the scrollbar handle coloring issue is in > 3.18 as well without the patch. 3.18 was before the css themeing changes in > gtk+ Sorry, I was wrong. I just tested with the current Firefox beta release, which obviously has no code from this bug included, and the problem exists there also. So, this is not a regression.

(In reply to Martin Stránský from comment #31) > I just realized that we need to also save/restore styles for gtk 3.20 > because mStyle[0] is always owned by Gtk now. But that can be fixed in next > patch iteration. I think we need to modify the widget's style context directly because using style_context_save() will change the hierarchy, which can break CSS rule matching. gtk_button_draw() for example doesn't save the context before drawing. That changed from saving the style context to using it without saving between GTK+-3.4 and GTK+-3.6. I suspect this was probably a core change affecting other widgets too. So even for versions 3.6 to 3.18 save() is no longer appropriate. I don't think we need to save() for 3.4 as a special case even, because the hierarchy change was counter-intuitive, so I doubt many themes depended on it. If that turns out to be a problem, we can adjust where required. gtkrange.c kept the save/restore when drawing what were essentially descendants and have now become css descendant nodes. The additional node in the hierarchy made some sense there and so using save/restore is still appropriate. gtk_css_gadget_draw() doesn't save/restore because the 3.20 css nodes already have different paths. When using the widget's style context without saving, we need to watch out that the style context will be modified during some modifications of the widget (such as set_direction()). As long as the style context state is set after any widget modifications, I think that should work out fine. If the state is modified during one GetStyleContext() it will be set appropriately again on the next call to get the same context.

Comment on attachment 8737389 [details] [diff] [review] WIP v.2 I like the get_window_widget/get_window_container_widget() approach but I prefer extending that approach over using MozGtkWidgetStyle inheritance. I think something simply based around a switch statement with get functions (and ensure functions where it makes sense to initialize several style contexts together) will be more flexible for special cases and easier to read. I think simple functions will let us share code at least as well as inheritance. MozGtkWidgetStyle() made the release of all the cached style contexts simpler, but I have a suggestion below for that. >+ if(gtk_check_version(3, 20, 0)) { >+ if (!StyleLock()) >+ return nullptr; >+ >+ // Return only base style for Gtk < 3.20 and lock it >+ return mStyle[0]; >+ } >+ else { Having some sort of check that the style is always released is good, but I suspect this is finer grained than necessary. I'd like the requirements on the caller not to depend on the GTK version because the intention is to provide a system such that the caller need not care about the GTK version. Can the check be as simple as allowing no more than one style context at a time, recording the context in use if any, and asserting that there is none in use when another is requested? >+bool MozGtkWidgetStyle::StyleLock(void) >+{ >+ if(mLock) { >+ return false; >+ } >+ mLock++; >+ gtk_style_context_save(mStyle[0]); >+ return true; >+} >+void MozGtkWidgetStyle::StyleUnLock(void) >+{ >+ if(mLock) { >+ mLock--; >+ gtk_style_context_restore(mStyle[0]); >+ } Given comment 40, having to save/restore the context will be the exception rather than the norm. Usually ReleaseStyleContext() will just be a no-op. I expect a file-static flag could be set when a restore is required, so that, when ReleaseStyleContext is called, it just needs to check the flag. This way the interface can be ReleaseStyleContext(GtkStyleContext*), so the caller can pass the style instead of the node type again, and the method need not use another set of switch cases. If a static flag seems too ugly, this could be qdata on the style context. (I wonder even about having separate GetStyleContext()/ClaimStyleContext() methods, where ClaimStyleContext() must be used for nodes where a restore may be required or the style context needs to be exclusive. In this way ReleaseStyleContext() would need only be called when ClaimStyleContext() is required, but that's just an idea.) (In reply to Martin Stránský from comment #30) > I reordered GtkThemeWidgetType to allow to index style nodes inside > MozGtkWidgetStyle. Reordering is fine. >+ CssNode path[] = { >+ { G_TYPE_NONE, 0, "trough", NULL, NULL }, >+ { G_TYPE_NONE, 1, "slider", NULL, NULL } >+ }; >+ AddStyles(path, 2); >+ int node = aObjectType - mBaseObjectType; >+ if (node == 1) { >+ gtk_style_context_add_class(mStyle[0], GTK_STYLE_CLASS_TROUGH); >+ } else if (node == 2) { >+ gtk_style_context_add_class(mStyle[0], GTK_STYLE_CLASS_SLIDER); >+ } Although the array indexing is efficient, it is not as easy to read. I prefer having names rather than having to know the order of the enumerations. enums would help, but having the same kind of object described with different values in different enums could be confusing and hard to ensure correct use. One way to keep the benefit of simpler releasing of style contexts in arrays is to have a file-static array of GtkStyleContext* indexed by GtkThemeWidgetType. Similarly, I liked the explicit calls to CreateCssNode() like in ensure_scrollbar_widget() of other patch, because the parent node was clearly named instead of indexed. When scrollbar_buttons and siblings are considered also, I think the AddStyles() method is going to be hard to adapt, and explicit CreateCssNode() calls will be simpler. This is a bit like gtk_css_custom_gadget_new() usage in GTK. >+#define CSS_NODE_NO_PARENT -1 >+typedef struct { >+ GType type; >+ gint parentNode; >+ const gchar *name; >+ const gchar *class1; >+ const gchar *class2; >+} CssNode; I expect the type is not required because it is always G_TYPE_NONE for style contexts that do not come from widgets. gtk_css_custom_gadget_new() doesn't add classes, so I suspect the class fields won't be required either. Using explicit calls to CreateCssNode() also makes the parentNode member unnecessary. That reduces then to something relating to gtk_css_custom_gadget_new(): GtkStyleContext* CreateCssNode(const char* name, GtkStyleContext *parent) >+ mStyle[0] = gtk_widget_get_style_context(GetWidget(mBaseObjectType)); If caching the style context of the widget, then please use g_object_ref() in case GTK changes at any stage to release its widget's style contexts, during theme changes perhaps. That also means that all style contexts can be released when done. >+/* -*- Mode: C++; tab-width: 4; indent-tabs-mode: nil; c-basic-offset: 4 -*- */ The Gecko style guide requests 2 space indentation, so please use that for new files. >+ (void)get_window_widget(); >+ if (!gProtoLayout) { >+ gProtoLayout = gtk_fixed_new(); >+ gtk_container_add(GTK_CONTAINER(gProtoWindow), gProtoLayout); Instead of using gProtoWindow here, move the get_window_widget() call: gtk_container_add(GTK_CONTAINER(get_window_widget()), gProtoLayout); >+ if(!gtk_check_version(3, 20, 0)) { space between if and ( >+ if (parent) >+ path = gtk_widget_path_copy(gtk_style_context_get_path (parent)); >+ else >+ path = gtk_widget_path_new(); I think parent will always be set here because these are all ancestors of styles from widgets, and so the gtk_widget_path_new() code path is not required. >+ typedef enum { >+ STYLE_SCROLLBAR_HORIZONTAL, >+ STYLE_SCROLLBAR_VERTICAL, >+ STYLE_NUM, >+ STYLE_NONE, >+ } StyleIndex; This won't be required without MozGtkWidgetStyle. >+void >+WidgetAdd(GtkWidget* widget); Gecko convention is usually to start the method name with a verb. These functions is not in any specific namespace and this function in particular could clash with other function declarations because GtkWidget* is likely to be used elsewhere, so please be more specific with the name. "AddWidgetToCachedWindow" or "AddWidgetToWindowContainer". > /* Paints the trough (track) of a GtkScrollbar. */ > MOZ_GTK_SCROLLBAR_HORIZONTAL, >- MOZ_GTK_SCROLLBAR_VERTICAL, >+ /* Paints the slidet path of a GtkScrollbar. */ >+ MOZ_GTK_SCROLLBAR_TROUGH_HORIZONTAL, The "path" is usually called a "track" in Gecko. I think the existing comment for MOZ_GTK_SCROLLBAR_HORIZONTAL should now be the comment for MOZ_GTK_SCROLLBAR_TROUGH_HORIZONTAL. The situation with MOZ_GTK_SCROLLBAR_HORIZONTAL is a bit complex, so it would be helpful to describe that. It is the GtkScrollbar widget, but when passed to moz_gtk_widget_paint(), it draws the trough. > moz_gtk_get_scrollbar_metrics(MozGtkScrollbarMetrics *metrics) > { >+ GtkStyleContext *style = GetStyleContext(MOZ_GTK_SCROLLBAR_HORIZONTAL); >+ gtk_style_context_get_style(style, > "slider_width", &metrics->slider_width, > "trough_border", &metrics->trough_border, > "stepper_size", &metrics->stepper_size, > "stepper_spacing", &metrics->stepper_spacing, > NULL); >+ ReleaseStyleContext(MOZ_GTK_SCROLLBAR_HORIZONTAL); >+ >+ if(!gtk_check_version(3, 20, 0)) { >+ } else { >+ metrics->min_slider_size = >+ gtk_range_get_min_slider_size(GTK_RANGE(GetWidget(MOZ_GTK_SCROLLBAR_HORIZONTAL))); gtk_style_context_get_style() with min-slider-length can be used with the style from above, instead of also getting the corresponding widget, but that requires some reordering so that only one style is in use at a time. Alternatively, as GetWidget(MOZ_GTK_SCROLLBAR_HORIZONTAL) is going to be implemented anyway, that could be used with gtk_widget_style_get() and gtk_range_get_min_slider_size(), instead of fetching the widget and its style separately.

I'm just posting here to note that actual users are now being hit by this bug, since GTK 3.20 / Gnome 3.20 has shipped and has recently been moved to stable repos on Arch Linux and other distributions. There's already a reddit thread about it here: https://www.reddit.com/r/firefox/comments/4d6wt1/firefox_on_linux_doesnt_show_scroll_bar_handles/ Since this report has been around since December, I imagine you all thought you had more time before users were affected, since some downstream builds (like the Arch package for Firefox stable) already ship a version of Martin Stránský's patch, but the issue is that most people using Firefox Developer Edition are grabbing packages from the Mozilla website, since there are no distro packages for that (even the Arch AUR package just fetches the tarballs from Mozilla's servers). I don't know when DevEdition/Aurora v48 is slated to release, but I think this patch needs to be merged by then at the very least, if not sooner in the v47 daily builds, otherwise the list of affected users is going to go up as more distributions start to ship the new GTK version.

(In reply to Azari from comment #42) > I'm just posting here to note that actual users are now being hit by this > bug, since GTK 3.20 / Gnome 3.20 has shipped and has recently been moved to > stable repos on Arch Linux and other distributions. IMHO that's a bit premature. Even Fedora which may be a gnome-home distro does not ship 3.20 in stable release, it's alpha now. The 3.20 release is pretty fresh, this report is here long time but it covers whole 3.20 development which begins with 3.19.x unstable releases and the patch for 3.19.x does not work in 3.20. So things were changing rapidly, there has been incompatible changes during 3.20 development and I we must target only the final 3.20 release in Mozilla Firefox. Distro can ship aligned patches for actual gtk version but Mozilla needs to target all versions. As you can see my patch used by distros is not even compatible with older 3.19.x versions.

(In reply to Martin Stránský from comment #43) > IMHO that's a bit premature. Even Fedora which may be a gnome-home distro > does not ship 3.20 in stable release, it's alpha now. That's only because Fedora itself is still in alpha, that doesn't mean Gnome is in alpha, if it is, then you shouldn't have released 3.20 in the first place. KDE has tried to make similar claims in the past, but the very reason people make these kinds of claims about how the "first pointrelease is actually the stable one" is because you know that most people aren't going to test and report bugs unless you actually make a release. So on one side the version number is communicating "we want people to use this now", and on the other hand we hear strange ideas like "it's still considered testing until first pointrelease". In any case, I wasn't trying to be provocative, I'm merely just reporting that users are now being affected by it, so that everyone here can factor this into whatever metric they use when deciding how to prioritize bugs/issues; what you decide to do with this information is up to you, I apologize if my previous comment sounded abrasive, it wasn't intended as such. I don't know how much effort remains to get the patch to work on both old and new GTK3 versions, but is it unrealistic to target DevEdition v48 for it? The exact kinds of people that are going to be running DevEdition are going to be the kinds of people running rolling release distros, which means they will be exposed to it soon if they haven't already.

(In reply to Azari from comment #44) > The exact kinds of people that are going to be running DevEdition are > going to be the kinds of people running rolling release distros, which means > they will be exposed to it soon if they haven't already. Perhaps developers from those rolling release distributions should contribute patches. Gtk+ is mostly developed by Red Hat. It's not a large surprise that they operate on their schedule. Of course the responsible thing from Mozilla would be to make gtk+ 2.x the default till gtk+ 3.20 support is in good shape, or better yet, fix this themselves.

(In reply to Hussam Al-Tayeb from comment #45) > Of course the responsible thing from Mozilla would be to make gtk+ 2.x the > default till gtk+ 3.20 support is in good shape, or better yet, fix this > themselves. Well I kind of agree here the default toolkit for everything other than nightly should be changed to cairo-gtk2 and then this can be worked without it being any kind of issue to users. The real issue here is that we switched from gtk2 to gtk3 for no really beneficial reason when it was not really ready for prime time.

(In reply to Bill Gianopoulos [:WG9s] from comment #46) > The real issue here is that we switched from gtk2 to gtk3 for no really beneficial reason That's simply not true.

(In reply to Mike Hommey [:glandium] from comment #47) > (In reply to Bill Gianopoulos [:WG9s] from comment #46) > > The real issue here is that we switched from gtk2 to gtk3 for no really beneficial reason > > That's simply not true. You didn't quote the whole message. "No real beneficial reason" and "no real beneficial reason when it is not ready for prime time" are two completely different statements. The first means no technical advantages (obviously not true). I know that some of the benefits are free HiDPI support and Wayland support. The second statement (what Bill said) means the benefits may not warrant the current set of regressions.

(In reply to Hussam Al-Tayeb from comment #48) > (In reply to Mike Hommey [:glandium] from comment #47) > > (In reply to Bill Gianopoulos [:WG9s] from comment #46) > > > The real issue here is that we switched from gtk2 to gtk3 for no really beneficial reason > > > > That's simply not true. > You didn't quote the whole message. > > "No real beneficial reason" and "no real beneficial reason when it is not > ready for prime time" are two completely different statements. Well yes I really meant did not seem to be a beneficial reason that we seemed to be yet taking any advantage of that i have seen.

If you check the tracking bug for GTK3, you'll notice quite a few bugs that have been blocked due to GTK3 for a very long time. Of those, there's three right off the bat that seem fairly important in terms of having feature-level consistency across platforms. https://bugzilla.mozilla.org/show_bug.cgi?id=1038800 https://bugzilla.mozilla.org/show_bug.cgi?id=711711 https://bugzilla.mozilla.org/show_bug.cgi?id=513159 (There's also HiDPI which was already mentioned by another commenter. 4K monitors have become pretty widespread, so that has been problematic since Linux can't yet scale legacy apps, and likely won't be able to until Wayland.) As a project you're free to prioritize what you want, but given that sticking to GTK2 has it's own baggage of problems, I'm not sure it's wise to keep postponing this forever minus a day. On another note, I feel I should mention that unlike many others, I've actually been really glad to see Mozilla's new direction recently, I think WebExtensions is a breath of fresh air, and I'm looking forward to electrolysis shipping in stable (e10s is the main reason I use DevEdition). Just noting this so my comments can be put into context, since text is unfortunately fairly bland when it comes to conveying intent. =)

Thanks. I'd like to ensure I understand exactly what do you mean before I post an updated patch. > I like the get_window_widget/get_window_container_widget() approach but I > prefer extending that approach over using MozGtkWidgetStyle inheritance. > I think something simply based around a switch statement with get functions > (and ensure functions where it makes sense to initialize several style > contexts together) will be more flexible for special cases and easier to > read. I think simple functions will let us share code at least as well as > inheritance. Do you mean to create widgets with corresponding style in MozGtkWidgetStyle()? To have GetWidget() member of MozGtkWidgetStyle() or each constructor creates the widget separately? Like MozGtkWidgetStyleScrollbar() will create the scrollbar widget? > >+bool MozGtkWidgetStyle::StyleLock(void) > >+{ > >+ if(mLock) { > >+ return false; > >+ } > >+ mLock++; > >+ gtk_style_context_save(mStyle[0]); > >+ return true; > >+} > > >+void MozGtkWidgetStyle::StyleUnLock(void) > >+{ > >+ if(mLock) { > >+ mLock--; > >+ gtk_style_context_restore(mStyle[0]); > >+ } > > Given comment 40, having to save/restore the context will be the exception > rather than the norm. Usually ReleaseStyleContext() will just be a no-op. Okay. btw when the style is created from path we can just copy it instead of save/restore. but that may be a future change and if we manage to lock/unlock the affected styles appropriately it may not be necessary anyway. > > moz_gtk_get_scrollbar_metrics(MozGtkScrollbarMetrics *metrics) > > { > > >+ GtkStyleContext *style = GetStyleContext(MOZ_GTK_SCROLLBAR_HORIZONTAL); > >+ gtk_style_context_get_style(style, > > "slider_width", &metrics->slider_width, > > "trough_border", &metrics->trough_border, > > "stepper_size", &metrics->stepper_size, > > "stepper_spacing", &metrics->stepper_spacing, > > NULL); > >+ ReleaseStyleContext(MOZ_GTK_SCROLLBAR_HORIZONTAL); > >+ > >+ if(!gtk_check_version(3, 20, 0)) { > > >+ } else { > >+ metrics->min_slider_size = > >+ gtk_range_get_min_slider_size(GTK_RANGE(GetWidget(MOZ_GTK_SCROLLBAR_HORIZONTAL))); > > gtk_style_context_get_style() with min-slider-length can be used with the > style from above, instead of also getting the corresponding widget, but that > requires some reordering so that only one style is in use at a time. Do you mean to replace gtk_range_get_min_slider_size() call with min-slider-length style query?

Review commit: https://reviewboard.mozilla.org/r/46353/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/46353/

Review commit: https://reviewboard.mozilla.org/r/46355/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/46355/

(In reply to Martin Stránský from comment #51) > > I think something simply based around a switch statement with get functions > > (and ensure functions where it makes sense to initialize several style > > contexts together) will be more flexible for special cases and easier to > > read. I think simple functions will let us share code at least as well as > > inheritance. > > Do you mean to create widgets with corresponding style in > MozGtkWidgetStyle()? To have GetWidget() member of MozGtkWidgetStyle() or > each constructor creates the widget separately? Like > MozGtkWidgetStyleScrollbar() will create the scrollbar widget? I don't think MozGtkWidgetStyle class is necessary at all. Please see https://reviewboard.mozilla.org/r/46355/diff/1 I haven't tested with 3.20 or self-reviewed thoroughly, but it indicates the structure. > > > moz_gtk_get_scrollbar_metrics(MozGtkScrollbarMetrics *metrics) > > > { > > > > >+ GtkStyleContext *style = GetStyleContext(MOZ_GTK_SCROLLBAR_HORIZONTAL); > > >+ gtk_style_context_get_style(style, > > > "slider_width", &metrics->slider_width, > > > "trough_border", &metrics->trough_border, > > > "stepper_size", &metrics->stepper_size, > > > "stepper_spacing", &metrics->stepper_spacing, > > > NULL); > > >+ ReleaseStyleContext(MOZ_GTK_SCROLLBAR_HORIZONTAL); > > >+ > > >+ if(!gtk_check_version(3, 20, 0)) { > > > > >+ } else { > > >+ metrics->min_slider_size = > > >+ gtk_range_get_min_slider_size(GTK_RANGE(GetWidget(MOZ_GTK_SCROLLBAR_HORIZONTAL))); > > > > gtk_style_context_get_style() with min-slider-length can be used with the > > style from above, instead of also getting the corresponding widget, but that > > requires some reordering so that only one style is in use at a time. > > Do you mean to replace gtk_range_get_min_slider_size() call with > min-slider-length style query? Yes. Would changes to moz_gtk_get_scrollbar_metrics alone be enough to make thumbs visible with 3.20, even if they don't look right? If so, that may be worth doing on its own first.

Comment on attachment 8741275 [details] MozReview Request: bug 1234158 rename GtkThemeWidgetType enum to WidgetNodeType as it will differentiate GTK CSS nodes r?stransky I didn't have a chance to go trough it but some first-time ideas: - I like your original idea of styles (and widget?) array indexed by WidgetNodeType. The #defines look's misleading to me and difficult to read. Especially when we port all styles there. - GetStyleInternal() uses two switches with almost identical labels (case:). Can we join those somehow? Again, when we port all styles here it would be a huge switch and duplicated. Maybe create a function pointer and initialize it according actual gtk version?

(In reply to Martin Stránský from comment #55) > - I like your original idea of styles (and widget?) array indexed by > WidgetNodeType. The #defines look's misleading to me and difficult to read. > Especially when we port all styles there. I figure that is something that can be added later, when there are more widgets to fill an array. Maybe the widgets and styles can be stored the same array even, as they are all GObjects, and we need only one or the other, not both. Feel free to list the variables individually for now if you prefer. > - GetStyleInternal() uses two switches with almost identical labels (case:). > Can we join those somehow? Again, when we port all styles here it would be a > huge switch and duplicated. Maybe create a function pointer and initialize > it according actual gtk version? Initializing function pointers and data in the switch, and making the function call after the switch, may work well in some situations. I don't know that there is a good way to merge pre- and post-3.20 code in general because even the parents can be different. The review request was for the GtkThemeWidgetType/WidgetNodeType change. The other patch was a prototype in reply to comment 51.

(In reply to Karl Tomlinson (ni?:karlt) from comment #56) > I don't > know that there is a good way to merge pre- and post-3.20 code in general > because even the parents can be different. And the 3.20 code will change when sibling support is added.

Comment on attachment 8741275 [details] MozReview Request: bug 1234158 rename GtkThemeWidgetType enum to WidgetNodeType as it will differentiate GTK CSS nodes r?stransky https://reviewboard.mozilla.org/r/46353/#review43293 We're fine here.

There's an updated one. It's basically your patch with merged style construction and unified switch. I removed the DEBUG macro from sCurrentStyleContext becase we need that for ResetWidgetCache()

(In reply to Martin Stránský from comment #60) > Created attachment 8742825 [details] [diff] [review] > scroll v.2 patch > > There's an updated one. It's basically your patch with merged style > construction and unified switch. I removed the DEBUG macro from > sCurrentStyleContext becase we need that for ResetWidgetCache() This section of the patch: +void +ResetWidgetCache(void) +{ + if (sStyleContextNeedsRestore) { + ReleaseStyleContext(sCurrentStyleContext); Results in these errors: 25:46.47 In file included from /home/wag/mozilla/mozilla2/fx-obj/widget/gtk/Unified_cpp_widget_gtk0.cpp:29:0: 25:46.47 /home/wag/mozilla/mozilla2/widget/gtk/WidgetStyleCache.cpp: In function ‘void ResetWidgetCache()’: 25:46.47 /home/wag/mozilla/mozilla2/widget/gtk/WidgetStyleCache.cpp:182:25: error: ‘sCurrentStyleContext’ was not declared in this scope 25:46.47 ReleaseStyleContext(sCurrentStyleContext);

Sorry, wrong patch.

(In reply to Martin Stránský from comment #62) > Created attachment 8743155 [details] [diff] [review] > scroll v.3 > > Sorry, wrong patch. Much better. Works fine using either Gtk3 or Gtk2 toolkits.

Fwiw, in OpenBSD we use Gtk 3.20, and i'm now using http://pkgs.fedoraproject.org/cgit/rpms/firefox.git/tree/firefox-gtk3-20.patch to fix all the issues when using Gtk 3.20 by default with Fx 46. So far so good.

(In reply to Landry Breuil (:gaston) from comment #64) > Fwiw, in OpenBSD we use Gtk 3.20, and i'm now using > http://pkgs.fedoraproject.org/cgit/rpms/firefox.git/tree/firefox-gtk3-20. > patch to fix all the issues when using Gtk 3.20 by default with Fx 46. So > far so good. The problem is that that patch causes serious regressions in pre 3.19.2 versions of gtk3. We are trying to develop a patch that works with any version of gtk3.

The patch is pretty intrusive. What do we plan to do for 47?

Comment on attachment 8741276 [details] MozReview Request: bug 1234158 add support for GTK 3.20 scrollbars r?karlt Review request updated; see interdiff: https://reviewboard.mozilla.org/r/46355/diff/1-2/

Comment on attachment 8743155 [details] [diff] [review] scroll v.3 Moved this to mozreview to make review easier, and added back the commit message.

Comment on attachment 8741276 [details] MozReview Request: bug 1234158 add support for GTK 3.20 scrollbars r?karlt https://reviewboard.mozilla.org/r/46355/#review47371 This looks close, thanks. ::: widget/gtk/WidgetStyleCache.cpp:29 (Diff revisions 1 - 2) > GetStyleInternal(WidgetNodeType aNodeType); > > static GtkWidget* > GetWindowWidget() > { > - if (!sProtoWindow) { > + static GtkWidget *widget = sWidgetStorage[MOZ_GTK_WINDOW]; No need for additional static storage for |widget| here, which is not cleared in ResetWidgetCache. Remove "static" and the compiler can optimize away the automatic temporary in the common case. Similarly in GetWindowContainerWidget(). ::: widget/gtk/WidgetStyleCache.cpp:113 (Diff revisions 1 - 2) > > return context; > } > > static GtkStyleContext* > -GetChildNodeStyle(GtkStyleContext** aStorage, const gchar* name, > +GetStyleWidget(WidgetNodeType aStyleType, Please keep the GetChildNodeStyle name or similar. The function is getting a style, not a widget, and the distinction from other nodes in GetStyleInternal() is that it is getting a child node. ::: widget/gtk/WidgetStyleCache.cpp:182 (Diff revisions 1 - 2) > - GtkStyleContext* style = *aStorage; > - if (style) { > + if (sStyleContextNeedsRestore) > + ReleaseStyleContext(sCurrentStyleContext); Just assert !sStyleContextNeedsRestore here. ReleaseStyleContext() should have already been called before ResetWidgetCache(). ::: widget/gtk/WidgetStyleCache.cpp:185 (Diff revisions 1 - 2) > -ReleaseAndClear(GtkStyleContext** aStorage) > { > - GtkStyleContext* style = *aStorage; > - if (style) { > - g_object_unref(style); > + if (sStyleContextNeedsRestore) > + ReleaseStyleContext(sCurrentStyleContext); > + > + if (!gtk_check_version(3, 20, 0)) { I wouldn't bother with the gtk_check_version() optimization here. ::: widget/gtk/WidgetStyleCache.cpp:190 (Diff revisions 1 - 2) > + if (!gtk_check_version(3, 20, 0)) { > + for (int i = 0; i < MOZ_GTK_WIDGET_NUM; i++) { > + if (sStyleStorage[i]) > + g_object_unref(sStyleStorage[i]); > - } > + } > - *aStorage = nullptr; > + memset(sStyleStorage, 0, sizeof(sStyleStorage)); This is correct, but please use PodArrayZero(), which makes it very hard to make mistakes. Similarly for sWidgetStorage. ::: widget/gtk/WidgetStyleCache.cpp:191 (Diff revisions 1 - 2) > + for (int i = 0; i < MOZ_GTK_WIDGET_NUM; i++) { > + if (sStyleStorage[i]) > + g_object_unref(sStyleStorage[i]); > - } > + } > - *aStorage = nullptr; > + memset(sStyleStorage, 0, sizeof(sStyleStorage)); > + sCurrentStyleContext = nullptr; Just assert !sCurrentStyleContext here. That means that sCurrentStyleContext is debug only. ::: widget/gtk/gtkdrawing.h:202 (Diff revisions 1 - 2) > MOZ_GTK_WINDOW, > /* Window container for all widgets */ > MOZ_GTK_WINDOW_CONTAINER, > /* Paints a GtkInfoBar, for notifications. */ > - MOZ_GTK_INFO_BAR > + MOZ_GTK_INFO_BAR, > + Please don't add new trailing whitespace. There is some elsewhere too. ::: widget/gtk/gtkdrawing.h:203 (Diff revisions 1 - 2) > /* Window container for all widgets */ > MOZ_GTK_WINDOW_CONTAINER, > /* Paints a GtkInfoBar, for notifications. */ > - MOZ_GTK_INFO_BAR > + MOZ_GTK_INFO_BAR, > + > + MOZ_GTK_WIDGET_NUM The affix _COUNT is much more common in Gecko than _NUM because NUM is sometimes an identifier rather than a quantity. Please call this MOZ_GTK_WIDGET_NODE_COUNT, as this is more about CSS nodes now than GtkWidgets. ::: widget/gtk/gtk3drawing.cpp (Diff revision 2) > if (!gProtoLayout) { > - gProtoLayout = gtk_fixed_new(); > + gProtoLayout = GetWidget(MOZ_GTK_WINDOW_CONTAINER); > - gtk_container_add(GTK_CONTAINER(gProtoWindow), gProtoLayout); > } > > - gtk_container_add(GTK_CONTAINER(gProtoLayout), widget); We need the gtk_container_add(). Perhaps this is the cause of (firefox:29710): Gtk-CRITICAL **: gtk_widget_realize: assertion 'widget->priv->anchored || GTK_IS_INVISIBLE (widget)' failed Sorry, I forgot to add that back in. ::: widget/gtk/gtk3drawing.cpp:1194 (Diff revision 2) > - ensure_scrollbar_widget(); > + GtkStyleContext* style = ClaimStyleContext(widget, direction); > - > - if (widget == MOZ_GTK_SCROLLBAR_THUMB_HORIZONTAL) > - scrollbar = GTK_SCROLLBAR(gHorizScrollbarWidget); > - else > - scrollbar = GTK_SCROLLBAR(gVertScrollbarWidget); > - > - gtk_widget_set_direction(GTK_WIDGET(scrollbar), direction); The direction could be relevant, so this should still be set somehow. gtk_style_context_set_direction() should be good enough for pre-3.20, though it needs to be after gtk_style_context_set_state() for the sake of GTK versions after the one that changed the method to start overwriting the direction. Post 3.20, I guess it should be set also on parent style contexts and widget path nodes? I suggest leaving that exercise to later. Similarly in trough_paint.

With array-based storage, it is easier to share this code and simply widget creation functions. Review commit: https://reviewboard.mozilla.org/r/50661/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/50661/

(In reply to Mike Hommey [:glandium] from comment #67) > The patch is pretty intrusive. What do we plan to do for 47? Once scrollbar troughs and sliders are done, I don't think checkboxes will be too complicated, and so putting this on 47 may be an option. Another option may be to disable the problem native widgets when running against 3.20 and use the CSS fallback. I don't know what state that is in.

(In reply to Karl Tomlinson (ni?:karlt) from comment #72) > Once scrollbar troughs and sliders are done, I don't think checkboxes will > be too complicated, and so putting this on 47 may be an option. The problem is that the pending patch here makes the checkboxes problem worse to the point that the browser is unusable on sites with checkboxes. It changes the issue from only visible if checked to completely invisible in all circumstances.

(In reply to Bill Gianopoulos [:WG9s] from comment #73) > The problem is that the pending patch here makes the checkboxes problem > worse to the point that the browser is unusable on sites with checkboxes. > It changes the issue from only visible if checked to completely invisible in > all circumstances. Thanks, Bill. Perhaps that's the missing gtk_container_add(). If not, that problem shouldn't be too hard to resolve, because the patch is aiming to leave checkboxes as they are.

(In reply to Karl Tomlinson (ni?:karlt) from comment #74) > Perhaps that's the missing gtk_container_add(). Thanks, Karl Restoring the missing gtk_container_add() did indeed fix the new checkbox regression as well as eliminate the gtk_widget_realize assertions.

https://reviewboard.mozilla.org/r/46355/#review47371 > No need for additional static storage for |widget| here, which is not cleared in ResetWidgetCache. Remove "static" and the compiler can optimize away the automatic temporary in the common case. > > Similarly in GetWindowContainerWidget(). fixed > Please keep the GetChildNodeStyle name or similar. > The function is getting a style, not a widget, and the distinction from other nodes in GetStyleInternal() is that it is getting a child node. fixed > Just assert !sStyleContextNeedsRestore here. > ReleaseStyleContext() should have already been called before ResetWidgetCache(). fixed > I wouldn't bother with the gtk_check_version() optimization here. fixed > This is correct, but please use PodArrayZero(), which makes it very hard to make mistakes. > Similarly for sWidgetStorage. fixed > Just assert !sCurrentStyleContext here. > That means that sCurrentStyleContext is debug only. fixed > Please don't add new trailing whitespace. > There is some elsewhere too. fixed > The affix _COUNT is much more common in Gecko than _NUM because NUM is sometimes an identifier rather than a quantity. > > Please call this MOZ_GTK_WIDGET_NODE_COUNT, as this is more about CSS nodes now than GtkWidgets. fixed > We need the gtk_container_add(). > Perhaps this is the cause of > (firefox:29710): Gtk-CRITICAL **: gtk_widget_realize: assertion 'widget->priv->anchored || GTK_IS_INVISIBLE (widget)' failed > > Sorry, I forgot to add that back in. fixed > The direction could be relevant, so this should still be set somehow. > gtk_style_context_set_direction() should be good enough for pre-3.20, though it needs to be after gtk_style_context_set_state() for the sake of GTK versions after the one that changed the method to start overwriting the direction. > > Post 3.20, I guess it should be set also on parent style contexts and widget path nodes? I suggest leaving that exercise to later. > > Similarly in trough_paint. fixed. I think we need to intergate direction to ClaimStyleContext() as a part of set_state. I set the gtk_style_context_set_direction() directly in the rendering functions now and we can address that in next patch.

There's the rename patch updated for latest trunk.

Updated scrollbar patch with the MozReview comments.

Comment on attachment 8749702 [details] [diff] [review] scrollbar patch v.3 Btw. This patch also includes the changes from "share code to check and set sWidgetStorage" patch.

(In reply to Martin Stránský from comment #78) > Created attachment 8749702 [details] [diff] [review] > scrollbar patch v.3 > > Updated scrollbar patch with the MozReview comments. I am re-spinning my Gtk3 build at http://www.wg9s.com/mozilla/firefox/ to contain the new patches here.

Comment on attachment 8741276 [details] MozReview Request: bug 1234158 add support for GTK 3.20 scrollbars r?karlt Review request updated; see interdiff: https://reviewboard.mozilla.org/r/46355/diff/2-3/

https://reviewboard.mozilla.org/r/46355/#review47371 > fixed. I think we need to intergate direction to ClaimStyleContext() as a part of set_state. > > I set the gtk_style_context_set_direction() directly in the rendering functions now and we can address that in next patch. Yes, that's good thanks. That probably works for most situations in 3.20 even. Some themes may match the direction on an ancestor node, but that's not something that needs to be addressed soon AFAIK. It looks like widget paths may be mostly irrelevant in 3.20, and only style contexts need to be adjusted.

Comment on attachment 8741276 [details] MozReview Request: bug 1234158 add support for GTK 3.20 scrollbars r?karlt https://reviewboard.mozilla.org/r/46355/#review47949 ::: widget/gtk/WidgetStyleCache.cpp:146 (Diff revisions 2 - 3) > static GtkStyleContext* > GetStyleInternal(WidgetNodeType aNodeType) > { > GtkWidget* widget = GetWidget(aNodeType); > if (widget) { > + // TODO - Do we need to save the style here? I don't think we want to save the style here, at least not in general, because GTK usually does not save the style context before using. Perhaps there are exceptions, though. ::: widget/gtk/gtk3drawing.cpp:1172 (Diff revisions 2 - 3) > + // TODO - intergate with ClaimStyleContext()? > + gtk_style_context_set_direction(style, direction); Looks like there might be tabs here, and in thumb_paint.

Comment on attachment 8741276 [details] MozReview Request: bug 1234158 add support for GTK 3.20 scrollbars r?karlt Review request updated; see interdiff: https://reviewboard.mozilla.org/r/46355/diff/3-4/

Comment on attachment 8741276 [details] MozReview Request: bug 1234158 add support for GTK 3.20 scrollbars r?karlt https://reviewboard.mozilla.org/r/46355/#review47953 I fixed up some whitespace and spelling, and removed the "Do we need to save the style here?" question.

Comment on attachment 8741276 [details] MozReview Request: bug 1234158 add support for GTK 3.20 scrollbars r?karlt https://reviewboard.mozilla.org/r/46355/#review47957 I didn't mean to remove the r+.

https://hg.mozilla.org/integration/mozilla-inbound/rev/7417f269f3bb https://hg.mozilla.org/integration/mozilla-inbound/rev/827f631db4de

Comment on attachment 8749702 [details] [diff] [review] scrollbar patch v.3 Moved this to mozreview and pushed to inbound, thanks.

[Tracking Requested - why for this release]: Bug 1230955 is addressed by the changes here. GTK 3.20 deliberately includes changes that cause apps using the themed drawing API to render some widgets completely incorrectly. The expectation is that apps will change as GTK directs. What it means is that distributions that want to run existing applications should not update GTK to a recent version without updating and testing all applications. Users of bleeding edge distributions that are already using 3.20 will be quite accustomed to having to work around problems and will be capable of switching to aurora or beta release channels, so we don't necessarily *need* to uplift to 47, but I think the option of shipping this ASAP is one to seriously consider.

(In reply to Karl Tomlinson (ni?:karlt) from comment #90) > Users of bleeding edge distributions that are already using 3.20 will be > quite accustomed to having to work around problems and will be capable of > switching to aurora or beta release channels, so we don't necessarily *need* > to uplift to 47, but I think the option of shipping this ASAP is one to > seriously consider. I highly support uplifting to beta/47 - OpenBSD is using Gtk 3.20, and telling users to 'switch to aurora or beta channel' is not really an option for us..

https://hg.mozilla.org/mozilla-central/rev/7417f269f3bb https://hg.mozilla.org/mozilla-central/rev/827f631db4de

[Tracking Requested - why for this release]: Bug 1230955 is addressed by the changes here. GTK 3.20 deliberately includes changes that cause apps using the themed drawing API to render some widgets completely incorrectly. The expectation is that apps will change as GTK directs. What it means is that distributions that want to run existing applications should not update GTK to a recent version without updating and testing all applications. Additionally although users of these bleeding edge distros probably have a distro provided version of Firefox that is patched for the release version, not uplifting to 48 would mean these users would not have an option to run the developers preview.

(In reply to Bill Gianopoulos [:WG9s] from comment #93) > Additionally although users of these bleeding edge distros probably have a > distro provided version of Firefox that is patched for the release version, > not uplifting to 48 would mean these users would not have an option to run > the developers preview. That's my situation (I commented earlier in this thread). Firefox-release is already patched in my distribution (Arch Linux), but the Arch User Repository 'package' for Firefox dev edition is just a script that fetches the dev edition builds from Mozilla's website and automatically makes an Arch package on the fly. I mainly switched to dev edition due to e10s, so it's inconvenient that release doesn't yet have e10s even as a toggle option, but dev edition has invisible scrollbars at the moment. =\

Karl can you request uplift to 48?

That is, if you think it's ready and will apply.

Or, is this Martin's patch?

Comment on attachment 8741276 [details] MozReview Request: bug 1234158 add support for GTK 3.20 scrollbars r?karlt Approval Request Comment [Feature/regressing bug #]: regression in GTK code https://bugzilla.gnome.org/show_bug.cgi?id=757503 [User impact if declined]: No scrollbar handles for people testing latest GTK. [Describe test coverage new/current, TreeHerder]: none. [Risks and why]: lowish risk. Although a fair bit of code is moved around, the effective changes for people running pre-3.20 GTK are minimal. [String/UUID change made/needed]: none.

(In reply to Karl Tomlinson (ni?:karlt) from comment #98) > Comment on attachment 8741276 [details] > MozReview Request: bug 1234158 add support for GTK 3.20 scrollbars r?karlt > > Approval Request Comment > [Feature/regressing bug #]: regression in GTK code > https://bugzilla.gnome.org/show_bug.cgi?id=757503 > [User impact if declined]: > No scrollbar handles for people testing latest GTK. > [Describe test coverage new/current, TreeHerder]: > none. > [Risks and why]: > lowish risk. Although a fair bit of code is moved around, the effective > changes for people running pre-3.20 GTK are minimal. > [String/UUID change made/needed]: > none. Actually the impact on people running pre 3.20 gtk3 is zero and the people running post gtk3.20 are more or less screwed without this.

Comment on attachment 8741276 [details] MozReview Request: bug 1234158 add support for GTK 3.20 scrollbars r?karlt Scrolling is important, let's uplift and verify on aurora

Hi Karl, is this something we should consider uplifting to Fx47? I would prefer to let this ride the train unless the overall end-user benefits outweigh the risks, specially the risk that this hasn't stabilized on Nightly/Aurora enough.

(In reply to Ritu Kothari (:ritu) from comment #101) > Hi Karl, is this something we should consider uplifting to Fx47? I would > prefer to let this ride the train unless the overall end-user benefits > outweigh the risks, specially the risk that this hasn't stabilized on > Nightly/Aurora enough. I think comment 99 also applies to beta - the sooner you get a fixed Gtk3 'experience' in a release build on environments with latest Gtk3.20, the better the user feedback...depends how many betas are left.

Hi, has problems to apply to aurora grafting 343844:827f631db4de "bug 1234158 add support for GTK 3.20 scrollbars r=karlt" merging widget/gtk/gtk3drawing.cpp merging widget/gtk/gtkdrawing.h warning: conflicts while merging widget/gtk/gtkdrawing.h! (edit, then use 'hg resolve --mark') abort: unresolved conflicts, can't continue could you take a look ? thanks!

(In reply to Carsten Book [:Tomcat] from comment #103) > Hi, has problems to apply to aurora > > grafting 343844:827f631db4de "bug 1234158 add support for GTK 3.20 > scrollbars r=karlt" > merging widget/gtk/gtk3drawing.cpp > merging widget/gtk/gtkdrawing.h > warning: conflicts while merging widget/gtk/gtkdrawing.h! (edit, then use > 'hg resolve --mark') > abort: unresolved conflicts, can't continue > > could you take a look ? thanks! Hm, tested with releases/mozilla-aurora and it applies fine. Make sure you use "rename patch v.2" from this bug instead the one from MozReview. The patch in MozReview (93f1cf1c81ae) is bitrotten and does not apply.

Except he was trying to graft/cherry-pick 827f631db4de, which is what landed on mozilla-central... Just in case, Carsten, did you graft 7417f269f3bb before?

(In reply to Mike Hommey [:glandium] from comment #105) > Except he was trying to graft/cherry-pick 827f631db4de, which is what landed > on mozilla-central... Just in case, Carsten, did you graft 7417f269f3bb > before? yeah i grafted 827f631db4de that had the problem. Does 7417f269f3bb need to be uplifted too (so maybe it need also approval since only 827f631db4de has)

Yes, you need also uplift 7417f269f3bb.

Comment on attachment 8741275 [details] MozReview Request: bug 1234158 rename GtkThemeWidgetType enum to WidgetNodeType as it will differentiate GTK CSS nodes r?stransky Approval Request Comment process

(In reply to Ritu Kothari (:ritu) from comment #101) > Hi Karl, is this something we should consider uplifting to Fx47? I would > prefer to let this ride the train unless the overall end-user benefits > outweigh the risks, specially the risk that this hasn't stabilized on > Nightly/Aurora enough. I think we could put this on 47, but this is only a partial solution to the many issues with 3.20 (see dependent bugs here and dependencies of bug 1264079). It does make a world of difference to being able to use the scrollbar though. Bug 1264079 is evidence that this issue is not yet completely understood, but that only affects users with 3.20, who are (at least usually) better off with this patch. In general, I'm less keen on pushing out workarounds quickly for recent system regressions. Here the risk is probably acceptable, but debatable. One thing I would want to know before uplifting to 47 is whether this breaks arc-theme, which is the only theme I know that has changes to provide good scrollbars with Firefox and GTK 3.20.

(In reply to Karl Tomlinson (ni?:karlt) from comment #109) I just tested the last nightly build [49.0a1 (2016-05-12)] in debian unstable using gtk3 3.20.4-1 and arc-theme from the master branch, and the scrollbars are ok both with and without the firefox theme.

Comment on attachment 8741275 [details] MozReview Request: bug 1234158 rename GtkThemeWidgetType enum to WidgetNodeType as it will differentiate GTK CSS nodes r?stransky Let's uplift to Aurora48 and stabilize before deciding whether to uplift to Fx47 or not. Will wait for KarlT to decide whether this is worth the risk or not for uplift to Beta.

Hi Wes, this is a long standing A+ for Aurora48 uplift. It somehow missed our queries. Please uplift at your earliest convenience. I think it is already too late for Beta47.

Hi Karl, this did not land on Aurora48 so far, does it still give us enough bake time to consider uplifting on Thursday, when I gtb 47.0b9? It seems a bit too late to me, which is why I marked it as wontfix for Fx47.

https://hg.mozilla.org/releases/mozilla-aurora/rev/788b1166ae59 https://hg.mozilla.org/releases/mozilla-aurora/rev/cb42beedc07e

(In reply to Yaacov Akiba Slama from comment #110) > (In reply to Karl Tomlinson (ni?:karlt) from comment #109) > I just tested the last nightly build [49.0a1 (2016-05-12)] in debian > unstable using gtk3 3.20.4-1 and arc-theme from the master branch, and the > scrollbars are ok both with and without the firefox theme. Thanks. That was my biggest concern, so good to know that is not a problem.

I landed this on Aurora yesterday. Still that doesn't give us much more testing. (Sorry, I had mis-assumed the bug report would be updated to show the commit automatically.) Uplifting this to 47 would also require uplifting the dependencies of this bug. Each of those alone is not too major a change, but together it is significant. This is really late in the cycle to do all that, and I don't have the time to check it all and see whether there are further conflicts. The value of the patch is high for those with 3.20 and more people are going to be using this version, but there are still bad bugs with GTK 3.20 that are not avoided by this patch. 3.20 users are still going to be better off switching to the beta channel, even with this patch. https://fedoraproject.org/wiki/Releases/24/Schedule says that Fedora 24, which will have the buggy GTK 3.20 is currently scheduled for 2016-06-14.

We also need bug 1262136 uplifted ASAP or still have a totally unusable browser on aurora for next uplift with current GTK3 versions.

FYI quick workaround to get scrollbars working, is to use a userstyle like : https://userstyles.org/styles/97199/blue-scrollbar-simple