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: set text position should not reset component text #49450

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

freeboub
Copy link

Summary:

fix: #49368
description is provided inside the ticket.
When we use TextInput on ios and manage selection with the selection prop, TextInput is reset when we change selection.

Changelog:

[IOS] [FIXED] - Fix selection makes TextInput clear its content when using children

Test Plan:

Tested with sample provided in ticket.
I also test it with my app on both android and ios, but I cannot share video

@facebook-github-bot
Copy link
Contributor

Hi @freeboub!

Thank you for your pull request and welcome to our community.

Action Required

In order to merge any pull request (code, docs, etc.), we require contributors to sign our Contributor License Agreement, and we don't seem to have one on file for you.

Process

In order for us to review and merge your suggested changes, please sign at https://code.facebook.com/cla. If you are contributing on behalf of someone else (eg your employer), the individual CLA may not be sufficient and your employer may need to sign the corporate CLA.

Once the CLA is signed, our tooling will perform checks and validations. Afterwards, the pull request will be tagged with CLA signed. The tagging process may take up to 1 hour after signing. Please give it that time before contacting us about it.

If you have received this in error or have any questions, please contact us at [email protected]. Thanks!

@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 Feb 15, 2025
@facebook-github-bot
Copy link
Contributor

Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Meta Open Source project. Thanks!

@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 Feb 15, 2025
@cipolleschi
Copy link
Contributor

Hi @freeboub, thanks for taking the time to investigate the issue and propose a solution.

I don't believe that the problem lies in there. The JS files are used by both iOS and Android and if that would have been the fix, the issue should affect also Android. Instead, from your issue and reproducer, it looks like Android is working fine and the problem is only on iOS.

That means that the issue is probably happening in the native layer that iOS is responsible for.

@freeboub
Copy link
Author

freeboub commented Feb 18, 2025

Hello, I only partially agree with you :)

First I pushed a new branch (androidReproduction) on the snack sample. I added a button change the selection: the idea is to make the selection growing when I press the button. With this sample I reproduce similar issue on android also.
See the code:

function App(): React.JSX.Element {
  const [selection, setSelection] = useState({start: 0, end: 0});
  return (
    <SafeAreaView style={styles.safeArea}>
      <TextInput style={styles.text}
                 selection={selection}>
        hello World!
      </TextInput>
      <TouchableOpacity style={styles.pressable}
      onPress={() => setSelection((selection) => ({start: 0, end: selection.end  + 1}))}
      >
        <Text>Press Me !</Text>
      </TouchableOpacity>
    </SafeAreaView>
  );
}

With this code, on android, the text disappear when we change the selection. see the video here under.

Screen_recording_20250218_084943.webm

My patch is also fixing this issue.

Where I agree with you is about the differences between platforms.
It looks like, on mount, ios initialize the text displayed earlier than on android, but I am not really sure this is something fixable and that is not the point here. I investigated the issue on iOS as it was really easy to reproduce without user interaction. I may have done a mistake and should have post directly for both platform, but on ios initialization problem it was so easy (and with a smaller sample app) !

Be carefull the issue is present only when the text is displayed in the children, using value instead, doesn't reproduce the issue.

Thank you

@cipolleschi
Copy link
Contributor

Thanks for the extra details! those clarifies the issue better.

@freeboub
Copy link
Author

I just push a fix for types. I think it should be better with that

@freeboub
Copy link
Author

There is still an issue with snapshot, I am checking !

@freeboub
Copy link
Author

@cipolleschi I have just fix the unit tests. Do you want me to add some new unit test with children usage ? It doesn't seem tested for now

@cipolleschi
Copy link
Contributor

@freeboub if you have time, yes, please. More tests will help us keep the issue in check.

@freeboub
Copy link
Author

@cipolleschi I added some unit tests here: 51ee597
but to be honest, it doesn't highlight the issue. The solved issue here is a mix between JS and native, without native implementation, we cannot do a representative unit test... Let me know if it is OK for you, I can revert this patch if it is not good enough

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. Shared with Meta Applied via automation to indicate that an Issue or Pull Request has been shared with the team.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[IOS] - empty TextInput with selection
3 participants