Bug 199322 - REGRESSION(r246033/r246496): [GTK] Kinetic scrolling doesn't work
Summary: REGRESSION(r246033/r246496): [GTK] Kinetic scrolling doesn't work
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKitGTK (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2019-06-28 10:06 PDT by Alexander Mikhaylenko
Modified: 2019-07-20 13:11 PDT (History)
13 users (show)

See Also:


Attachments
Patch (6.50 KB, patch)
2019-07-02 05:03 PDT, Alexander Mikhaylenko
no flags Details | Formatted Diff | Diff
Patch (6.46 KB, patch)
2019-07-02 06:05 PDT, Alexander Mikhaylenko
no flags Details | Formatted Diff | Diff
Patch (7.56 KB, patch)
2019-07-19 06:18 PDT, Alexander Mikhaylenko
no flags Details | Formatted Diff | Diff
Patch (7.54 KB, patch)
2019-07-19 13:56 PDT, Alexander Mikhaylenko
no flags Details | Formatted Diff | Diff
Patch (7.65 KB, patch)
2019-07-20 06:20 PDT, Alexander Mikhaylenko
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Alexander Mikhaylenko 2019-06-28 10:06:44 PDT
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.
Comment 1 Adrian Perez 2019-06-28 10:16:04 PDT
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.
Comment 2 Adrien Plazas 2019-07-02 01:33:16 PDT
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.
Comment 3 Alexander Mikhaylenko 2019-07-02 05:03:36 PDT
Created attachment 373315 [details]
Patch
Comment 4 Build Bot 2019-07-02 05:04:35 PDT
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.
Comment 5 Alexander Mikhaylenko 2019-07-02 05:07:35 PDT
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 6 Michael Catanzaro 2019-07-02 06:03:02 PDT
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.
Comment 7 Alexander Mikhaylenko 2019-07-02 06:05:30 PDT
Created attachment 373317 [details]
Patch
Comment 8 Carlos Garcia Campos 2019-07-02 06:12:10 PDT
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 9 Carlos Garcia Campos 2019-07-02 06:12:57 PDT
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
Comment 10 Michael Catanzaro 2019-07-02 06:25:16 PDT
(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.
Comment 11 Michael Catanzaro 2019-07-02 06:26:34 PDT
(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
Comment 12 Alexander Mikhaylenko 2019-07-02 06:29:30 PDT
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.
Comment 13 Alexander Mikhaylenko 2019-07-02 10:57:52 PDT
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?..
Comment 14 Michael Catanzaro 2019-07-02 16:01:12 PDT
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.
Comment 15 Alexander Mikhaylenko 2019-07-02 16:07:26 PDT
Can anyone with a computer that doesn't overheat run that build? :) I'd _really_ not do a clean webkit build again.
Comment 16 Carlos Garcia Campos 2019-07-03 00:45:45 PDT
(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.
Comment 17 Michael Catanzaro 2019-07-03 07:44:56 PDT
(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?)
Comment 18 Alexander Mikhaylenko 2019-07-03 07:50:09 PDT
> (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.
Comment 19 Alexander Mikhaylenko 2019-07-03 07:51:39 PDT
(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.
Comment 20 Michael Catanzaro 2019-07-03 07:59:51 PDT
(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?
Comment 21 Alexander Mikhaylenko 2019-07-03 09:00:51 PDT
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 22 Michael Catanzaro 2019-07-13 16:03:09 PDT
Comment on attachment 373317 [details]
Patch

Forgot about this. We had agreed to change the guards here.
Comment 23 Alexander Mikhaylenko 2019-07-19 06:18:43 PDT
Created attachment 374461 [details]
Patch
Comment 24 Michael Catanzaro 2019-07-19 08:07:13 PDT
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());
                                                                                  ^~~~~~~~~~~~~
Comment 25 Michael Catanzaro 2019-07-19 08:10:34 PDT
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.
Comment 26 Alexander Mikhaylenko 2019-07-19 10:18:45 PDT
Huh. I wonder why it worked before then. Is kinetic scrolling on WPE generally a thing?
Comment 27 Michael Catanzaro 2019-07-19 11:35:41 PDT
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.
Comment 28 Alexander Mikhaylenko 2019-07-19 13:55:12 PDT
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 🤷
Comment 29 Alexander Mikhaylenko 2019-07-19 13:56:29 PDT
Created attachment 374493 [details]
Patch
Comment 30 Michael Catanzaro 2019-07-20 05:53:55 PDT
Comment on attachment 374493 [details]
Patch

The Windows EWS failure appears to be unrelated.
Comment 31 WebKit Commit Bot 2019-07-20 05:55:02 PDT
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 32 Michael Catanzaro 2019-07-20 06:12:54 PDT
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.
Comment 33 Alexander Mikhaylenko 2019-07-20 06:19:52 PDT
Oops, sorry
Comment 34 Alexander Mikhaylenko 2019-07-20 06:20:23 PDT
Created attachment 374549 [details]
Patch
Comment 35 WebKit Commit Bot 2019-07-20 13:11:12 PDT
Comment on attachment 374549 [details]
Patch

Clearing flags on attachment: 374549

Committed r247670: <https://trac.webkit.org/changeset/247670>
Comment 36 WebKit Commit Bot 2019-07-20 13:11:15 PDT
All reviewed patches have been landed.  Closing bug.