REGRESSION(r246033/r246496): [GTK] Kinetic scrolling doesn't work

• Component: WebKitGTK
• Status: RESOLVED FIXED

[Alice Mikhaylenko]

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.

[Adrian Perez]

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.

[Adrien Plazas]

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.

[Alice Mikhaylenko]

Created attachment 373315 [details]
Patch

[EWS Watchlist]

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.

[Alice Mikhaylenko]

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.

[Michael Catanzaro]

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.

[Alice Mikhaylenko]

Created attachment 373317 [details]
Patch

[Carlos Garcia Campos]

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?

[Carlos Garcia Campos]

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

[Michael Catanzaro]

(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.

[Michael Catanzaro]

(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

[Alice Mikhaylenko]

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.

[Alice Mikhaylenko]

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?..

[Michael Catanzaro]

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.

[Alice Mikhaylenko]

Can anyone with a computer that doesn't overheat run that build? :) I'd _really_ not do a clean webkit build again.

[Carlos Garcia Campos]

(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.

[Michael Catanzaro]

(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?)

[Alice Mikhaylenko]

> (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.

[Alice Mikhaylenko]

(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.

[Michael Catanzaro]

(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?

[Alice Mikhaylenko]

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 :/

[Michael Catanzaro]

Comment on attachment 373317 [details]
Patch

Forgot about this. We had agreed to change the guards here.

[Alice Mikhaylenko]

Created attachment 374461 [details]
Patch

[Michael Catanzaro]

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());
                                                                                  ^~~~~~~~~~~~~

[Michael Catanzaro]

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.

[Alice Mikhaylenko]

Huh. I wonder why it worked before then. Is kinetic scrolling on WPE generally a thing?

[Michael Catanzaro]

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.

[Alice Mikhaylenko]

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 🤷

[Alice Mikhaylenko]

Created attachment 374493 [details]
Patch

[Michael Catanzaro]

Comment on attachment 374493 [details]
Patch

The Windows EWS failure appears to be unrelated.

[WebKit Commit Bot]

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

[Michael Catanzaro]

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.

[Alice Mikhaylenko]

Oops, sorry

[Alice Mikhaylenko]

Created attachment 374549 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 374549 [details]
Patch

Clearing flags on attachment: 374549

Committed r247670: <https://trac.webkit.org/changeset/247670>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.