After multiple bisections, history: 1. https://trac.webkit.org/changeset/246033/webkit disabled/broke kinetic scrolling 2. https://trac.webkit.org/changeset/246467/webkit broke scrolling completely 3. https://trac.webkit.org/changeset/246494/webkit the last change was reverted 4. https://trac.webkit.org/changeset/246496/webkit the second version of the same change, now scrolling jumps at the end instead.
In case it helps: The scrolling at the end of pages also happens to me when using the touchpad. As a matter of fact, any swipe to scroll down always results in a small upwards movement after the scrolling ends. It happens both with smooth-scrolling enabled and disabled.
We experience that bug on the Librem 5 devkit too: https://source.puri.sm/Librem5/Apps_Issues/issues/121. I also tried a touchscreen plugged into my Fedora workstation, using Epiphany Technology Preview, and I can reproduce all of that, including jumping right at the end.
Created attachment 373315 [details] Patch
Attachment 373315 [details] did not pass style-queue: ERROR: Source/WebCore/ChangeLog:11: You should remove the 'No new tests' and either add and list tests, or explain why no new tests were possible. [changelog/nonewtests] [5] Total errors found: 1 in 5 files If any of these errors are false positives, please file a bug against check-webkit-style.
I thought there were tests for async scrolling... This would be tricky to test, since there's currently no mechanism for setting source device type for events generated by WebKitTestRunner. I had to explicitly allow any device types for simulated events in ViewGestureController because of this.
Comment on attachment 373315 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=373315&action=review > Source/WebCore/ChangeLog:11 > + No new tests (OOPS!). Just remove this line.
Created attachment 373317 [details] Patch
Comment on attachment 373317 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=373317&action=review > Source/WebCore/ChangeLog:3 > + REGRESSION(r246033/r246496): [GTK] Kinetic scrolling doesn't work Do we know now which revision made the code depend on async scrolling and why? Could we update the title accordingly?
Comment on attachment 373317 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=373317&action=review >> Source/WebCore/ChangeLog:3 >> + REGRESSION(r246033/r246496): [GTK] Kinetic scrolling doesn't work > > Do we know now which revision made the code depend on async scrolling and why? Could we update the title accordingly? Do we know now which revision made the code depend on async scrolling and why? Could we update the title accordingly? > Source/WebCore/platform/PlatformWheelEvent.h:51 > -#if ENABLE(ASYNC_SCROLLING) > +#if ENABLE(ASYNC_SCROLLING) || PLATFORM(GTK) I'm asking because this is weird, there might be a better condition
(In reply to Carlos Garcia Campos from comment #9) > Do we know now which revision made the code depend on async scrolling and > why? Could we update the title accordingly? r238928. I don't think that revision should be the REGRESSION in the title, though, because it didn't break anything. ENABLE(ASYNC_SCROLLING) was guaranteed to be on for GTK/WPE since r231043 until r246033 so the regression is from r246033.
(In reply to Michael Catanzaro from comment #10) > ENABLE(ASYNC_SCROLLING) was > guaranteed to be on for GTK/WPE since r231043 until r246033 Er, except with -DENABLE_OPENGL=OFF
https://github.com/WebKit/webkit/commit/abd7839603c75d500d5341a15c99e829827c2463#diff-2e0f3a82f1ba4521bd1823259f8f1084L149 The relevant bug: https://bugs.webkit.org/show_bug.cgi?id=186374 I don't see explanations there, though I missed WPE.
So, what should the check be at the end? It was `PLATFORM(COCOA) || PLATFORM(GTK) || PLATFORM(WPE)` before and variations on that. Maybe it should be `ENABLE_OPENGL` instead, though that seems weird too. What does it have to do with opengl?..
I'll let Carlos Garcia comment on his preferred choice of guards, since it's clear that the people who've edited these guards in the past maybe did not choose the best ones. Regardless, we should probably test a build with -DENABLE_OPENGL=OFF and see how it fails. It was probably changed to require ENABLE_OPENGL for a reason.
Can anyone with a computer that doesn't overheat run that build? :) I'd _really_ not do a clean webkit build again.
(In reply to Alexander Mikhaylenko from comment #13) > So, what should the check be at the end? > > It was `PLATFORM(COCOA) || PLATFORM(GTK) || PLATFORM(WPE)` before and > variations on that. That makes more sense to me, but it seems cocoa port only wants that when async scrolling is enabled. > Maybe it should be `ENABLE_OPENGL` instead, though that > seems weird too. What does it have to do with opengl?.. Maybe we can add ENABLE_WHEEL_EVENT_PHASE to Platform.h that is 1 for PLATFORM(GTK) || PLATFORM(WPE) || (PLATFORM(COCOA) && ENABLE(ASYNC_SCROLLING)). But, I don't want to make this more complicated, if you guys are ok with current patch, it's fine with me too.
(In reply to Alexander Mikhaylenko from comment #15) > Can anyone with a computer that doesn't overheat run that build? :) I'd > _really_ not do a clean webkit build again. Yes, I'll test once we have a final patch proposal. (In reply to Carlos Garcia Campos from comment #16) > Maybe we can add ENABLE_WHEEL_EVENT_PHASE to Platform.h that is 1 for > PLATFORM(GTK) || PLATFORM(WPE) || (PLATFORM(COCOA) && > ENABLE(ASYNC_SCROLLING)). That seems nicer. Or should it be ENABLE_KINETIC_SCROLLING if wheel event phase == kinetic scrolling? (What is wheel event phase?)
> (What is wheel event phase?) ``` enum PlatformWheelEventPhase : uint8_t { PlatformWheelEventPhaseNone = 0, PlatformWheelEventPhaseBegan = 1 << 0, PlatformWheelEventPhaseStationary = 1 << 1, PlatformWheelEventPhaseChanged = 1 << 2, PlatformWheelEventPhaseEnded = 1 << 3, PlatformWheelEventPhaseCancelled = 1 << 4, PlatformWheelEventPhaseMayBegin = 1 << 5, }; ``` But that's mostly a mac thing, on GTK it's Ended if gdk_event_is_scroll_stop_event() else Changed. If it's Ended, we can try to trigger kinetic scrolling, otherwise follow fingers.
(In reply to Carlos Garcia Campos from comment #16) > But, I don't want to make this more complicated, if you guys are ok with > current patch, it's fine with me too. The current patch is missing WPE, so I need to update it anyway. So adding ENABLE_WHEEL_EVENT_PHASE/ENABLE_KINETIC_SCROLLING should be fine, since that would make it a lot cleaner.
(In reply to Alexander Mikhaylenko from comment #0) > 4. https://trac.webkit.org/changeset/246496/webkit the second version of the > same change, now scrolling jumps at the end instead. Does your patch fix this somehow? Looks like it only addresses the first problem?
I cannot reproduce the second problem with the patch, so I guess the jumping was another symptom of lack of kinetic scrolling handling. Though there's a chance it still shows up with -DENABLE_OPENGL=OFF :/
Comment on attachment 373317 [details] Patch Forgot about this. We had agreed to change the guards here.
Created attachment 374461 [details] Patch
Please fix WPE: In file included from DerivedSources/WebKit/unified-sources/UnifiedSource-50d0d8dd-5.cpp:4:0: ../../Source/WebKit/Shared/WebEventConversion.cpp: In constructor ‘WebKit::WebKit2PlatformWheelEvent::WebKit2PlatformWheelEvent(const WebKit::WebWheelEvent&)’: ../../Source/WebKit/Shared/WebEventConversion.cpp:161:74: error: ‘const class WebKit::WebWheelEvent’ has no member named ‘phase’ m_phase = static_cast<WebCore::PlatformWheelEventPhase>(webEvent.phase()); ^~~~~ ../../Source/WebKit/Shared/WebEventConversion.cpp:162:82: error: ‘const class WebKit::WebWheelEvent’ has no member named ‘momentumPhase’ m_momentumPhase = static_cast<WebCore::PlatformWheelEventPhase>(webEvent.momentumPhase()); ^~~~~~~~~~~~~
Problem is WebWheelEvent in WebEvent.h, it uses #if PLATFORM(COCOA) and #if PLATFORM(GTK) guards but isn't prepared for PLATFORM(WPE). I think it would probably be easy to adjust the guards to work for WPE. If not, could always disable kinetic scrolling for WPE.
Huh. I wonder why it worked before then. Is kinetic scrolling on WPE generally a thing?
Honestly, I doubt anybody uses a mouse with WPE so who knows. Obviously it can't be too important since it was disabled so casually.
Well, it was disabled for GTK as well, and yet it broke scrolling on touchpad, touchscreen and trackpoint. :p Ok, not enabling it for now, after all the issue has [GTK] in the title 🤷
Created attachment 374493 [details] Patch
Comment on attachment 374493 [details] Patch The Windows EWS failure appears to be unrelated.
Comment on attachment 374493 [details] Patch Rejecting attachment 374493 [details] from commit-queue. Failed to run "['/Volumes/Data/EWS/WebKit/Tools/Scripts/webkit-patch', '--status-host=webkit-queues.webkit.org', '--bot-id=webkit-cq-01', 'validate-changelog', '--check-oops', '--non-interactive', 374493, '--port=mac']" exit_code: 1 cwd: /Volumes/Data/EWS/WebKit /Volumes/Data/EWS/WebKit/Source/WTF/ChangeLog neither lists a valid reviewer nor contains the string "Unreviewed" or "Rubber stamp" (case insensitive). Full output: https://webkit-queues.webkit.org/results/12779237
Comment on attachment 374493 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=374493&action=review > Source/WTF/ChangeLog:6 > + REGRESSION(r246033/r246496): [GTK] Kinetic scrolling doesn't work > + https://bugs.webkit.org/show_bug.cgi?id=199322 > + > + Introduce ENABLE_KINETIC_SCROLLING to explicitly always have kinetic scrolling on GTK. Uh-oh, you removed the Reviewed by (NOBODY!) lines. You need to keep those or they won't be rewritten.
Oops, sorry
Created attachment 374549 [details] Patch
Comment on attachment 374549 [details] Patch Clearing flags on attachment: 374549 Committed r247670: <https://trac.webkit.org/changeset/247670>
All reviewed patches have been landed. Closing bug.