Clément Lefèvre Reporter
	Description • 8 years ago

As stated in the title, scrollbar background just changed to black on today's nightly instead of light grey before, making it a lot noticeable on the UI.

See joined screenshot for the example

	Clément Lefèvre Reporter
	Comment 1 • 8 years ago

Does not seem related to overlay scrollbars implementation of bug 1147847, but could be underlying work on this issue?

	Karl Tomlinson (:karlt)
	Comment 2 • 8 years ago

I'm guessing from the dark lines around the search box that this is GTK+ 3.20.
I don't know what caused the change.
The nightly from https://hg.mozilla.org/mozilla-central/rev/46fe2115d46a5bb40523b8466341d8f9a26e1bdf
didn't include the changes for bug 1272194.

	Martin Stránský [:stransky] (ni? me)
	Comment 3 • 8 years ago

A simple patch which fixes only a background of the scrollbar.

	Martin Stránský [:stransky] (ni? me)
	Comment 4 • 8 years ago

It fixes only the background but we also need to render the frame around scrollbar, which is defined by left/light/top/bottom classes of the main (root) css node.

	Martin Stránský [:stransky] (ni? me)
	Comment 5 • 8 years ago

AFAIK It's caused by missing "contents" child style in the scrollbar CSS node hierarchy (https://developer.gnome.org/gtk3/stable/GtkScrollbar.html)

	Martin Stránský [:stransky] (ni? me)
	Comment 6 • 8 years ago

I'm seeking feedback here because I'm not sure how gecko handles horizontal scrollbars. Are there any or can we drop them?

	Martin Stránský [:stransky] (ni? me)
	Comment 7 • 8 years ago

Comment on attachment 8756836 [details] [diff] [review]
patch

Just a note - the scrollbar background and border frame is really rendered from MOZ_GTK_SCROLLBAR_VERTICAL/HORIZONTAL nodes in gtk3.20.

	Karl Tomlinson (:karlt)
	Comment 8 • 8 years ago

Comment on attachment 8756836 [details] [diff] [review]
patch

Looks good, thanks.
Please continue to note which style nodes are >= GTK 3.20 only in
GetStyleInternal().

>+    if (gtk_check_version(3, 20, 0)) {

Please compare with nullptr, to hint that the return value is not the expected
boolean.  Or "gtk_get_minor_version() < 20" may be clearer, even.

>+    if (gtk_check_version(3, 20, 0)) {
>+        style = ClaimStyleContext(widget == MOZ_GTK_SCROLLBAR_HORIZONTAL ?
>+                                  MOZ_GTK_SCROLLBAR_TROUGH_HORIZONTAL :
>+                                  MOZ_GTK_SCROLLBAR_TROUGH_VERTICAL,
>+                                  direction);
>+     } else {
>+        style = ClaimStyleContext(widget, direction);

(In reply to Martin Stránský from comment #7)
> Just a note - the scrollbar background and border frame is really rendered
> from MOZ_GTK_SCROLLBAR_VERTICAL/HORIZONTAL nodes in gtk3.20.

The trough is also still drawn even in 3.20, as well as the contents gadget
AFAICS.  However, I don't know whether any themes style these, and so it is
fine to start with this if you like.

>+        style = ClaimStyleContext(widget, direction);
>+        if (widget == MOZ_GTK_SCROLLBAR_VERTICAL) {
>+            if (direction == GTK_TEXT_DIR_LTR) {
>+                gtk_style_context_add_class(style, "right");
>+            } else {
>+                gtk_style_context_add_class(style, "left");
>+            }
>+        } else {
>+            gtk_style_context_add_class(style, "bottom");
>+        }

I'm guessing the associated borders are going to be needed for layout
calculations at some stage, in which case these would be better handled
in WidgetStyleCache.  However, the code can start here, if that is easier.

Use GTK_STYLE_CLASS_RIGHT, etc.

_RIGHT will need to be removed if direction is ltr, and _LEFT for rtl.

(In reply to Martin Stránský from comment #6)
> I'm not sure how gecko handles horizontal
> scrollbars. Are there any or can we drop them?

If you make a Firefox window showing a bugzilla page narrow enough, a
horizontal scrollbar will be on the bottom.  I assume it is never on the top.

	Hussam Al-Tayeb
	Comment 9 • 8 years ago

(In reply to Martin Stránský from comment #6)
> Created attachment 8756836 [details] [diff] [review]
> patch
> 
> I'm seeking feedback here because I'm not sure how gecko handles horizontal
> scrollbars. Are there any or can we drop them?

This patch plus the one in bug 1274745 not only remove the black background but the scrollbar trough is also back as well which is great.
Only remaining issue I can see the scrollbar now is that the scrollbar handle is not colored correctly while dragged (bug 1261576).

	Martin Stránský [:stransky] (ni? me)
	Comment 10 • 8 years ago

Karl, there's a problem which we didn't see before. When the base class (GTK_STYLE_CLASS_SCROLLBAR in this case) is derived from widget and not constructed as a CSS path it behaves differently. 

When the scrollbar base class is derived from widget, we have to set state/direction explicitly (both of them) to the context to have correct rendering result. Otherwise the frame around scrollbar through is not rendered, only a background.

It's probably gtk3 bug but I don't expect gtk team will fix that (I'll ask). There may be other glitches and side effects like that when time comes and Gtk moves towards to path/css nodes.

I think we should consider to divide the code to pre and post 3.20 and use the style path as Gtk3 does in 3.20 even for the base widgets.

	Martin Stránský [:stransky] (ni? me)
	Comment 11 • 8 years ago

Hi, there's a patch I created after some thinking (as I wrote in comment 10). It solves various issues so I may divide it to different parts/bug but I'll introduce it here as a working example.

It divides style creation for pre and post 3.20. I think I was wrong when I asked you to have them in one switch, but two almost identical switches are also suboptimal. So I come with sStyleNameCache which allows to map WidgetNodeType to Gtk style class directly. 

sStyleNameCache allows to create pre-3.20 styles almost automatically (by CreateWidgetStyle) and also simplifies the post-3.20 code - GetChildNodeStyle can be reduced due to this change.

The base styles in 3.20 code are created by GetBaseNodeStyle() with a fallback we use before.

There's a new method moz_gtk_update_scrollbar_style() for now. I have a patch where GtkTextDirection is translated to GtkStateFlags (as GTK_STATE_FLAG_DIR_LTR/RTL) but I'd address that later - brings compatibility issues and needs to be passed to GetCssNodeStyleInternal/GetChildNodeStyle.

	Martin Stránský [:stransky] (ni? me)
	Comment 12 • 8 years ago

A second thought - when we do such reduction of GetChildNodeStyle, we can move the sStyleStorage[aStyleType] to upper level (to GetCssNodeStyleInternal) because GetCssNodeStyleInternal handles only 3.20 syles now.

It means we may remove GetChildNodeStyle/GetBaseNodeStyle and construct the styles in GetCssNodeStyleInternal switch more flexible.

	Karl Tomlinson (:karlt)
	Comment 13 • 8 years ago

(In reply to Martin Stránský from comment #10)
> Karl, there's a problem which we didn't see before. When the base class
> (GTK_STYLE_CLASS_SCROLLBAR in this case) is derived from widget and not
> constructed as a CSS path it behaves differently. 
> 
> When the scrollbar base class is derived from widget, we have to set
> state/direction explicitly (both of them) to the context to have correct
> rendering result. Otherwise the frame around scrollbar [trough] is not
> rendered, only a background.

Can you show me exactly what code you are using when you notice this, please?
i.e. does this occur with attachment 8756836 [details] [diff] [review]?
And can you show me the minimal code change that causes the frame to render?

> It's probably gtk3 bug but I don't expect gtk team will fix that (I'll ask).
> There may be other glitches and side effects like that when time comes and
> Gtk moves towards to path/css nodes.
> 
> I think we should consider to divide the code to pre and post 3.20 and use
> the style path as Gtk3 does in 3.20 even for the base widgets.

Constructing the style context from scratch may be best, but I don't know.

It would probably help to convince me that Gecko needs to construct the style
context itself if you can tell me why the context of the widget doesn't behave
as expected.  That would also help clarify whether this need be for all
widgets.

The behavior of these style contexts is a minefield of surprises.  If we make
changes without understanding why, then there is plenty of risk that the new
version won't behave as desired either.

If Gecko needs to construct the style contexts for post-3.20 widget, then I'd be
happy also doing that for pre-3.20 widgets.  Sometimes the pre-3.20 styles are similar except for the class/node change, but often enough they are not.  At least we know the pre-3.20 GTK code is not going to change.  (Picking up changes in GTK automatically was one of the main advantages of using the widget's style
context.)

If WidgetStyleCache is not going to use the styles from the widgets, then
gtk3drawing() should revert to getting style properties from the style context
so that widgets are not created unnecessarily (for duplicate style contexts).

Although much can be divided pre/post 3.20, I suspect there is still going to
be some things that can share code.  Exactly how that can best be done will
become more clear as more is implemented.  3.22 or 3.24 will probably
introduce more differences.  At this stage, I'm happy will separate functions
for the things that are different enough pre/post 3.20, but I prefer code to
be shared where practical.  The GetWidget()/gtk_widget_get_style_context()
fallback at least can be shared by moving it to the caller.

	Karl Tomlinson (:karlt)
	Comment 14 • 8 years ago

Comment on attachment 8758714 [details] [diff] [review]
patch v.2

(In reply to Martin Stránský from comment #11)
> It divides style creation for pre and post 3.20. I think I was wrong when I
> asked you to have them in one switch, but two almost identical switches are
> also suboptimal. So I come with sStyleNameCache which allows to map
> WidgetNodeType to Gtk style class directly. 
> 
> sStyleNameCache allows to create pre-3.20 styles almost automatically (by
> CreateWidgetStyle) and also simplifies the post-3.20 code -
> GetChildNodeStyle can be reduced due to this change.

I'm not fond of this kind of array use, sorry.  A static array of pointers
requires relocations and it is not easy to see that separate lists are in
consistent order.

I prefer having the class names inline with the function call from the case
label.

Perhaps there are macro tricks that can avoid duplication of repetitive code,
but as you pointed out that can make things harder to read, and I wonder
whether there are going to be enough special cases to mean this is not
worthwhile.

> 
> The base styles in 3.20 code are created by GetBaseNodeStyle() with a
> fallback we use before.

The pre 3.20 base nodes may be similar to share code.  The parents can be the
same at least, I assume.  Post 3.20 still needs to include parents up to
GtkWindow for inheritance at least.

There could be 3 switch statements: one where sharing for pre/post 3.20 is
suitable, and one for each of pre/post 3.20.

> There's a new method moz_gtk_update_scrollbar_style() for now.

Note that left/right/bottom classes are applied only on the base (scrollbar
widget) style context.

moz_gtk_draw_styled_frame() looks good, thanks.

> I have a patch where GtkTextDirection is translated to GtkStateFlags (as
> GTK_STATE_FLAG_DIR_LTR/RTL) but I'd address that later - brings
> compatibility issues and needs to be passed to
> GetCssNodeStyleInternal/GetChildNodeStyle.

Yes, best if that can be added separately, thanks.

	Karl Tomlinson (:karlt)
	Comment 15 • 8 years ago

(In reply to Martin Stránský from comment #12)
> A second thought - when we do such reduction of GetChildNodeStyle, we can
> move the sStyleStorage[aStyleType] to upper level (to
> GetCssNodeStyleInternal) because GetCssNodeStyleInternal handles only 3.20
> syles now.
>
> It means we may remove GetChildNodeStyle/GetBaseNodeStyle and construct the
> styles in GetCssNodeStyleInternal switch more flexible.

Sounds fine.

A potential advantage of constructing even pre-3.20 styles from scratch
might be that ClaimStyleContext would never need to save and
RestoreStyleContext would be unnecessary.  However, I'm not clear whether we
can imitate the hierarchy of save/restore accurately enough to match the same
selectors.

Still, it would be helpful to know what is going wrong with widget styles, so we know the right solution.

	Karl Tomlinson (:karlt)
	Comment 16 • 8 years ago

(In reply to Karl Tomlinson (back June 7 ni?:karlt) from comment #14)
> The pre 3.20 base nodes may be similar to share code.  The parents can be the
> same at least, I assume.  Post 3.20 still needs to include parents up to
> GtkWindow for inheritance at least.
> 
> There could be 3 switch statements: one where sharing for pre/post 3.20 is
> suitable, and one for each of pre/post 3.20.

OTOH even class names and node names are different enough concepts that maybe there's not much that can be shared and it is simpler just to have separate pre/post code.

This is just something to consider.  I think there a few options that would be fine.

	Hussam Al-Tayeb
	Comment 17 • 8 years ago

So this needs a new patch based on bug 1277818?

	Martin Stránský [:stransky] (ni? me)
	Comment 18 • 8 years ago

Thanks, there's an updated patch which depends on Bug 1277818.

	Bill Gianopoulos [:WG9s]
	Comment 20 • 8 years ago

With this patch included, the build is very crashy.  Almost everything that either displays a context window or a pop-up seems to crash almost every time.  Examples include:

1. Right clicking on an unused area of the tabbar, or an unused area of the url toolbar.
2. Right clicking on a link to display the context menu.
3. Clicking on a link that results in the do you want to open or save this file dialog window.

It could be this crashes when a window is created that has scrollbars disabled.  It only seems to crash in the Gtk 3.20 case.

	Martin Stránský [:stransky] (ni? me)
	Comment 21 • 8 years ago

There's a new one which uses CreateStyleForWidget() for the default styles. I can't reproduce the crashes mentioned before, on Fedora 24/gtk3-3.20.6.

	Bill Gianopoulos [:WG9s]
	Comment 22 • 8 years ago

(In reply to Martin Stránský from comment #21)
> Created attachment 8761180 [details] [diff] [review]
> patch v.4
> 
> There's a new one which uses CreateStyleForWidget() for the default styles.
> I can't reproduce the crashes mentioned before, on Fedora 24/gtk3-3.20.6.

Yes!  This seems much better!  I have not been able to get it to crash either.

	Bill Gianopoulos [:WG9s]
	Comment 23 • 8 years ago

That said, this has introduced a new regression.  Now the blue background on the hovered over menu items from context menus is not present so it switches the text to white and leaves the background white so the hovered over item from a context menu becomes invisible.

	Bill Gianopoulos [:WG9s]
	Comment 24 • 8 years ago

This also applies to Menu items selected from the MenuBar.

	Karl Tomlinson (:karlt)
	Comment 25 • 8 years ago

Comment on attachment 8761180 [details] [diff] [review]
patch v.4

>-      GtkWidget* widget = GetWidget(aNodeType);
>-      return gtk_widget_get_style_context(widget);
>+      style = CreateStyleForWidget(GetWidget(aNodeType), nullptr);
>+      break;

The lack of parent style here means the path doesn't contain the hierarchy.

I expect we should be able to get rid of most of the cached widgets in this file and construct the hierarchy from the style contexts.  That same code can be used regardless of GTK version.

I suggest returning the widget style for now, and the border issue can be addressed separately.

Sorry, I haven't reviewed the rest of the patch, yet.

	Karl Tomlinson (:karlt)
	Comment 26 • 8 years ago

Comment on attachment 8760268 [details] [diff] [review]
patch v.3

>+      GtkWidgetPath *path = gtk_widget_get_path(widget);
>+      style = gtk_style_context_new();
>+      gtk_style_context_set_path(style, path);
>+      gtk_widget_path_unref(path);

The crash was due to the path_unref, I assume.
gtk_widget_get_path(widget) leaves ownership with the widget.

gtk_widget_get_path() gets the hierarchy from the widget hierarchy, but the style context still needs a parent for inheritance.  I think CreateStyleForWidget() will do the right thing once the parent is specified.

	Martin Stránský [:stransky] (ni? me)
	Comment 27 • 8 years ago

Also I think we may not export MOZ_GTK_SCROLLBAR_CONTENTS_* but construct the TROUGH style directly as:

style = CreateChildCSSNode("contents", MOZ_GTK_SCROLLBAR_*);
style = CreateCSSNode(GTK_STYLE_CLASS_TROUGH, style);

	Bill Gianopoulos [:WG9s]
	Comment 28 • 8 years ago

(In reply to Karl Tomlinson (ni?:karlt) from comment #26)
> Comment on attachment 8760268 [details] [diff] [review]
> patch v.3
> 
> >+      GtkWidgetPath *path = gtk_widget_get_path(widget);
> >+      style = gtk_style_context_new();
> >+      gtk_style_context_set_path(style, path);
> >+      gtk_widget_path_unref(path);
> 
> The crash was due to the path_unref, I assume.
> gtk_widget_get_path(widget) leaves ownership with the widget.
> 
> gtk_widget_get_path() gets the hierarchy from the widget hierarchy, but the
> style context still needs a parent for inheritance.  I think
> CreateStyleForWidget() will do the right thing once the parent is specified.

You are correct.  I reverted to the version 3 patch and commented out the unref and that resulted in no crashes and without the menu choice selection regression.

	Bill Gianopoulos [:WG9s]
	Comment 30 • 8 years ago

(In reply to Bill Gianopoulos [:WG9s] from comment #28)
> (In reply to Karl Tomlinson (ni?:karlt) from comment #26)
> > Comment on attachment 8760268 [details] [diff] [review]
> > patch v.3
> > 
> > >+      GtkWidgetPath *path = gtk_widget_get_path(widget);
> > >+      style = gtk_style_context_new();
> > >+      gtk_style_context_set_path(style, path);
> > >+      gtk_widget_path_unref(path);
> > 
> > The crash was due to the path_unref, I assume.
> > gtk_widget_get_path(widget) leaves ownership with the widget.
> > 
> > gtk_widget_get_path() gets the hierarchy from the widget hierarchy, but the
> > style context still needs a parent for inheritance.  I think
> > CreateStyleForWidget() will do the right thing once the parent is specified.
> 
> You are correct.  I reverted to the version 3 patch and commented out the
> unref and that resulted in no crashes and without the menu choice selection
> regression.

I just realized this also fixes the issue in bug 1276534, albeit for the post 3.20 case only.  The issue still remains under gtk 3.18.

	Karl Tomlinson (:karlt)
	Comment 31 • 8 years ago

Comment on attachment 8761180 [details] [diff] [review]
patch v.4

Most of this is good, thanks.

>-      GtkWidget* widget = GetWidget(aNodeType);
>-      return gtk_widget_get_style_context(widget);
>+      style = CreateStyleForWidget(GetWidget(aNodeType), nullptr);
>+      break;

Please leave GetWidget()/gtk_widget_get_style_context() for now, as changing
inheritance or node hierarchy risks introducing problems for other nodes.

>+static void
>+moz_gtk_update_scrollbar_style(GtkStyleContext* style,
>+                               bool aIsHorizontal,
>+                               GtkTextDirection direction)

Please make the second parameter of type WidgetNodeType, and pass
MOZ_GTK_SCROLLBAR_HORIZONTAL or _VERTICAL, so that it is clear what the
argument means at the call site.

>+        style = ClaimStyleContext(widget, direction);
>+        moz_gtk_update_scrollbar_style(style, isHorizontal, direction);
>+        moz_gtk_draw_styled_frame(style, cr, rect, state->focused);
>+        ReleaseStyleContext(style);
>+
>+        style = ClaimStyleContext(isHorizontal ?
>+                                  MOZ_GTK_SCROLLBAR_CONTENTS_HORIZONTAL :
>+                                  MOZ_GTK_SCROLLBAR_CONTENTS_VERTICAL,
>+                                  direction);
>+        moz_gtk_draw_styled_frame(style, cr, rect, state->focused);
>+        moz_gtk_update_scrollbar_style(style, isHorizontal, direction);
>+        ReleaseStyleContext(style);
>     }
>+    style = ClaimStyleContext(isHorizontal ?
>+                              MOZ_GTK_SCROLLBAR_TROUGH_HORIZONTAL :
>+                              MOZ_GTK_SCROLLBAR_TROUGH_VERTICAL,
>+                              direction);
>+    moz_gtk_update_scrollbar_style(style, isHorizontal, direction);

left/right/bottom classes are applied only on the base (scrollbar widget)
style context, and so call moz_gtk_update_scrollbar_style() only for the first
style.

(In reply to Martin Stránský from comment #27)
> Also I think we may not export MOZ_GTK_SCROLLBAR_CONTENTS_* but construct
> the TROUGH style directly as:
> 
> style = CreateChildCSSNode("contents", MOZ_GTK_SCROLLBAR_*);
> style = CreateCSSNode(GTK_STYLE_CLASS_TROUGH, style);

The latest patch is drawing the contents box which is consistent with what GTK
does, and so I would leave MOZ_GTK_SCROLLBAR_CONTENTS_* as is.

	Martin Stránský [:stransky] (ni? me)
	Comment 32 • 8 years ago

Thanks, I hope this it one solves that

	Martin Stránský [:stransky] (ni? me)
	Comment 33 • 8 years ago

Ahh, sorry, this one is correct.

	Martin Stránský [:stransky] (ni? me)
	Comment 34 • 8 years ago

Comment on attachment 8762493 [details] [diff] [review]
patch v.6

Try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=b198329e511e

	Pulsebot
	Comment 35 • 8 years ago

Pushed by cbook@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/da4ab896002a
Adds CSS contents node to have correct scrollbar background on Gtk >= 3.20. r=karlt

	Carsten Book [:Tomcat]
	Comment 36 • 8 years ago
bugherder

https://hg.mozilla.org/mozilla-central/rev/da4ab896002a

	Bill Gianopoulos [:WG9s]
	Comment 37 • 8 years ago

Good timing on this.  It appears that all the fedora24 blocker bugs are closed, so it is highly likely that fedora24 with Gtk 3.20.6 will be released next Tuesday.

	Shmerl
	Comment 38 • 8 years ago

When will this arrive in the release channel? Mozilla build of Firefox has been broken in Debian testing for a while when used with gnome-breeze theme in KDE, since Debian testing switched to GTK 3.20 and this bug is a major usability issue. Will it be possible to backport this fix in the upcoming release?

	Karl Tomlinson (:karlt)
	Comment 39 • 8 years ago

(In reply to Shmerl from comment #38)
> When will this arrive in the release channel? Mozilla build of Firefox has
> been broken in Debian testing for a while when used with gnome-breeze theme
> in KDE, since Debian testing switched to GTK 3.20 and this bug is a major
> usability issue.

I can't imagine the background color being a major usability issue, and so I'm guessing your main issue is bug 1230955, which was fixed in bug 234158.  That is on Firefox 48 beta right now.

GTK 3.20 is at a similar stage of development to Firefox 48 beta, so if Debian testing is not going to reconsider the switch to 3.20, then I'd recommend using Firefox 48 beta.

> Will it be possible to backport this fix in the upcoming release?

I don't think that will be an option for this patch.