Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

REGRESSION (263724@main): Jumpiness and jitteriness in scrolling on searchfox.org #14718

Merged
merged 1 commit into from
Jun 7, 2023
Merged

REGRESSION (263724@main): Jumpiness and jitteriness in scrolling on searchfox.org #14718

merged 1 commit into from
Jun 7, 2023

Conversation

rniwa
Copy link
Member

@rniwa rniwa commented Jun 7, 2023 •
edited by webkit-early-warning-system

7c2201e

REGRESSION (263724@main): Jumpiness and jitteriness in scrolling on searchfox.org
https://bugs.webkit.org/show_bug.cgi?id=257576
<rdar://problem/110095194>

Reviewed by Simon Fraser.

The bug was caused by WebKit no longer clearing the scroll anchoring element upon page load completion.
This turned out to be not a Web compatible change.

Fix fast/dynamic/anchor-lock.html in another way by remembering to which node scrolling was scheduled
in LocalFrameView instead of relying on m_maintainScrollPositionAnchor to be not cleared upon page load
completion, which is not generally Web compatible.

* LayoutTests/fast/scrolling/scroll-anchoring-after-page-load-expected.html: Added.
* LayoutTests/fast/scrolling/scroll-anchoring-after-page-load.html: Added.
* Source/WebCore/loader/FrameLoader.cpp:
(WebCore::FrameLoader::completed):
* Source/WebCore/page/LocalFrameView.cpp:
(WebCore::LocalFrameView::scheduleScrollToAnchorAndTextFragment):
* Source/WebCore/page/LocalFrameView.h:

Canonical link: https://commits.webkit.org/264940@main

3c402c5

Misc iOS, tvOS & watchOS macOS Linux Windows
❌ 🧪 style ✅ 🛠 ios ✅ 🛠 mac ✅ 🛠 wpe ✅ 🛠 wincairo
✅ 🧪 bindings ✅ 🛠 ios-sim ✅ 🛠 mac-AS-debug ✅ 🧪 wpe-wk2
✅ 🧪 webkitperl ✅ 🧪 ios-wk2   🧪 api-mac ✅ 🛠 gtk
✅ 🧪 ios-wk2-wpt ✅ 🧪 mac-wk1 ✅ 🧪 gtk-wk2
✅ 🧪 api-ios ✅ 🧪 mac-wk2 ✅ 🧪 api-gtk
✅ 🛠 tv ✅ 🧪 mac-AS-debug-wk2
✅ 🛠 tv-sim ✅ 🧪 mac-wk2-stress
✅ 🛠 watch
✅ 🛠 🧪 unsafe-merge ✅ 🛠 watch-sim

@rniwa rniwa requested a review from cdumez as a code owner June 7, 2023 00:59
@rniwa rniwa self-assigned this Jun 7, 2023
@rniwa rniwa added the Scrolling Bugs related to main thread and off-main thread scrolling label Jun 7, 2023
@rniwa rniwa requested a review from smfr June 7, 2023 01:00
@webkit-early-warning-system
Copy link
Collaborator

EWS run on current version of this PR (hash 3c402c5)

Misc iOS, tvOS & watchOS macOS Linux Windows
❌ 🧪 style ✅ 🛠 ios ✅ 🛠 mac ✅ 🛠 wpe ✅ 🛠 wincairo
✅ 🧪 bindings ✅ 🛠 ios-sim ✅ 🛠 mac-AS-debug ✅ 🧪 wpe-wk2
✅ 🧪 webkitperl ✅ 🧪 ios-wk2   🧪 api-mac ✅ 🛠 gtk
✅ 🧪 ios-wk2-wpt ✅ 🧪 mac-wk1 ✅ 🧪 gtk-wk2
✅ 🧪 api-ios ✅ 🧪 mac-wk2 ✅ 🧪 api-gtk
✅ 🛠 tv ✅ 🧪 mac-AS-debug-wk2
✅ 🛠 tv-sim ✅ 🧪 mac-wk2-stress
✅ 🛠 watch
✅ 🛠 🧪 unsafe-merge ✅ 🛠 watch-sim

smfr
smfr approved these changes Jun 7, 2023
View reviewed changes
Source/WebCore/page/LocalFrameView.cpp
@@ -3657,7 +3662,12 @@ void LocalFrameView::scheduleScrollToAnchorAndTextFragment()
RefPtr protectedThis = weakThis.get();
if (!protectedThis)
return;
protectedThis->scrollToAnchorAndTextFragmentNowIfNeeded();
if (!protectedThis->m_maintainScrollPositionAnchor) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I find this logic hard to follow. What if we just pass the relevant anchor as an argument to scrollToAnchorAndTextFragmentNowIfNeeded()?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That doesn't quite work because various functions called in scrollToAnchorAndTextFragmentNowIfNeeded rely on m_maintainScrollPositionAnchor directly.

@rniwa rniwa added the unsafe-merge-queue Applied to send a pull request to merge-queue, but skip building and testing label Jun 7, 2023
@rniwa
REGRESSION (263724@main): Jumpiness and jitteriness in scrolling on s…
7c2201e 
…earchfox.org

https://bugs.webkit.org/show_bug.cgi?id=257576
<rdar://problem/110095194>

Reviewed by Simon Fraser.

The bug was caused by WebKit no longer clearing the scroll anchoring element upon page load completion.
This turned out to be not a Web compatible change.

Fix fast/dynamic/anchor-lock.html in another way by remembering to which node scrolling was scheduled
in LocalFrameView instead of relying on m_maintainScrollPositionAnchor to be not cleared upon page load
completion, which is not generally Web compatible.

* LayoutTests/fast/scrolling/scroll-anchoring-after-page-load-expected.html: Added.
* LayoutTests/fast/scrolling/scroll-anchoring-after-page-load.html: Added.
* Source/WebCore/loader/FrameLoader.cpp:
(WebCore::FrameLoader::completed):
* Source/WebCore/page/LocalFrameView.cpp:
(WebCore::LocalFrameView::scheduleScrollToAnchorAndTextFragment):
* Source/WebCore/page/LocalFrameView.h:

Canonical link: https://commits.webkit.org/264940@main
@webkit-commit-queue
Copy link
Collaborator

Committed 264940@main (7c2201e): https://commits.webkit.org/264940@main

Reviewed commits have been landed. Closing PR #14718 and removing active labels.

@webkit-commit-queue webkit-commit-queue merged commit 7c2201e into WebKit:main Jun 7, 2023
@webkit-commit-queue webkit-commit-queue removed the unsafe-merge-queue Applied to send a pull request to merge-queue, but skip building and testing label Jun 7, 2023
@rniwa rniwa deleted the fix257576 branch June 7, 2023 13:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@smfr smfr smfr approved these changes

@cdumez cdumez Awaiting requested review from cdumez cdumez is a code owner

Assignees

@rniwa rniwa

Labels
Scrolling Bugs related to main thread and off-main thread scrolling
Projects
None yet
Milestone
No milestone
4 participants
@rniwa @webkit-early-warning-system @webkit-commit-queue @smfr