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.

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

Repeated calls to scrollIntoView({ block: 'center' }) can cause jiggling (affects Spotify lyrics) #19794

Conversation

smfr
Copy link
Contributor

@smfr smfr commented Oct 31, 2023

7688e6c

Repeated calls to scrollIntoView({ block: 'center' }) can cause jiggling (affects Spotify lyrics)
https://bugs.webkit.org/show_bug.cgi?id=263995
rdar://117755250

Reviewed by Tim Horton.

Computation of the destination scroll position for `scrollIntoView({ block: 'center' })` involves a divide
by 2, which gives a fractional result for some scroller sizes. This causes the computed scroll position to
alternate between two values 1px apart when called repeatedly, since our scroll positions are integral.

Fix by ceiling the target y value in the `ScrollAlignment::Behavior::AlignCenter` case in
`ScrollableArea::getRectToExposeForScrollIntoView()`.

* LayoutTests/TestExpectations:
* LayoutTests/fast/scrolling/scroll-into-view-block-center-expected.txt: Added.
* LayoutTests/fast/scrolling/scroll-into-view-block-center.html: Added.
* Source/WebCore/platform/ScrollableArea.cpp:
(WebCore::ScrollableArea::getRectToExposeForScrollIntoView const):
* Source/WebCore/rendering/RenderLayerScrollableArea.cpp:

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

31cc753

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
βœ… πŸ›  πŸ§ͺ merge βœ… πŸ›  watch
βœ… πŸ›  watch-sim

@smfr smfr self-assigned this Oct 31, 2023
@smfr smfr added the Scrolling Bugs related to main thread and off-main thread scrolling label Oct 31, 2023
@webkit-ews-buildbot webkit-ews-buildbot added the merging-blocked Applied to prevent a change from being merged label Oct 31, 2023
@smfr smfr removed the merging-blocked Applied to prevent a change from being merged label Oct 31, 2023
@smfr smfr force-pushed the eng/Repeated-calls-to-scrollIntoView-block-center--can-cause-jiggling-affects-Spotify-lyrics branch from 0b86c3f to 39f0b22 Compare November 1, 2023 00:01
@webkit-ews-buildbot webkit-ews-buildbot added the merging-blocked Applied to prevent a change from being merged label Nov 1, 2023
@smfr smfr removed the merging-blocked Applied to prevent a change from being merged label Nov 2, 2023
@smfr smfr force-pushed the eng/Repeated-calls-to-scrollIntoView-block-center--can-cause-jiggling-affects-Spotify-lyrics branch from 39f0b22 to f322b4a Compare November 2, 2023 00:24
@webkit-ews-buildbot webkit-ews-buildbot added the merging-blocked Applied to prevent a change from being merged label Nov 2, 2023
@smfr smfr removed the merging-blocked Applied to prevent a change from being merged label Nov 2, 2023
@smfr smfr force-pushed the eng/Repeated-calls-to-scrollIntoView-block-center--can-cause-jiggling-affects-Spotify-lyrics branch from f322b4a to 5523685 Compare November 2, 2023 17:22
@webkit-ews-buildbot webkit-ews-buildbot added the merging-blocked Applied to prevent a change from being merged label Nov 2, 2023
@smfr smfr removed the merging-blocked Applied to prevent a change from being merged label Nov 2, 2023
@smfr smfr force-pushed the eng/Repeated-calls-to-scrollIntoView-block-center--can-cause-jiggling-affects-Spotify-lyrics branch 2 times, most recently from f322b4a to 31cc753 Compare November 2, 2023 22:11
@smfr smfr added the merge-queue Applied to send a pull request to merge-queue label Nov 3, 2023
…ing (affects Spotify lyrics)

https://bugs.webkit.org/show_bug.cgi?id=263995
rdar://117755250

Reviewed by Tim Horton.

Computation of the destination scroll position for `scrollIntoView({ block: 'center' })` involves a divide
by 2, which gives a fractional result for some scroller sizes. This causes the computed scroll position to
alternate between two values 1px apart when called repeatedly, since our scroll positions are integral.

Fix by ceiling the target y value in the `ScrollAlignment::Behavior::AlignCenter` case in
`ScrollableArea::getRectToExposeForScrollIntoView()`.

* LayoutTests/TestExpectations:
* LayoutTests/fast/scrolling/scroll-into-view-block-center-expected.txt: Added.
* LayoutTests/fast/scrolling/scroll-into-view-block-center.html: Added.
* Source/WebCore/platform/ScrollableArea.cpp:
(WebCore::ScrollableArea::getRectToExposeForScrollIntoView const):
* Source/WebCore/rendering/RenderLayerScrollableArea.cpp:

Canonical link: https://commits.webkit.org/270160@main
@webkit-commit-queue webkit-commit-queue force-pushed the eng/Repeated-calls-to-scrollIntoView-block-center--can-cause-jiggling-affects-Spotify-lyrics branch from 31cc753 to 7688e6c Compare November 3, 2023 04:19
@webkit-commit-queue
Copy link
Collaborator

Committed 270160@main (7688e6c): https://commits.webkit.org/270160@main

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

@webkit-commit-queue webkit-commit-queue merged commit 7688e6c into WebKit:main Nov 3, 2023
@webkit-commit-queue webkit-commit-queue removed the merge-queue Applied to send a pull request to merge-queue label Nov 3, 2023
@smfr smfr deleted the eng/Repeated-calls-to-scrollIntoView-block-center--can-cause-jiggling-affects-Spotify-lyrics branch December 22, 2023 16:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Scrolling Bugs related to main thread and off-main thread scrolling
Projects
None yet
6 participants