Bug 203914 - [GTK] kinetic scroll speed should be cumulative
Summary: [GTK] kinetic scroll speed should be cumulative
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKitGTK (show other bugs)
Version: WebKit Nightly Build
Hardware: PC Linux
: P2 Normal
Assignee: Chris Lord
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2019-11-06 12:53 PST by Yariv
Modified: 2020-11-09 09:55 PST (History)
5 users (show)

See Also:


Attachments
Patch (5.68 KB, patch)
2020-10-30 03:46 PDT, Chris Lord
no flags Details | Formatted Diff | Diff
Patch (6.52 KB, patch)
2020-10-30 07:36 PDT, Chris Lord
no flags Details | Formatted Diff | Diff
Patch (6.53 KB, patch)
2020-10-30 07:36 PDT, Chris Lord
no flags Details | Formatted Diff | Diff
Patch (6.70 KB, patch)
2020-11-09 08:38 PST, Chris Lord
no flags Details | Formatted Diff | Diff
Patch (6.70 KB, patch)
2020-11-09 09:19 PST, Chris Lord
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Yariv 2019-11-06 12:53:02 PST
When using a touchpad swipe gesture to start a kinetic scroll in the Linux version of Firefox, it is possible to accelerate the scroll speed by initiating that gesture again while the window is still scrolling. With each successive gesture the scrolling speed doubles, more or less. A similar behavior can be observed with mobile web browsers.

This is a pretty important usability feature, especially when scrolling through very long pages. On macOS kinetic scroll is cumulative and is done on the server side, so it is supported for all applications. On Linux each application/toolkit has to implement kinetic scroll by its own, and currently WebKitGTK doesn't support cumulative kinetic scroll.

This is an alternative, and possibly better, solution for https://bugs.webkit.org/show_bug.cgi?id=197100 and https://bugs.webkit.org/show_bug.cgi?id=166649.

Corresponding Epiphany ticket https://gitlab.gnome.org/GNOME/epiphany/issues/989.
Comment 1 Alice Mikhaylenko 2020-10-26 02:07:03 PDT
Implementing it server-side isn't viable, since on Linux we also have touchscreen scrolling. For touchscreen you have no way of knowing whether this particular swipe was scrolling or something else server-side. With multitouch especially this can get very complex, e.g. consider a pinch gesture like you would use for zooming, but with both fingers being in different windows. In that case both have to be treated like independent pan/scroll gestures with regular kinetic deceleration after.

So all in all implementing it client-side makes total sense to me.

Agree it should be cumulative, especially on touch. Currently Librem 5 ships a downstream patch to reduce the deceleration factor from 4 to 1 to work around touch scrolling being too slow.
Comment 2 Chris Lord 2020-10-30 03:46:31 PDT
Created attachment 412729 [details]
Patch
Comment 3 Chris Lord 2020-10-30 03:53:16 PDT
Some notes;

This patch, rather than discarding all information when the animation is stopped, stops the animation but retains the data. When you initiate a scroll, it calculates what the velocity would've been if the scroll had continued, and if it's in the same direction as the new scroll animation, accumulates that velocity.

Note, the effect is actually quite subtle with the current settings, deceleration is so fast that it takes quite some effort to accumulate velocity (but I've verified programmatically that it's working). I think we may want to consider changing the model settings, or the model itself in a subsequent patch.
Comment 4 Alice Mikhaylenko 2020-10-30 04:25:53 PDT
This should be done in coordination with GTK, we probably want scrolling to behave the same in both.
Comment 5 Yariv 2020-10-30 05:22:44 PDT
A corresponding feature for cumulative scroll is that of using a 2-finger tap (1-finger on touchscreens) to stop kinetic scrolling. It is not a must, however it improves the usability of cumulative kinetic scroll.

Right now a gesture for stopping kinetic scroll is just an RFC on Linux, see https://gitlab.freedesktop.org/libinput/libinput/-/issues/300. macOS already has it implemented, however for Linux it will probably require support from both libinput and GTK.
Comment 6 Chris Lord 2020-10-30 05:42:49 PDT
I'm just looking at implementing this same patch for Gtk at the moment.
Comment 7 Alice Mikhaylenko 2020-10-30 06:16:17 PDT
Bug: if you scroll multiple times and then slowly release, but still in that direction, it will have way higher velocity than it should be.

I have no idea what the proper condition would be to fix this though, but basically, it should somehow distinguish another quick flick (where it should happen) from you essentially stopping existing scrolling and restarting it (where it should not happen).
Comment 8 Chris Lord 2020-10-30 06:23:27 PDT
(In reply to Alexander Mikhaylenko from comment #7)
> Bug: if you scroll multiple times and then slowly release, but still in that
> direction, it will have way higher velocity than it should be.
> 
> I have no idea what the proper condition would be to fix this though, but
> basically, it should somehow distinguish another quick flick (where it
> should happen) from you essentially stopping existing scrolling and
> restarting it (where it should not happen).

I think a better heuristic would be that the velocity is in the same direction and lower than the new initial velocity, I'll update the patch with that.
Comment 9 Chris Lord 2020-10-30 06:26:24 PDT
(In reply to Chris Lord from comment #8)
> (In reply to Alexander Mikhaylenko from comment #7)
> > Bug: if you scroll multiple times and then slowly release, but still in that
> > direction, it will have way higher velocity than it should be.
> > 
> > I have no idea what the proper condition would be to fix this though, but
> > basically, it should somehow distinguish another quick flick (where it
> > should happen) from you essentially stopping existing scrolling and
> > restarting it (where it should not happen).
> 
> I think a better heuristic would be that the velocity is in the same
> direction and lower than the new initial velocity, I'll update the patch
> with that.

On thinking about this, I immediately disagree with myself, that was short-sighted... I guess there needs to be some kind of variable that controls how close the velocities are before accumulating them...
Comment 10 Alice Mikhaylenko 2020-10-30 06:34:15 PDT
Honestly IDK if we even have the data for this atm. This might need research on how other platforms do it, e.g. Android.
Comment 11 Chris Lord 2020-10-30 07:36:07 PDT
Created attachment 412740 [details]
Patch
Comment 12 Chris Lord 2020-10-30 07:36:58 PDT
Created attachment 412742 [details]
Patch
Comment 13 Chris Lord 2020-10-30 07:40:12 PDT
This now only accumulates velocity if newVelocity >= currentVelocity * accumulationRatio (0.5). I'm going to look at what Firefox does, but given some of the odd behaviour I see while intentionally scrolling in weird ways to examine behaviour on Firefox for Android, I don't think it's really too important right now as long as what we do doesn't feel obviously bad/broken.
Comment 14 Chris Lord 2020-10-30 11:46:00 PDT
The equivalent patch for GTK: https://gitlab.gnome.org/GNOME/gtk/-/merge_requests/2768
Comment 15 Chris Lord 2020-11-09 08:38:38 PST
Created attachment 413586 [details]
Patch
Comment 16 Chris Lord 2020-11-09 09:19:23 PST
Created attachment 413593 [details]
Patch
Comment 17 Chris Lord 2020-11-09 09:20:29 PST
Comment on attachment 413593 [details]
Patch

Patch updated to match Gtk patch that just got merged.
Comment 18 EWS 2020-11-09 09:55:30 PST
Committed r269583: <https://trac.webkit.org/changeset/269583>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 413593 [details].