Bug 153405 – [GTK] Implement overlay scrollbars

• Component: WebKitGTK
• Status: RESOLVED FIXED

[Carlos Garcia Campos]

For consistency with all other GTK+ widgets, but also to make scrollbars work with the threaded compositor.

[Carlos Garcia Campos]

Created attachment 269692 [details]
Patch

[Darin Adler]

Comment on attachment 269692 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=269692&action=review

I have some comments, but I’m not sure enough about everything for either a review+ or a review-.

Generally looks good. I am a bit confused by some of the critical details, though.

> Source/WebCore/platform/ScrollAnimatorNone.cpp:54
> +static const double kFrameRate = 60;
> +static const double kTickTime = 1 / kFrameRate;
> +static const double kMinimumTimerInterval = .001;
> +static const double kOverflowScrollbarsAnimationDuration = 1;
> +static const double kOverflowScrollbarsAnimationHideDelay = 2;

This (existing, not new) use of k prefixes doesn’t seem right for WebKit coding style.

> Source/WebCore/platform/ScrollAnimatorNone.cpp:631
> +    gdouble progress = 1;

All the rest are using double, but this one place uses gdouble. Surprised this compiles at all on non-GTK platforms.

> Source/WebCore/platform/ScrollAnimatorNone.h:53
>      explicit ScrollAnimatorNone(ScrollableArea&);

I don’t understand how ScrollAnimatorNone is still the right name for this class, now that it’s implementing scrollbar appear/disappear animations. Is this the right way to factor this.

> Source/WebCore/platform/ScrollAnimatorNone.h:188
> +    Scrollbar* m_horizontalOverlayScrollbar { nullptr };
> +    Scrollbar* m_verticalOverlayScrollbar { nullptr };
> +    bool m_overlayScrollbarsLocked { false };
> +    Timer m_overlayScrollbarAnimationTimer;
> +    double m_overlayScrollbarAnimationSource { 0 };
> +    double m_overlayScrollbarAnimationTarget { 0 };
> +    double m_overlayScrollbarAnimationCurrent { 0 };
> +    double m_overlayScrollbarAnimationStartTime { 0 };
> +    double m_overlayScrollbarAnimationEndTime { 0 };

That looks like a lot of unconditional overhead for ports that never use this, which currently includes EFL at least, perhaps also Mac and iOS? Or maybe not? Is there a way to make this cost conditional?

In new code we are trying to use std::chrono for time, rather than plain doubles.

> Source/WebCore/platform/Scrollbar.h:134
> +    double opacity() const { return m_opacity; }
> +    void setOpacity(double opacity) { m_opacity = opacity; }

A little strange to use double for this. Don’t we use float for this kind of thing elsewhere?

[Michael Catanzaro]

(In reply to comment #2)
> I have some comments, but I’m not sure enough about everything for either a
> review+ or a review-.

Thanks Darin!

I'll give it a final review later, after actually trying it out. (Haven't tested it yet.)

> This (existing, not new) use of k prefixes doesn’t seem right for WebKit
> coding style.

Yuck, possibly my least-favorite element of Google's otherwise nice code style. Carlos can do a follow-up patch to remove the k prefixes. Best to match the existing style in this patch.

> > Source/WebCore/platform/ScrollAnimatorNone.cpp:631
> > +    gdouble progress = 1;
> 
> All the rest are using double, but this one place uses gdouble. Surprised
> this compiles at all on non-GTK platforms.

Good catch. I bet it doesn't compile on non-GLib platforms (EFL does use GLib), but Mac and IOS have separate ScrollAnimator classes so surely they don't use this code. I don't know why it works on Windows, though.

> > Source/WebCore/platform/ScrollAnimatorNone.h:53
> >      explicit ScrollAnimatorNone(ScrollableArea&);
> 
> I don’t understand how ScrollAnimatorNone is still the right name for this
> class, now that it’s implementing scrollbar appear/disappear animations. Is
> this the right way to factor this.

I was going to make the same complaint. Perhaps ScrollAnimatorDefault? Or is there a distinction in that ScrollAnimatorNone somehow does not animate the scroll (whatever that means)? I presume scrollableArea.scrollAnimatorEnabled() is returning false for GTK?

> > Source/WebCore/platform/ScrollAnimatorNone.h:188
> That looks like a lot of unconditional overhead for ports that never use
> this, which currently includes EFL at least, perhaps also Mac and iOS? Or
> maybe not? Is there a way to make this cost conditional?

It might be best the way Carlos has it; it would be nice to avoid platform-specific compilation in this file, and I doubt it can be easily split out without causing lots of code duplication. Carlos, what do you think?

[Michael Catanzaro]

Comment on attachment 269692 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=269692&action=review

> Source/WebCore/platform/gtk/ScrollbarThemeGtk.cpp:238
> +    m_usesOverlayScrollbars = g_strcmp0(g_getenv("GTK_OVERLAY_SCROLLING"), "0");

Is this really the right test...? It's not a style property? I assumed this variable was to turn off overlay scrolling for debugging purposes. Are all themes expected to implement overlay scrollbars?

I'm halfway pleased that the technical limitation of threaded compositor necessitates implementing overlay scrollbars, because now we have overlay scrollbars. But it's disappointing that threaded compositor will prevent us from rendering non-overlay scrollbars. I expect this could cause scrollbars to look quite bad with anything besides Adwaita or Ambiance. Can you or Yoon explain the technical limitation here?

> Source/WebCore/platform/gtk/ScrollbarThemeGtk.cpp:432
> +#if GTK_CHECK_VERSION(3, 19, 8)

I don't understand this version check. 3.19.7 was released last week; I don't see any scrollbar-related changes since then?

I would just get rid of the check; it's enough that the code works with GTK+ master, that's what you and I are going to use, we're the people testing it so optimize for us. Otherwise I'd have to manually remove the check to test this code.

[Carlos Garcia Campos]

(In reply to comment #2)
> Comment on attachment 269692 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=269692&action=review
> 
> I have some comments, but I’m not sure enough about everything for either a
> review+ or a review-.

Sure, thanks for looking at it!.

> Generally looks good. I am a bit confused by some of the critical details,
> though.
> 
> > Source/WebCore/platform/ScrollAnimatorNone.cpp:54
> > +static const double kFrameRate = 60;
> > +static const double kTickTime = 1 / kFrameRate;
> > +static const double kMinimumTimerInterval = .001;
> > +static const double kOverflowScrollbarsAnimationDuration = 1;
> > +static const double kOverflowScrollbarsAnimationHideDelay = 2;
> 
> This (existing, not new) use of k prefixes doesn’t seem right for WebKit
> coding style.

I agree, I just followed the style.

> > Source/WebCore/platform/ScrollAnimatorNone.cpp:631
> > +    gdouble progress = 1;
> 
> All the rest are using double, but this one place uses gdouble. Surprised
> this compiles at all on non-GTK platforms.

Oops, that was not intentional.

> > Source/WebCore/platform/ScrollAnimatorNone.h:53
> >      explicit ScrollAnimatorNone(ScrollableArea&);
> 
> I don’t understand how ScrollAnimatorNone is still the right name for this
> class, now that it’s implementing scrollbar appear/disappear animations. Is
> this the right way to factor this.

I has always been confusing, even when it only implements smooth scrolling.

> > Source/WebCore/platform/ScrollAnimatorNone.h:188
> > +    Scrollbar* m_horizontalOverlayScrollbar { nullptr };
> > +    Scrollbar* m_verticalOverlayScrollbar { nullptr };
> > +    bool m_overlayScrollbarsLocked { false };
> > +    Timer m_overlayScrollbarAnimationTimer;
> > +    double m_overlayScrollbarAnimationSource { 0 };
> > +    double m_overlayScrollbarAnimationTarget { 0 };
> > +    double m_overlayScrollbarAnimationCurrent { 0 };
> > +    double m_overlayScrollbarAnimationStartTime { 0 };
> > +    double m_overlayScrollbarAnimationEndTime { 0 };
> 
> That looks like a lot of unconditional overhead for ports that never use
> this, which currently includes EFL at least, perhaps also Mac and iOS? Or
> maybe not? Is there a way to make this cost conditional?

My initial idea was to add ScrollAnimationGtk derived from None and add there the scrollbars code, but then I realized that the code is not actually GTK specific, and that EFL port could benefit from it eventually. Since this is only used by EFL and GTK+, I'll ask the EFL guys.

> In new code we are trying to use std::chrono for time, rather than plain
> doubles.

Yes, here it was very convenient to use doubles, because m_overlayScrollbarAnimationCurrent is used directly as the scrollbar opacity value.

> > Source/WebCore/platform/Scrollbar.h:134
> > +    double opacity() const { return m_opacity; }
> > +    void setOpacity(double opacity) { m_opacity = opacity; }
> 
> A little strange to use double for this. Don’t we use float for this kind of
> thing elsewhere?

You are right, float should be enough and it's what we use in GraphicsContext for example.

[Carlos Garcia Campos]

> > Source/WebCore/platform/ScrollAnimatorNone.h:188
> > +    Scrollbar* m_horizontalOverlayScrollbar { nullptr };
> > +    Scrollbar* m_verticalOverlayScrollbar { nullptr };
> > +    bool m_overlayScrollbarsLocked { false };
> > +    Timer m_overlayScrollbarAnimationTimer;
> > +    double m_overlayScrollbarAnimationSource { 0 };
> > +    double m_overlayScrollbarAnimationTarget { 0 };
> > +    double m_overlayScrollbarAnimationCurrent { 0 };
> > +    double m_overlayScrollbarAnimationStartTime { 0 };
> > +    double m_overlayScrollbarAnimationEndTime { 0 };
> 
> That looks like a lot of unconditional overhead for ports that never use
> this, which currently includes EFL at least, perhaps also Mac and iOS? Or
> maybe not? Is there a way to make this cost conditional?

Ossy, Gyuyoung, what do you think about this? If this is too much overhead for EFL or you don't plan to reuse this code at all, I'll add a scroll animator class for GTK to move this there.

[Carlos Garcia Campos]

(In reply to comment #3)
> (In reply to comment #2)
> > I have some comments, but I’m not sure enough about everything for either a
> > review+ or a review-.
> 
> Thanks Darin!
> 
> I'll give it a final review later, after actually trying it out. (Haven't
> tested it yet.)
> 
> > This (existing, not new) use of k prefixes doesn’t seem right for WebKit
> > coding style.
> 
> Yuck, possibly my least-favorite element of Google's otherwise nice code
> style. Carlos can do a follow-up patch to remove the k prefixes. Best to
> match the existing style in this patch.
> 
> > > Source/WebCore/platform/ScrollAnimatorNone.cpp:631
> > > +    gdouble progress = 1;
> > 
> > All the rest are using double, but this one place uses gdouble. Surprised
> > this compiles at all on non-GTK platforms.
> 
> Good catch. I bet it doesn't compile on non-GLib platforms (EFL does use
> GLib), but Mac and IOS have separate ScrollAnimator classes so surely they
> don't use this code. I don't know why it works on Windows, though.
> 
> > > Source/WebCore/platform/ScrollAnimatorNone.h:53
> > >      explicit ScrollAnimatorNone(ScrollableArea&);
> > 
> > I don’t understand how ScrollAnimatorNone is still the right name for this
> > class, now that it’s implementing scrollbar appear/disappear animations. Is
> > this the right way to factor this.
> 
> I was going to make the same complaint. Perhaps ScrollAnimatorDefault? Or is
> there a distinction in that ScrollAnimatorNone somehow does not animate the
> scroll (whatever that means)? I presume
> scrollableArea.scrollAnimatorEnabled() is returning false for GTK?
> 
> > > Source/WebCore/platform/ScrollAnimatorNone.h:188
> > That looks like a lot of unconditional overhead for ports that never use
> > this, which currently includes EFL at least, perhaps also Mac and iOS? Or
> > maybe not? Is there a way to make this cost conditional?
> 
> It might be best the way Carlos has it; it would be nice to avoid
> platform-specific compilation in this file, and I doubt it can be easily
> split out without causing lots of code duplication. Carlos, what do you
> think?

ScrollAnimatorNone was thought as an intermediate class to inherit from, that's why it's not final and it has protected methods. So, we can just add ScrollAnimatorGtk without adding any news ifdefs

[Carlos Garcia Campos]

(In reply to comment #4)
> Comment on attachment 269692 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=269692&action=review
> 
> > Source/WebCore/platform/gtk/ScrollbarThemeGtk.cpp:238
> > +    m_usesOverlayScrollbars = g_strcmp0(g_getenv("GTK_OVERLAY_SCROLLING"), "0");
> 
> Is this really the right test...?

Yes.

> It's not a style property?

No. It's GtkScrolledWindow API, gtk_scrolled_window_set_overlay_scrolling(). We could probably add a WebKitSetting.

> I assumed this
> variable was to turn off overlay scrolling for debugging purposes.

Yes, this is probably just for debugging, I'm not sure it's documented. I added it to match what GTK does, so that if someone runs any GTK+ app containing a WebView with the env var, we will honor it too. And also made my life easier when developing this. 

> Are all
> themes expected to implement overlay scrollbars?

It doesn't really matter. If a theme doesn't implement overlay scrollbars, the overlay-indicator class will not have any effect in the rendering, so we will end up rendering the scrollbars following the theme but on top of the contents and hiding/showing them. Same would happen with GtkScrolledWindow, and I would say it's a theme issue not ours.

> I'm halfway pleased that the technical limitation of threaded compositor
> necessitates implementing overlay scrollbars, because now we have overlay
> scrollbars. But it's disappointing that threaded compositor will prevent us
> from rendering non-overlay scrollbars. I expect this could cause scrollbars
> to look quite bad with anything besides Adwaita or Ambiance. Can you or Yoon
> explain the technical limitation here?

I have no idea. But I don't think it's that problematic, GTK+ is unusable with any other theme than Adwaita nowadays, and new themes will probably implement olverlay-indicator to look good with GtkScrolledwindow.

> > Source/WebCore/platform/gtk/ScrollbarThemeGtk.cpp:432
> > +#if GTK_CHECK_VERSION(3, 19, 8)
> 
> I don't understand this version check. 3.19.7 was released last week; I
> don't see any scrollbar-related changes since then?

https://git.gnome.org/browse/gtk+/commit/?id=0f7b4dad0f6cef80172638efb019ce094d7eadee

> I would just get rid of the check; it's enough that the code works with GTK+
> master, that's what you and I are going to use, we're the people testing it
> so optimize for us. Otherwise I'd have to manually remove the check to test
> this code.

It will probably crash if someone uses 3.19.7, it's just temporary, we should change all the ifdefs in this file (and RenderThemeGtk) once 3.20 is released.

[Michael Catanzaro]

I tried this out today. It is impressive; you will fool most users into thinking it's a real GtkScrollbar.

The patch would be better with ScrollAnimator tests, but since there's no infrastructure to test those currently, I don't want to block on that.

There is just one issue I see that I think needs fixed. If you mouse over the scrollbar, then move your mouse outside the window to the right, the scrollbar track stays visible forever. It should instead disappear as soon as the mouse is moved outside the window. (Could it be related to bug #116691?)

See also: bug #122859 and bug #115363.

[Michael Catanzaro]

(In reply to comment #9)
> (Could it be related to bug #116691?)

Probably not, as in this case the window is active, the mouse is just outside of it.

[Beth Dakin]

I do want to strongly encourage you to consider making tests. Don't make the same mistake I did!! I assure you that our experience on Mac has been that these behaviors can be subtle enough that we have introduced regressions that took a long time to notice -- I imagine that this will be true for you too. Especially if you support both "legacy" and "overlay" scrollbars (more things to get wrong). Since we have already experienced a number of regressions over the years it feels inevitable to me we will  accidentally break each other's code at some point. All of that will be less painful with tests.

I regret not figuring this out a long time ago, and it still nags at me even though I have a ton of other things to work on. Simon and I were talking today. Maybe there is a way to add tests that just turn on overlay scrollbar logging, and then the scrollbars log something every time they appear or disappear? Or add some way to query current overlay scrollbar state (visible or not, hover-appearance or not, etc)? If you take the lead on fleshing this out, I can implement the Mac support.

Besides this strong encouragement, I also have to insist that you change the name of ScrollAnimatorNone. It seems much more like the presence of scroll animations than the absence! I appreciate the desire to make it more cross-platform-friendly, but I think that ScrollAnimatorGTK would be a much better name.

[Carlos Garcia Campos]

(In reply to comment #9)
> I tried this out today. It is impressive; you will fool most users into
> thinking it's a real GtkScrollbar.

Until a new GTK+ version breaks it :-D

> The patch would be better with ScrollAnimator tests, but since there's no
> infrastructure to test those currently, I don't want to block on that.

That's fine as long as we actually add test for this soon.

> There is just one issue I see that I think needs fixed. If you mouse over
> the scrollbar, then move your mouse outside the window to the right, the
> scrollbar track stays visible forever. It should instead disappear as soon
> as the mouse is moved outside the window. (Could it be related to bug
> #116691?)
> 
> See also: bug #122859 and bug #115363.

I noticed that problem too, but didn't fixed it in this patch because I think the problem is a more general issue with the web view focus handling, so fixing it in this patch would actually hide the actual bug. I'll look at it in more detail, since that's not a blocker at all.

[Carlos Garcia Campos]

(In reply to comment #11)
> I do want to strongly encourage you to consider making tests. Don't make the
> same mistake I did!! I assure you that our experience on Mac has been that
> these behaviors can be subtle enough that we have introduced regressions
> that took a long time to notice -- I imagine that this will be true for you
> too. Especially if you support both "legacy" and "overlay" scrollbars (more
> things to get wrong). Since we have already experienced a number of
> regressions over the years it feels inevitable to me we will  accidentally
> break each other's code at some point. All of that will be less painful with
> tests.

I agree.

> I regret not figuring this out a long time ago, and it still nags at me even
> though I have a ton of other things to work on. Simon and I were talking
> today. Maybe there is a way to add tests that just turn on overlay scrollbar
> logging, and then the scrollbars log something every time they appear or
> disappear? Or add some way to query current overlay scrollbar state (visible
> or not, hover-appearance or not, etc)? If you take the lead on fleshing this
> out, I can implement the Mac support.

I also thought about it. I think we could add a mock scroll animator that logs all the events somehow (mouse entered/exited scrollable area, mouse entered/existed scrollbar, etc.). That behavior should be common to all ports, no matter if overlay scrollbars are used or not.

> Besides this strong encouragement, I also have to insist that you change the
> name of ScrollAnimatorNone. It seems much more like the presence of scroll
> animations than the absence! I appreciate the desire to make it more
> cross-platform-friendly, but I think that ScrollAnimatorGTK would be a much
> better name.

Ok, I'll add ScrollAnimatorGtk then, if EFL eventually wants to use our overlay scrollbars implementation we will find the way to share the code.

[Carlos Garcia Campos]

Created attachment 269881 [details]
Updated patch

[Carlos Garcia Campos]

Created attachment 269994 [details]
Rebased patch

[Michael Catanzaro]

Comment on attachment 269994 [details]
Rebased patch

I reviewed the "updated patch" from yesterday and just let it sit a day to see if anyone else would comment; I presume there aren't many changes in "rebased patch."

You addressed most of my comments already. The one thing I would add is that ENABLE_SMOOTH_SCROLLING is not a public option for our port, so I would remove the include guards for it. If you want to allow disabling smooth scrolling, we should make it a public option.

I'm looking forward to the scroll animator tests requested by Simon/Darin/Beth. :)

[Carlos Garcia Campos]

(In reply to comment #16)
> Comment on attachment 269994 [details]
> Rebased patch
> 
> I reviewed the "updated patch" from yesterday and just let it sit a day to
> see if anyone else would comment; I presume there aren't many changes in
> "rebased patch."

No changes, I submitted it again rebased just to make sure it passed EWS.

> You addressed most of my comments already. The one thing I would add is that
> ENABLE_SMOOTH_SCROLLING is not a public option for our port, so I would
> remove the include guards for it. If you want to allow disabling smooth
> scrolling, we should make it a public option.

In that case we should just remove the option from GTK cmake files, and enable it unconditionally in Platform.h, and then remove the ifdefs from GTK files. Not something I would do as part of this patch.

> I'm looking forward to the scroll animator tests requested by
> Simon/Darin/Beth. :)

The major blocker of this patch is bug #153404, so I'll wait a bit to land it, or I could land it disabled by default (inverting the logic of the env variable) until bug #153404 is fixed. Meanwhile I will work on the tests and some other minor issues related to scrollbars that we still have pending.

[Michael Catanzaro]

By the way, did you see my comment about the scrollbar getting stuck on if you move your mouse out the right-hand side of the window? It happens with legacy scrollbars too (they get stuck on prelight).

> In that case we should just remove the option from GTK cmake files, and
> enable it unconditionally in Platform.h, and then remove the ifdefs from GTK
> files. Not something I would do as part of this patch.

I don't agree. OptionsGTK.cmake is the right place to override the value of any WebKit feature declared in WebKitFeatures.cmake.

[Carlos Garcia Campos]

(In reply to comment #18)
> By the way, did you see my comment about the scrollbar getting stuck on if
> you move your mouse out the right-hand side of the window? It happens with
> legacy scrollbars too (they get stuck on prelight).

Replied in comment #12.

> > In that case we should just remove the option from GTK cmake files, and
> > enable it unconditionally in Platform.h, and then remove the ifdefs from GTK
> > files. Not something I would do as part of this patch.
> 
> I don't agree. OptionsGTK.cmake is the right place to override the value of
> any WebKit feature declared in WebKitFeatures.cmake.

If we support a feature unconditionally there's no reason to mess up makefiles with options we don't really support.

[Carlos Garcia Campos]

Committed r195810: <http://trac.webkit.org/changeset/195810>

[Carlos Garcia Campos]

(In reply to comment #20)
> Committed r195810: <http://trac.webkit.org/changeset/195810>

Disabled by default until bug #153404 is fixed.

[Michael Catanzaro]

(In reply to comment #19)
> If we support a feature unconditionally there's no reason to mess up
> makefiles with options we don't really support.

I much prefer to keep all build options organized nicely in WebKitFeatures.cmake and OptionsGTK.cmake rather than in Platform.h, which is very cluttered. Since we only have two build systems now, we can probably drastically simplify Platform.h.

[Carlos Garcia Campos]

(In reply to comment #12)
> (In reply to comment #9)
> > I tried this out today. It is impressive; you will fool most users into
> > thinking it's a real GtkScrollbar.
> 
> Until a new GTK+ version breaks it :-D
> 
> > The patch would be better with ScrollAnimator tests, but since there's no
> > infrastructure to test those currently, I don't want to block on that.
> 
> That's fine as long as we actually add test for this soon.
> 
> > There is just one issue I see that I think needs fixed. If you mouse over
> > the scrollbar, then move your mouse outside the window to the right, the
> > scrollbar track stays visible forever. It should instead disappear as soon
> > as the mouse is moved outside the window. (Could it be related to bug
> > #116691?)
> > 
> > See also: bug #122859 and bug #115363.
> 
> I noticed that problem too, but didn't fixed it in this patch because I
> think the problem is a more general issue with the web view focus handling,
> so fixing it in this patch would actually hide the actual bug. I'll look at
> it in more detail, since that's not a blocker at all.

https://bugs.webkit.org/show_bug.cgi?id=153740