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

Keyboard smooth scrolling animation is not disabled when NSScrollAnimationEnabled is false #6409

Merged
merged 1 commit into from Nov 14, 2022
Merged

Keyboard smooth scrolling animation is not disabled when NSScrollAnimationEnabled is false #6409

merged 1 commit into from Nov 14, 2022

Conversation

rr-codes
Copy link
Contributor

@rr-codes rr-codes commented Nov 11, 2022 •
edited by webkit-early-warning-system

207c359

Keyboard smooth scrolling animation is not disabled when `NSScrollAnimationEnabled` is `false`
https://bugs.webkit.org/show_bug.cgi?id=247822
rdar://101225956

Reviewed by Simon Fraser.

Previously, setting `NSScrollAnimationEnabled` to `false` would disable all
scroll animations. However, enabling keyboard smooth scrolling regressed this
and did not respect this setting any longer.

This PR fixes this by having `EventHandlerDrivenSmoothScrolling` respect the
value of `NSScrollAnimationEnabled` so now scroll animations may once again be
disabled.

* Source/WTF/Scripts/Preferences/WebPreferencesInternal.yaml:

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

cbf021e

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
✅ 🛠 🧪 jsc ✅ 🛠 tv ❌ 🧪 mac-wk1 ✅ 🛠 jsc-armv7
✅ 🛠 tv-sim ✅ 🧪 mac-wk2 ✅ 🧪 jsc-armv7-tests
✅ 🛠 🧪 merge ✅ 🛠 watch ✅ 🧪 mac-AS-debug-wk2 ✅ 🛠 jsc-mips
✅ 🛠 watch-sim ✅ 🧪 mac-wk2-stress ✅ 🧪 jsc-mips-tests

@rr-codes rr-codes self-assigned this Nov 11, 2022
@rr-codes rr-codes added Other Scrolling Bugs related to main thread and off-main thread scrolling labels Nov 11, 2022
@pxlcoder
Copy link
Member

Can we give the bug/PR a better title?

@rr-codes rr-codes changed the title How can I disable smooth scrolling / re-enable jump scrolling in Safari? Keyboard smooth scrolling animation is not disabled when NSScrollAnimationEnabled is false Nov 11, 2022
pxlcoder
pxlcoder reviewed Nov 11, 2022
View reviewed changes
Source/WTF/Scripts/Preferences/WebPreferencesInternal.yaml Outdated
@@ -289,7 +289,8 @@ EventHandlerDrivenSmoothKeyboardScrollingEnabled:
WebKitLegacy:
default: false
WebKit:
"PLATFORM(COCOA)": true
"PLATFORM(MAC)": WebKit::defaultScrollAnimatorEnabled()
"PLATFORM(IOS)": true
Copy link
Member

Choose a reason for hiding this comment

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

This should be PLATFORM(IOS_FAMILY) since PLATFORM(COCOA) is PLATFORM(MAC) || PLATFORM(IOS_FAMILY).

@hortont424 is it expected that EventHandlerDrivenSmoothKeyboardScrollingEnabled was true on iOS?

Copy link
Contributor

Choose a reason for hiding this comment

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

Seems like a surprising choice :) I don't recall how WKKeyboardScrollingAnimator wins....

Copy link
Contributor Author

Choose a reason for hiding this comment

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

should I change it to be false by default and only then WebKit::defaultScrollAnimatorEnabled() for MAC?

@webkit-early-warning-system
Copy link
Collaborator

EWS run on previous version of this PR (hash c8af2c3)

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

@webkit-ews-buildbot webkit-ews-buildbot added the merging-blocked Applied to prevent a change from being merged label Nov 12, 2022
@rr-codes rr-codes removed the merging-blocked Applied to prevent a change from being merged label Nov 13, 2022
@webkit-early-warning-system
Copy link
Collaborator

EWS run on previous version of this PR (hash c7245fa)

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
✅ 🛠 🧪 jsc ✅ 🛠 tv   🧪 mac-wk1 ✅ 🛠 jsc-armv7
✅ 🛠 tv-sim ✅ 🧪 mac-wk2 ✅ 🧪 jsc-armv7-tests
✅ 🛠 watch   🧪 mac-AS-debug-wk2 ✅ 🛠 jsc-mips
✅ 🛠 watch-sim ✅ 🧪 mac-wk2-stress ✅ 🧪 jsc-mips-tests

@webkit-ews-buildbot webkit-ews-buildbot added the merging-blocked Applied to prevent a change from being merged label Nov 13, 2022
@rr-codes rr-codes removed the merging-blocked Applied to prevent a change from being merged label Nov 14, 2022
@webkit-early-warning-system
Copy link
Collaborator

Starting EWS tests for cbf021e. Live statuses available at the PR page, #6409

@webkit-early-warning-system
Copy link
Collaborator

EWS run on current version of this PR (hash cbf021e)

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
✅ 🛠 🧪 jsc ✅ 🛠 tv ❌ 🧪 mac-wk1 ✅ 🛠 jsc-armv7
✅ 🛠 tv-sim ✅ 🧪 mac-wk2 ✅ 🧪 jsc-armv7-tests
✅ 🛠 🧪 merge ✅ 🛠 watch ✅ 🧪 mac-AS-debug-wk2 ✅ 🛠 jsc-mips
✅ 🛠 watch-sim ✅ 🧪 mac-wk2-stress ✅ 🧪 jsc-mips-tests

@webkit-ews-buildbot webkit-ews-buildbot added the merging-blocked Applied to prevent a change from being merged label Nov 14, 2022
@rr-codes rr-codes added merge-queue Applied to send a pull request to merge-queue and removed merging-blocked Applied to prevent a change from being merged labels Nov 14, 2022
@rr-codes
Keyboard smooth scrolling animation is not disabled when `NSScrollAni…
207c359 
…mationEnabled` is `false`

https://bugs.webkit.org/show_bug.cgi?id=247822
rdar://101225956

Reviewed by Simon Fraser.

Previously, setting `NSScrollAnimationEnabled` to `false` would disable all
scroll animations. However, enabling keyboard smooth scrolling regressed this
and did not respect this setting any longer.

This PR fixes this by having `EventHandlerDrivenSmoothScrolling` respect the
value of `NSScrollAnimationEnabled` so now scroll animations may once again be
disabled.

* Source/WTF/Scripts/Preferences/WebPreferencesInternal.yaml:

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

Committed 256661@main (207c359): https://commits.webkit.org/256661@main

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

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

@pxlcoder pxlcoder pxlcoder left review comments

@smfr smfr smfr approved these changes

@hortont424 hortont424 Awaiting requested review from hortont424

@JonWBedard JonWBedard Awaiting requested review from JonWBedard JonWBedard 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
7 participants
@rr-codes @pxlcoder @webkit-early-warning-system @webkit-commit-queue @smfr @hortont424 @webkit-ews-buildbot