Bug 230953 – [GTK][WPE] REGRESSION: Async scrolling jumps to the top of the page until keyboard-initiated paging

• Component: Scrolling
• Status: RESOLVED FIXED

[Chris Lord]

Currently, async scrolling is broken on GTK/WPE (which, for WPE, means all scrolling is broken) - any scroll action snaps the page back to the top after appearing to 'try' to scroll. My best guess is the mechanism that synchronises main-thread scroll position with the async scroll tree's position is broken somehow. The odd thing is if you press space to scroll a page, scrolling then works correctly. This is almost certainly fall-out from https://bugs.webkit.org/show_bug.cgi?id=230739.

[Martin Robinson]

(In reply to Chris Lord from comment #0)
> Currently, async scrolling is broken on GTK/WPE (which, for WPE, means all
> scrolling is broken) - any scroll action snaps the page back to the top
> after appearing to 'try' to scroll. My best guess is the mechanism that
> synchronises main-thread scroll position with the async scroll tree's
> position is broken somehow. The odd thing is if you press space to scroll a
> page, scrolling then works correctly. This is almost certainly fall-out from
> https://bugs.webkit.org/show_bug.cgi?id=230739.

I think the keyboard scrolling is always done in the WebProcess, so that sort of makes sense.

[Chris Lord]

(In reply to Martin Robinson from comment #1)
> (In reply to Chris Lord from comment #0)
> > Currently, async scrolling is broken on GTK/WPE (which, for WPE, means all
> > scrolling is broken) - any scroll action snaps the page back to the top
> > after appearing to 'try' to scroll. My best guess is the mechanism that
> > synchronises main-thread scroll position with the async scroll tree's
> > position is broken somehow. The odd thing is if you press space to scroll a
> > page, scrolling then works correctly. This is almost certainly fall-out from
> > https://bugs.webkit.org/show_bug.cgi?id=230739.
> 
> I think the keyboard scrolling is always done in the WebProcess, so that
> sort of makes sense.

I meant that after pressing space, then async scrolling works correctly - perhaps it's flipping some kind of flag somewhere or triggering some timer... I expect it's some kind of logic error with a flag, but I've not debugged it yet, I'm still just reading through the changes.

[Simon Fraser (smfr)]

I suggest eliminating the ScrollAnimations in ScrollingTreeScrollingNodeDelegateNicosia so the Nicosia code path is more similar to other platforms. That might fix this.

[Chris Lord]

Created attachment 440050 [details]
Patch

[Martin Robinson]

This looks like a really nice cleanup! I'll give this a review once it applies cleanly.

[Chris Lord]

Created attachment 440053 [details]
Patch

[Chris Lord]

Created attachment 440059 [details]
Patch

[Simon Fraser (smfr)]

This breaks fast/scrolling/rtl-scrollbars-listbox-scroll.html on macOS because now we enter ScrollingEffectsController::handleWheelEvent() for RenderListBox ScrollableAreas and that doesn't ever move content for a Began event, which is a bit surprising.

[Chris Lord]

Created attachment 440181 [details]
Patch

[Chris Lord]

Created attachment 440186 [details]
Patch

[Martin Robinson]

Comment on attachment 440186 [details]
Patch

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

This is a very nice refactor.

> Source/WebCore/platform/ScrollingEffectsController.cpp:231
> +        m_currentAnimation->stop();

What I understand here is that it's not a big deal to not clear the animation here. Since it is cleared below though, clear it here or don't clear it below for the sake of consistency.

[Chris Lord]

(In reply to Martin Robinson from comment #11)
> Comment on attachment 440186 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=440186&action=review
> 
> This is a very nice refactor.
> 
> > Source/WebCore/platform/ScrollingEffectsController.cpp:231
> > +        m_currentAnimation->stop();
> 
> What I understand here is that it's not a big deal to not clear the
> animation here. Since it is cleared below though, clear it here or don't
> clear it below for the sake of consistency.

This is only stopping the animation if it's a kinetic animation and it's only cleared when it's *not* a kinetic animation (the kinetic animation object is retained so that momentum can be calculated correctly). I do think this function could read better, but I think that's probably best left for another patch.

Thanks for the review!

[EWS]

Committed r283548 (242513@main): <https://commits.webkit.org/242513@main>

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

[Radar WebKit Bug Importer]

<rdar://problem/83884276>