Bug 197100 – [GTK] Slow scrolling (not matching GTK native scroll amount)

• Component: Scrolling
• Status: RESOLVED FIXED

[François Grabenstaetter]

The WebKitGTK scrolling In Epiphany browser is too slow and does not match the GTK native scrolling amount.

A similar ticket asks for options to adjust scroll amount and scroll acceleration here: https://bugs.webkit.org/show_bug.cgi?id=166649
But I think we should start to make the default scroll amount match the GTK native scroll.

Steps to Reproduce:
* Open Epiphany and a website
* Scroll with the mouse

Actual Results:
=> Too slow scrolling

Expected Results: 
=> A scrolling that matches the GTK native scrolling (~ X2 scroll amount ?)

[Alice Mikhaylenko]

More info:

- This doesn't affect touchpad/per-pixel scrolling
- This happens regardless of async scrolling or not
- Happens with keyboard too

An easy reproducer - hold pgdown on https://planet.webkitgtk.org/

My theory is - it's related to smooth scrolling, when you have quickly incoming scroll events, the next event cancels animation from the last one and triggers a new animation without compensating for the fact the last animation stopped early. So if you press pgdown repeatedly but wait for the animation to finish each time, it's a lot faster than when you hold it, BUT the first (because key repeat delay) and last animations finish and so it's visible how the beginning and end of the scrolling is faster.

That happens in trunk though - 2.33.91 (sorry, 2.34.0 is not in the flatpak platform yet) seems to do something different and pgdown is too fast instead (while mouse scrolling is same as in trunk) - no idea here.

[Chris Lord]

(In reply to Alexander Mikhaylenko from comment #1)
> More info:
> 
> - This doesn't affect touchpad/per-pixel scrolling
> - This happens regardless of async scrolling or not
> - Happens with keyboard too
> 
> An easy reproducer - hold pgdown on https://planet.webkitgtk.org/
> 
> My theory is - it's related to smooth scrolling, when you have quickly
> incoming scroll events, the next event cancels animation from the last one
> and triggers a new animation without compensating for the fact the last
> animation stopped early. So if you press pgdown repeatedly but wait for the
> animation to finish each time, it's a lot faster than when you hold it, BUT
> the first (because key repeat delay) and last animations finish and so it's
> visible how the beginning and end of the scrolling is faster.
> 
> That happens in trunk though - 2.33.91 (sorry, 2.34.0 is not in the flatpak
> platform yet) seems to do something different and pgdown is too fast instead
> (while mouse scrolling is same as in trunk) - no idea here.

Testing on master, I don't see this problem - behaviour between smooth and non-smooth in terms of the distance scrolled per mouse-wheel ticks in very quick succession (so they interrupt the animation) is the same.

However, I do see the interrupted smooth scrolling issue with keyboard scrolling (which is a different code-path, so doesn't surprise me). Could you possibly test again using minibrowser from a master checkout and update here what results you get and what you expect?

For reference, here is the code that stops this from happening with smooth scrolling from mouse-wheel events (and it's shared between sync and async paths): https://github.com/WebKit/WebKit/blob/a7eb93bd9b7414f627d8c7038a7163f3cc3945fd/Source/WebCore/platform/ScrollingEffectsController.cpp#L344

[Chris Lord]

For now I'm looking at the keyboard scrolling code and seeing what we can do here. It doesn't look like the code is Mac-specific, but I don't think we call into it (and async side, we don't have a keyboard scrolling animator at all), so there's some work to be done here.

[Chris Lord]

Created attachment 441740 [details]
Patch

[Chris Lord]

So this patch is what I consider "good enough", but isn't the most ideal thing. For interrupted smooth scrolling, this patch makes it recalculate the duration and change the animation curve from ease-in-out to ease-out - at least perceptibly, this basically does what we want, but isn't mathematically correct.

Regarding the keyboard, this adds a new function to ScrollingEffectsController called offsetAnimatedScrollDestination exactly for this situation, and the previous special-case for mouse-wheel smooth scrolling now just calls this function.

In terms of the 'correct' way of implementing this, we would ideally use the inverse of the animation curve calculation to make the perfect alteration of animation parameters to extend the existing ease-in-out animation in a perfect way. I've done this before, but in practice it's a pain, and it'd be much easier to get rid of an animation curve altogether and use a very basic physics simulation to control the animation instead.

I notice that ScrollingAnimationMomentum is basically a reimplementation of the ScrollingAnimationKinetic interface, but allowing pluggable behaviour - At some point we need to convert ScrollingAnimationKinetic into a GenericScrollingMomentumCalculator and share more code. It's a shame this wasn't done as part of the patch that added it in the first place.

[Chris Lord]

Unfortunately I'm out of time today as well, so I'll have to rebase this tomorrow - if anyone gets to reviewing this, do feel free to go ahead if you think the rebase doesn't change things in any meaningful way.

[Simon Fraser (smfr)]

Comment on attachment 441740 [details]
Patch

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

> Source/WebCore/platform/ScrollAnimation.h:78
> +    virtual FloatPoint predictedDestinationOffset() const = 0;

The "predicted" part is only true for momentum animations; for other animations, the destination is specified up-front. Maybe this should just be std::optional<FloatPoint> destinationOffset()?

> Source/WebCore/platform/ScrollAnimationSmooth.cpp:62
> +    // FIXME: This is a hangover of previous behaviour, but given there's a specific
> +    //        retargetActiveAnimation function, maybe this should always restart the animation?

Yeah I think this should always use ease-in-out.

> Source/WebCore/platform/ScrollingEffectsController.h:138
> +    bool offsetAnimatedScrollDestination(FloatSize offset);

Use of "offset" as a verb here is confusing. We don't do that anywhere else in scrolling code. Maybe retargetAnimatedScrollBy(FloatSize)

[Chris Lord]

(In reply to Simon Fraser (smfr) from comment #7)
> Comment on attachment 441740 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=441740&action=review
> 
> > Source/WebCore/platform/ScrollAnimation.h:78
> > +    virtual FloatPoint predictedDestinationOffset() const = 0;
> 
> The "predicted" part is only true for momentum animations; for other
> animations, the destination is specified up-front. Maybe this should just be
> std::optional<FloatPoint> destinationOffset()?
> 
> > Source/WebCore/platform/ScrollAnimationSmooth.cpp:62
> > +    // FIXME: This is a hangover of previous behaviour, but given there's a specific
> > +    //        retargetActiveAnimation function, maybe this should always restart the animation?
> 
> Yeah I think this should always use ease-in-out.
> 
> > Source/WebCore/platform/ScrollingEffectsController.h:138
> > +    bool offsetAnimatedScrollDestination(FloatSize offset);
> 
> Use of "offset" as a verb here is confusing. We don't do that anywhere else
> in scrolling code. Maybe retargetAnimatedScrollBy(FloatSize)

These are all excellent suggestions, implementing now.

[Chris Lord]

Created attachment 441891 [details]
Patch

[Simon Fraser (smfr)]

Comment on attachment 441891 [details]
Patch

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

> Source/WebCore/platform/ScrollAnimationSmooth.cpp:81
> +    m_timingFunction = CubicBezierTimingFunction::create(CubicBezierTimingFunction::TimingFunctionPreset::EaseOut);

It's possible that this won't result in a smooth transition from old to new curve. Ideally we'd keep the ease-in-out and just readjust the current time within the existing interval.

[Chris Lord]

(In reply to Simon Fraser (smfr) from comment #10)
> Comment on attachment 441891 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=441891&action=review
> 
> > Source/WebCore/platform/ScrollAnimationSmooth.cpp:81
> > +    m_timingFunction = CubicBezierTimingFunction::create(CubicBezierTimingFunction::TimingFunctionPreset::EaseOut);
> 
> It's possible that this won't result in a smooth transition from old to new
> curve. Ideally we'd keep the ease-in-out and just readjust the current time
> within the existing interval.

Agreed/understood, though doing the inverse of the ease-in-out calculation is non-trivial and try as I might, given the parameters going in on a GNOME3 desktop at least, I couldn't get this to look or feel bad... So I went for the simpler fix. Worth reinvestigating when more of this code is shared between all platforms.

[EWS]

Committed r284596 (243327@main): <https://commits.webkit.org/243327@main>

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

[Radar WebKit Bug Importer]

<rdar://problem/84499836>