Summary: | [GTK] kinetic scroll speed should be cumulative | ||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Yariv <oigevald+webkit> | ||||||||||||
Component: | WebKitGTK | Assignee: | Chris Lord <clord> | ||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||
Severity: | Normal | CC: | alicem, bugs-noreply, cgarcia, clord, mcatanzaro | ||||||||||||
Priority: | P2 | ||||||||||||||
Version: | WebKit Nightly Build | ||||||||||||||
Hardware: | PC | ||||||||||||||
OS: | Linux | ||||||||||||||
See Also: |
https://bugs.webkit.org/show_bug.cgi?id=197100 https://bugs.webkit.org/show_bug.cgi?id=166649 |
||||||||||||||
Attachments: |
|
Description
Yariv
2019-11-06 12:53:02 PST
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. Created attachment 412729 [details]
Patch
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. This should be done in coordination with GTK, we probably want scrolling to behave the same in both. 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. I'm just looking at implementing this same patch for Gtk at the moment. 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). (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. (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... Honestly IDK if we even have the data for this atm. This might need research on how other platforms do it, e.g. Android. Created attachment 412740 [details]
Patch
Created attachment 412742 [details]
Patch
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. The equivalent patch for GTK: https://gitlab.gnome.org/GNOME/gtk/-/merge_requests/2768 Created attachment 413586 [details]
Patch
Created attachment 413593 [details]
Patch
Comment on attachment 413593 [details]
Patch
Patch updated to match Gtk patch that just got merged.
Committed r269583: <https://trac.webkit.org/changeset/269583> All reviewed patches have been landed. Closing bug and clearing flags on attachment 413593 [details]. |