Bug 178519 - [GTK] Scrolling sometimes jumps around
Summary: [GTK] Scrolling sometimes jumps around
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKitGTK (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Gustavo Noronha (kov)
URL:
Keywords:
: 182669 (view as bug list)
Depends on:
Blocks:
 
Reported: 2017-10-19 09:54 PDT by Gustavo Noronha (kov)
Modified: 2018-02-11 15:09 PST (History)
7 users (show)

See Also:


Attachments
Patch (1.64 KB, patch)
2018-02-09 01:50 PST, Gustavo Noronha (kov)
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Gustavo Noronha (kov) 2017-10-19 09:54:38 PDT
I noticed after updating to a more recent WebKitGTK+ that sometimes when I'm panning through a page using two-finger scrolling it will jump to the end of the page (or to the top if panning towards the top). I haven't investigated deeply yet, but I have a gut feeling it may be related to Bug 155750. I think the kinetic scrolling might be getting triggered with bad values. I added this debugging printf to the top of ScrollAnimationKinetic::start:

fprintf(stderr, "%s: %.2fx%.2f\n", __func__, velocity.x(), velocity.y());

It prints this when the jump happens:

start: -136363,47x534090,25
Comment 1 Michael Catanzaro 2017-10-19 18:20:09 PDT
(In reply to Gustavo Noronha (kov) from comment #0)
> I haven't investigated
> deeply yet, but I have a gut feeling it may be related to Bug 155750.

Almost surely.
Comment 2 Gustavo Noronha (kov) 2017-10-20 06:22:27 PDT
Yes, I think computeVelocity() is going nuts and calculating something that causes a very speedy kinetic scrolling animation to be started when a non-momentum scroll ends.

computeVelocity: -28410,07x308959,53 accumDelta.y(): -27,19 (last - first).value(): 0,09

The first values are what computeVelocity is about to return, with the components of the formula it uses following it. Remember that accumDelta.y() is multiplied by -1000 before it is divided by last - first.

I forgot to say I'm on Wayland, unsure if it is relevant, but hey.

Adding Adrien to CC as he's probably the one who understands what's intended for computeVelocity() better =)
Comment 3 Michael Catanzaro 2017-10-20 09:34:15 PDT
This code was actually written by Adrien Plazas, not Adrian Perez. Adrien was just doing an internship with Igalia, so I doubt we can CC him directly on this bug. But he as active in the gamepad thread on the webkit-gtk mailing list, so it shouldn't be hard to get his attention.
Comment 4 Gustavo Noronha (kov) 2018-02-09 01:50:37 PST
Created attachment 333468 [details]
Patch
Comment 5 Carlos Garcia Campos 2018-02-09 02:29:26 PST
Comment on attachment 333468 [details]
Patch

hmm, it would be interesting to know why GTK+ has 1000 there if we are going to diverge.
Comment 6 Michael Catanzaro 2018-02-10 09:45:08 PST
*** Bug 182669 has been marked as a duplicate of this bug. ***
Comment 7 ManDay 2018-02-11 00:46:47 PST
Thanks for the patch. Works on my end on 2.19.90.
Comment 8 Michael Catanzaro 2018-02-11 11:46:06 PST
Comment on attachment 333468 [details]
Patch

I have other users complaining about this, so if it fixes the regression, let's land it.
Comment 9 Michael Catanzaro 2018-02-11 11:47:16 PST
(In reply to Carlos Garcia Campos from comment #5)
> Comment on attachment 333468 [details]
> Patch
> 
> hmm, it would be interesting to know why GTK+ has 1000 there if we are going
> to diverge.

I'd be interested to know this too.
Comment 10 Michael Catanzaro 2018-02-11 11:49:07 PST
^ Christian, you might be the best person to answer the question above. IIRC you wrote the original code that we copied from GTK+ (right?)
Comment 11 WebKit Commit Bot 2018-02-11 12:10:02 PST
Comment on attachment 333468 [details]
Patch

Clearing flags on attachment: 333468

Committed r228368: <https://trac.webkit.org/changeset/228368>
Comment 12 WebKit Commit Bot 2018-02-11 12:10:03 PST
All reviewed patches have been landed.  Closing bug.
Comment 13 Christian Hergert 2018-02-11 15:09:44 PST
(In reply to Michael Catanzaro from comment #10)
> ^ Christian, you might be the best person to answer the question above. IIRC
> you wrote the original code that we copied from GTK+ (right?)

I only wrote the patch to make it settle without jitter.

But looking quickly over that code, the 1000 is probably related to the adjustment scaling in gtkscrolledwindow.c get_scroll_unit().