Bug 218133 – [GTK] Smooth scrolling should not apply to continuous scrolling with sync scrolling

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKitGTK

[Alice Mikhaylenko]

Smooth scrolling makes sense for discrete jumps such as:

* Keyboard navigation
* Mouse wheels

Meanwhile, it does not make sense for scrolling that's already continuous, such as:

* Touch panning
* Touchpad scrolling
* Trackpoint scrolling
* Freely scrolling mouse wheels - these are a difficult case

This kind of scrolling consists of frequent scroll events (ideally one event per frame, although really it varies) and trying to smooth them just makes them laggy instead. Additionally, it adds a lot of overhead, to the point scrolling on a 4K screen on my laptop becomes unusably slow.

macOS doesn't enable it for touchpad scrolling either.

See also: https://gitlab.gnome.org/GNOME/epiphany/-/issues/802

May also apply to WPE, not sure.

[Chris Lord]

Was this tested post r268522 landing? Smooth scrolling didn't apply to touchpad scrolling at the least on GTK when I checked, I haven't checked touch panning and I don't have a trackpoint, but I expect it uses the same mechanism as touchpad scrolling.

After that commit landed, smooth scrolling doesn't apply if there are accurate scroll deltas (which is the case with touchpad scrolling - it should be the case with touch scrolling, but given I haven't explicitly tested it, perhaps there's some code missing to set the flag).

Some more detail about reproduction steps would be good - is this with or without async scrolling enabled, what commit of WebKit was this, etc.

[Alice Mikhaylenko]

Nope, just got the laptop yesterday and was horrified by laggy scrolling in 2.30.1/ephy 3.38.1 and remembered there was a bug I promised to open long time ago and forgot about it. :)

Haven't built webkit on it yet, so yes, there's a good possibility this was fixed with your changes. I'll build it a bit later and check.

[Alice Mikhaylenko]

Alright, I have webkit built now.

No, I don't see changes. It seems it's always active with async scrolling, and active when smooth scrolling is enabled otherwise.

[Alice Mikhaylenko]

Actually, nevermind the async scrolling part. That behaves properly now, I just chose a slow page for testing.

This is still relevant for non-async scrolling though.

[Chris Lord]

(In reply to Alexander Mikhaylenko from comment #4)
> Actually, nevermind the async scrolling part. That behaves properly now, I
> just chose a slow page for testing.
> 
> This is still relevant for non-async scrolling though.

This is good to know - I've retitled the bug and will take care of it, thanks for investigating :)

[Michael Catanzaro]

When is async scrolling disabled? (Is it only used in AC mode?)

[Alice Mikhaylenko]

Not only AC mode, but specifically hardware-acceleration-policy=always.

[Michael Catanzaro]

OK so async scrolling is only enabled when hardware acceleration policy is set to always? Then that means it will *never* be enabled for end users, unless they change our hidden gsetting to set that policy. A lot of work to develop a feature that almost never gets used?

That is a little weird; why does it matter whether AC mode was activated on demand or always?

[Alice Mikhaylenko]

Well, Librem 5 images currently override the default to always, as async scrolling in particular greatly improves scroll responsivity there.

FTR I also set it to always on my new laptop for the same reason, with ondemand I can't scroll this very page without lag when the window is maximized.

I assume there are problems with enabling/disabling it on the fly, but yes, it's never used in Epiphany right now unless you change the hidden setting. Which is why not many people complained about that scrolling issue before 2.30.1 got released, I suppose :)

[Carlos Garcia Campos]

I don't remember the details, but the setting needs to be activated soon and can't be changed, so it's not compatible with ondemand. It's not easy to make it work ondemand and probably not worth it either. The plan for GTK4 is to remove the hardware acceleration policy setting and enable it by default (or maybe make it a construct only property of the context, so we can always disable it for very specific cases, we'll see)

[Michael Catanzaro]

(In reply to Carlos Garcia Campos from comment #10)
> The plan for GTK4 is
> to remove the hardware acceleration policy setting and enable it by default
> (or maybe make it a construct only property of the context, so we can always
> disable it for very specific cases, we'll see)

I think we previously determined that one GL context per web process is too much RAM usage, so will the GL context move to the UI process? Or the GPU process?

[Chris Lord]

Created attachment 412440 [details]
Patch

[Chris Lord]

(In reply to Alexander Mikhaylenko from comment #4)
> Actually, nevermind the async scrolling part. That behaves properly now, I
> just chose a slow page for testing.
> 
> This is still relevant for non-async scrolling though.

Hi Alexander, do you think you could test the attached patch? This gives me expected behaviour with my touchpad and mouse-wheel with sync scrolling, but I don't have a touch-screen convenient to verify there (I possibly can if need be, but it sounds like you're already prepared).

[Alice Mikhaylenko]

No difference here even on touchpad.

[Alice Mikhaylenko]

Sorry, I mean no difference compared to trunk. As in, smooth scrolling does affect and slow down touchpad and touchscreen scrolling.

[Chris Lord]

(In reply to Alexander Mikhaylenko from comment #15)
> Sorry, I mean no difference compared to trunk. As in, smooth scrolling does
> affect and slow down touchpad and touchscreen scrolling.

I'm wondering at this point if it's definitely smooth scrolling and not just the browser event queue lagging behind... Are you on IRC/Riot or something similar where we could discuss this live, perhaps?

[Alice Mikhaylenko]

It's certainly not lagging behind, it's visibly different depending on the smooth scrolling checkbox in minibrowser prefs. :)

@alexm:gnome.org on matrix/bridged to irc, we can discuss it in #epiphany irc channel, for example.

[Chris Lord]

Created attachment 412515 [details]
Patch

[Chris Lord]

Created attachment 412534 [details]
Patch

[Alice Mikhaylenko]

Works now 👍️

[Michael Catanzaro]

So this will allow us to remove the smooth scrolling setting from Epiphany, yes? Because with this change, it should always be desired?

[Alice Mikhaylenko]

Yes, exactly.

[Adrian Perez]

Comment on attachment 412534 [details]
Patch

Logic LGTM. Take a look at the comments below before landing to
avoid breaking the GTK4 build.

View in context: https://bugs.webkit.org/attachment.cgi?id=412534&action=review

> Source/WebCore/platform/PlatformWheelEvent.h:-144
> -#if PLATFORM(COCOA) || PLATFORM(GTK) || USE(LIBWPE)

Nice to see conditional compilation guards go away \o/

> Source/WebCore/platform/gtk/PlatformWheelEventGtk.cpp:74
> +            if (auto* device = gdk_event_get_source_device(reinterpret_cast<GdkEvent*>(event)))

This function is gone in GTK4, so this will break builds configured with
USE_GTK4=ON. The function is now gdk_event_get_device() and does exactly
the same, so we can add in Source/WebCore/platform/gtk/GtkVersioning.h a
definition of gdk_event_get_device() which when building for GTK3 calls
gdk_event_get_source_device() — there are other similar functions already
in that header 😉️

> Source/WebKit/Shared/gtk/WebEventFactory.cpp:288
> +            if (auto* device = gdk_event_get_source_device(event))

Same here :)

[Carlos Garcia Campos]

Comment on attachment 412534 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=412534&action=review

> Source/WebCore/platform/ScrollAnimator.h:75
> +    virtual bool scrollWithoutAnimation(ScrollbarOrientation, ScrollGranularity, float step, float multiplier);

Why is this virtual? and public?

>> Source/WebCore/platform/gtk/PlatformWheelEventGtk.cpp:74
>> +            if (auto* device = gdk_event_get_source_device(reinterpret_cast<GdkEvent*>(event)))
> 
> This function is gone in GTK4, so this will break builds configured with
> USE_GTK4=ON. The function is now gdk_event_get_device() and does exactly
> the same, so we can add in Source/WebCore/platform/gtk/GtkVersioning.h a
> definition of gdk_event_get_device() which when building for GTK3 calls
> gdk_event_get_source_device() — there are other similar functions already
> in that header 😉️

I'm pretty sure this entire file is death code nowadays, we no longer use GdkEvents in the WebProcess, so this constructor is never used. So this can be just removed, either as part of this commit or a follow up one.

[Chris Lord]

Created attachment 412630 [details]
Patch

[Chris Lord]

(In reply to Carlos Garcia Campos from comment #24)
> Comment on attachment 412534 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=412534&action=review
> 
> > Source/WebCore/platform/ScrollAnimator.h:75
> > +    virtual bool scrollWithoutAnimation(ScrollbarOrientation, ScrollGranularity, float step, float multiplier);
> 
> Why is this virtual? and public?

No reason for this to be virtual, I've changed that (and if it ever needs overriding, it can be changed then). I think it should be public though, to match with the scrollToOffset API, to make the comment above a bit less ambiguous, and I can imagine wanting to scroll directly and bypass animation.

> >> Source/WebCore/platform/gtk/PlatformWheelEventGtk.cpp:74
> >> +            if (auto* device = gdk_event_get_source_device(reinterpret_cast<GdkEvent*>(event)))
> > 
> > This function is gone in GTK4, so this will break builds configured with
> > USE_GTK4=ON. The function is now gdk_event_get_device() and does exactly
> > the same, so we can add in Source/WebCore/platform/gtk/GtkVersioning.h a
> > definition of gdk_event_get_device() which when building for GTK3 calls
> > gdk_event_get_source_device() — there are other similar functions already
> > in that header 😉️
> 
> I'm pretty sure this entire file is death code nowadays, we no longer use
> GdkEvents in the WebProcess, so this constructor is never used. So this can
> be just removed, either as part of this commit or a follow up one.

Removed, thanks :)

[EWS]

Committed r269143: <https://trac.webkit.org/changeset/269143>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 412630 [details].