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 with a fixed height scrolls to the bottom when prepending new lines to the text #60

Conversation

fabOnReact
Copy link

@fabOnReact fabOnReact commented Jul 29, 2023

Upstream PR Link

Upstream PR facebook#38679
fixes Expensify/App#19507

Summary:

Please re-state the problem that we are trying to solve in this issue.

Multiline TextInput with a fixed height will scroll to the bottom of the screen when prepending new lines to the text.

What is the root cause of that problem?

The issue is caused by iOS UITextView:

  • The cursor moves to the end of the text when prepending new lines.
  • Moving the cursor to the end of the text triggers the scroll to the bottom.

The behavior was reproduced on an iOS App (without react-native).
The example included below implements a Component RCTUITextView based on UITextView, which modifies the UITextView attributedText with the textViewDidChange callback (source code available in this comment).

Adding a new line on top of the UITextView on iOS results in:
Issue 1) The cursor moves to the end of TextInput text
Issue 2) The TextInput scrolls to the bottom

Reproducing the issue on an iOS App without react-native

2023-06-17.18-16-16.mp4

Issue 1) is already fixed in react-native, which restores the previous cursor position (on Fabric with _setAttributedString) after changing the text.
Issue 2) needs to be fixed in react-native.

What changes do you think we should make in order to solve the problem?

Disable scrollingEnabled when changing the attributedText.

Changelog:

[IOS] [FIXED] - Fix Multiline TextInput with a fixed height scrolls to the bottom when prepending new lines to the text

Test Plan:

Fabric (reproduces on controlled/not controlled TextInput example):

Before After
Simulator.Screen.Recording.-.iPhone.14.Pro.-.2023-07-29.at.11.57.04.mp4
Simulator.Screen.Recording.-.iPhone.14.Pro.-.2023-07-29.at.11.55.11.mp4

Paper (reproduces only on controlled TextInput example):

function TextInputExample() {
  const [text, setText] = React.useState('');

  return (
    <View style={{marginTop: 200}}>
      <TextInput
        style={{height: 50, backgroundColor: 'white'}}
        multiline={true}
        value={text}
        onChangeText={text => {
          setText(text);
        }}
      />
    </View>
  );
}
Before After
Simulator.Screen.Recording.-.iPhone.14.Pro.-.2023-07-29.at.13.05.48.mp4
Simulator.Screen.Recording.-.iPhone.14.Pro.-.2023-07-29.at.13.09.48.mp4

Expensify

Before After
Simulator.Screen.Recording.-.iPhone.14.Pro.-.2023-07-29.at.13.36.02.mp4
Simulator.Screen.Recording.-.iPhone.14.Pro.-.2023-07-29.at.13.39.42.mp4

@fabOnReact
Copy link
Author

fabOnReact commented Jul 29, 2023

The fix #54 is required for base branch ExpensifyRC1-0.72.0-alpha.0.

It's a fix for #49. There Missing of defining of selectionOrigin in textInputDidChangeSelection
It was discovered in updating RN process Expensify/App#18507 (comment)

@dangrous dangrous self-requested a review July 31, 2023 20:16
Copy link

@eVoloshchak eVoloshchak left a comment

Choose a reason for hiding this comment

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

Tests well!

Copy link

@dangrous dangrous left a comment

Choose a reason for hiding this comment

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

Easy enough!

@dangrous dangrous merged commit 306533b into Expensify:ExpensifyRC1-0.72.0-alpha.0 Aug 2, 2023
4 checks passed
@AndrewGable
Copy link

FYI we are removing our fork, so we don't need to merge anything in this fork. We need to get this merged into React Native upstream and deployed.

@dangrous
Copy link

dangrous commented Aug 2, 2023

Ah okay cool. Then, we'll just hold on the upstream one! But it's good to have this fully tested here.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants