keyboard scrolling spring damping animation doesn't finish and leads to endless flickering scrolling

• Component: Scrolling
• Status: RESOLVED FIXED

[Fujii Hironori]

keyboard scrolling spring damping animation doesn't finish in WinCairo Debug with debug logging

1. Start WinCairo Debug with enabling some debug logging
   $env:WebCoreLogging = "Scrolling"
   devenv -debugexe .\WebKitBuild\Debug\bin64\MiniBrowser.exe
   Turn "Enable child process debugging" on
   Start Debugging
2. Go to a heavy and long page. For example, https://news.google.com/ and https://edition.cnn.com/
3. Press Page-Down key

Expected: The scrolling animation stops.
Actual: The scrolling animation never stops. It goes back and forth.

[Fujii Hironori]

https://github.com/WebKit/WebKit/blob/b736c55b81845e8e159eb97b3fcba1a8315da7d1/Source/WebCore/platform/KeyboardScrollingAnimator.cpp#L125,L127

>  FloatSize acceleration = force.scaled(1. / params.springMass);
>  m_velocity += acceleration.scaled(frameDuration);
>  FloatPoint newPosition = m_scrollAnimator.currentPosition() + m_velocity.scaled(frameDuration);

This expressions are assuming that frameDuration is short.

[Fujii Hironori]

https://en.wikipedia.org/wiki/Damping
https://en.wikipedia.org/wiki/Mass-spring-damper_model

[Tim Horton]

This feature isn't ready yet, how did it get turned on?

[Tim Horton]

That said, I don't think we were aware of this particular problem.

[Fujii Hironori]

(In reply to Tim Horton from comment #3)
> This feature isn't ready yet, how did it get turned on?

Good to know. Then, it's another bug.
Filed: Bug 229733 – REGRESSION(r280928) The smooth keyboard scrolling is enabled only for PageUp and PageDown keys

[Fujii Hironori]

Epiphany (WebKitGTK) has a bug report.

Bug 232701 – [GTK] Pressing "End" keyboard key on instagram.com leads to endless flickering scrolling

[Radar WebKit Bug Importer]

<rdar://problem/90106824>

[Fujii Hironori]

255031@main (bug#228159) enabled EventHandlerDrivenSmoothKeyboardScrollingEnabled by default.
Now, I can reproduce this issue even with WinCairo MiniBrowser Release build.

[Tim Horton]

Oops, sorry. We probably should have only enabled for cocoa ports. Richard?

[Richard Robinson]

(In reply to Tim Horton from comment #9)
> Oops, sorry. We probably should have only enabled for cocoa ports. Richard?

Yup, I'm on it.

[Richard Robinson]

Pull request: https://github.com/WebKit/WebKit/pull/4914

[Fujii Hironori]

It's assuming that frameDuration is short (comment#1). I think we should choose more robust algorithm.

[Tim Horton]

(In reply to Fujii Hironori from comment #12)
> It's assuming that frameDuration is short (comment#1). I think we should
> choose more robust algorithm.

This has been shipping for years on iOS.
Why is frameDuration *not* short for you?

[Fujii Hironori]

Created attachment 462767 [details]
log

I added the following log. I pressed down arrow key and page down key in https://webkit.org/ .
The sprint went into divergent vibration.

diff --git a/Source/WebCore/platform/KeyboardScrollingAnimator.cpp b/Source/WebCore/platform/KeyboardScrollingAnimator.cpp
index 9863ebac3fd6..d604c3e15560 100644
--- a/Source/WebCore/platform/KeyboardScrollingAnimator.cpp
+++ b/Source/WebCore/platform/KeyboardScrollingAnimator.cpp
@@ -218,6 +218,7 @@ void KeyboardScrollingAnimator::updateKeyboardScrollPosition(MonotonicTime curre
 
     m_scrollAnimator.scrollToPositionWithoutAnimation(newPosition);
 
+    ALWAYS_LOG_WITH_STREAM(stream << "frameDuration:" << frameDuration << " newPosition:" << newPosition << " m_velocity:" << m_velocity);
     if (!m_scrollTriggeringKeyIsPressed && m_velocity.diagonalLengthSquared() < 1) {
         m_scrollController.didStopKeyboardScrolling();
         m_velocity = { };

[EWS]

Committed 255099@main (5f888cf9998e): <https://commits.webkit.org/255099@main>

Reviewed commits have been landed. Closing PR #4914 and removing active labels.

[Fujii Hironori]

I misunderstood a little. The problem reproducible by Release build + PageDown is a bit different issue.
I filed bug#245982 to the bug.

[Fujii Hironori]

I tried comment#0 step. I just pressed a single down arrow key in this case.

frameDuration:0.01 newPosition:(0,0.05) m_velocity:width=0 height=7.30
frameDuration:0.04 newPosition:(0,1.69) m_velocity:width=0 height=44.33
frameDuration:0.14 newPosition:(0,73.07) m_velocity:width=0 height=507.16
frameDuration:0.20 newPosition:(0,-375.81) m_velocity:width=0 height=-2243.49
frameDuration:0.09 newPosition:(0,193.07) m_velocity:width=0 height=2158.95
frameDuration:0.33 newPosition:(0,-5452.72) m_velocity:width=0 height=-17336.29
frameDuration:0.14 newPosition:(0,4149.97) m_velocity:width=0 height=30474.31
frameDuration:3.10 newPosition:(0,-9244337) m_velocity:width=0 height=-2981108.50
frameDuration:1.02 newPosition:(0,58668016) m_velocity:width=0 height=57673556
frameDuration:1.92 newPosition:(0,-4164551680.00) m_velocity:width=0 height=-2163899648.00
frameDuration:0.98 newPosition:(0,39856992256.00) m_velocity:width=0 height=40464424960.00
frameDuration:0.72 newPosition:(0,-385372749824.00) m_velocity:width=0 height=-538594639872.00
frameDuration:1.12 newPosition:(0,12795050459136.00) m_velocity:width=0 height=11473761861632.00

[Fujii Hironori]

This bug is reproducible for Mac port MiniBrowser (Debug build) on my Mac mini (2018).
1. Enable "Scrolling" debug log channel
2. Start MiniBrowser (Debug build)
3. Go to https://edition.cnn.com/
4. Press arrow keys and PageUp and Page Down keys to scroll
Not 100% reproducible. But, it's easy to reproduce.

[Fujii Hironori]

Created attachment 462846 [details]
Patch

[Tim Horton]

Comment on attachment 462846 [details]
Patch

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

> Source/WebCore/platform/KeyboardScrollingAnimator.cpp:223
> +    if (axesToApplySpring.width() && displacement.width() * newDisplacement.width() < 0)

This is fine, though I bet you want an areEssentiallyEqual / FLT_EPSILON / whatever?

(Also separately super confused about what's going on with your frame times, and why this was never necessary on iOS (not sure if there was a mistake when the code got moved here, or what)). But that's ok!

Thank you for fixing.

[Fujii Hironori]

Comment on attachment 462846 [details]
Patch

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

Thank you very much for the review.

>> Source/WebCore/platform/KeyboardScrollingAnimator.cpp:223
>> +    if (axesToApplySpring.width() && displacement.width() * newDisplacement.width() < 0)
> 
> This is fine, though I bet you want an areEssentiallyEqual / FLT_EPSILON / whatever?
> 
> (Also separately super confused about what's going on with your frame times, and why this was never necessary on iOS (not sure if there was a mistake when the code got moved here, or what)). But that's ok!
> 
> Thank you for fixing.

The first condition "axesToApplySpring.width()" is checking if x-axis is to apply spring.
The second condition "displacement.width() * newDisplacement.width() < 0" is checking if either of the previous or next x-axis displacement is negative and the other is positive.
I don't think areEssentiallyEqual is needed here.

It should be reproducible even for iOS if we can slow down the scrolling somehow or if we use a stronger spring (bigger params.springStiffness).

[Tim Horton]

(In reply to Fujii Hironori from comment #21)
> Comment on attachment 462846 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=462846&action=review
> 
> 
> The first condition "axesToApplySpring.width()" is checking if x-axis is to
> apply spring.
> The second condition "displacement.width() * newDisplacement.width() < 0" is
> checking if either of the previous or next x-axis displacement is negative
> and the other is positive.
> I don't think areEssentiallyEqual is needed here.

Right! I misread. Apologies.

[EWS]

Committed 255306@main (367b4c1a2c7f): <https://commits.webkit.org/255306@main>

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