Bug 16123

Summary: [GTK] Smooth scrolling support
Product: WebKit Reporter: Alp Toker <alp>
Component: WebKitGTKAssignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Enhancement CC: andersca, carlosg, cgarcia, eric, fishd, gns, jamesr, mrobinson, nayankk, racarr, sam, scottbyer, tony, vangelis, webkit.review.bot, xan.lopez, zan
Priority: P2 Keywords: Gtk
Version: 528+ (Nightly build)   
Hardware: All   
OS: All   
Bug Depends on: 32356    
Bug Blocks:    
Attachments:
Description Flags
Patch
tony: review-
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Updated patch none

Description Alp Toker 2007-11-24 18:23:09 PST
I find smooth scrolling greatly enhances browsing in Firefox and IE.

It would be good to add support for this, but perhaps not enable the feature by default since it's not conventional for GTK+ widgets (and also slightly controversial IIRC).

I think we can take a clever shortcut here by plugging into the CSS animation controllers to provide a smooth (bezier, cubic?) function for scrolling.

Scrolling should be smooth not only in the outermost frame, but also child frames.
Comment 1 Alp Toker 2007-11-24 20:47:05 PST
Relevant GTK+ bug:

http://bugzilla.gnome.org/show_bug.cgi?id=444659
Bug 444659 – Add GtkTimeline to ease animations development
Comment 2 Robert Carr 2008-12-31 02:29:46 PST
Just a relevant thing, I'm currently working on a set of patches to xf86-input-synaptics, and GTK to enable smooth scrolling support for a window, however not smooth scrolling in the firefox sense, but in the sense of the scroll events being reported with pixel granularity by the mouse driver, which I find to be MUCH nicer than firefox smooth scrolling. This is also the model used by OSX.

There are some videos of it being used with the Seed webkit browser at:
http://www.hortont.com/racarr/?p=20
Comment 3 Scott Byer 2011-06-01 10:38:21 PDT
Created attachment 95622 [details]
Patch
Comment 4 Scott Byer 2011-06-01 10:46:39 PDT
This is a start; it adds the plumbing for run-time enablement and an initial implementation (that leaves lots of tuning to do). Currently doesn't affect the standalone WebKit build at all. The initial focus is Linux/Chromium OS, but eventually this code wants to be refactored with the existing but turned off Windows smooth scrolling code into something more cross platform. Some of this will also dovetail with the platform gestures work.
Comment 5 Vangelis Kokkevis 2011-06-01 10:57:56 PDT
I haven't had the chance to review the code but at the high-level there doesn't seem to be anything linux-specific here. Should the class be renamed to something more generic or move to platform/chromium and become ScrollAnimatorChromium?
Comment 6 Scott Byer 2011-06-01 11:04:58 PDT
No, there really isn't anything Linux specific, and I don't think there will be (except for initialization paramaters, possibly, especially since we want to keep the system provided Mac touchpad behavior). The move to platform/chromium does make sense to me.
Comment 7 Tony Chang 2011-06-01 11:24:21 PDT
This is the wrong bug for this patch.  This bug is for smooth scrolling in WebKit GTK+ (e.g., build-webkit --gtk).  Please open a new bug for adding smooth scrolling to Chromium.
Comment 8 Tony Chang 2011-06-01 11:25:55 PDT
Comment on attachment 95622 [details]
Patch

I agree with Vangelis that these files should be in platform/chromium and shouldn't have Linux in the filename if they're not Linux specific.
Comment 9 James Robinson 2011-06-03 12:07:39 PDT
This bug is for the GTK port, not chromium.
Comment 10 Zan Dobersek 2011-08-01 09:47:29 PDT
Created attachment 102527 [details]
Patch

This patch uses ScrollAnimatorNone class to provide smooth scrolling and introduces a new flag to enable it (it is disabled by default)
Comment 11 Gustavo Noronha (kov) 2012-02-16 15:12:17 PST
API looks straight forward. Any reason to keep this behind a compilation option, though? I'd just leave it on and rely on the runtime option.
Comment 12 Martin Robinson 2012-02-16 15:27:36 PST
Comment on attachment 102527 [details]
Patch

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

Looks good! Is there any compelling reason to make this a compile-time option? Why not just compile it in always. r- because of the missing documentation. We need one more GTK+ reviewer to approve the API, but I think it's pretty non-controversial. Do you think you could add the setting to WebKit2 as well?

> Source/WebKit/gtk/webkit/webkitwebsettings.cpp:932
> +    * Whether ...

You're missing the documentation here. :(
Comment 13 Zan Dobersek 2012-02-17 02:30:35 PST
Created attachment 127560 [details]
Patch
Comment 14 Zan Dobersek 2012-02-17 02:34:39 PST
Comment on attachment 127560 [details]
Patch

This patch also adds support for enabling smooth scrolling in WebKit2. Documentation should now be properly processed in both WebKit1 and WebKit2. This also adds two additional functions to the core WebKit2 C API, along with a basic preference in WebPreferencesStore.

Also settings the cq flag.

Thanks for the reviews!
Comment 15 Martin Robinson 2012-02-17 09:35:23 PST
Comment on attachment 127560 [details]
Patch

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

This looks good to me. I think we just need some Mac guys to look at the proposed C API changes.

> Source/JavaScriptCore/wtf/Platform.h:490
> +#define ENABLE_SMOOTH_SCROLLING 1

Nit. This is probably something that we want to set in the GNUmakefile. We can just do it there unconditionally.
Comment 16 Martin Robinson 2012-02-17 09:37:58 PST
Anders or Sam, do you mind giving a nod to these WebKit2 C API changes?
Comment 17 Zan Dobersek 2012-02-18 01:09:16 PST
Created attachment 127700 [details]
Patch
Comment 18 Zan Dobersek 2012-02-18 01:30:02 PST
(In reply to comment #15)
> > Source/JavaScriptCore/wtf/Platform.h:490
> > +#define ENABLE_SMOOTH_SCROLLING 1
> 
> Nit. This is probably something that we want to set in the GNUmakefile. We can just do it there unconditionally.

Created a small section near the top of Source/WebCore/GNUmakefile.am that appends such features to FEATURE_DEFINES and webcore_cppflags in the latest patch. In future if more features require the same, the comment in that section can be expanded to nicely list all such features.
Comment 19 Carlos Garcia Campos 2012-02-18 01:36:40 PST
Comment on attachment 127700 [details]
Patch

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

Please, add a test the new WEbKit2 setting in Source/WebKit2/UIProcess/API/gtk/tests/TestWebKitSettings.cpp

> Source/WebKit2/UIProcess/API/gtk/WebKitSettings.cpp:2053
> +    return WKPreferencesGetEnableScrollAnimator(settings->priv->preferences.get());

I'm a bit confused, the scroll animator always does smooth scrolling or are there other types of scroll animations?
Comment 20 Zan Dobersek 2012-02-18 08:41:45 PST
(In reply to comment #19)
> (From update of attachment 127700 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=127700&action=review
> 
> Please, add a test the new WEbKit2 setting in Source/WebKit2/UIProcess/API/gtk/tests/TestWebKitSettings.cpp
> 

Sure, will do.

> > Source/WebKit2/UIProcess/API/gtk/WebKitSettings.cpp:2053
> > +    return WKPreferencesGetEnableScrollAnimator(settings->priv->preferences.get());
> 
> I'm a bit confused, the scroll animator always does smooth scrolling or are there other types of scroll animations?

The pure ScrollAnimator class only does the normal scrolling (the one in use today). It is platform-independent. Smooth scrolling is provided by ScrollAnimatorWin class for Windows platform, ScrollAnimatorMac class for Mac platform, and ScrollAnimatorNone class, which provides smooth scrolling in a platform-independent way. The ScrollAnimatorNone is obviously the only option, but is only used instead of the core ScollAnimator class if the scroll animator setting is enabled.
Comment 21 Anders Carlsson 2012-02-18 08:51:43 PST
Comment on attachment 127700 [details]
Patch

Does this really need to be a setting? Isn't there some default "use animations" setting or style property in gtk? When would you ever want to disable this?
Comment 22 Zan Dobersek 2012-02-18 11:54:49 PST
(In reply to comment #21)
> (From update of attachment 127700 [details])
> Does this really need to be a setting? Isn't there some default "use animations" setting or style property in gtk? When would you ever want to disable this?

There indeed is a 'gtk-use-animations' property in GtkSettings, which defaults to true. If acceptable to reviewers, the code could just connect to the notify::gtk-use-animation signal of GtkSettings and change the WebCore setting appropriately.

Though, without the WKPreferences API additions, this would require the connecting and adjusting to the 'gtk-use-animations' value to be performed in WebProcess. But ideally, the GtkSettings instance would be gathered from the specific GtkWidget (the web view), which is a part of UIProcess, and as such AFAIK not reachable from WebProcess.

How about keeping the WKPreferences API additions private, in WKPreferencesPrivate.h?

[1] http://developer.gnome.org/gtk3/stable/GtkSettings.html#GtkSettings--gtk-enable-animations
Comment 23 Martin Robinson 2012-02-18 12:00:32 PST
(In reply to comment #22)
> (In reply to comment #21)
> > (From update of attachment 127700 [details] [details])
> > Does this really need to be a setting? Isn't there some default "use animations" setting or style property in gtk? When would you ever want to disable this?
> 
> There indeed is a 'gtk-use-animations' property in GtkSettings, which defaults to true. If acceptable to reviewers, the code could just connect to the notify::gtk-use-animation signal of GtkSettings and change the WebCore setting appropriately.

When gtk-use-animations is true though, does GTK+ use smooth scrolling? If not, then we'll need to look elsewhere to maintain consitency with GTK+.
Comment 24 Zan Dobersek 2012-02-18 12:24:57 PST
(In reply to comment #23)
> (In reply to comment #22)
> > (In reply to comment #21)
> > > (From update of attachment 127700 [details] [details] [details])
> > > Does this really need to be a setting? Isn't there some default "use animations" setting or style property in gtk? When would you ever want to disable this?
> > 
> > There indeed is a 'gtk-use-animations' property in GtkSettings, which defaults to true. If acceptable to reviewers, the code could just connect to the notify::gtk-use-animation signal of GtkSettings and change the WebCore setting appropriately.
> 
> When gtk-use-animations is true though, does GTK+ use smooth scrolling? If not, then we'll need to look elsewhere to maintain consitency with GTK+.

Oops, it's actually gtk-enable-animations.

The only Gtk application I've seen to date to perform smooth scrolling is the gnome-about dialog. It doesn't respect the gtk-use-animations setting. Grepping through the gtk source, this setting is used to decide whether or not to animate smaller animations, such as expanding the tree view or sliding a toolbar item etc.

Considering this, gtk-enable-animations might not be appropriate to control whether or not to enable smooth scrolling.
Comment 25 Carlos Garnacho 2012-02-19 02:23:38 PST
In the end, smooth scrolling is going to be handled through a new GDK_SMOOTH_SCROLL_MASK evmask flag, but there's not going to be a setting as such to turn it off/on, widgets just do decide which kind of scroll events fit them better
Comment 26 Carlos Garcia Campos 2012-02-19 23:54:19 PST
So, I would just enable this unconditionally. If someone asks for a way to disable it we can then discuss whether to add a new setting to enable/disable it.
Comment 27 Martin Robinson 2012-02-20 00:14:09 PST
(In reply to comment #26)
> So, I would just enable this unconditionally. If someone asks for a way to disable it we can then discuss whether to add a new setting to enable/disable it.

Just to clarify, you mean we should do smooth scrolling unconditionally when the GDK_SMOOTH_SCROLL_MASK flag is active? If so, I believe I agree.
Comment 28 Carlos Garcia Campos 2012-02-20 00:39:02 PST
(In reply to comment #27)
> (In reply to comment #26)
> > So, I would just enable this unconditionally. If someone asks for a way to disable it we can then discuss whether to add a new setting to enable/disable it.
> 
> Just to clarify, you mean we should do smooth scrolling unconditionally when the GDK_SMOOTH_SCROLL_MASK flag is active? If so, I believe I agree.

No, we don't need GDK_SMOOTH_SCROLL_MASK because we have our own scroll animator that does smooth scrolling. GDK_SMOOTH_SCROLL_MASK will not have any effect in WebView because we don't implement GtkScrollable. I'm not sure it's clear what Carlos said. GTK+ doesn't still support smooth scrolling. Current implementation (in smooth-scrolling branch) adds GDK_SMOOTH_SCROLL_MASK which makes that delta information is included in GdkScroll events. I will be implemented in GtkScrolledWindow, and widgets will decide whether they want smooth scrolling or not, by adding GDK_SMOOTH_SCROLL_MASK to its own events masks (typically done in realize()). So, there won't be any API nor setting to enable/disable smooth scrolling in widgets. If GTK+ devs decide that GtkTreeView does smooth scrolling, it will include the GDK_SMOOTH_SCROLL_MASK and will do smooth scrolling unconditionally.
My opinion is that we should do the same and decide to use smooth scrolling unconditionally in WebView (in our case using the scroll animator, I don't need GDK_SMOOTH_SCROLL_MASK). If we eventually see that people want to be able to disable it, we can just add a setting later.
Comment 29 Martin Robinson 2012-02-20 00:44:53 PST
(In reply to comment #28)

> > Just to clarify, you mean we should do smooth scrolling unconditionally when the GDK_SMOOTH_SCROLL_MASK flag is active? If so, I believe I agree.
> 

> My opinion is that we should do the same and decide to use smooth scrolling unconditionally in WebView (in our case using the scroll animator, I don't need GDK_SMOOTH_SCROLL_MASK). If we eventually see that people want to be able to disable it, we can just add a setting later.

I would like to understand more about whether there are plans to switch all built-in GTK+ widgets to smooth scrolling or how this decision is made if it's done for some widgets and not others. Either way, adding a setting up-front seems low risk.
Comment 30 Carlos Garcia Campos 2012-02-20 01:00:35 PST
(In reply to comment #29)
> (In reply to comment #28)
> 
> > > Just to clarify, you mean we should do smooth scrolling unconditionally when the GDK_SMOOTH_SCROLL_MASK flag is active? If so, I believe I agree.
> > 
> 
> > My opinion is that we should do the same and decide to use smooth scrolling unconditionally in WebView (in our case using the scroll animator, I don't need GDK_SMOOTH_SCROLL_MASK). If we eventually see that people want to be able to disable it, we can just add a setting later.
> 
> I would like to understand more about whether there are plans to switch all built-in GTK+ widgets to smooth scrolling or how this decision is made if it's done for some widgets and not others. Either way, adding a setting up-front seems low risk.

It will depend on every widget, I think tree view, icon view, text view will do smooth scrolling, for example. So it's up to use to decide. I don't mind if we add the setting in wk1, but for for wk2 we could try to enable it by default and add the setting later if we see we really needed.
Comment 31 Gustavo Noronha (kov) 2012-02-20 06:14:38 PST
(In reply to comment #30)
> It will depend on every widget, I think tree view, icon view, text view will do smooth scrolling, for example. So it's up to use to decide. I don't mind if we add the setting in wk1, but for for wk2 we could try to enable it by default and add the setting later if we see we really needed.

Sounds like a good compromise to me.
Comment 32 Anders Carlsson 2012-02-20 08:51:58 PST
(In reply to comment #22)
> (In reply to comment #21)
> > (From update of attachment 127700 [details] [details])
> > Does this really need to be a setting? Isn't there some default "use animations" setting or style property in gtk? When would you ever want to disable this?
> 
> There indeed is a 'gtk-use-animations' property in GtkSettings, which defaults to true. If acceptable to reviewers, the code could just connect to the notify::gtk-use-animation signal of GtkSettings and change the WebCore setting appropriately.
> 
> Though, without the WKPreferences API additions, this would require the connecting and adjusting to the 'gtk-use-animations' value to be performed in WebProcess. But ideally, the GtkSettings instance would be gathered from the specific GtkWidget (the web view), which is a part of UIProcess, and as such AFAIK not reachable from WebProcess.
> 

Sure, you'd have to keep track of the state in the UI process and send it over to the web process when it changes. We do this for a number of different settings.
Comment 33 Zan Dobersek 2012-02-21 01:28:55 PST
(In reply to comment #31)
> (In reply to comment #30)
> > It will depend on every widget, I think tree view, icon view, text view will do smooth scrolling, for example. So it's up to use to decide. I don't mind if we add the setting in wk1, but for for wk2 we could try to enable it by default and add the setting later if we see we really needed.
> 
> Sounds like a good compromise to me.

Very well, this actually requires no changes to be made in any part of WebKit2.

Patch incoming.
Comment 34 Zan Dobersek 2012-02-21 01:31:33 PST
Created attachment 127937 [details]
Patch
Comment 35 Zan Dobersek 2012-02-22 06:37:45 PST
(In reply to comment #34)
> Created an attachment (id=127937) [details]
> Patch

Contains wrong 'Since' version in signal definition, will upload the correct one.
Comment 36 Zan Dobersek 2012-02-22 07:33:34 PST
Created attachment 128212 [details]
Patch
Comment 37 Martin Robinson 2012-02-24 13:14:08 PST
Comment on attachment 128212 [details]
Patch

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

> Source/WebKit/gtk/webkit/webkitwebsettings.cpp:964
> +    * Since: 1.7.90

This should probably be 1.9.0 now, since we've already branched for stable.
Comment 38 Carlos Garcia Campos 2012-02-25 01:52:30 PST
(In reply to comment #37)
> (From update of attachment 128212 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=128212&action=review
> 
> > Source/WebKit/gtk/webkit/webkitwebsettings.cpp:964
> > +    * Since: 1.7.90
> 
> This should probably be 1.9.0 now, since we've already branched for stable.

1.10 I guess, 1.9.x will be the unstable versions of the next release cycle.
Comment 39 Zan Dobersek 2012-02-25 02:04:30 PST
(In reply to comment #38)
> (In reply to comment #37)
> > (From update of attachment 128212 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=128212&action=review
> > 
> > > Source/WebKit/gtk/webkit/webkitwebsettings.cpp:964
> > > +    * Since: 1.7.90
> > 
> > This should probably be 1.9.0 now, since we've already branched for stable.
> 
> 1.10 I guess, 1.9.x will be the unstable versions of the next release cycle.

There's plenty of places where an unstable version is given as the 'Since:' value. This will be a part of the next unstable release (1.9.0) and subsequently of the next stable release as well (1.10.0).

All the 'Since:' tags should either have a value of an unstable release (1.7.5, 1.9.0, 1.9.1 etc.), stating in which release exactly API was added, or a value of a stable release (1.6, 1.8, 1.10 etc.), stating in what stable release API was first present.

AFAIK WebKitGtk+ is mostly if not completely using the first approach.
Comment 40 Carlos Garcia Campos 2012-02-25 04:52:32 PST
(In reply to comment #39)
> (In reply to comment #38)
> > (In reply to comment #37)
> > > (From update of attachment 128212 [details] [details] [details])
> > > View in context: https://bugs.webkit.org/attachment.cgi?id=128212&action=review
> > > 
> > > > Source/WebKit/gtk/webkit/webkitwebsettings.cpp:964
> > > > +    * Since: 1.7.90
> > > 
> > > This should probably be 1.9.0 now, since we've already branched for stable.
> > 
> > 1.10 I guess, 1.9.x will be the unstable versions of the next release cycle.
> 
> There's plenty of places where an unstable version is given as the 'Since:' value. This will be a part of the next unstable release (1.9.0) and subsequently of the next stable release as well (1.10.0).

I think that comes from the time were webkitgtk did follow the gnome release cycle with stable/unstable releases, I'm not sure, though.

> All the 'Since:' tags should either have a value of an unstable release (1.7.5, 1.9.0, 1.9.1 etc.), stating in which release exactly API was added, or a value of a stable release (1.6, 1.8, 1.10 etc.), stating in what stable release API was first present.
> 
> AFAIK WebKitGtk+ is mostly if not completely using the first approach.

So, maybe we should consider changing it.
Comment 41 Martin Robinson 2012-02-25 10:46:44 PST
(In reply to comment #40)

> > AFAIK WebKitGtk+ is mostly if not completely using the first approach.
> 
> So, maybe we should consider changing it.

I agree that we should consider changing it, but probably it should be handled separately from this bug. If such a change it made, we can go back and fix all existing Since: tags.
Comment 42 Zan Dobersek 2012-03-02 02:05:57 PST
Created attachment 129852 [details]
Updated patch

Since: tag updated to 1.9.0
Comment 43 Carlos Garcia Campos 2012-03-02 02:25:24 PST
(In reply to comment #42)
> Created an attachment (id=129852) [details]
> Updated patch
> 
> Since: tag updated to 1.9.0

I still think we shouldn't use unstable version numbers in Since: tags. If this patrch will be merged into 1.8, this should be Since: 1.8 otherwise it should be 1.10
Comment 44 WebKit Review Bot 2012-03-02 10:04:15 PST
Comment on attachment 129852 [details]
Updated patch

Clearing flags on attachment: 129852

Committed r109584: <http://trac.webkit.org/changeset/109584>
Comment 45 WebKit Review Bot 2012-03-02 10:04:23 PST
All reviewed patches have been landed.  Closing bug.
Comment 46 Zan Dobersek 2012-03-02 10:07:07 PST
(In reply to comment #43)
> (In reply to comment #42)
> > Created an attachment (id=129852) [details] [details]
> > Updated patch
> > 
> > Since: tag updated to 1.9.0
> 
> I still think we shouldn't use unstable version numbers in Since: tags. If this patrch will be merged into 1.8, this should be Since: 1.8 otherwise it should be 1.10

Should I open a new bug covering this issue?
Comment 47 Martin Robinson 2012-03-02 10:29:09 PST
(In reply to comment #46)

> Should I open a new bug covering this issue?

Yeah, as I suggested above, deciding what versions to use in Since tags is not a question that this bug needs to solve and shouldn't block this patch. Probably it deserves a short note to the mailing list to gather opinions. It's such a minor issue that hopefully it shouldn't be too controversial. :) Once we decide what to do, we can open a bug to fix all the existing Since tags. It's totally orthoganol to smooth scrolling though.