1454897 - Honour Ambiance scrollbar theme

Status: RESOLVED FIXED

(Core :: Widget: Gtk, enhancement)

Description

[Martin Stránský [:stransky] (ni? me)]

Bug 1355143 fixes scrollbar track size. We should also honor Ambiance scrollbar theme and resize scrollbar thumb when mouse is/isn't over it.

Comment 1

[Martin Stránský [:stransky] (ni? me)]

Attachment: Bug 1454897 - Ubuntu/Ambiance - Render scrollbar thumb with different sizes in active/normal state,

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

Comment 2

[Jan Horak [:jhorak]]

Comment on attachment 8968860 [details]
Bug 1454897 - Ubuntu/Ambiance - Render scrollbar thumb with different sizes in active/normal state,

https://reviewboard.mozilla.org/r/237584/#review243376

::: widget/gtk/gtk3drawing.cpp:1026
(Diff revision 1)
> -    gtk_render_slider(style, cr,
> -                      rect.x,
> -                      rect.y,
> -                      rect.width,
> -                      rect.height,
> -                     (widget == MOZ_GTK_SCROLLBAR_THUMB_HORIZONTAL) ?
> +    GtkOrientation orientation = (widget == MOZ_GTK_SCROLLBAR_THUMB_HORIZONTAL) ?
> +        GTK_ORIENTATION_HORIZONTAL : GTK_ORIENTATION_VERTICAL;
> +
> +    if (!(state_flags & (GTK_STATE_FLAG_ACTIVE|GTK_STATE_FLAG_PRELIGHT))) {
> +        const ScrollbarGTKMetrics* metrics = GetScrollbarMetrics(orientation);
> +        const ScrollbarGTKMetrics* metricsSelected =

The 'selected' is probably not correct word there, because you can't select thumb element. Use 'metricsActive' or 'metricsPrelight'

::: widget/gtk/gtk3drawing.cpp:1029
(Diff revision 1)
> +        if (metrics->size.thumb < metricsSelected->size.thumb) {
> +            GtkBorder marginDiff =
> +                metrics->margin.scrollbar - metricsSelected->margin.scrollbar;
> +            Inset(&rect, marginDiff);

Instead of applying InsetByMargin on line 1019 and calculating difference between metrics and matricsSelected and doing Inset again, couldn't you just get margin from the correct metrics (normal/active) and apply it?

This also remove need for the operator- which is not very common in mozilla source base.

::: widget/gtk/gtk3drawing.cpp:2992
(Diff revision 1)
>      }
>  
>      // GTK version > 3.20
>      // scrollbar
>      metrics->border.scrollbar = GetMarginBorderPadding(style);
> +    metrics->margin.scrollbar = GetMargin(style);

Just use gtk_style_context_get_margin, there's no need to wrap it in GetMargin().

Comment 3

[Martin Stránský [:stransky] (ni? me)]

Comment on attachment 8968860 [details]
Bug 1454897 - Ubuntu/Ambiance - Render scrollbar thumb with different sizes in active/normal state,

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

Comment 4

[Martin Stránský [:stransky] (ni? me)]

Comment on attachment 8968860 [details]
Bug 1454897 - Ubuntu/Ambiance - Render scrollbar thumb with different sizes in active/normal state,

https://reviewboard.mozilla.org/r/237584/#review243376

> Instead of applying InsetByMargin on line 1019 and calculating difference between metrics and matricsSelected and doing Inset again, couldn't you just get margin from the correct metrics (normal/active) and apply it?
> 
> This also remove need for the operator- which is not very common in mozilla source base.

I can't do that as the first margin is for "Thumb" and the secont is margin difference between active/inactive scrollbar. But I can polish that somehow.

Comment 5

[Martin Stránský [:stransky] (ni? me)]

Comment on attachment 8968860 [details]
Bug 1454897 - Ubuntu/Ambiance - Render scrollbar thumb with different sizes in active/normal state,

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

Comment 6

[Martin Stránský [:stransky] (ni? me)]

Comment on attachment 8968860 [details]
Bug 1454897 - Ubuntu/Ambiance - Render scrollbar thumb with different sizes in active/normal state,

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

Comment 7

[Jan Horak [:jhorak]]

Comment on attachment 8968860 [details]
Bug 1454897 - Ubuntu/Ambiance - Render scrollbar thumb with different sizes in active/normal state,

https://reviewboard.mozilla.org/r/237584/#review244570

Please also rerun the try.

::: widget/gtk/gtk3drawing.cpp:44
(Diff revision 4)
>  #endif
>  
> +static GtkBorder
> +operator-(const GtkBorder& first, const GtkBorder& second)
> +{
> +    GtkBorder tmp;

better name result than tmp.

::: widget/gtk/gtk3drawing.cpp:3086
(Diff revision 4)
> +          metrics->margin.thumb += metrics->margin.scrollbar -
> +                                   metricsActive->margin.scrollbar;

Can't you use border (ie. margin+border+padding) instead of just margin there?
(metricsActive->border.scrollbar + metricsActive->border.track) -  (metrics->border.scrollbar + metrics->border.track)

In case the theme changes padding instead of margin.

Comment 8

[Martin Stránský [:stransky] (ni? me)]

Comment on attachment 8968860 [details]
Bug 1454897 - Ubuntu/Ambiance - Render scrollbar thumb with different sizes in active/normal state,

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

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/05f610551597
Ubuntu/Ambiance - Render scrollbar thumb with different sizes in active/normal state, r=jhorak

Comment 10

[Cristian Brindusan [:cbrindusan]]

https://hg.mozilla.org/mozilla-central/rev/05f610551597