1464723 - Implement scrollbar color and width properties for GTK widget

Status: RESOLVED FIXED

(Core :: Widget: Gtk, enhancement)

Comment 1

[Xidorn Quan [:xidorn] UTC+10]

Attachment: Bug 1464723 part 1 - Add scrollbar colors support to GTK widget.

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

Comment 2

[Xidorn Quan [:xidorn] UTC+10]

Attachment: Bug 1464723 part 2 - Add root attribute for scrollcorner.

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

Comment 3

[Xidorn Quan [:xidorn] UTC+10]

Attachment: Bug 1464723 part 3 - Have GTK widget support scrollcorner widget.

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

Comment 4

[Daniel Holbert [:dholbert]]

Comment on attachment 8994810 [details]
Bug 1464723 part 1 - Add scrollbar colors support to GTK widget.

https://reviewboard.mozilla.org/r/259350/#review266446

Drive-by nit on the first patch:

::: widget/gtk/nsNativeThemeGTK.cpp:1103
(Diff revision 1)
> +struct GtkStyleProviderHolder
> +{
> +  // A pointer to ComputedStyle. Made opaque so that we don't accidently
> +  // access it, since it may be dangling.
> +  void* mStyle;
> +  GtkStyleProvider* mProvider;

typo: s/accidently/accidentally/

Also, perhaps it'd be worth adding a hint in this comment about why we bother to track this pointer at all? (given that we never expect to dereference it and we're aware that it might be dangling)

(It looks like we use it as a key for invalidation checking.  Does this usage account for the possibility that it might've been deleted and then another ComputedStyle object could've been allocated in the same memory location?  Does that possibility foil our pointer comparison logic in some edge cases?)

Comment 5

[Daniel Holbert [:dholbert]]

Comment on attachment 8994811 [details]
Bug 1464723 part 2 - Add root attribute for scrollcorner.

https://reviewboard.mozilla.org/r/259352/#review266450

This patch seems probably fine, but I don't see any explanation for what this added |root="true"| actually does.

Would you mind adding a comment to the extended commit message and/or in the code, to explain the implications/purpose of setting this attribute?

Comment 6

[Xidorn Quan [:xidorn] UTC+10]

Comment on attachment 8994810 [details]
Bug 1464723 part 1 - Add scrollbar colors support to GTK widget.

https://reviewboard.mozilla.org/r/259350/#review266446

> typo: s/accidently/accidentally/
> 
> Also, perhaps it'd be worth adding a hint in this comment about why we bother to track this pointer at all? (given that we never expect to dereference it and we're aware that it might be dangling)
> 
> (It looks like we use it as a key for invalidation checking.  Does this usage account for the possibility that it might've been deleted and then another ComputedStyle object could've been allocated in the same memory location?  Does that possibility foil our pointer comparison logic in some edge cases?)

That is pretty unlikely unless we do more than one style flush between the paints. The style system always need to keep the old computed style for comparison, so old and new would coexist for a single style flush.

But maybe it's better to use colors themselves as the invalidation key...

Comment 7

[Xidorn Quan [:xidorn] UTC+10]

Comment on attachment 8994810 [details]
Bug 1464723 part 1 - Add scrollbar colors support to GTK widget.

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

Comment 8

[Xidorn Quan [:xidorn] UTC+10]

Comment on attachment 8994811 [details]
Bug 1464723 part 2 - Add root attribute for scrollcorner.

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

Comment 9

[Xidorn Quan [:xidorn] UTC+10]

Comment on attachment 8994812 [details]
Bug 1464723 part 3 - Have GTK widget support scrollcorner widget.

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

Comment 10

[Xidorn Quan [:xidorn] UTC+10]

Comment on attachment 8994810 [details]
Bug 1464723 part 1 - Add scrollbar colors support to GTK widget.

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

Comment 11

[Xidorn Quan [:xidorn] UTC+10]

Comment on attachment 8994811 [details]
Bug 1464723 part 2 - Add root attribute for scrollcorner.

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

Comment 12

[Xidorn Quan [:xidorn] UTC+10]

Comment on attachment 8994812 [details]
Bug 1464723 part 3 - Have GTK widget support scrollcorner widget.

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

Comment 13

[Daniel Holbert [:dholbert]]

Comment on attachment 8994811 [details]
Bug 1464723 part 2 - Add root attribute for scrollcorner.

https://reviewboard.mozilla.org/r/259352/#review266774

::: commit-message-49f73:3
(Diff revision 3)
> +This attribute is added for the next commit that scrollcorner can be
> +made transparent for non-top level scrollbars on GTK like scrollbar
> +tracks.

I think there are a few words missing here.

Maybe the first line wants to say:
 "...for use in the next commit, so that..."

(adding "use" & ", so")

Comment 14

[Karl Tomlinson (:karlt)]

Comment on attachment 8994812 [details]
Bug 1464723 part 3 - Have GTK widget support scrollcorner widget.

https://reviewboard.mozilla.org/r/259354/#review266558

I like the C++ parts of this patch, thanks.

::: commit-message-c80e0:1
(Diff revision 3)
> +Bug 1464723 part 3 - Have GTK widget support scrollcorner widget. r?karlt
> +

Please highlight here the reason for adding this.
It is not really drawing a GTK scrollcorner and so the comment is misleading.

I assume it is to get a transparent corner (where appropriate)?
If so, that sounds good.

What was the behavior when ThemeSupportsWidget() returned false?

::: commit-message-c80e0:3
(Diff revision 3)
> +mix-blend-mode-parent-element-overflow-scroll.html is already broken if
> +the scrollbars are transparent. This patch makes the scrollcorner trans-

How do you know it is the test that is broken, not the browser, with transparent scrollbars?

::: testing/web-platform/meta/css/css-writing-modes/scrollbar-vertical-rl-ref.html.ini:1
(Diff revision 3)
> +[scrollbar-vertical-rl.html]
> +  expected: FAIL

I assume this patch does not make this test fail on *all* platforms.

::: testing/web-platform/tests/css/compositing/mix-blend-mode/reference/mix-blend-mode-parent-element-overflow-scroll-ref.html:11
(Diff revision 3)
>          <link rel="author" title="Mirela Budăeș" href="mailto:[e-mail]">
>          <link rel="author" title="Ion Roșca" href="mailto:[e-mail]">
>          <link rel="reviewer" title="Mihai Țică" href="mailto:[e-mail]">
>          <style type="text/css">
>              .parent {
> +                background: yellow;

wpt test changes should be in a separate patch and commit message because these will be upstreamed.  Someone familiar with what is being tested should review this.

::: widget/gtk/nsNativeThemeGTK.cpp:436
(Diff revision 3)
>          *aWidgetFlags = CheckBooleanAttr(aFrame, nsGkAtoms::parentfocused);
>        }
>      }
>    }
>  
> +  auto maySetOpaqueFlag = [this, aFrame, aWidgetType, aWidgetFlags]() {

This needs to (and does) initialize even when there is no opaque flag, and so
I think this would be better called something like "InitFlagsForOpacity".

Comment 15

[Karl Tomlinson (:karlt)]

Comment on attachment 8994810 [details]
Bug 1464723 part 1 - Add scrollbar colors support to GTK widget.

https://reviewboard.mozilla.org/r/259350/#review266836

This is a valiant effort, but I strongly recommend against attempting
to inject document-specified colors into the theme's CSS.  The API is not
designed for this, and so this is unlikely to work well.  It is like
setting an unrelated background color on any element of a web page.
Anything that does have a chance of working well will be complicated and slow,
and this is not worth our time.

I wondered about instead taking the alpha channel from the native drawing and
using that with the document-specified color.  (Ideally the lightness channel
would also be taken from native drawing to provide shape, but that's rather
expensive and merging with document lightness is tricky.)  That might work
OKish for opaque document colors with track and thumb, but would not work well
in general for buttons, and there is a question of how to merge document alpha
with native translucency.

Once native colors are not used, scrollbars are not native, and so there is
little value in using native drawing.  If native scrollbars with native colors
are not suitable, then I think what would work best would be to have a
non-native design that is known to work well (including providing mouseover and
click feedback) regardless of document colors.  Having ThemeSupportsWidget()
return false with a solution to bug 1469741 may be the best way to do that.

The remaining lifetime of native drawing in content processes is expected to
be limited anyway due to bug 1470983 and other reasons:
https://docs.google.com/document/d/1eKlYHx2X8aqkcxl6hbQcKUvg3rhASRlKnj_o5HFjZ5Q/edit#

::: commit-message-99e87:2
(Diff revision 3)
> +Bug 1464723 part 1 - Add scrollbar colors support to GTK widget. r?karlt
> +

I don't see anything implementing "If a platform has other parts which need
different colors, we would derive such colors from the given one based on the
native scheme and constrast", for the buttons.

::: widget/gtk/gtk3drawing.cpp:972
(Diff revision 3)
> +    AutoAddGtkStyleProvider(GtkStyleContext* aContext,
> +                            GtkStyleProvider* aProvider)
> +        : mContext(aContext)
> +        , mProvider(aProvider)
> +    {
> +        const auto PRIORITY = GTK_STYLE_PROVIDER_PRIORITY_APPLICATION;
> +        if (mProvider) {

Adding and removing the style provider slows things down because it invalidates
style caches.

::: widget/gtk/nsNativeThemeGTK.cpp:1144
(Diff revision 3)
> +      // By default, CSS node scrollbar, contents, and trough in GTK+
> +      // are transparent, and we don't want to change that. However,

I'm not clear what "By default" means but the scrollbar node in the default
theme is not transparent due to this in Adwaita/_colors.scss:

$scrollbar_bg_color: if($variant == 'light', mix($bg_color, $fg_color, 80%), mix($base_color, $bg_color, 50%));

However, the default theme is largely irrelevant, because each distribution
has its own theme.

Keeping the native color of the trough means that a document-colored slider
may not be visible on the trough.

But changing the trough color through injecting CSS is going to be problematic
for all sorts of reasons.  e.g. it may intentionally be transparent.

::: widget/gtk/nsNativeThemeGTK.cpp:1153
(Diff revision 3)
> +      // used below. See moz_gtk_widget_paint.
> +      // This is fine as this style is only used by scrollbar currently.
> +      // It may need to be changed if we reuse this mechanism in the
> +      // future for something else.
> +      data.AppendPrintf(
> +        "window { background: rgba(%u,%u,%u,%.2f); }",

Drawing a document color behind the scrollbar may be OK sometimes, but I doubt
it will generalize well.  Perhaps scrollbars with transparent portions are
designed to be drawn over any color, but the transparent portions are intended
to show the background.  It is odd to draw the document-specified Track color
where the background is intended and then draw the native trough on top of
that.  Even if drawing a background is desirable, don't use GTK to fill areas
with document-specified colors.  That will just be slow.

::: widget/gtk/nsNativeThemeGTK.cpp:1160
(Diff revision 3)
> +        (1.f / 255) * NS_GET_A(color));
> +    }
> +    if (!ui->mScrollbarFaceColor.IsAuto()) {
> +      nscolor color = ui->mScrollbarFaceColor.CalcColor(style);
> +      data.AppendPrintf(
> +        "scrollbar slider { background: rgba(%u,%u,%u,%.2f); }",

Many themes use background to draw the slider, but some themes use other
features such as border, background-image, and/or box-shadow.  Throwing a
random background color into the mix is unlikely to work well.

::: widget/gtk/nsNativeThemeGTK.cpp:1160
(Diff revision 3)
> +        (1.f / 255) * NS_GET_A(color));
> +    }
> +    if (!ui->mScrollbarFaceColor.IsAuto()) {
> +      nscolor color = ui->mScrollbarFaceColor.CalcColor(style);
> +      data.AppendPrintf(
> +        "scrollbar slider { background: rgba(%u,%u,%u,%.2f); }",

This is likely to erase mouseover and click feedback provided by the theme,
and I don't see any replacement.

Comment 16

[Xidorn Quan [:xidorn] UTC+10]

The point of scrollbar color properties is to provide a rendering close to native scrollbar with color specified by the author, and we somehow managed to do so for Windows and macOS.

I agree that the ecosystem of Linux is more fragile than other platforms, so maybe we should give up on being native-looking and accept whatever scrollbar rendering which may look reasonable enough on Linux...

I tested this patch with the default theme of Ubuntu, and it seems to work well, but like what you explained, I can imagine that it may cause problem with other distros.

Comment 17

[Xidorn Quan [:xidorn] UTC+10]

Attachment: Bug 1464723 - Implement custom scrollbar support for GTK widget. r=karlt

When any scrollbar color is specified, or scrollbar-width is thin, we
switch to use the fallback rendering.

The change to xulscrollbars.css is for ensuring that the scrollbar is
displayed for scrollbar-width: thin when there is no scrollbar color
specified. It wouldn't affect cases where -moz-appearance takes effect.

Comment 18

[Xidorn Quan [:xidorn] UTC+10]

Attachment: screenshot

Screenshot of this implementation on my Ubuntu VM.

Comment 19

[Karl Tomlinson (:karlt)]

Comment on attachment 9003034 [details]
Bug 1464723 - Implement custom scrollbar support for GTK widget. r=karlt

Karl Tomlinson (:karlt) has approved the revision.

Comment 20

[Daniel Holbert [:dholbert]]

Comment on attachment 9003034 [details]
Bug 1464723 - Implement custom scrollbar support for GTK widget. r=karlt

Daniel Holbert [:dholbert] has approved the revision.

Comment 21

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/fcae76a34c32
Implement custom scrollbar support for GTK widget. r=karlt,dholbert

Comment 22

[Arthur Iakab [arthur_iakab]]

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