Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add animation to vertical wheel scrolling for smoother experience. #10509

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

gokaysatir
Copy link
Contributor

Change-Id: I5c6cce08f68ec6ac99ac668bb78c12274ce72f3e

  • Resolves: #
  • Target version: master

Summary

TODO

  • ...

Checklist

  • I have run make prettier-write and formatted the code.
  • All commits have Change-Id
  • I have run tests with make check
  • I have issued make run and manually verified that everything looks okay
  • Documentation (manuals or wiki) has been updated or is not required

@mmeeks
Copy link
Contributor

mmeeks commented Nov 18, 2024

This may be smoother - if so good, but it 'feels' awful with a touchpad - I can't control where I'm scrolling to at all it seems - all fine control is lost :-) please re-test and re-work for touchpad.

@gokaysatir gokaysatir force-pushed the private/gokay/scroll-desktop branch 2 times, most recently from 3d6e7c5 to 90dea55 Compare November 19, 2024 08:09
…nctionality.

It's not used anywhere and it's buggy. Sections should check the events and stop animating when they need.

Signed-off-by: Gökay Şatır <gokaysatir@collabora.com>
Change-Id: I32cc878228623e54f280ace1b5d1d75b93188df1
Signed-off-by: Gökay Şatır <gokaysatir@collabora.com>
Change-Id: I5c6cce08f68ec6ac99ac668bb78c12274ce72f3e
@eszkadev
Copy link
Contributor

It works much worse with laptop trackpad with this change. Previously it was smooth

@mmeeks
Copy link
Contributor

mmeeks commented Nov 19, 2024

It works much worse with laptop trackpad with this change. Previously it was smooth

That's not my experience - I think there has been significant scope for improvement here for a good while.

@Cwiiis
Copy link
Contributor

Cwiiis commented Nov 20, 2024

Without this patch, my experience is that trackpad scrolling is smooth and wheel-scrolling is instant (but has a jerky appearance as it's scrolling a large area at once). With this patch, trackpad scrolling is broken (hard to describe exactly, will go into more detail below) and wheel scrolling is smoother, but not as smooth as I'd expect (i.e. <60Hz), at least in Writer.

Trackpad scrolling is broken because onMouseWheel will always animate, regardless of the magnitude or frequency of the wheel events - so in the case of a trackpad, you very frequently[1] receive events of small magnitude. Each event stops and resets the last animation and so it only ever scrolls a tiny fraction of a tiny amount until it stops receiving events.

I'm not sure why scrollwheel scrolling isn't smooth, I would need to profile - it should be driven by requestAnimationFrame and so it not hitting the screen refresh tells me it's doing too much work.

There are two problems:
1- We need to distinguish between wheel events and trackpad events. The latter need to perform scrolling directly, as they did prior to this patch. The bad news is there isn't a reliable way to do this (well, there is in Firefox and possible in some backends of WebKit, but that's not all that useful).
2- When an animation is interrupted, we need to sensibly take into account the old destination and adjust the new destination accordingly. You shouldn't be able to scroll faster prior to this patch, essentially.

Solving the second problem would make trackpad scrolling work, but feel a bit laggy compared to fixing the first. When a wheel event is received while a previous scroll is still animating, I would suggest altering the animation parameters so that the first animation is extended to end at the sum of the two deltas. The new animation's start velocity needs to match its current velocity for it not to feel weird.

For the first problem, in Firefox, wheel events have deltaMode[2] property set to LINE. This isn't the case in Chrome it seems, unfortunately, so you need a heuristic. My suggestion would be any wheel event less than half a line height received in quick succession of another (where quick = a screen refresh) deactivating animation until no more events are received for a few frames (let's say 50ms).

My more left-field suggestion would be to structure things so that we can take advantage of the browser's native scrolling support, which will be correct per platform and take away a bunch of maintenance burden - but given how things are organised now, this wouldn't be simple and is a more long-term thing.

[1] the browser should throttle this at the screen refresh, but I don't know that this is always true for all browsers. In many years past in engines I don't remember, this would sometimes just be the refresh of the device - which can be many times the screen refresh...
[2] https://developer.mozilla.org/en-US/docs/Web/API/WheelEvent/deltaMode

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: To Review
Development

Successfully merging this pull request may close these issues.

4 participants