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

Scrolling with page granularity using keyboard smooth scrolling doesn't account for fixed content #4766

Merged
merged 1 commit into from Sep 28, 2022
Merged

Scrolling with page granularity using keyboard smooth scrolling doesn't account for fixed content #4766

merged 1 commit into from Sep 28, 2022

Conversation

rr-codes
Copy link
Contributor

@rr-codes rr-codes commented Sep 27, 2022 •
edited by webkit-early-warning-system

fdd3720

Scrolling with page granularity using keyboard smooth scrolling doesn't account for fixed content
https://bugs.webkit.org/show_bug.cgi?id=245746
rdar://100469555

Reviewed by Tim Horton.

When keyboard smooth scrolling is enabled, `KeyboardScrollingAnimator::scrollDistance`
is used, which is analagous to `ScrollableArea::scroll`. However, while the
latter accounts for page's having fixed content when scrolling with page
granularity, the former never does.

This PR fixes this by including the same logic that `ScrollableArea::scroll` has
to adjust the scroll step into `KeyboardScrollingAnimator::scrollDistance`.

* LayoutTests/scrollbars/scrolling-backward-by-page-accounting-bottom-fixed-elements-on-keyboard-spacebar.html:
* Source/WebCore/platform/KeyboardScrollingAnimator.cpp:
(WebCore::KeyboardScrollingAnimator::scrollDistance const):
* Source/WebCore/platform/ScrollableArea.h:

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

6c28a27

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

@rr-codes rr-codes self-assigned this Sep 27, 2022
@rr-codes rr-codes added Scrolling Bugs related to main thread and off-main thread scrolling WebKit Nightly Build labels Sep 27, 2022
@webkit-early-warning-system
Copy link
Collaborator

EWS run on previous version of this PR (hash 7287f44)

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

@webkit-early-warning-system
Copy link
Collaborator

EWS run on previous version of this PR (hash 90bb6b0)

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

@webkit-early-warning-system
Copy link
Collaborator

EWS run on current version of this PR (hash 6c28a27)

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

@rr-codes rr-codes added the merge-queue Applied to send a pull request to merge-queue label Sep 28, 2022
@rr-codes
Scrolling with page granularity using keyboard smooth scrolling doesn…
fdd3720 
…'t account for fixed content

https://bugs.webkit.org/show_bug.cgi?id=245746
rdar://100469555

Reviewed by Tim Horton.

When keyboard smooth scrolling is enabled, `KeyboardScrollingAnimator::scrollDistance`
is used, which is analagous to `ScrollableArea::scroll`. However, while the
latter accounts for page's having fixed content when scrolling with page
granularity, the former never does.

This PR fixes this by including the same logic that `ScrollableArea::scroll` has
to adjust the scroll step into `KeyboardScrollingAnimator::scrollDistance`.

* LayoutTests/scrollbars/scrolling-backward-by-page-accounting-bottom-fixed-elements-on-keyboard-spacebar.html:
* Source/WebCore/platform/KeyboardScrollingAnimator.cpp:
(WebCore::KeyboardScrollingAnimator::scrollDistance const):
* Source/WebCore/platform/ScrollableArea.h:

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

Committed 254963@main (fdd3720): https://commits.webkit.org/254963@main

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

@webkit-early-warning-system webkit-early-warning-system merged commit fdd3720 into WebKit:main Sep 28, 2022
@webkit-commit-queue webkit-commit-queue removed the merge-queue Applied to send a pull request to merge-queue label Sep 28, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@hortont424 hortont424 hortont424 approved these changes

@pxlcoder pxlcoder Awaiting requested review from pxlcoder

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

Assignees

@rr-codes rr-codes

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