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.
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.
https://en.wikipedia.org/wiki/Damping https://en.wikipedia.org/wiki/Mass-spring-damper_model
This feature isn't ready yet, how did it get turned on?
That said, I don't think we were aware of this particular problem.
(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
Epiphany (WebKitGTK) has a bug report. Bug 232701 – [GTK] Pressing "End" keyboard key on instagram.com leads to endless flickering scrolling
<rdar://problem/90106824>
255031@main (bug#228159) enabled EventHandlerDrivenSmoothKeyboardScrollingEnabled by default. Now, I can reproduce this issue even with WinCairo MiniBrowser Release build.
Oops, sorry. We probably should have only enabled for cocoa ports. Richard?
(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.
Pull request: https://github.com/WebKit/WebKit/pull/4914
It's assuming that frameDuration is short (comment#1). I think we should choose more robust algorithm.
(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?
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 = { };
Committed 255099@main (5f888cf9998e): <https://commits.webkit.org/255099@main> Reviewed commits have been landed. Closing PR #4914 and removing active labels.
I misunderstood a little. The problem reproducible by Release build + PageDown is a bit different issue. I filed bug#245982 to the bug.
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
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.
Created attachment 462846 [details] Patch
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.
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).
(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.
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].