1289148 - [GTK 3.20] scrollbar metric obsolete for GTK >= 3.20

Status: RESOLVED FIXED

(Core :: Widget: Gtk, defect, P3)

Description

[Jan Horak [:jhorak]]

According to some scrollbars rendering and GTK documentation [1] the scrollbar metrics are no longer valid for GTK >= 3.20. It is using standard box model for thumb, trough and scrollbar.

[1] https://developer.gnome.org/gtk3/unstable/GtkRange.html#GtkRange--s-slider-width and more

Downstream bug: https://bugzilla.redhat.com/show_bug.cgi?id=1321553

Comment 1

[Jan Horak [:jhorak]]

Attachment: scrollbars-fix-3.20.patch

Attached patch which is trying to fix scrollbar metrics for GTK >= 3.20.

Could you please give me some feedback if that's the right direction?

I've tested the changes with Gtk 3.20 and some custom theme. There's still problems with stepper buttons appearance which I'd like to address in separate bug.

Comment 2

[Karl Tomlinson (:karlt)]

Comment on attachment 8776571 [details] [diff] [review]
scrollbars-fix-3.20.patch

This looks like the right kind of approach, thanks.

I would prefer that moz_gtk_get_widget_border() were used instead of
AddBorderAndMargin() so as to keep the code for similar computations together.

Similarly, I would like GetGtkWidgetAndState() used instead of adding another
switch statement in NativeThemeToGtkTheme().  NativeThemeToGtkTheme() could
use GetGtkWidgetAndState() if you like.

># Bug 1289148 - Fixing scrollbar metrics for GTK >= 3.20

Please describe what is different about the new code for GTK 3.20.

Is this the format from hg, or is this hand edited?
Checkin comments don't usually begin with '#' AFAIK.

I wonder whether this is why the commit message from
https://bug1205643.bmoattachments.org/attachment.cgi?id=8705003
is not in https://hg.mozilla.org/mozilla-central/rev/a151ddf25e53

>+    case MOZ_GTK_SCROLLBAR_BUTTON:
>+      style = CreateChildCSSNode(GTK_STYLE_CLASS_BUTTON,
>+                                 MOZ_GTK_SCROLLBAR_CONTENTS_VERTICAL);
>+      break;

Will need a pre-3.20 equivalent?

>+static void
>+moz_gtk_subtract_margin(GtkStyleContext* style, GtkWidgetState* state, GdkRectangle* rect)
>+{
>+    MOZ_ASSERT(rect);
>+    GtkBorder margin;
>+
>+    GtkStateFlags state_flags = GetStateFlagsFromGtkWidgetState(state);
>+    gtk_style_context_get_margin(style, state_flags, &margin);

I expect the state_flags here will be the same as used elsewhere.
That would avoid invalidating GTK's style resolution cache, and I don't think
there is a need to use different flags.
gtk_style_context_get_state(style) should work.

>     /* Scrollbar button has to be inset by trough_border because its DOM element
>      * is filling width of vertical scrollbar's track (or height in case
>      * of horizontal scrollbars). */

>+    if (!gtk_check_version(3,20,0)) {
>+      // Scrollbar metrics is obsolete since Gtk >= 3.20
>+      moz_gtk_subtract_margin(style, state, rect);
>     } else {
>+      MozGtkScrollbarMetrics metrics;
>+      moz_gtk_get_scrollbar_metrics(&metrics);
>+      if (flags & MOZ_GTK_STEPPER_VERTICAL) {
>+        rect->x += metrics.trough_border;
>+        rect->width = metrics.slider_width;
>+      } else {
>+        rect->y += metrics.trough_border;
>+        rect->height = metrics.slider_width;
>+      }

The comment about insetting because the DOM element fills the scrollbar
doesn't apply with GTK 3.20 because its stepper buttons are always children of
the "contents" gadget.  The insetting code is there because this code is
assuming the trough_under_steppers layout where the steppers are inside the
trough.  I think this is worth explaining, and please leave the existing
comment beside the code it describes.

>@@ -990,16 +1010,17 @@ moz_gtk_scrollbar_trough_paint(WidgetNod

>     bool isHorizontal = (widget == MOZ_GTK_SCROLLBAR_HORIZONTAL);

This needs attention.

>     GtkStyleContext* style;
> 
>     // Draw all child CSS Nodes for Gtk >= 3.20
>     if (gtk_check_version(3, 20, 0) == nullptr) {
>         style = ClaimStyleContext(widget, direction);
>+        moz_gtk_subtract_margin(style, state, rect);
>         moz_gtk_update_scrollbar_style(style, widget, direction);
>         moz_gtk_draw_styled_frame(style, cr, rect, state->focused);

Check this is included in the border.
Can draw_styled_frame handle the margin?

This is what moz_gtk_scrollbar_paint() is drawing, I assume.

>         style = ClaimStyleContext(isHorizontal ?
>                                   MOZ_GTK_SCROLLBAR_CONTENTS_HORIZONTAL :
>                                   MOZ_GTK_SCROLLBAR_CONTENTS_VERTICAL,
>                                   direction);

This should also move to moz_gtk_scrollbar_paint() as steppers are children of
the contents widget.

>+moz_gtk_scrollbar_paint(WidgetNodeType widget,
>+                        cairo_t *cr, GdkRectangle* rect,
>+                        GtkWidgetState* state,
>+                        GtkTextDirection direction)
>+{
>+    GtkStyleContext* style =
>+        ClaimStyleContext(widget == MOZ_GTK_SCROLLBAR_HORIZONTAL ?
>+                          MOZ_GTK_SCROLLBAR_HORIZONTAL :
>+                          MOZ_GTK_SCROLLBAR_VERTICAL,
>+                          direction);

?: doesn't look right.

>+    moz_gtk_subtract_margin(style, state, rect);
>+    gtk_render_background(style, cr, rect->x, rect->y, rect->width, rect->height);
>+    gtk_render_frame(style, cr, rect->x, rect->y, rect->width, rect->height);

moz_gtk_draw_styled_frame()

>+    // For Gtk >= 3.20 scrollbar metrics are obsolete

s/obsolete/ignored/ to be more specific.

>@@ -1376,16 +1456,33 @@ nsNativeThemeGTK::GetWidgetPadding(nsDev

>+    case NS_THEME_SCROLLBAR_VERTICAL:
>+    case NS_THEME_SCROLLBAR_HORIZONTAL:
>+    case NS_THEME_SCROLLBARTRACK_VERTICAL:
>+    case NS_THEME_SCROLLBARTRACK_HORIZONTAL:
>+    case NS_THEME_SCROLLBARTHUMB_VERTICAL:
>+    case NS_THEME_SCROLLBARTHUMB_HORIZONTAL:
>+      if (!gtk_check_version(3,20,0)) {
>+        GtkStyleContext* style = ClaimStyleContext(NativeThemeToGtkTheme(aWidgetType));
>+        GtkBorder padding;
>+        gtk_style_context_get_padding (style, gtk_style_context_get_state(style), &padding);
>+        ReleaseStyleContext(style);
>+        aResult->top = padding.top;
>+        aResult->bottom = padding.bottom;
>+        aResult->left = padding.left;
>+        aResult->right = padding.right;
>+        return true;
>+      }

This would mean different interactions with document padding.  I assume there
is no reason why this should behave differently with different GTK versions,
and so please make this a separate change if required.  Padding can be kept
with the border for now.

>+          gtk_style_context_get(style, gtk_style_context_get_state(style),
>+                                "min-height", &(aResult->height),
>+                                "min-width", &(aResult->width),
>+                                nullptr);
>+          GtkBorder border, margin, padding;
>+          gtk_style_context_get_border (style, gtk_style_context_get_state(style), &border);
>+          gtk_style_context_get_padding (style, gtk_style_context_get_state(style), &padding);
>+          gtk_style_context_get_margin (style, gtk_style_context_get_state(style), &margin);

>+          aResult->width += border.left + border.right + margin.left + margin.right + padding.left + padding.right;
>+          aResult->height += border.top + border.bottom + margin.top + margin.bottom + padding.top + padding.bottom;

This pattern repeats.  I assume a helper method would tidy this up.

Comment 3

[Jan Horak [:jhorak]]

Attachment: scrollbars-fix-3.20 v2

Thanks for the feedback. The changes what I made is mostly mentioned at the beginning of the patch. I also need to get system with pre-gtk 3.20 and test the patch with it. Meanwhile please have a look.


> Is this the format from hg, or is this hand edited?
> Checkin comments don't usually begin with '#' AFAIK.
Ouch, yes, hand edited. Fixed.

> >+    case MOZ_GTK_SCROLLBAR_BUTTON:
> >+      style = CreateChildCSSNode(GTK_STYLE_CLASS_BUTTON,
> >+                                 MOZ_GTK_SCROLLBAR_CONTENTS_VERTICAL);
> >+      break;
> 
> Will need a pre-3.20 equivalent?
Not required ATM. We'll most likely need it for proper rendering of scrollbar buttons which I'd like to fix in another bug.

> >     GtkStyleContext* style;
> > 
> >     // Draw all child CSS Nodes for Gtk >= 3.20
> >     if (gtk_check_version(3, 20, 0) == nullptr) {
> >         style = ClaimStyleContext(widget, direction);
> >+        moz_gtk_subtract_margin(style, state, rect);
> >         moz_gtk_update_scrollbar_style(style, widget, direction);
> >         moz_gtk_draw_styled_frame(style, cr, rect, state->focused);
> 
> Check this is included in the border.
> Can draw_styled_frame handle the margin?
What do you mean by handle? To subtract it automatically for scrollbar's elements only?

> This would mean different interactions with document padding.  I assume there
> is no reason why this should behave differently with different GTK versions,
> and so please make this a separate change if required.  Padding can be kept
> with the border for now.
I've transferred padding to the moz_gtk_get_widget_border.

Comment 4

[Karl Tomlinson (:karlt)]

Comment on attachment 8778854 [details] [diff] [review]
scrollbars-fix-3.20 v2

(In reply to Jan Horak from comment #3)
> > Can draw_styled_frame handle the margin?
> What do you mean by handle? To subtract it automatically for scrollbar's
> elements only?

I wonder whether it would be helpful if moz_gtk_draw_styled_frame() called
moz_gtk_subtract_margin().

>+moz_gtk_subtract_margin(GtkStyleContext* style, GtkWidgetState* state, GdkRectangle* rect)

|state| is unused.

>+    if (!gtk_check_version(3,20,0)) {
>+      // Scrollbar metrics is ignored since Gtk >= 3.20

Please be more specific, because "scrollbar metrics" is not a GTK concept.
Something like '"trough-border" is not used since GTK 3.20.  The stepper margin
box occupies the full width of the "contents" gadget content box.'

"GTK" is an acronym and so is all upper case, except when used to identify GTK
types or symbols.

>+      moz_gtk_subtract_margin(style, state, rect);
>+    } else {
>     /* Scrollbar button has to be inset by trough_border because its DOM element
>      * is filling width of vertical scrollbar's track (or height in case
>      * of horizontal scrollbars). */

>+      MozGtkScrollbarMetrics metrics;
>+      moz_gtk_get_scrollbar_metrics(&metrics);
>+      if (flags & MOZ_GTK_STEPPER_VERTICAL) {
>+        rect->x += metrics.trough_border;
>+        rect->width = metrics.slider_width;
>+      } else {
>+        rect->y += metrics.trough_border;
>+        rect->height = metrics.slider_width;
>+      }
>     }

Please indent the comment with the code it describes.

Here and elsewhere there is risk that modifying the caller's rect could cause
some surprises.
The function parameter would preferably be const GdkRectangle*.
Can the function instead modify a copy of the callers argument please?
Similarly elsewhere.
In Gecko, that would typically be done by naming the parameter |aRect| and
using that to initialize a local variable |rect|.

>@@ -984,60 +1003,60 @@ moz_gtk_scrollbar_trough_paint(WidgetNod

>-        style = ClaimStyleContext(isHorizontal ?
>-                                  MOZ_GTK_SCROLLBAR_CONTENTS_HORIZONTAL :
>-                                  MOZ_GTK_SCROLLBAR_CONTENTS_VERTICAL,
>-                                  direction);
>-        moz_gtk_draw_styled_frame(style, cr, rect, state->focused);
>-        ReleaseStyleContext(style);

Need to move this to moz_gtk_scrollbar_paint().

>     if (gtk_check_version(3, 20, 0) == nullptr) {
>+        moz_gtk_subtract_margin(style, state, rect);
>     }

In GTK+ 3.18 also at least, the trough (and slider) margins are subtracted
before drawing (but not for size request).

> moz_gtk_get_scrollbar_metrics(MozGtkScrollbarMetrics *metrics)
> {
>+    // For Gtk >= 3.20 scrollbar metrics are ignored

Please assert minor version < 20.

>+    case MOZ_GTK_SCROLLBAR_VERTICAL:
>+    case MOZ_GTK_SCROLLBAR_TROUGH_HORIZONTAL:
>+        {
>+          if (!gtk_check_version(3,20,0)) {

== nullptr, and elsewhere.

>+            style = ClaimStyleContext(widget);
>+            moz_gtk_add_style_margin(style, left, top, right, bottom);
>+            moz_gtk_add_style_border(style, left, top, right, bottom);
>+            moz_gtk_add_style_padding(style, left, top, right, bottom);
>+            ReleaseStyleContext(style);

Include the contents gadget effects for SCROLLBAR_VERTICAL/HORIZONTAL.

>     case MOZ_GTK_SCROLLBAR_HORIZONTAL:
>     case MOZ_GTK_SCROLLBAR_VERTICAL:

>+        if (!gtk_check_version(3,20,0)) {
>+          return moz_gtk_scrollbar_paint(widget, cr, rect, state, direction);
>+        } else {
>+          return moz_gtk_scrollbar_trough_paint(widget, cr, rect,
>+                                                state,
>+                                                (GtkScrollbarTrackFlags) flags,
>+                                                direction);

Need to pass the trough widget enum value to _trough_paint now.

>+WidgetNodeType
>+nsNativeThemeGTK::NativeThemeToGtkTheme(uint8_t aWidgetType, nsIFrame* aFrame)
>+{
>+  WidgetNodeType gtkWidgetType;
>+  gint unusedFlags;
>+
>+  if (!GetGtkWidgetAndState(aWidgetType, aFrame, gtkWidgetType, nullptr,
>+                            &unusedFlags))
>+  {
>+    return MOZ_GTK_WINDOW;

Please assert this is not reached.

>+static void
>+GetWidgetMinDimentions(WidgetNodeType aGtkWidgetType, LayoutDeviceIntSize* aResult)
>+{
>+  GtkStyleContext* style = ClaimStyleContext(aGtkWidgetType);
>+  gtk_style_context_get(style, gtk_style_context_get_state(style),
>+                        "min-height", &(aResult->height),
>+                        "min-width", &(aResult->width),
>+                        nullptr);
>+  GtkBorder border, padding, margin;
>+  gtk_style_context_get_border (style, gtk_style_context_get_state(style), &border);
>+  gtk_style_context_get_padding (style, gtk_style_context_get_state(style), &padding);
>+  gtk_style_context_get_margin (style, gtk_style_context_get_state(style), &margin);
>+  ReleaseStyleContext(style);
>+  aResult->width += border.left + border.right + margin.left + margin.right + padding.left + padding.right;
>+  aResult->height += border.top + border.bottom + margin.top + margin.bottom + padding.top + padding.bottom;
>+}

Spelling is "Dimensions".

The border/padding/margin pattern repeats.
I suspect putting this in gtk3drawing will permit sharing code.
moz_gtk_get_scale_metrics() and moz_gtk_get_entry_min_height() are also
related, so it would be good to keep these in a single file, to hopefully
share code one day.

Some adjustments are required for the GTK2 code.

Comment 5

[Jan Horak [:jhorak]]

Attachment: scrollbars-fix-3.20 v3

Thanks for the feedback, I've made following changes:
* Added changes to gtk2
* Modifying copy of rect instead its instance
* Tested with GTK 3.16, looks good so far
* Added asserts
* Fixed moz_gtk_scrollbar_trough_paint
* GetWidgetMinDimentions moved to gtk2/3drawing.c as moz_gtk_get_widget_min_size

> > Can draw_styled_frame handle the margin?
> > What do you mean by handle? To subtract it automatically for scrollbar's
> > elements only?

> I wonder whether it would be helpful if moz_gtk_draw_styled_frame() called
> moz_gtk_subtract_margin().
Moved there, that would work if we want to use moz_gtk_draw_styled_frame just for scrollbar and related stuff.

> >     if (gtk_check_version(3, 20, 0) == nullptr) {
> >+        moz_gtk_subtract_margin(style, state, rect);
> >     }

> In GTK+ 3.18 also at least, the trough (and slider) margins are subtracted
> before drawing (but not for size request).
Hm, not sure what you mean, could you please elaborate?

Comment 6

[Karl Tomlinson (:karlt)]

Attachment: Bug 1289148 - Fixing scrollbar metrics for GTK >= 3.20

The Gtk 3.20 scrollbars has moved towards usual box model. The scrollbar,
trough,thumb and scrollbar button can now have margin, padding and border set,
different for each direction (ie. left, right, bottom, top). The scrollbar
metrics become ignored in Gtk 3.20 and later.

For 3.20 we've made following changes:
* We've started to drawing scrollbar element
* The border for scrollbar, trough, thumb and scrollbar buttons is newly
  calculated as margin+padding+border.
* The margin is subtracted for scrollbar, trough, thumb and sb buttons during
  paint function
* We've also transfered all scrollbar widget's borders from
  nsNativeThemeGTK::GetWidgetBorder to the moz_gtk_get_widget_border.
* Added helper fuction NativeThemeToGtkTheme for mapping mozilla's widget type
  to the gtk widget type.

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

Comment 7

[Karl Tomlinson (:karlt)]

Comment on attachment 8780965 [details]
Bug 1289148 - Fixing scrollbar metrics for GTK >= 3.20

https://reviewboard.mozilla.org/r/71476/#review68992

v2

Comment 8

[Karl Tomlinson (:karlt)]

Comment on attachment 8780965 [details]
Bug 1289148 - Fixing scrollbar metrics for GTK >= 3.20

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/71476/diff/1-2/

Comment 9

[Karl Tomlinson (:karlt)]

Comment on attachment 8780965 [details]
Bug 1289148 - Fixing scrollbar metrics for GTK >= 3.20

https://reviewboard.mozilla.org/r/71476/#review68996

::: widget/gtk/gtk3drawing.cpp:652
(Diff revisions 1 - 2)
> +moz_gtk_get_widget_min_size(WidgetNodeType aGtkWidgetType, int* width, int* height) {
> +  GtkStyleContext* style = ClaimStyleContext(aGtkWidgetType);
> +  gtk_style_context_get(style, gtk_style_context_get_state(style),
> +                        "min-height", height,
> +                        "min-width", width,
> +                        nullptr);
> +  GtkBorder border, padding, margin;
> +  gtk_style_context_get_border (style, gtk_style_context_get_state(style), &border);
> +  gtk_style_context_get_padding (style, gtk_style_context_get_state(style), &padding);
> +  gtk_style_context_get_margin (style, gtk_style_context_get_state(style), &margin);
> +  ReleaseStyleContext(style);
> +  *width += border.left + border.right + margin.left + margin.right + padding.left + padding.right;
> +  *height += border.top + border.bottom + margin.top + margin.bottom + padding.top + padding.bottom;

Wrap lines to keep within 80 columns.
No space before '('.

::: widget/gtk/gtk3drawing.cpp:797
(Diff revisions 1 - 2)
> -    gtk_render_frame(style, cr, rect->x, rect->y, rect->width, rect->height);
> +    if (gtk_check_version(3, 20, 0) == nullptr) {
> +        moz_gtk_subtract_margin(style, &rect);
> +    }

(In reply to Jan Horak from comment #5)
> > In GTK+ 3.18 also at least, the trough (and slider) margins are subtracted
> > before drawing (but not for size request).
> Hm, not sure what you mean, could you please elaborate?

Subtracting the margin is good, but the lack of this was a bug that existed before the GTK 3.20 regressions.

Instead of testing the minor version against 20, please find the appropriate minor version where this change happened in GTK.  It is somewhere between 4 and 16.

::: widget/gtk/gtk3drawing.cpp:2457
(Diff revisions 1 - 2)
>              style = ClaimStyleContext(widget);
>              moz_gtk_add_style_margin(style, left, top, right, bottom);
>              moz_gtk_add_style_border(style, left, top, right, bottom);
>              moz_gtk_add_style_padding(style, left, top, right, bottom);
>              ReleaseStyleContext(style);

This pattern repeats, so please use a helper function to share the code.

::: widget/gtk/nsNativeThemeGTK.cpp:1442
(Diff revisions 1 - 2)
>  
>    switch (aWidgetType) {
>      case NS_THEME_SCROLLBARBUTTON_UP:
>      case NS_THEME_SCROLLBARBUTTON_DOWN:
>        {
> +        if (!gtk_check_version(3,20,0)) {

== nullptr

::: widget/gtk/nsNativeThemeGTK.cpp:1443
(Diff revisions 1 - 2)
> +          moz_gtk_get_widget_min_size(MOZ_GTK_SCROLLBAR_BUTTON,
> +              &(aResult->width), &(aResult->height));

Looks like there is enough horizontal space here to align & with M.

Similarly below.

::: widget/gtk/nsNativeThemeGTK.cpp:1459
(Diff revisions 1 - 2)
>        }
>        break;
>      case NS_THEME_SCROLLBARBUTTON_LEFT:
>      case NS_THEME_SCROLLBARBUTTON_RIGHT:
>        {
> +        if (!gtk_check_version(3,20,0)) {

== nullptr

::: widget/gtk/gtk3drawing.cpp:825
(Diff revision 2)
>      GtkStyleContext* style;
> -
> -    // Draw all child CSS Nodes for Gtk >= 3.20
>      if (gtk_check_version(3, 20, 0) == nullptr) {
> -        style = ClaimStyleContext(widget, direction);
> +      style = ClaimStyleContext(widget, direction);
> -        moz_gtk_update_scrollbar_style(style, widget, direction);
> +      moz_gtk_update_scrollbar_style(style, widget, direction);

These styles should apply to the scrollbar widget.
I suspect it is sufficient to set them in moz_gtk_scrollbar_paint().

::: widget/gtk/gtk3drawing.cpp:2834
(Diff revision 2)
>      case MOZ_GTK_SCROLLBAR_HORIZONTAL:
>      case MOZ_GTK_SCROLLBAR_VERTICAL:
> +        if (gtk_check_version(3,20,0) == nullptr) {
> +          return moz_gtk_scrollbar_paint(widget, cr, rect, state, direction);
> +        } else {
> -        return moz_gtk_scrollbar_trough_paint(widget, cr, rect,
> +          return moz_gtk_scrollbar_trough_paint(widget, cr, rect,

Please pass _TROUGH_HORIZONTAL/VERTICAL here to _trough_paint() so that _trough_paint() needs only one ClaimStyleContext() call and readers of that function don't need to understand the pre/post 3.20 difference.

::: widget/gtk/nsNativeThemeGTK.cpp:1490
(Diff revision 2)
> +      if (!gtk_check_version(3,20,0)) {
> +          moz_gtk_get_widget_min_size(NativeThemeToGtkTheme(aWidgetType, aFrame),
> +              &(aResult->width), &(aResult->height));
> +      } else {
> -      /* While we enforce a minimum size for the thumb, this is ignored
> +        /* While we enforce a minimum size for the thumb, this is ignored
> -       * for the some scrollbars if buttons are hidden (bug 513006) because
> +         * for the some scrollbars if buttons are hidden (bug 513006) because
> -       * the thumb isn't a direct child of the scrollbar, unlike the buttons
> +         * the thumb isn't a direct child of the scrollbar, unlike the buttons
> -       * or track. So add a minimum size to the track as well to prevent a
> +         * or track. So add a minimum size to the track as well to prevent a
> -       * 0-width scrollbar. */
> +         * 0-width scrollbar. */

Assuming the comment here is still true, the slider and trough border still need to be considered in determining the minimum size with 3.20.

Adwaita, for example, doesn't set a minimum size on the scrollbar AFAICS.

Comment 10

[Karl Tomlinson (:karlt)]

Comment on attachment 8780479 [details] [diff] [review]
scrollbars-fix-3.20 v3

Moved to reviewboard/mozreview.

Comment 11

[Jan Horak [:jhorak]]

(In reply to Karl Tomlinson (:karlt) from comment #9)
> Comment on attachment 8780965 [details]
> Bug 1289148 - Fixing scrollbar metrics for GTK >= 3.20
> 
> https://reviewboard.mozilla.org/r/71476/#review68996
> 
> ::: widget/gtk/gtk3drawing.cpp:652
> (Diff revisions 1 - 2)
> > +moz_gtk_get_widget_min_size(WidgetNodeType aGtkWidgetType, int* width, int* height) {
> > +  GtkStyleContext* style = ClaimStyleContext(aGtkWidgetType);
> > +  gtk_style_context_get(style, gtk_style_context_get_state(style),
> > +                        "min-height", height,
> > +                        "min-width", width,
> > +                        nullptr);
> > +  GtkBorder border, padding, margin;
> > +  gtk_style_context_get_border (style, gtk_style_context_get_state(style), &border);
> > +  gtk_style_context_get_padding (style, gtk_style_context_get_state(style), &padding);
> > +  gtk_style_context_get_margin (style, gtk_style_context_get_state(style), &margin);
> > +  ReleaseStyleContext(style);
> > +  *width += border.left + border.right + margin.left + margin.right + padding.left + padding.right;
> > +  *height += border.top + border.bottom + margin.top + margin.bottom + padding.top + padding.bottom;
> 
> Wrap lines to keep within 80 columns.
> No space before '('.
> 
> ::: widget/gtk/gtk3drawing.cpp:797
> (Diff revisions 1 - 2)
> > -    gtk_render_frame(style, cr, rect->x, rect->y, rect->width, rect->height);
> > +    if (gtk_check_version(3, 20, 0) == nullptr) {
> > +        moz_gtk_subtract_margin(style, &rect);
> > +    }
> 
> (In reply to Jan Horak from comment #5)
> > > In GTK+ 3.18 also at least, the trough (and slider) margins are subtracted
> > > before drawing (but not for size request).
> > Hm, not sure what you mean, could you please elaborate?
> 
> Subtracting the margin is good, but the lack of this was a bug that existed
> before the GTK 3.20 regressions.
> 
> Instead of testing the minor version against 20, please find the appropriate
> minor version where this change happened in GTK.  It is somewhere between 4
> and 16.
> 
> ::: widget/gtk/gtk3drawing.cpp:2457
> (Diff revisions 1 - 2)
> >              style = ClaimStyleContext(widget);
> >              moz_gtk_add_style_margin(style, left, top, right, bottom);
> >              moz_gtk_add_style_border(style, left, top, right, bottom);
> >              moz_gtk_add_style_padding(style, left, top, right, bottom);
> >              ReleaseStyleContext(style);
> 
> This pattern repeats, so please use a helper function to share the code.
> 
> ::: widget/gtk/nsNativeThemeGTK.cpp:1442
> (Diff revisions 1 - 2)
> >  
> >    switch (aWidgetType) {
> >      case NS_THEME_SCROLLBARBUTTON_UP:
> >      case NS_THEME_SCROLLBARBUTTON_DOWN:
> >        {
> > +        if (!gtk_check_version(3,20,0)) {
> 
> == nullptr
> 
> ::: widget/gtk/nsNativeThemeGTK.cpp:1443
> (Diff revisions 1 - 2)
> > +          moz_gtk_get_widget_min_size(MOZ_GTK_SCROLLBAR_BUTTON,
> > +              &(aResult->width), &(aResult->height));
> 
> Looks like there is enough horizontal space here to align & with M.
> 
> Similarly below.
> 
> ::: widget/gtk/nsNativeThemeGTK.cpp:1459
> (Diff revisions 1 - 2)
> >        }
> >        break;
> >      case NS_THEME_SCROLLBARBUTTON_LEFT:
> >      case NS_THEME_SCROLLBARBUTTON_RIGHT:
> >        {
> > +        if (!gtk_check_version(3,20,0)) {
> 
> == nullptr
> 
> ::: widget/gtk/gtk3drawing.cpp:825
> (Diff revision 2)
> >      GtkStyleContext* style;
> > -
> > -    // Draw all child CSS Nodes for Gtk >= 3.20
> >      if (gtk_check_version(3, 20, 0) == nullptr) {
> > -        style = ClaimStyleContext(widget, direction);
> > +      style = ClaimStyleContext(widget, direction);
> > -        moz_gtk_update_scrollbar_style(style, widget, direction);
> > +      moz_gtk_update_scrollbar_style(style, widget, direction);
> 
> These styles should apply to the scrollbar widget.
> I suspect it is sufficient to set them in moz_gtk_scrollbar_paint().
> 
> ::: widget/gtk/gtk3drawing.cpp:2834
> (Diff revision 2)
> >      case MOZ_GTK_SCROLLBAR_HORIZONTAL:
> >      case MOZ_GTK_SCROLLBAR_VERTICAL:
> > +        if (gtk_check_version(3,20,0) == nullptr) {
> > +          return moz_gtk_scrollbar_paint(widget, cr, rect, state, direction);
> > +        } else {
> > -        return moz_gtk_scrollbar_trough_paint(widget, cr, rect,
> > +          return moz_gtk_scrollbar_trough_paint(widget, cr, rect,
> 
> Please pass _TROUGH_HORIZONTAL/VERTICAL here to _trough_paint() so that
> _trough_paint() needs only one ClaimStyleContext() call and readers of that
> function don't need to understand the pre/post 3.20 difference.
> 
> ::: widget/gtk/nsNativeThemeGTK.cpp:1490
> (Diff revision 2)
> > +      if (!gtk_check_version(3,20,0)) {
> > +          moz_gtk_get_widget_min_size(NativeThemeToGtkTheme(aWidgetType, aFrame),
> > +              &(aResult->width), &(aResult->height));
> > +      } else {
> > -      /* While we enforce a minimum size for the thumb, this is ignored
> > +        /* While we enforce a minimum size for the thumb, this is ignored
> > -       * for the some scrollbars if buttons are hidden (bug 513006) because
> > +         * for the some scrollbars if buttons are hidden (bug 513006) because
> > -       * the thumb isn't a direct child of the scrollbar, unlike the buttons
> > +         * the thumb isn't a direct child of the scrollbar, unlike the buttons
> > -       * or track. So add a minimum size to the track as well to prevent a
> > +         * or track. So add a minimum size to the track as well to prevent a
> > -       * 0-width scrollbar. */
> > +         * 0-width scrollbar. */
> 
> Assuming the comment here is still true, the slider and trough border still
> need to be considered in determining the minimum size with 3.20.
> 
> Adwaita, for example, doesn't set a minimum size on the scrollbar AFAICS.

The min-width/min-height for thumb seems to be enough for gtk 3.20 to prevent zero width scrollbar with or without buttons (at least by my testing). Or do you see another problem here? Do you suspect that minimum widget size of thumb does not bubble up to scrollbar? I didn't hit it, how could that happen? Is it on Gecko or GTK site?

Comment 12

[Karl Tomlinson (:karlt)]

(In reply to Jan Horak from comment #11)
> (In reply to Karl Tomlinson (:karlt) from comment #9)
> > ::: widget/gtk/nsNativeThemeGTK.cpp:1490
> > (Diff revision 2)
> > > +      if (!gtk_check_version(3,20,0)) {
> > > +          moz_gtk_get_widget_min_size(NativeThemeToGtkTheme(aWidgetType, aFrame),
> > > +              &(aResult->width), &(aResult->height));
> > > +      } else {
> > > -      /* While we enforce a minimum size for the thumb, this is ignored
> > > +        /* While we enforce a minimum size for the thumb, this is ignored
> > > -       * for the some scrollbars if buttons are hidden (bug 513006) because
> > > +         * for the some scrollbars if buttons are hidden (bug 513006) because
> > > -       * the thumb isn't a direct child of the scrollbar, unlike the buttons
> > > +         * the thumb isn't a direct child of the scrollbar, unlike the buttons
> > > -       * or track. So add a minimum size to the track as well to prevent a
> > > +         * or track. So add a minimum size to the track as well to prevent a
> > > -       * 0-width scrollbar. */
> > > +         * 0-width scrollbar. */
> > 
> > Assuming the comment here is still true, the slider and trough border still
> > need to be considered in determining the minimum size with 3.20.
> > 
> > Adwaita, for example, doesn't set a minimum size on the scrollbar AFAICS.
> 
> The min-width/min-height for thumb seems to be enough for gtk 3.20 to
> prevent zero width scrollbar with or without buttons (at least by my
> testing). Or do you see another problem here? Do you suspect that minimum
> widget size of thumb does not bubble up to scrollbar? I didn't hit it, how
> could that happen? Is it on Gecko or GTK site?

The comment was claiming that the thumb size did not bubble up in Gecko
layout.  If the comment is incorrect, and, it may be, then there is no need to
do anything further here.

Comment 13

[Karl Tomlinson (:karlt)]

Comment on attachment 8780965 [details]
Bug 1289148 - Fixing scrollbar metrics for GTK >= 3.20

https://reviewboard.mozilla.org/r/71476/#review68996

> Assuming the comment here is still true, the slider and trough border still need to be considered in determining the minimum size with 3.20.
> 
> Adwaita, for example, doesn't set a minimum size on the scrollbar AFAICS.

If that is not necessary, and a theme may not explicitly set a minimum size, then *aIsOverridable should not be reset to false, because height and width should be > 0.
It is possible that layout is ignoring this boolean value, but still better to return the correct state.

Comment 14

[Jan Horak [:jhorak]]

Attachment: Bug 1289148 - Fixing scrollbar metrics for GTK >= 3.20 v2

Thanks you've looked into it. Attaching fixed patch. I've also noticed that I've misplaced "*aIsOverridable = false" for NS_THEME_SCROLLBARBUTTON_LEFT:

About margin subtraction, good point, I've found out that it has been introduced in GTK 3.6.

Comment 15

[Karl Tomlinson (:karlt)]

(In reply to Jan Horak from comment #11)
> (In reply to Karl Tomlinson (:karlt) from comment #9)
> > ::: widget/gtk/nsNativeThemeGTK.cpp:1490
> > (Diff revision 2)
> > > +      if (!gtk_check_version(3,20,0)) {
> > > +          moz_gtk_get_widget_min_size(NativeThemeToGtkTheme(aWidgetType, aFrame),
> > > +              &(aResult->width), &(aResult->height));
> > > +      } else {
> > > -      /* While we enforce a minimum size for the thumb, this is ignored
> > > +        /* While we enforce a minimum size for the thumb, this is ignored
> > > -       * for the some scrollbars if buttons are hidden (bug 513006) because
> > > +         * for the some scrollbars if buttons are hidden (bug 513006) because
> > > -       * the thumb isn't a direct child of the scrollbar, unlike the buttons
> > > +         * the thumb isn't a direct child of the scrollbar, unlike the buttons
> > > -       * or track. So add a minimum size to the track as well to prevent a
> > > +         * or track. So add a minimum size to the track as well to prevent a
> > > -       * 0-width scrollbar. */
> > > +         * 0-width scrollbar. */
> > 
> > Assuming the comment here is still true, the slider and trough border still
> > need to be considered in determining the minimum size with 3.20.
> > 
> > Adwaita, for example, doesn't set a minimum size on the scrollbar AFAICS.
> 
> The min-width/min-height for thumb seems to be enough for gtk 3.20 to
> prevent zero width scrollbar with or without buttons (at least by my
> testing). Or do you see another problem here? Do you suspect that minimum
> widget size of thumb does not bubble up to scrollbar? I didn't hit it, how
> could that happen? Is it on Gecko or GTK site?

This comment, and the dimensions on the minor axis were added for bug 513006.
As you say, I can't find a situation where the scrollbars disappear when this
is removed.

The dimensions on the major axis were added for bug 1171696.

These apply in a situation such as
data:text/html,<div style="width:30px;background:green;overflow:auto"><div style="width:900px">A

When the width of the outer div is too small to fit the scrollbar thumb or
buttons, the scrollbar is hidden because it can't be used.
This is consistent with the behaviour on winnt when the buttons will not fit.
In 2008 this behaviour did not exist but roc thought it might be
preferable.  I don't know whether it was ever implemented on Mac or not.
https://bugzilla.mozilla.org/show_bug.cgi?id=416752#c32

These currently do not work quite right in the case of a textarea because
space for the resizer is apparently not considered.
data:text/html,<textarea style="width:50px">Hello World
can be resized to demonstrate this.

Bug 726258 argues that scrollbars should still be shown when there is not
enough room because they provide an indication that there is hidden content.
Once the user is aware of the hidden content, they can use other means to
scroll the content.  In 2012, scrollbars were no longer hidden when there was
not enough room on the minor axis, but the same change was not made for the
major axis.

Chromium does not hide the scrollbar when there is not enough room for the
buttons.

My opinion is that displaying a track even when there is not enough room for a
thumb is useful, even though it may be confusing because clicking on the track
has no effect.  Therefore, I don't think the thumb need be considered in the
scrollbar or track minimum size.  If it did, then I think it is the role of
layout code to consider the minimum size of the thumb.

nsGfxScrollFrame::TryLayout() uses, via GetScrollbarMetrics(),
GetXULMinSize() for the major axis and GetXULPrefSize() for the minor axis.
http://searchfox.org/mozilla-central/rev/1cac8d1e6d7e0f62967b197e5dbe390b988a6969/layout/generic/nsGfxScrollFrame.cpp#368

nsScrollbarFrame inherits GetXULMinSize() from nsBoxFrame.  If width or height
is not set to a non-zero value in nsIFrame::AddXULMinSize(), then
nsSprocketLayout::GetXULMinSize() considers child min sizes.

nsSliderFrame::GetXULMinSize(), for the track child, skips
nsBoxFrame::GetXULMinSize() and uses just nsBox::GetXULMinSize(), which does
not consider the thumb child.
https://dxr.mozilla.org/mozilla-central/rev/3ba5426a03b495b6417fffb872d42874edb80855/layout/xul/nsSliderFrame.cpp#1349

nsSliderFrame::GetXULPrefSize() uses nsBoxFrame::GetXULPrefSize(), which
considers children.

nsButtonBoxFrame, for the thumb child of the track, inherits
nsBoxFrame::GetXULPrefSize() which considers GetXULMinSize(), which is
inherited from nsBoxFrame.

Comment 16

[Karl Tomlinson (:karlt)]

Comment on attachment 8780965 [details]
Bug 1289148 - Fixing scrollbar metrics for GTK >= 3.20

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/71476/diff/2-3/

Comment 17

[Karl Tomlinson (:karlt)]

Comment on attachment 8784387 [details] [diff] [review]
Bug 1289148 - Fixing scrollbar metrics for GTK >= 3.20 v2

Moved to mozreview.

Comment 18

[Karl Tomlinson (:karlt)]

Comment on attachment 8780965 [details]
Bug 1289148 - Fixing scrollbar metrics for GTK >= 3.20

https://reviewboard.mozilla.org/r/71476/#review75286

::: widget/gtk/gtk3drawing.cpp:683
(Diff revisions 2 - 3)
>  {
>      MOZ_ASSERT(rect);
>      GtkBorder margin;
>  
> -    gtk_style_context_get_margin(style, gtk_style_context_get_state(style), &margin);
> +    gtk_style_context_get_margin(style, gtk_style_context_get_state(style),
> +        &margin);

Align |&margin| with |style|

::: widget/gtk/gtk3drawing.cpp:2239
(Diff revisions 2 - 3)
>      *right += padding.right;
>      *top += padding.top;
>      *bottom += padding.bottom;
>  }
>  
> +static void moz_gtk_add_style_whole_box_model(GtkStyleContext *style,

This function is hard to name.
I suggest add_all_frame_widths().

::: widget/gtk/gtk3drawing.cpp:660
(Diff revision 3)
> +  GtkBorder border, padding, margin;
> +  gtk_style_context_get_border(style, gtk_style_context_get_state(style),
> +                               &border);
> +  gtk_style_context_get_padding(style, gtk_style_context_get_state(style),
> +                               &padding);
> +  gtk_style_context_get_margin(style, gtk_style_context_get_state(style),
> +                               &margin);

Either save the result of gtk_style_context_get_state() so that it is called only once or use add_all_frame_widths() here.

::: widget/gtk/gtkdrawing.h:517
(Diff revision 3)
>  
> +/**
> + * Get minimum widget size as sum of margin, padding, border and min-width,
> + * min-height.
> + */
> +void moz_gtk_get_widget_min_size(WidgetNodeType aGtkWidgetType, int* width, int* height);

80 columns.

Comment 19

[Karl Tomlinson (:karlt)]

Comment on attachment 8780965 [details]
Bug 1289148 - Fixing scrollbar metrics for GTK >= 3.20

https://reviewboard.mozilla.org/r/71476/#review75286

> This function is hard to name.
> I suggest add_all_frame_widths().

Actually, add_margin_border_padding() is clearer and not much longer.

Comment 20

[Karl Tomlinson (:karlt)]

Comment on attachment 8780965 [details]
Bug 1289148 - Fixing scrollbar metrics for GTK >= 3.20

https://reviewboard.mozilla.org/r/71476/#review75310

Thanks for touching this up.  It is almost there.

> For 3.20 we've made following changes:

Many of these changes or not 3.20 specific, so please adjust the checkin
comment to separate 3.20-specific changes from general changes.

> * The margin is subtracted for scrollbar, trough, thumb and sb buttons during
>   paint function

The change for trough is not just 3.20.

The margin was already subtracted for the thumb in the existing code.

Comment 21

[Karl Tomlinson (:karlt)]

Please also mention in the checkin comment the change in behavior where 3.20 scrollbar troughs are now drawn even when there is not enough room for the thumb.

Comment 22

[Karl Tomlinson (:karlt)]

Comment on attachment 8780965 [details]
Bug 1289148 - Fixing scrollbar metrics for GTK >= 3.20

https://reviewboard.mozilla.org/r/71476/#review68996

> These styles should apply to the scrollbar widget.
> I suspect it is sufficient to set them in moz_gtk_scrollbar_paint().

I see this has been added to scrollbar_paint(), thanks, but please remove this call for the trough.

> Please pass _TROUGH_HORIZONTAL/VERTICAL here to _trough_paint() so that _trough_paint() needs only one ClaimStyleContext() call and readers of that function don't need to understand the pre/post 3.20 difference.

Thanks for the change to the call site.
Please adjust moz_gtk_scrollbar_trough_paint() for the change.

Comment 23

[Jan Horak [:jhorak]]

Attachment: scrollbars-fix-3.20 v4

Sorry for later reply, I was busy with something else. Attaching hopefully fixed patch.

Comment 24

[Karl Tomlinson (:karlt)]

Comment on attachment 8780965 [details]
Bug 1289148 - Fixing scrollbar metrics for GTK >= 3.20

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/71476/diff/3-4/

Comment 25

[Karl Tomlinson (:karlt)]

Comment on attachment 8791532 [details] [diff] [review]
scrollbars-fix-3.20 v4

Moved to mozreview revision 4

Comment 26

[Karl Tomlinson (:karlt)]

Comment on attachment 8780965 [details]
Bug 1289148 - Fixing scrollbar metrics for GTK >= 3.20

https://reviewboard.mozilla.org/r/71476/#review84926

All good.  Thank you, Jan!

Comment 27

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/013a4088a230
Fixing scrollbar metrics for GTK >= 3.20 r=karlt

Comment 28

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/013a4088a230