Summary: | [GTK][WPE] Add support for smooth scrolling animation with async scrolling | ||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Guilaume Ayoub <guillaume.webkit> | ||||||||||||||||||||
Component: | WebKitGTK | Assignee: | Chris Lord <clord> | ||||||||||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||||||||||
Severity: | Normal | CC: | alicem, aperez, bugs-noreply, cgarcia, clord, cmarcelo, darin, ews-watchlist, fred.wang, jamesr, luiz, megan_gardner, simon.fraser, tonikitoo, wenson_hsieh, zan | ||||||||||||||||||||
Priority: | P2 | ||||||||||||||||||||||
Version: | WebKit Nightly Build | ||||||||||||||||||||||
Hardware: | Unspecified | ||||||||||||||||||||||
OS: | Linux | ||||||||||||||||||||||
See Also: | https://bugs.webkit.org/show_bug.cgi?id=211325 | ||||||||||||||||||||||
Attachments: |
|
Description
Guilaume Ayoub
2020-04-11 08:11:40 PDT
I can't reproduce this. 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. Does it happen when hw acceleration policy is ondemand (on a page that uses accelerated compositing)? (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. It must be async scrolling issue then, but I can't reproduce it. I’ve found that it only happens when I use the "smooth scrolling" option in Epiphany. Could it be a bug in Epiphany? 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. Created attachment 406846 [details]
Patch
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). 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 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? (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 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. Created attachment 407095 [details]
Patch
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) Created attachment 407096 [details]
Patch
(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 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 =)
Created attachment 407665 [details]
Patch
Committed r266390: <https://trac.webkit.org/changeset/266390> All reviewed patches have been landed. Closing bug and clearing flags on attachment 407665 [details]. 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. 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. (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. (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? (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. 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. 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. 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. Created attachment 411205 [details]
Patch
Created attachment 411206 [details]
Patch
Created attachment 411207 [details]
Patch
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? Created attachment 411425 [details]
Patch
(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. Created attachment 411434 [details]
Patch
Committed r268522: <https://trac.webkit.org/changeset/268522> All reviewed patches have been landed. Closing bug and clearing flags on attachment 411434 [details]. |