WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
RESOLVED FIXED
230953
[GTK][WPE] REGRESSION: Async scrolling jumps to the top of the page until keyboard-initiated paging
https://bugs.webkit.org/show_bug.cgi?id=230953
Summary
[GTK][WPE] REGRESSION: Async scrolling jumps to the top of the page until key...
Chris Lord
Reported
2021-09-29 07:04:54 PDT
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
.
Attachments
Patch
(50.20 KB, patch)
2021-10-04 03:39 PDT
,
Chris Lord
no flags
Details
Formatted Diff
Diff
Patch
(50.08 KB, patch)
2021-10-04 04:20 PDT
,
Chris Lord
no flags
Details
Formatted Diff
Diff
Patch
(49.94 KB, patch)
2021-10-04 07:36 PDT
,
Chris Lord
no flags
Details
Formatted Diff
Diff
Patch
(48.42 KB, patch)
2021-10-05 01:35 PDT
,
Chris Lord
no flags
Details
Formatted Diff
Diff
Patch
(47.97 KB, patch)
2021-10-05 02:27 PDT
,
Chris Lord
no flags
Details
Formatted Diff
Diff
Show Obsolete
(4)
View All
Add attachment
proposed patch, testcase, etc.
Martin Robinson
Comment 1
2021-09-29 07:06:09 PDT
(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
Comment 2
2021-09-29 07:16:23 PDT
(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)
Comment 3
2021-09-29 07:47:24 PDT
I suggest eliminating the ScrollAnimations in ScrollingTreeScrollingNodeDelegateNicosia so the Nicosia code path is more similar to other platforms. That might fix this.
Chris Lord
Comment 4
2021-10-04 03:39:53 PDT
Created
attachment 440050
[details]
Patch
Martin Robinson
Comment 5
2021-10-04 03:46:28 PDT
This looks like a really nice cleanup! I'll give this a review once it applies cleanly.
Chris Lord
Comment 6
2021-10-04 04:20:09 PDT
Created
attachment 440053
[details]
Patch
Chris Lord
Comment 7
2021-10-04 07:36:58 PDT
Created
attachment 440059
[details]
Patch
Simon Fraser (smfr)
Comment 8
2021-10-04 11:32:41 PDT
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
Comment 9
2021-10-05 01:35:09 PDT
Created
attachment 440181
[details]
Patch
Chris Lord
Comment 10
2021-10-05 02:27:07 PDT
Created
attachment 440186
[details]
Patch
Martin Robinson
Comment 11
2021-10-05 05:09:08 PDT
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
Comment 12
2021-10-05 05:17:04 PDT
(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
Comment 13
2021-10-05 06:57:42 PDT
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
Comment 14
2021-10-05 06:58:20 PDT
<
rdar://problem/83884276
>
Note
You need to
log in
before you can comment on or make changes to this bug.
Top of Page
Format For Printing
XML
Clone This Bug