Bug 210382 - [GTK][WPE] Add support for smooth scrolling animation with async scrolling
Summary: [GTK][WPE] Add support for smooth scrolling animation with async scrolling
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKitGTK (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Linux
: P2 Normal
Assignee: Chris Lord
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2020-04-11 08:11 PDT by Guilaume Ayoub
Modified: 2020-10-15 06:47 PDT (History)
16 users (show)

See Also:


Attachments
Patch (31.31 KB, patch)
2020-08-19 09:15 PDT, Chris Lord
no flags Details | Formatted Diff | Diff
Patch (31.95 KB, patch)
2020-08-24 05:02 PDT, Chris Lord
no flags Details | Formatted Diff | Diff
Patch (32.89 KB, patch)
2020-08-24 06:28 PDT, Chris Lord
no flags Details | Formatted Diff | Diff
Patch (32.90 KB, patch)
2020-09-01 02:00 PDT, Chris Lord
no flags Details | Formatted Diff | Diff
Patch (37.10 KB, patch)
2020-10-13 02:35 PDT, Chris Lord
no flags Details | Formatted Diff | Diff
Patch (37.55 KB, patch)
2020-10-13 02:41 PDT, Chris Lord
no flags Details | Formatted Diff | Diff
Patch (37.64 KB, patch)
2020-10-13 03:23 PDT, Chris Lord
no flags Details | Formatted Diff | Diff
Patch (42.54 KB, patch)
2020-10-15 04:10 PDT, Chris Lord
no flags Details | Formatted Diff | Diff
Patch (42.55 KB, patch)
2020-10-15 06:11 PDT, Chris Lord
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Guilaume Ayoub 2020-04-11 08:11:40 PDT
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
Comment 1 Carlos Garcia Campos 2020-04-12 04:38:16 PDT
I can't reproduce this.
Comment 2 Guilaume Ayoub 2020-04-12 04:45:28 PDT
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.
Comment 3 Carlos Garcia Campos 2020-04-12 04:52:15 PDT
Does it happen when hw acceleration policy is ondemand (on a page that uses accelerated compositing)?
Comment 4 Guilaume Ayoub 2020-04-12 05:38:30 PDT
(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.
Comment 5 Carlos Garcia Campos 2020-04-12 06:18:55 PDT
It must be async scrolling issue then, but I can't reproduce it.
Comment 6 Guilaume Ayoub 2020-07-29 05:21:32 PDT
I’ve found that it only happens when I use the "smooth scrolling" option in Epiphany. Could it be a bug in Epiphany?
Comment 7 Carlos Garcia Campos 2020-07-29 06:58:51 PDT
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.
Comment 8 Chris Lord 2020-08-19 09:15:22 PDT
Created attachment 406846 [details]
Patch
Comment 9 Chris Lord 2020-08-19 09:17:17 PDT
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).
Comment 10 Chris Lord 2020-08-20 02:03:18 PDT
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).
Comment 11 Carlos Garcia Campos 2020-08-20 02:12:57 PDT
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?
Comment 12 Chris Lord 2020-08-20 02:29:09 PDT
(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).
Comment 13 Carlos Garcia Campos 2020-08-20 03:07:23 PDT
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.
Comment 14 Chris Lord 2020-08-24 05:02:03 PDT
Created attachment 407095 [details]
Patch
Comment 15 Chris Lord 2020-08-24 05:03:34 PDT
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)
Comment 16 Chris Lord 2020-08-24 06:28:20 PDT
Created attachment 407096 [details]
Patch
Comment 17 Chris Lord 2020-08-24 06:28:56 PDT
(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.
Comment 18 Adrian Perez 2020-09-01 01:37:44 PDT
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 =)
Comment 19 Chris Lord 2020-09-01 02:00:09 PDT
Created attachment 407665 [details]
Patch
Comment 20 EWS 2020-09-01 02:51:31 PDT
Committed r266390: <https://trac.webkit.org/changeset/266390>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 407665 [details].
Comment 21 Carlos Garcia Campos 2020-09-04 00:45:38 PDT
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.
Comment 22 Alexander Mikhaylenko 2020-10-07 02:32:59 PDT
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.
Comment 23 Chris Lord 2020-10-07 02:36:21 PDT
(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.
Comment 24 Michael Catanzaro 2020-10-07 06:57:33 PDT
(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?
Comment 25 Adrian Perez 2020-10-07 07:02:35 PDT
(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.
Comment 26 Alexander Mikhaylenko 2020-10-07 07:05:20 PDT
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.
Comment 27 Chris Lord 2020-10-09 01:55:58 PDT
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.
Comment 28 Chris Lord 2020-10-12 02:52:57 PDT
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.
Comment 29 Chris Lord 2020-10-13 02:35:40 PDT
Created attachment 411205 [details]
Patch
Comment 30 Chris Lord 2020-10-13 02:41:25 PDT
Created attachment 411206 [details]
Patch
Comment 31 Chris Lord 2020-10-13 03:23:04 PDT
Created attachment 411207 [details]
Patch
Comment 32 Carlos Garcia Campos 2020-10-15 02:42:23 PDT
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?
Comment 33 Chris Lord 2020-10-15 04:10:05 PDT
Created attachment 411425 [details]
Patch
Comment 34 Chris Lord 2020-10-15 04:33:26 PDT
(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.
Comment 35 Chris Lord 2020-10-15 06:11:46 PDT
Created attachment 411434 [details]
Patch
Comment 36 EWS 2020-10-15 06:46:59 PDT
Committed r268522: <https://trac.webkit.org/changeset/268522>

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