Bug 210382 – [GTK][WPE] Add support for smooth scrolling animation with async scrolling

• Component: WebKitGTK
• Status: RESOLVED FIXED

[Guilaume Ayoub]

When hardware acceleration is set to "always", page scrolling is broken when using the mouse wheel and then the keyboard.

Steps to reproduce:

- Set hardware acceleration to "always".
- Open a web page that needs scrolling.
- Go to the middle of the page using the mouse wheel.
- Hit the page-down key of the keyboard.

What’s expected: the page keeps scrolling from the place it was.

What I get: the page scrolls from the top.

I use:

- Epiphany 3.36.0
- WebKitGTK 2.28.0

[Carlos Garcia Campos]

I can't reproduce this.

[Guilaume Ayoub]

Here is my configuration (if it can help):

-- Enabled features:
--  ENABLE_ACCELERATED_2D_CANVAS .................. OFF
--  ENABLE_BUBBLEWRAP_SANDBOX                       OFF
--  ENABLE_DRAG_SUPPORT ........................... ON
--  ENABLE_GLES2                                    yes
--  ENABLE_GTKDOC ................................. no
--  ENABLE_INTROSPECTION                            yes
--  ENABLE_MEDIA_SOURCE ........................... ON
--  ENABLE_MINIBROWSER                              OFF
--  ENABLE_OPENGL ................................. ON
--  ENABLE_QUARTZ_TARGET                            no
--  ENABLE_SHAREABLE_RESOURCE ..................... ON
--  ENABLE_SPELLCHECK                               yes
--  ENABLE_TOUCH_EVENTS ........................... ON
--  ENABLE_VIDEO                                    yes
--  ENABLE_WAYLAND_TARGET ......................... yes
--  ENABLE_WEBDRIVER                                ON
--  ENABLE_WEB_AUDIO .............................. yes
--  ENABLE_WEB_CRYPTO                               ON
--  ENABLE_X11_TARGET ............................. yes
--  USE_LIBHYPHEN                                   ON
--  USE_LIBNOTIFY ................................. yes
--  USE_LIBSECRET                                   yes
--  USE_OPENJPEG .................................. no
--  USE_WOFF2                                       ON
--  USE_WPE_RENDERER .............................. OFF

I use Epiphany on Wayland.

I’ve had this problem with previous versions of WebKitGTK too, but I had to disable hardware accelerations because of other blocking bugs.

I can try to change some configuration flags if it can help to find the source of the bug, you can tell me which ones are different from your configuration.

[Carlos Garcia Campos]

Does it happen when hw acceleration policy is ondemand (on a page that uses accelerated compositing)?

[Guilaume Ayoub]

(In reply to Carlos Garcia Campos from comment #3)
> Does it happen when hw acceleration policy is ondemand (on a page that uses
> accelerated compositing)?

It doesn’t.

[Carlos Garcia Campos]

It must be async scrolling issue then, but I can't reproduce it.

[Guilaume Ayoub]

I’ve found that it only happens when I use the "smooth scrolling" option in Epiphany. Could it be a bug in Epiphany?

[Carlos Garcia Campos]

This is because smooth scrolling animation is not supported by async scrolling. In r259112 kinetic scrolling was implemented for async scrolling, we need something similar for smooth scrolling.

[Chris Lord]

Created attachment 406846 [details]
Patch

[Chris Lord]

I've only tested this on WPE, though the result should be the same in GTK - I'll test it now, if there are any issues I'll report back (but assume it's ok if I don't).

[Chris Lord]

I've only tested this on WPE, though the result should be the same in GTK - I'll test it now, if there are any issues I'll report back (but assume it's ok if I don't).

[Carlos Garcia Campos]

Comment on attachment 406846 [details]
Patch

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

> Source/WebCore/page/scrolling/ScrollingTreeScrollingNode.cpp:-261
> -    if (scrollType == ScrollType::Programmatic)
> -        stopScrollAnimations();
> -

Why? This is cross-platform code.

> Source/WebCore/page/scrolling/nicosia/ScrollingTreeFrameScrollingNodeNicosia.cpp:155
>          m_kineticAnimation->start(currentScrollPosition(), m_kineticAnimation->computeVelocity(), canHaveHorizontalScrollbar(), canHaveVerticalScrollbar());
>          m_kineticAnimation->clearScrollHistory();
> +        return WheelEventHandlingResult::handled();

Is this an existing bug?

> Source/WebCore/page/scrolling/nicosia/ScrollingTreeFrameScrollingNodeNicosia.cpp:160
>          m_kineticAnimation->start(currentScrollPosition(), wheelEvent.swipeVelocity(), canHaveHorizontalScrollbar(), canHaveVerticalScrollbar());
>          m_kineticAnimation->clearScrollHistory();
> +        return WheelEventHandlingResult::handled();

Ditto.

> Source/WebCore/page/scrolling/nicosia/ScrollingTreeFrameScrollingNodeNicosia.cpp:-148
> -    // FIXME: This needs to return whether the event was handled.

I think this FIXME still applies. It's not just whether there's a delta to scroll, but also whether we actually scrolled anything. When not using async scrolling, if you reach the end of frame for example, the next scroll down event is not considered handled and it's forwarded to the parent scrollable, normally the main frame view. This doesn't happen with async scrolling.

> Source/WebCore/page/scrolling/nicosia/ScrollingTreeFrameScrollingNodeNicosia.cpp:166
> +    if (deltaX || deltaY) {

I think we could return early if !deltaX && !deltaY and reduce the indentation of the code below.

> Source/WebCore/platform/ScrollAnimationSmooth.cpp:51
> +#if USE(GLIB)

#if USE(GLIB_EVENT_LOOP)

> Source/WebCore/platform/ScrollAnimationSmooth.cpp:52
> +    m_animationTimer.setPriority(WTF::RunLoopSourcePriority::DisplayRefreshMonitorTimer);

I would be good to give this a name too for debugging.

> Source/WebCore/platform/ScrollAnimationSmooth.h:63
> -        PerAxisData() = delete;
> +        PerAxisData() { }

= default? or just remove it. Why is now used, btw?

[Chris Lord]

(In reply to Carlos Garcia Campos from comment #11)
> Comment on attachment 406846 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=406846&action=review
> 
> > Source/WebCore/page/scrolling/ScrollingTreeScrollingNode.cpp:-261
> > -    if (scrollType == ScrollType::Programmatic)
> > -        stopScrollAnimations();
> > -
> 
> Why? This is cross-platform code.

I added this in the previous kinetic scrolling patch, but I realise the logic isn't correct and there are non-programmatic cases where you want to stop the scroll animations (paging with the space-bar or using the scrollbar, for example). I looked at all the callers of this function and found the correct place to put it was actually in the requestScroll blocks in the nicosia scroll nodes.

> > Source/WebCore/page/scrolling/nicosia/ScrollingTreeFrameScrollingNodeNicosia.cpp:155
> >          m_kineticAnimation->start(currentScrollPosition(), m_kineticAnimation->computeVelocity(), canHaveHorizontalScrollbar(), canHaveVerticalScrollbar());
> >          m_kineticAnimation->clearScrollHistory();
> > +        return WheelEventHandlingResult::handled();
> 
> Is this an existing bug?
> 
> > Source/WebCore/page/scrolling/nicosia/ScrollingTreeFrameScrollingNodeNicosia.cpp:160
> >          m_kineticAnimation->start(currentScrollPosition(), wheelEvent.swipeVelocity(), canHaveHorizontalScrollbar(), canHaveVerticalScrollbar());
> >          m_kineticAnimation->clearScrollHistory();
> > +        return WheelEventHandlingResult::handled();
> 
> Ditto.
> 
> > Source/WebCore/page/scrolling/nicosia/ScrollingTreeFrameScrollingNodeNicosia.cpp:-148
> > -    // FIXME: This needs to return whether the event was handled.
> 
> I think this FIXME still applies. It's not just whether there's a delta to
> scroll, but also whether we actually scrolled anything. When not using async
> scrolling, if you reach the end of frame for example, the next scroll down
> event is not considered handled and it's forwarded to the parent scrollable,
> normally the main frame view. This doesn't happen with async scrolling.

Ah, I thought I was fixing this, but I'm obviously not unless I actually check the extents - that seems like an easy thing to do though, so I'll add that code and leave it as is.

> > Source/WebCore/page/scrolling/nicosia/ScrollingTreeFrameScrollingNodeNicosia.cpp:166
> > +    if (deltaX || deltaY) {
> 
> I think we could return early if !deltaX && !deltaY and reduce the
> indentation of the code below.

Good call.

> > Source/WebCore/platform/ScrollAnimationSmooth.cpp:51
> > +#if USE(GLIB)
> 
> #if USE(GLIB_EVENT_LOOP)

Thanks, I'll fix this in ScrollAnimationKinetic too.

> > Source/WebCore/platform/ScrollAnimationSmooth.cpp:52
> > +    m_animationTimer.setPriority(WTF::RunLoopSourcePriority::DisplayRefreshMonitorTimer);
> 
> I would be good to give this a name too for debugging.

I'll have a look (and do the same for Kinetic).

> > Source/WebCore/platform/ScrollAnimationSmooth.h:63
> > -        PerAxisData() = delete;
> > +        PerAxisData() { }
> 
> = default? or just remove it. Why is now used, btw?

The initialisation values rely on a function call now, so can't be initialised on object creation (at least not without a bunch of redundant calls).

[Carlos Garcia Campos]

Comment on attachment 406846 [details]
Patch

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

>>> Source/WebCore/page/scrolling/ScrollingTreeScrollingNode.cpp:-261
>>> -
>> 
>> Why? This is cross-platform code.
> 
> I added this in the previous kinetic scrolling patch, but I realise the logic isn't correct and there are non-programmatic cases where you want to stop the scroll animations (paging with the space-bar or using the scrollbar, for example). I looked at all the callers of this function and found the correct place to put it was actually in the requestScroll blocks in the nicosia scroll nodes.

Ok, this explanation should be included in the ChangeLog. Maybe it would be better to split this patch and move to a new bug the bug fixes, and then add the smooth scrolling on top.

>>> Source/WebCore/page/scrolling/nicosia/ScrollingTreeFrameScrollingNodeNicosia.cpp:155
>>> +        return WheelEventHandlingResult::handled();
>> 
>> Is this an existing bug?
> 
> Ah, I thought I was fixing this, but I'm obviously not unless I actually check the extents - that seems like an easy thing to do though, so I'll add that code and leave it as is.

I mean that in current code currentScrollPosition() is the position after the previous scrollBy(), the updated one. Now we are returning before even calling scrollBy.

[Chris Lord]

Created attachment 407095 [details]
Patch

[Chris Lord]

Addressed comments, for the most part. I didn't make the GLIB->GLIB_EVENT_LOOP change as inspecting the relevant header, that code isn't guarded by GLIB_EVENT_LOOP, so it seems like a reasonable assumption that those priorities might be used regardless of whether the GLib event loop is used. (if that's incorrect, I'm happy to make the change though)

[Chris Lord]

Created attachment 407096 [details]
Patch

[Chris Lord]

(In reply to Chris Lord from comment #15)
> Addressed comments, for the most part. I didn't make the
> GLIB->GLIB_EVENT_LOOP change as inspecting the relevant header, that code
> isn't guarded by GLIB_EVENT_LOOP, so it seems like a reasonable assumption
> that those priorities might be used regardless of whether the GLib event
> loop is used. (if that's incorrect, I'm happy to make the change though)

Scratch that, Carlos clarified that the priority setting function is only defined with GLIB_EVENT_LOOP, so I've made that change also.

[Adrian Perez]

Comment on attachment 407096 [details]
Patch

I *think* that this is now ready to land, but I am not well
acquainted with these parts of the code, if somebody else could
also take a quick look that would be great =)

[Chris Lord]

Created attachment 407665 [details]
Patch

[EWS]

Committed r266390: <https://trac.webkit.org/changeset/266390>

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

[Carlos Garcia Campos]

I'm re-opening since the problem explained in the description is not fixed yet. I see a different scrolling behavior, but it still jumps when scrolling with he keyboard after using wheel events.

[Alice Mikhaylenko]

Looks like the patch introduced 2 regressions:

* When async scrolling is enabled, smooth scrolling is always used for touchpad and touch scrolling regardless of the setting
* It makes scrolling reaaaaaally slow, it doesn't follow finger on touch anymore and instead seems to move at a constant speed.

[Chris Lord]

(In reply to Alexander Mikhaylenko from comment #22)
> Looks like the patch introduced 2 regressions:
> 
> * When async scrolling is enabled, smooth scrolling is always used for
> touchpad and touch scrolling regardless of the setting
> * It makes scrolling reaaaaaally slow, it doesn't follow finger on touch
> anymore and instead seems to move at a constant speed.

This is bad, I'll take a look at this this week.

[Michael Catanzaro]

(In reply to Chris Lord from comment #23)
> > * When async scrolling is enabled, smooth scrolling is always used for
> > touchpad and touch scrolling regardless of the setting

Question: that's the opposite of what we want, right? We want smooth scrolling for mouse always, but never for touchpad or touch... right? So we can drop the GUI setting to enable/disable smooth scrolling?

[Adrian Perez]

(In reply to Michael Catanzaro from comment #24)
> (In reply to Chris Lord from comment #23)
> > > * When async scrolling is enabled, smooth scrolling is always used for
> > > touchpad and touch scrolling regardless of the setting
> 
> Question: that's the opposite of what we want, right? We want smooth
> scrolling for mouse always, but never for touchpad or touch... right? So we
> can drop the GUI setting to enable/disable smooth scrolling?

Smooth scrolling should be also used for keyboard-initiated
scroll (arrows, PageDown, PageUp), but only when
“WebKitSettings.enable-smooth-scrolling” is enabled.

I think.

[Alice Mikhaylenko]

Right, it should be used for mouse scrolling and keynav, i.e. discrete scrolling.

I consider that to be a separate issue though, right now it probably also always uses smooth scrolling with mouse, I just haven't tested that.

[Chris Lord]

I have a patch that fixes the worst of this regression, I'm now looking at keyboard events and respecting the setting. Progress may be a little slow due to meetings over the next few days, but I should have a patch up for review soon.

[Chris Lord]

Short update, I've reconciled the different scrolling behaviour between async/non-async, still have keyboard initiated scroll to investigate and settings to respect.

I'm seeing the occasional hang with async scrolling, not sure if that's related to this or not, need to check on that too.

[Chris Lord]

Created attachment 411205 [details]
Patch

[Chris Lord]

Created attachment 411206 [details]
Patch

[Chris Lord]

Created attachment 411207 [details]
Patch

[Carlos Garcia Campos]

Comment on attachment 411207 [details]
Patch

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

> Source/WebCore/page/scrolling/ThreadedScrollingTree.h:61
> +    bool scrollAnimatorEnabled() { return m_scrollAnimatorEnabled; }

const

> Source/WebCore/page/scrolling/nicosia/ScrollingTreeFrameScrollingNodeNicosia.cpp:211
> +    ensureScrollAnimationSmooth();
> +    if (m_smoothAnimation && !wheelEvent.hasPreciseScrollingDeltas()) {

I think it would be clearer to either rename ensureScrollAnimationSmooth() as tryEnsureScrollAnimationSmooth() and return a bool or move the m_scrollAnimatorEnabled check here and only call ensure when it's true.

> Source/WebKit/Shared/gtk/WebEventFactory.cpp:291
> +        false,

Why is it always false for GTK port?

[Chris Lord]

Created attachment 411425 [details]
Patch

[Chris Lord]

(In reply to Carlos Garcia Campos from comment #32)
> Comment on attachment 411207 [details]
> Patch
> 
> > Source/WebKit/Shared/gtk/WebEventFactory.cpp:291
> > +        false,
> 
> Why is it always false for GTK port?

Just realised, I haven't addressed this - this API I think is only currently used when synthesising mouse-wheel events and has public-facing API (which I assume shouldn't be changed unless for good reason). Making this false maintains the previous behaviour (events are treated as wheel events and not as touchpad scroll events), as I understand it.

[Chris Lord]

Created attachment 411434 [details]
Patch

[EWS]

Committed r268522: <https://trac.webkit.org/changeset/268522>

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