We may export CreateCSSNode() to allow other modules to create styles on the fly. We may also divide the pre/post 3.20 code to add more flexibility to style construction. Follow up from: https://bugzilla.mozilla.org/show_bug.cgi?id=1271523#c15 (tooltips background bug) https://bugzilla.mozilla.org/show_bug.cgi?id=1275407 (scrollbar bug) https://bugzilla.mozilla.org/show_bug.cgi?id=1250704 (tooltip text color bug)

The simplest patch.

Comment on attachment 8759636 [details] [diff] [review] patch A final patch also needs to remove GetStyleInternal() declaration which I forget to do in this patch.

Looks like we really need to create all styles differently for 3.20. There's the conversation I had with Company on #gtk+ <stransky> Company, Hi, is there a problem when we create the root style from widget and then child CSS nodes by the path? In Gtk 3.20 <Company> stransky: i wouldn't do that <stransky> Company, for instance for scrollbar widgets. to get the root style by gtk_widget_get_style_context(scrollbar widget) and then use path to get CSS child styles <Company> because then you have parts of the code that update based on the widget tree and parts of the code that update based on your tree <Company> and your tree changes the widget tree by hooking into it, too <Company> i don't know off-hand of things that would go wrong <Company> but i wouldn't be surprised by it <stransky> Company, okay, But it's okay to still get style from widgets if we don't use the hierarchy? <Company> stransky: why do you want to do that? btw? <stransky> Company, to share code between pre/post 3.20 code in Firefox <stransky> Company, because FF needs to keep compatibility from gtk 3.4 <Company> hrm <Company> i'm not sure sharing code there is that good of an idea <stransky> Company, well, so do i need to create all styles from path for gtk3.20? <Company> GTK3.4 and 3.20 style machinery is almost as far apart internally as GTK2 and GTK3 <Company> stransky: i would do that <Company> stransky: you can get the widget path from a widget and create the style from that <stransky> and how? <Company> gtk_widget_get_path() <Company> and then create a style context with that path <stransky> Company, so do you basically say that call gtk_widget_get_style_context(widget) just for base styles is a bad idea on 3.20? <Company> stransky: I think mixing those style contexts with style contexts created via paths is tricky <stransky> Company, Ok, thanks. <Company> stransky: both because the first kind get changed all the time and because adding style contexts changes :first-child etc behaviors by adding new nodes to the trtee

Comment on attachment 8759636 [details] [diff] [review] patch > static GtkStyleContext* >+GetCssNodeStyleInternal(WidgetNodeType aNodeType) Can you add a comment here please to indicate that this is used only when the GTK version > 3.20? >+ style = CreateCSSNode(GTK_STYLE_CLASS_TROUGH, >+ GetCssNodeStyleInternal(MOZ_GTK_SCROLLBAR_HORIZONTAL)); Please add static StyleContext* CreateChildCSSNode(const char* aName, WidgetNodeType aParentNodeType) or static StyleContext* CreateCSSNode(const char* aName, WidgetNodeType aParentNodeType, GType aType = G_TYPE_NONE) so that these simplify to style = CreateCSSNode(GTK_STYLE_CLASS_TROUGH, MOZ_GTK_SCROLLBAR_HORIZONTAL); (and will fit in 80 columns.) >+ /* Actual progress bar indicator for Gtk3.20+ only. */ Remove "for Gtk3.20+ only." because this whole function is 3.20+ now. >+ default: >+ GtkWidget* widget = GetWidget(aNodeType); >+ if (widget) { >+ return gtk_widget_get_style_context(widget); >+ } >+ break; >+ } Given the code below is asserting that style is set, the "if (widget)" test and "break;" are not required here. MOZ_ASSERT(widget), if you like. >+ if (style) { >+ sStyleStorage[aNodeType] = style; >+ return style; >+ } >+ >+ MOZ_ASSERT_UNREACHABLE("missing style context for node type"); >+ return nullptr; MOZ_ASSERT(style, "missing style context for node type"); sStyleStorage[aNodeType] = style; return style; >+static GtkStyleContext* >+CreateWidgetStyle(WidgetNodeType aWidgetType, >+ const gchar* aStyleClass) >+{ "GetWidgetStyle" because this does not "Create", and ownership is not passed to the caller. Or "GetWidgetStyleWithClass" may be clearer even. >+ if (aStyleClass) { aStyleClass is always non-null and so this check is not required. >+// CreateCSSNode is useful for gtk >= 3.20 only. Perhaps this function could be useful pre-3.20 one day if adjusted to skip sGtkWidgetPathIterSetObjectName when not available, so I suggest "CreateCSSNode is implemented for gtk >= 3.20 only." (In reply to Martin Stránský from comment #2) > A final patch also needs to remove GetStyleInternal() declaration which I > forget to do in this patch. OK. Thanks!

Thanks! There's a new one attached. Try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=05b1fbe8cd4c

Comment on attachment 8760235 [details] [diff] [review] patch v.2 >+ return GetWidgetStyleWithClass(MOZ_GTK_SCROLLBAR_HORIZONTAL, >+ GTK_STYLE_CLASS_TROUGH); >+ case MOZ_GTK_SCROLLBAR_THUMB_HORIZONTAL: >+ return GetWidgetStyleWithClass(MOZ_GTK_SCROLLBAR_HORIZONTAL, >+ GTK_STYLE_CLASS_SLIDER); Looks good, thanks. Can you touch up the indentation in these calls, please?

With both the patch for this bug and the patch for bug 1275407 applied, I find the browser crashing often with this (or similar) stack: Program wg9s_64/firefox (pid = 2301) received signal 11. Stack: #01: ???[/home/wag/wg9s_64/libxul.so +0x6ea478d] #02: ???[/lib64/libpthread.so.0 +0x10c10] #03: gtk_widget_path_copy[/lib64/libgtk-3.so.0 +0x373d68] #04: ???[/lib64/libgtk-3.so.0 +0x36c19d] #05: ???[/lib64/libgtk-3.so.0 +0x15806e] #06: gtk_container_get_path_for_child[/lib64/libgtk-3.so.0 +0x15e1be] #07: ???[/lib64/libgtk-3.so.0 +0x15806e] #08: gtk_container_get_path_for_child[/lib64/libgtk-3.so.0 +0x15e1be] #09: ???[/lib64/libgtk-3.so.0 +0x18e8b5] #10: ???[/lib64/libgtk-3.so.0 +0x2c6d8e] #11: gtk_widget_style_get_valist[/lib64/libgtk-3.so.0 +0x370775] #12: gtk_widget_style_get[/lib64/libgtk-3.so.0 +0x370aa4] #13: ???[/home/wag/wg9s_64/libxul.so +0x460b1e8] #14: ???[/home/wag/wg9s_64/libxul.so +0x4631f15] #15: ???[/home/wag/wg9s_64/libxul.so +0x4c8c260] #16: ???[/home/wag/wg9s_64/libxul.so +0x4c8bf73] #17: ???[/home/wag/wg9s_64/libxul.so +0x4c8db4d] #18: ???[/home/wag/wg9s_64/libxul.so +0x4c91885] #19: ???[/home/wag/wg9s_64/libxul.so +0x4cc089b] #20: ???[/home/wag/wg9s_64/libxul.so +0x4c8f7d2] #21: ???[/home/wag/wg9s_64/libxul.so +0x4ca57c4] #22: ???[/home/wag/wg9s_64/libxul.so +0x4cc07dd] #23: ???[/home/wag/wg9s_64/libxul.so +0x4c8f7d2] #24: ???[/home/wag/wg9s_64/libxul.so +0x4cc07dd] #25: ???[/home/wag/wg9s_64/libxul.so +0x4c8f7d2] #26: ???[/home/wag/wg9s_64/libxul.so +0x4b101d1] #27: ???[/home/wag/wg9s_64/libxul.so +0x4cc07dd] #28: ???[/home/wag/wg9s_64/libxul.so +0x4c8f7d2] #29: ???[/home/wag/wg9s_64/libxul.so +0x4cc07dd] #30: ???[/home/wag/wg9s_64/libxul.so +0x4c8f7d2] #31: ???[/home/wag/wg9s_64/libxul.so +0x4cc07dd] #32: ???[/home/wag/wg9s_64/libxul.so +0x4c8f7d2] #33: ???[/home/wag/wg9s_64/libxul.so +0x4ca7387] #34: ???[/home/wag/wg9s_64/libxul.so +0x4ca394c] #35: ???[/home/wag/wg9s_64/libxul.so +0x4c8c981] #36: ???[/home/wag/wg9s_64/libxul.so +0x4cbe9dd] #37: ???[/home/wag/wg9s_64/libxul.so +0x4c8fdc2] #38: ???[/home/wag/wg9s_64/libxul.so +0x4c8c981] #39: ???[/home/wag/wg9s_64/libxul.so +0x4cbe9dd] #40: ???[/home/wag/wg9s_64/libxul.so +0x4c8fdc2] #41: ???[/home/wag/wg9s_64/libxul.so +0x4c8c981] #42: ???[/home/wag/wg9s_64/libxul.so +0x4b1ac7b] #43: ???[/home/wag/wg9s_64/libxul.so +0x4b1b3b6] #44: ???[/home/wag/wg9s_64/libxul.so +0x4b105f6] #45: ???[/home/wag/wg9s_64/libxul.so +0x4c8c981] #46: ???[/home/wag/wg9s_64/libxul.so +0x4cbe9dd] #47: ???[/home/wag/wg9s_64/libxul.so +0x4c8fdc2] #48: ???[/home/wag/wg9s_64/libxul.so +0x4c8c981] #49: ???[/home/wag/wg9s_64/libxul.so +0x4cbe9dd] #50: ???[/home/wag/wg9s_64/libxul.so +0x4c8fdc2] #51: ???[/home/wag/wg9s_64/libxul.so +0x4c8c981] #52: ???[/home/wag/wg9s_64/libxul.so +0x4cbe9dd] #53: ???[/home/wag/wg9s_64/libxul.so +0x4c8fdc2] #54: ???[/home/wag/wg9s_64/libxul.so +0x4c8c981] #55: ???[/home/wag/wg9s_64/libxul.so +0x4cbe9dd] #56: ???[/home/wag/wg9s_64/libxul.so +0x4c8fdc2] #57: ???[/home/wag/wg9s_64/libxul.so +0x4c8c981] #58: ???[/home/wag/wg9s_64/libxul.so +0x4cbe9dd] #59: ???[/home/wag/wg9s_64/libxul.so +0x4c8fdc2] #60: ???[/home/wag/wg9s_64/libxul.so +0x4c8c981] #61: ???[/home/wag/wg9s_64/libxul.so +0x4cbe9dd] #62: ???[/home/wag/wg9s_64/libxul.so +0x4c8fdc2] #63: ???[/home/wag/wg9s_64/libxul.so +0x4c8c981] #64: ???[/home/wag/wg9s_64/libxul.so +0x4cc223f] #65: ???[/home/wag/wg9s_64/libxul.so +0x4c8fdc2] #66: ???[/home/wag/wg9s_64/libxul.so +0x4c93225] #67: ???[/home/wag/wg9s_64/libxul.so +0x4c8c981] #68: ???[/home/wag/wg9s_64/libxul.so +0x4cbe9dd] #69: ???[/home/wag/wg9s_64/libxul.so +0x4c8fdc2] #70: ???[/home/wag/wg9s_64/libxul.so +0x4c8c981] #71: ???[/home/wag/wg9s_64/libxul.so +0x4cc223f] #72: ???[/home/wag/wg9s_64/libxul.so +0x4c8fdc2] #73: ???[/home/wag/wg9s_64/libxul.so +0x4c8c981] #74: ???[/home/wag/wg9s_64/libxul.so +0x4c8f509] #75: ???[/home/wag/wg9s_64/libxul.so +0x4caff92] #76: ???[/home/wag/wg9s_64/libxul.so +0x4accaa1] #77: ???[/home/wag/wg9s_64/libxul.so +0x4bc0bab] #78: ???[/home/wag/wg9s_64/libxul.so +0x4a4a8ea] #79: ???[/home/wag/wg9s_64/libxul.so +0x4a4b308] #80: ???[/home/wag/wg9s_64/libxul.so +0x4a385f8] #81: ???[/home/wag/wg9s_64/libxul.so +0x4917487] #82: ???[/home/wag/wg9s_64/libxul.so +0x4919de0] #83: ???[/home/wag/wg9s_64/libxul.so +0x4919c02] #84: ???[/home/wag/wg9s_64/libxul.so +0x4919d01] #85: ???[/home/wag/wg9s_64/libxul.so +0x491ae87] #86: ???[/home/wag/wg9s_64/libxul.so +0x491aa82] #87: ???[/home/wag/wg9s_64/libxul.so +0x4926aaf] #88: ???[/home/wag/wg9s_64/libxul.so +0x49269c2] #89: ???[/home/wag/wg9s_64/libxul.so +0x4926863] #90: ???[/home/wag/wg9s_64/libxul.so +0xe04294] #91: NS_InvokeByIndex[/home/wag/wg9s_64/libxul.so +0xe2d68a] #92: ???[/home/wag/wg9s_64/libxul.so +0x1e1b395] #93: ???[/home/wag/wg9s_64/libxul.so +0x1e18fb5] #94: ???[/home/wag/wg9s_64/libxul.so +0x1dfe310] #95: ???[/home/wag/wg9s_64/libxul.so +0x1e06d2a] #96: ??? (???:???) This would appear to be from here: GtkStyleContext* CreateCSSNode(const char* aName, GtkStyleContext* aParentStyle, GType aType) { static auto sGtkWidgetPathIterSetObjectName = reinterpret_cast<void (*)(GtkWidgetPath *, gint, const char *)> (dlsym(RTLD_DEFAULT, "gtk_widget_path_iter_set_object_name")); GtkWidgetPath* path = aParentStyle ? gtk_widget_path_copy(gtk_style_context_get_path(aParentStyle)) : gtk_widget_path_new();

(In reply to Martin Stránský from comment #3) > <stransky> Company, Hi, is there a problem when we create the root style > from widget and then child CSS nodes by the path? In Gtk 3.20 > <Company> stransky: i wouldn't do that > <stransky> Company, for instance for scrollbar widgets. to get the root > style by gtk_widget_get_style_context(scrollbar widget) and then use path to > get CSS child styles > <Company> because then you have parts of the code that update based on the > widget tree and parts of the code that update based on your tree > <Company> and your tree changes the widget tree by hooking into it, too I'm not clear what is meant by this. I'm not aware of any CSS state that is affected by descendants, and so adding child style contexts will only inherit from the parent. The parent/widget context is not affected by having a child. There may be good reasons not to use widget style contexts, but I don't follow how this reasoning applies to Gecko. > <Company> i don't know off-hand of things that would go wrong > <Company> but i wouldn't be surprised by it There is a risk there, but there is proven cost in constructing styles from scratch. > <stransky> Company, okay, But it's okay to still get style from widgets if > we don't use the hierarchy? > <Company> stransky: why do you want to do that? btw? > <stransky> Company, to share code between pre/post 3.20 code in Firefox > <stransky> Company, because FF needs to keep compatibility from gtk 3.4 > <Company> hrm > <Company> i'm not sure sharing code there is that good of an idea > <stransky> Company, well, so do i need to create all styles from path for > gtk3.20? > <Company> GTK3.4 and 3.20 style machinery is almost as far apart internally > as GTK2 and GTK3 Yes, but the widget and the style context object are in common. > <Company> stransky: i would do that > <Company> stransky: you can get the widget path from a widget and create the > style from that > <stransky> and how? > <Company> gtk_widget_get_path() > <Company> and then create a style context with that path That raises some good options, thanks. It would pick up the object/node name automatically. It needs gtk_widget_get_style_context() beforehand for the side-effect of setting widget->priv->context for getting the classes in gtk_widget_path_append_for_widget(). That would also work for pre-3.20 GTK. I suspect it is simpler to just use gtk_widget_path_append_for_widget() than to create all ancestor widgets for the hierarchy for gtk_widget_get_path(). > <stransky> Company, so do you basically say that call > gtk_widget_get_style_context(widget) just for base styles is a bad idea on > 3.20? > <Company> stransky: I think mixing those style contexts with style contexts > created via paths is tricky > <stransky> Company, Ok, thanks. > <Company> stransky: both because the first kind get changed all the time That's why trying to construct all contexts from scratch would be a losing battle. Gecko needs these changes reflected in its style context. But gtk_widget_path_append_for_widget() should address that, it seems. > and because adding style contexts changes :first-child etc behaviors by > adding new nodes to the trtee I think this would actually work out OK unless there a subtlety that I'm missing here. Take, for example, the selector notebook tab:first-child label If the notebook context comes from a widget and the tab and label contexts are constructed from scratch and gtk_style_context_set_parent(), then the :first-child status is dependent only on the siblings specified when constructing the path for the tab context. For style contexts, :first-child is not determined by examining the child nodes of the notebook. (If it were, then Gecko would need to add child style contexts in order.) If notebook has other tab subnodes (or another widget has other subnodes), they would be affected by the new tab child, I assume, but Gecko doesn't use those subnodes. Still, I like constructing a style context from a path constructed from widgets, thanks, if it means not having to keep widgets alive with the style context.

Thanks, there's the indent fix attached.

(In reply to Bill Gianopoulos [:WG9s] from comment #7) > With both the patch for this bug and the patch for bug 1275407 applied, I > find the browser crashing often with this (or similar) stack: I don't see it but let's solve that in Bug 1275407.

Pushed by cbook@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/da10d713f676 exports CreateCSSNode() and divides pre/post 3.20 styles construction, r=karlt

Comment on attachment 8760661 [details] [diff] [review] patch v.3 patch for check-in Approval Request Comment [Feature/regressing bug #]: GTK3 port [User impact if declined]: Bug 1250704 (incorrect tooltip text color, unreadable with some themes) depends on this. [Describe test coverage new/current, TreeHerder]: none. [Risks and why]: This is a refactor, with no intended side effects and which has been on m-c since 2016-06-07. [String/UUID change made/needed]: none.

Marking other affected branches. Cornel, can your team verify the fix on aurora once this lands? Thanks!

Comment on attachment 8760661 [details] [diff] [review] patch v.3 patch for check-in Fix for GTK themes, so this has been an issue since 46. Let's uplift to aurora and make sure it works as intended.

Hello! We tried to test this issue (see Bug 1250704, comments 42-44), but unfortunately, we do not have available the required Linux distribution, so we asked the reporter Martin Giger [:freaktechnik] to verify this.