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

Fix: multiline textinput start "jerking" when trying to move cursor. #32179

Conversation

xiankuncheng
Copy link
Contributor

@xiankuncheng xiankuncheng commented Sep 10, 2021

Summary

Fixes #30748: on iOS 14, when trying to hold down the space bar and move the cursor on a multi-line TextInput with lots of lines, the cursor could not be scrolled to the desired point. It works as expected on iOS 13 and before.

Figured out that iOS14 acting as expected without [setContentOffset:animated:], so exclude it when iOS version is and above 14.

Credit to @efstathiosntonas for the finding and solution provides.

Related issue

Changelog

[iOS] [Fixed] - Fixed the issue when moving cursor in multi-line TextInput.

Test Plan

  1. Launch RNTester app on iOS
  2. Open TextInput in tab Components
  3. Scroll to Multiline section and focus on the first child
  4. Input lots of dummy texts
  5. Hold the space bar (on device) or press down the mouse inside TextInput (simulator without showing keyboard)

Video Before:

RPReplay_Final1631248010.MP4

Video After:

RPReplay_Final1631248159.MP4

@facebook-github-bot facebook-github-bot added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label Sep 10, 2021
@analysis-bot
Copy link

analysis-bot commented Sep 10, 2021

Platform Engine Arch Size (bytes) Diff
ios - universal n/a --

Base commit: 3562364

@analysis-bot
Copy link

analysis-bot commented Sep 10, 2021

Platform Engine Arch Size (bytes) Diff
android hermes arm64-v8a 7,716,151 -58
android hermes armeabi-v7a 7,244,374 -63
android hermes x86 8,135,468 -66
android hermes x86_64 8,101,021 -60
android jsc arm64-v8a 9,635,411 +141
android jsc armeabi-v7a 8,551,154 +141
android jsc x86 9,649,007 +138
android jsc x86_64 10,258,241 +138

Base commit: 3562364

@facebook-github-bot facebook-github-bot added the Shared with Meta Applied via automation to indicate that an Issue or Pull Request has been shared with the team. label Sep 10, 2021
@xiankuncheng xiankuncheng marked this pull request as ready for review September 11, 2021 19:43
@react-native-bot react-native-bot added Bug Platform: iOS iOS applications. labels Oct 1, 2021
@lunaleaps
Copy link
Contributor

Can we add a comment about why this is needed?

Copy link
Contributor

@lunaleaps lunaleaps left a comment

Choose a reason for hiding this comment

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

Can we leave a comment about what this is for

@xiankuncheng
Copy link
Contributor Author

@lunaleaps thank you for the review.
I've added some comments to explain the reason for the fix. Please let me know I could make it better 🙏

Copy link
Contributor

@lunaleaps lunaleaps left a comment

Choose a reason for hiding this comment

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

Great thank you! I might even add a link to the issue in the comments if that's okay with you? #30748

@facebook-github-bot
Copy link
Contributor

@lunaleaps has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

@xiankuncheng
Copy link
Contributor Author

@lunaleaps that makes sense, added one more comment for the link, thanks for all the help ❤️

@facebook-github-bot
Copy link
Contributor

@lunaleaps has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

Copy link
Contributor

@philIip philIip left a comment

Choose a reason for hiding this comment

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

great work! thanks for doing this.

// Excluding this for iOS 14 and above.
// This fixes multiline textinput "jerking" when trying to move cursor.
// See https://github.com/facebook/react-native/issues/30748 for more context.
#if defined(__IPHONE_OS_VERSION_MAX_ALLOWED) && __IPHONE_OS_VERSION_MAX_ALLOWED < 140000
Copy link
Contributor

Choose a reason for hiding this comment

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

Sorry for the back and forth but chatting internally, we'll update this comment to the following since it seems like setContentOffset animated:NO was originally a fix for "flaky scrolling" for versions prior to iOS 14

// Turn off scroll animation to fix flaky scrolling.
// This is only necessary for iOS <= 13

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No worries at all, this makes more sense and actually reduces the chance of confusion for the next maintainer, thanks for that.

@react-native-bot
Copy link
Collaborator

This pull request was successfully merged by @xiankuncheng in 2280187.

When will my fix make it into a release? | Upcoming Releases

@react-native-bot react-native-bot added the Merged This PR has been merged. label Oct 6, 2021
@xiankuncheng xiankuncheng deleted the fix/multiline-textinput-cursor-moving-on-ios14 branch October 6, 2021 20:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. Merged This PR has been merged. Platform: iOS iOS applications. Shared with Meta Applied via automation to indicate that an Issue or Pull Request has been shared with the team.
Projects
None yet
6 participants