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

Discourage using controlled inputs in React Native because they are broken #4247

Closed
wants to merge 1 commit into from

Conversation

gaearon
Copy link
Contributor

@gaearon gaearon commented Oct 5, 2024

I don't know what else to do.

Basic controlled text editing is broken in React Native.

For example:

import {useState} from 'react';
import {TextInput} from 'react-native';

let App = () => {
  const [value, setValue] = useState('');
  return (
    <TextInput
      style={{marginTop: 200, padding: 100, fontSize: 20}}
      placeholder="Edit me"
      onChangeText={text => setValue(text)}
      value={value}
    />
  );
};
export default App;

This is literally the simplest controlled text example you can think of.

However, React Native is not able to correctly handle if I type fast enough:

oh_no.mov

(The cursor should not lag behind typing — it causes incorrect user input.)

This is not a theoretical bug. We have it in production. It's embarrassing. Many kinds of bugs are tolerable, but this is a "hello world" example and it's broken. This is contributing to the perception that React Native is incapable of producing high quality native apps. I haven't figured out a way to convince the team to prioritize this, so maybe it should just be deprecated.

Why put it in the docs? We need to rip out all controlled inputs from the app, and I think other teams using RN or choosing to adopt it should be aware of this issue. (We've previously reported it in April.) It's not a fun thing to discover when you already have a lot of existing code. It's better to know early.

This is also far from the only bug related to controlled inputs. Here's another one I just ran into today: facebook/react-native#46850. That's actually what pushed me over the line to file this issue.

The typical workaround is to use defaultValue and avoid controlled inputs completely — it should be a recommendation if this cannot get prioritized.

I know there's been a focus on the team on getting things fixed with the New Architecture. That's understandable. However it's equally broken in the New Architecture.

fabric.mov

I know it takes a lot to develop the framework and this issue may simply not be enough of a priority but I think it's important to communicate honestly when one of the most fundamental features is broken at the "hello world" level.

Hence, this PR.

Thanks for consideration.

Copy link

netlify bot commented Oct 5, 2024

Deploy Preview for react-native ready!

Name Link
🔨 Latest commit 6bf4309
🔍 Latest deploy log https://app.netlify.com/sites/react-native/deploys/6701157527184d0008ff43e3
😎 Deploy Preview https://deploy-preview-4247--react-native.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

@gaearon
Copy link
Contributor Author

gaearon commented Oct 5, 2024

Eh, I don't know why the edit isn't showing up in the docs page preview. Would appreciate pointers.

@magus
Copy link

magus commented Oct 5, 2024

I’m on an old version of react-native (0.61.5) which doesn’t seem to repro on iOS 18. Could this be a regression?

@gaearon
Copy link
Contributor Author

gaearon commented Oct 5, 2024

Yeah I think it might've regressed a few times. See facebook/react-native#44157 for current findings.

@goguda
Copy link

goguda commented Oct 5, 2024

I’m on an old version of react-native (0.61.5) which doesn’t seem to repro on iOS 18. Could this be a regression?

We’re on 0.75.3 in production and I was just able to reproduce this on iOS 18. Not on new architecture but are using Hermes.

I had to type really fast and I only saw it happen once over the span of several minutes of typing. But it does seem to be an actual problem. Interesting.

@Simek
Copy link
Collaborator

Simek commented Oct 6, 2024

Eh, I don't know why the edit isn't showing up in the docs page preview. Would appreciate pointers.

Hey Dan, you have altered only the unversioned doc, which means that changes will be only present in the "Next" docs version:

Screenshot 2024-10-06 at 14 16 42

If you want to alter the documentation for already released version, the change need to be applied for a given versioned file:

@@ -995,6 +995,8 @@ The color of the `TextInput` underline.

### `value`

> The `value` field implementation is broken in many subtle ways and is not recommended for use. Until these bugs are fixed, we recommend sticking with `defaultValue` and uncontrolled inputs.
Copy link

@andreialecu andreialecu Oct 6, 2024

Choose a reason for hiding this comment

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

Suggested change
> The `value` field implementation is broken in many subtle ways and is not recommended for use. Until these bugs are fixed, we recommend sticking with `defaultValue` and uncontrolled inputs.
> The `value` field implementation is broken in many subtle ways and is not recommended for use. Until these bugs are fixed, we recommend sticking with `defaultValue` and uncontrolled inputs. See: [#4247](https://github.com/facebook/react-native-website/pull/4247)

Just something I usually like to see attached to these sort of warnings, some sort of issue or link where I can dig deeper.

@rodperottoni
Copy link

I'd like to point out that a lot of people would't know what a "controlled" component is. Introducing this word into the docs might bring a lot of ?'s into the brains of beginners. Should the maintainers perhaps give a brief explanation or point to relevant docs on controlled/uncontrolled components?

@mary-ext
Copy link

mary-ext commented Oct 7, 2024

React docs has a section on controlled/uncontrolled
https://react.dev/learn/sharing-state-between-components#controlled-and-uncontrolled-components

@rodperottoni
Copy link

Great! :)

@goguda
Copy link

goguda commented Oct 8, 2024

So, if you need to change the value of the input programmatically, are you pretty much stuck using the value prop for this case? For example, clearing a password field on wrong password attempt?

@andreialecu
Copy link

So, if you need to change the value of the input programmatically, are you pretty much stuck using the value prop for this case? For example, clearing a password field on wrong password attempt?

See:
https://reactnative.dev/docs/direct-manipulation#setnativeprops-to-edit-textinput-value

@goguda
Copy link

goguda commented Oct 8, 2024

So, if you need to change the value of the input programmatically, are you pretty much stuck using the value prop for this case? For example, clearing a password field on wrong password attempt?

See: https://reactnative.dev/docs/direct-manipulation#setnativeprops-to-edit-textinput-value

Awesome did not know about this, thank you!

NickGerleman added a commit to NickGerleman/react-native that referenced this pull request Oct 11, 2024
…n controlled single line TextInput on iOS (New Arch)

Summary:
This one is a bit of a doozy...

During auto-correct in UITextField (used for single line TextInput) iOS will mutate the buffer in two parts, non-atomically. After the first part, selection is in the wrong position. If we set full new AttributedText at this point, we propagate the incorrect cursor position, and it is never restored. Afaict, there is not a public way to queue our own mutation until after this happens.

In the common case, where we are not mutating text in the controlled component, we shouldn't need to be setting AttributedString in the first place, and we do have an equality comparison there currently. But it is defeated because attributes are not identical. There are a few sources of that:
1. NSParagraphStyle is present in backing input, but not the AttributedString we are setting. 
2. Backing text has an NSShadow with no color (does not render) not in the AttributedText
3. Event emitter attributes change on each update, and new text does not inherit the attributes.

The first two are part of the backing input `typingAttributes`, even if we set a dictionary without them. To solve for them, we make attribute comparison insensitive to the attribute values in a default initialized control. There is code around here fully falling back to attribute insensitive comparison, which we would ideally fix to instead role into this "effective" attribute comparison.

The event emitter attributes being misaligned is a real problem. We fix in a couple ways.
1. We treat the attribute values as equal if the backing event emitter is the same
2. We preserve attributes that we already set and want to expand as part of typingAttributes
3. We set paragraph level event emitter as a default attribute so the first typed character receives it

After these fixes, scenario in facebook/react-native-website#4247 no longer repros in new arch.

Changes which do mutate content may be susceptible to the same style of issue, though on web/`react-dom` in Chrome, this seems to not try to preserve selection at all if the selection is uncontrolled, so this seems like less of an issue. 

I haven't yet looked at old arch, but my guess is we have similar issues there, and could be fixed in similar ways (so, we've been trying to avoid changing it as much as possible, and 0.76+ has new arch as default, so not sure if worth fixing in old impl as well if this is very long running issue).

Changelog:
[iOS][Fixed] - Fix cursor moving in iOS controlled single line TextInput on Autocorrection (New Arch)

Differential Revision: D64121570
NickGerleman added a commit to NickGerleman/react-native that referenced this pull request Oct 11, 2024
…n controlled single line TextInput on iOS (New Arch) (facebook#46970)

Summary:

This one is a bit of a doozy...

During auto-correct in UITextField (used for single line TextInput) iOS will mutate the buffer in two parts, non-atomically. After the first part, selection is in the wrong position. If we set full new AttributedText at this point, we propagate the incorrect cursor position, and it is never restored. Afaict, there is not a public way to queue our own mutation until after this happens.

In the common case, where we are not mutating text in the controlled component, we shouldn't need to be setting AttributedString in the first place, and we do have an equality comparison there currently. But it is defeated because attributes are not identical. There are a few sources of that:
1. NSParagraphStyle is present in backing input, but not the AttributedString we are setting. 
2. Backing text has an NSShadow with no color (does not render) not in the AttributedText
3. Event emitter attributes change on each update, and new text does not inherit the attributes.

The first two are part of the backing input `typingAttributes`, even if we set a dictionary without them. To solve for them, we make attribute comparison insensitive to the attribute values in a default initialized control. There is code around here fully falling back to attribute insensitive comparison, which we would ideally fix to instead role into this "effective" attribute comparison.

The event emitter attributes being misaligned is a real problem. We fix in a couple ways.
1. We treat the attribute values as equal if the backing event emitter is the same
2. We preserve attributes that we already set and want to expand as part of typingAttributes
3. We set paragraph level event emitter as a default attribute so the first typed character receives it

After these fixes, scenario in facebook/react-native-website#4247 no longer repros in new arch.

Changes which do mutate content may be susceptible to the same style of issue, though on web/`react-dom` in Chrome, this seems to not try to preserve selection at all if the selection is uncontrolled, so this seems like less of an issue. 

I haven't yet looked at old arch, but my guess is we have similar issues there, and could be fixed in similar ways (so, we've been trying to avoid changing it as much as possible, and 0.76+ has new arch as default, so not sure if worth fixing in old impl as well if this is very long running issue).

Changelog:
[iOS][Fixed] - Fix cursor moving in iOS controlled single line TextInput on Autocorrection (New Arch)

Differential Revision: D64121570
NickGerleman added a commit to NickGerleman/react-native that referenced this pull request Oct 11, 2024
…n controlled single line TextInput on iOS (New Arch) (facebook#46970)

Summary:

This one is a bit of a doozy...

During auto-correct in UITextField (used for single line TextInput) iOS will mutate the buffer in two parts, non-atomically. After the first part, after iOS triggers `textFieldDidChange`, selection is in the wrong position. If we set full new AttributedText at this point, we propagate the incorrect cursor position, and it is never restored.

In the common case, where we are not mutating text in the controlled component, we shouldn't need to be setting AttributedString in the first place, and we do have an equality comparison there currently. But it is defeated because attributes are not identical. There are a few sources of that:
1. NSParagraphStyle is present in backing input, but not the AttributedString we are setting. 
2. Backing text has an NSShadow with no color (does not render) not in the AttributedText
3. Event emitter attributes change on each update, and new text does not inherit the attributes.

The first two are part of the backing input `typingAttributes`, even if we set a dictionary without them. To solve for them, we make attribute comparison insensitive to the attribute values in a default initialized control. There is code around here fully falling back to attribute insensitive comparison, which we would ideally fix to instead role into this "effective" attribute comparison.

The event emitter attributes being misaligned is a real problem. We fix in a couple ways.
1. We treat the attribute values as equal if the backing event emitter is the same
2. We preserve attributes that we already set and want to expand as part of typingAttributes
3. We set paragraph level event emitter as a default attribute so the first typed character receives it

After these fixes, scenario in facebook/react-native-website#4247 no longer repros in new arch. Typing in debug build also subjectively seems faster? (we are not doing second invalidation of the control on every keypress).

Changes which do mutate content may be susceptible to the same style of issue, though on web/`react-dom` in Chrome, this seems to not try to preserve selection at all if the selection is uncontrolled, so this seems like less of an issue. 

I haven't yet looked at old arch, but my guess is we have similar issues there, and could be fixed in similar ways (so, we've been trying to avoid changing it as much as possible, and 0.76+ has new arch as default, so not sure if worth fixing in old impl as well if this is very long running issue).

Changelog:
[iOS][Fixed] - Fix cursor moving in iOS controlled single line TextInput on Autocorrection (New Arch)

Differential Revision: D64121570
NickGerleman added a commit to NickGerleman/react-native that referenced this pull request Oct 11, 2024
…n controlled single line TextInput on iOS (New Arch) (facebook#46970)

Summary:

This one is a bit of a doozy...

During auto-correct in UITextField (used for single line TextInput) iOS will mutate the buffer in two parts, non-atomically. After the first part, after iOS triggers `textFieldDidChange`, selection is in the wrong position. If we set full new AttributedText at this point, we propagate the incorrect cursor position, and it is never restored.

In the common case, where we are not mutating text in the controlled component, we shouldn't need to be setting AttributedString in the first place, and we do have an equality comparison there currently. But it is defeated because attributes are not identical. There are a few sources of that:
1. NSParagraphStyle is present in backing input, but not the AttributedString we are setting. 
2. Backing text has an NSShadow with no color (does not render) not in the AttributedText
3. Event emitter attributes change on each update, and new text does not inherit the attributes.

The first two are part of the backing input `typingAttributes`, even if we set a dictionary without them. To solve for them, we make attribute comparison insensitive to the attribute values in a default initialized control. There is code around here fully falling back to attribute insensitive comparison, which we would ideally fix to instead role into this "effective" attribute comparison.

The event emitter attributes being misaligned is a real problem. We fix in a couple ways.
1. We treat the attribute values as equal if the backing event emitter is the same
2. We preserve attributes that we already set and want to expand as part of typingAttributes
3. We set paragraph level event emitter as a default attribute so the first typed character receives it

After these fixes, scenario in facebook/react-native-website#4247 no longer repros in new arch. Typing in debug build also subjectively seems faster? (we are not doing second invalidation of the control on every keypress).

Changes which do mutate content may be susceptible to the same style of issue, though on web/`react-dom` in Chrome, this seems to not try to preserve selection at all if the selection is uncontrolled, so this seems like less of an issue. 

I haven't yet looked at old arch, but my guess is we have similar issues there, and could be fixed in similar ways (though, we've been trying to avoid changing it as much as possible, and 0.76+ has new arch as default, so not sure if worth fixing in old impl as well if this is very long running issue).

Changelog:
[iOS][Fixed] - Fix cursor moving in iOS controlled single line TextInput on Autocorrection (New Arch)

Differential Revision: D64121570
@rodperottoni
Copy link

rodperottoni commented Oct 12, 2024

I'd like to point out that a lot of people would't know what a "controlled" component is. Introducing this word into the docs might bring a lot of ?'s into the brains of beginners. Should the maintainers perhaps give a brief explanation or point to relevant docs on controlled/uncontrolled components?

Bro do you even work? It's like asking to put some JSX or hooks explanation there because people might now know what is it. Controlled/Uncontrolled components are typical Junior React Developer interview questions, how did you miss that?

Your behaviour is similar to what I've seen in a lot of cocky mid/senior developers who forgot where they started during my 12 years of app development. It's also the reason a lot of them are stuck as developers and will always need a leader to tame their ego.

Implied knowledge is difficult. Common sense says that if you can make someone else's life easier, then why wouldn't you? In this case, seems like this was already addressed in another doc. End of story.

Have some respect, kid.

@maheshmuttintidev
Copy link

maheshmuttintidev commented Oct 13, 2024

I'd like to point out that a lot of people would't know what a "controlled" component is. Introducing this word into the docs might bring a lot of ?'s into the brains of beginners. Should the maintainers perhaps give a brief explanation or point to relevant docs on controlled/uncontrolled components?

Bro do you even work? It's like asking to put some JSX or hooks explanation there because people might now know what is it. Controlled/Uncontrolled components are typical Junior React Developer interview questions, how did you miss that?

Your behaviour is similar to what I've seen in a lot of cocky mid/senior developers who forgot where they started during my 12 years of app development. It's also the reason a lot of them are stuck as developers and will always need a leader to tame their ego.

Implied knowledge is difficult. Common sense says that if you can make someone else's life easier, then why wouldn't you? In this case, seems like this was already addressed in another doc. End of story.

Have some respect, kid.

Very reasonable reply mate, I like your reply. There are many senior developers miss very basic things sometimes. It's not an issue when that is addressed by giving proper references, so that they can learn. We all know, we are all lifetime learners ✨.

@mahishdino
Copy link

@gaearon weird behaviour over texts

@elicwhite
Copy link
Member

elicwhite commented Oct 14, 2024

Thanks for the report and reproducer. We can confirm that this reproduces, including in our products at Meta. It seems to be an iOS-specific issue. A recent iOS release made our current Controlled TextInput experience worse, specifically due to how Apple’s new predictive text / autocorrect features work.

This error shows up more frequently when using React Native on a device with a hardware keyboard (simulator, attached keyboard to device, desktop) that allows you to type significantly faster than on touchscreens, and on older devices where the CPU is slower.

We kicked off an investigation into the root cause of the regression and what would be required to fix it. @NickGerleman from our team at Meta has a fix up here, with deep investigation into the problem: facebook/react-native#46970. We will be working to validate that PR in our products at Meta before merging. As such, and as TextInput is intended to work in this mode, I’m going to close out this website PR.

@elicwhite elicwhite closed this Oct 14, 2024
NickGerleman added a commit to NickGerleman/react-native that referenced this pull request Oct 14, 2024
…n controlled single line TextInput on iOS (New Arch) (facebook#46970)

Summary:

Fixes facebook#44157

This one is a bit of a doozy...

During auto-correct in UITextField (used for single line TextInput) iOS will mutate the buffer in two parts, non-atomically. After the first part, after iOS triggers `textFieldDidChange`, selection is in the wrong position. If we set full new AttributedText at this point, we propagate the incorrect cursor position, and it is never restored.

In the common case, where we are not mutating text in the controlled component, we shouldn't need to be setting AttributedString in the first place, and we do have an equality comparison there currently. But it is defeated because attributes are not identical. There are a few sources of that:
1. NSParagraphStyle is present in backing input, but not the AttributedString we are setting. 
2. Backing text has an NSShadow with no color (does not render) not in the AttributedText
3. Event emitter attributes change on each update, and new text does not inherit the attributes.

The first two are part of the backing input `typingAttributes`, even if we set a dictionary without them. To solve for them, we make attribute comparison insensitive to the attribute values in a default initialized control. There is code around here fully falling back to attribute insensitive comparison, which we would ideally fix to instead role into this "effective" attribute comparison.

The event emitter attributes being misaligned is a real problem. We fix in a couple ways.
1. We treat the attribute values as equal if the backing event emitter is the same
2. We set paragraph level event emitter as a default attribute so the first typed character receives it

After these fixes, scenario in facebook/react-native-website#4247 no longer repros in new arch. Typing in debug build also subjectively seems faster? (we are not doing second invalidation of the control on every keypress).

Changes which do mutate content may be susceptible to the same style of issue, though on web/`react-dom` in Chrome, this seems to not try to preserve selection at all if the selection is uncontrolled, so this seems like less of an issue. 

I haven't yet looked at old arch, but my guess is we have similar issues there, and could be fixed in similar ways (though, we've been trying to avoid changing it as much as possible, and 0.76+ has new arch as default, so not sure if worth fixing in old impl as well if this is very long running issue).

Changelog:
[iOS][Fixed] - Fix cursor moving in iOS controlled single line TextInput on Autocorrection (New Arch)

Differential Revision: D64121570
@gaearon
Copy link
Contributor Author

gaearon commented Oct 15, 2024

Fantastic work, thank you!

NickGerleman added a commit to NickGerleman/react-native that referenced this pull request Oct 15, 2024
…riggered in controlled single line TextInput on iOS (New Arch) (facebook#46970)

Summary:

Fixes facebook#44157

This one is a bit of a doozy...

During auto-correct in UITextField (used for single line TextInput) iOS will mutate the buffer in two parts, non-atomically. After the first part, after iOS triggers `textFieldDidChange`, selection is in the wrong position. If we set full new AttributedText at this point, we propagate the incorrect cursor position, and it is never restored.

In the common case, where we are not mutating text in the controlled component, we shouldn't need to be setting AttributedString in the first place, and we do have an equality comparison there currently. But it is defeated because attributes are not identical. There are a few sources of that:
1. NSParagraphStyle is present in backing input, but not the AttributedString we are setting. 
2. Backing text has an NSShadow with no color (does not render) not in the AttributedText
3. Event emitter attributes change on each update, and new text does not inherit the attributes.

The first two are part of the backing input `typingAttributes`, even if we set a dictionary without them. To solve for them, we make attribute comparison insensitive to the attribute values in a default initialized control. There is code around here fully falling back to attribute insensitive comparison, which we would ideally fix to instead role into this "effective" attribute comparison.

The event emitter attributes being misaligned is a real problem. We fix in a couple ways.
1. We treat the attribute values as equal if the backing event emitter is the same
2. We set paragraph level event emitter as a default attribute so the first typed character receives it

After these fixes, scenario in facebook/react-native-website#4247 no longer repros in new arch. Typing in debug build also subjectively seems faster? (we are not doing second invalidation of the control on every keypress).

Changes which do mutate content may be susceptible to the same style of issue, though on web/`react-dom` in Chrome, this seems to not try to preserve selection at all if the selection is uncontrolled, so this seems like less of an issue. 

I haven't yet looked at old arch, but my guess is we have similar issues there, and could be fixed in similar ways (though, we've been trying to avoid changing it as much as possible, and 0.76+ has new arch as default, so not sure if worth fixing in old impl as well if this is very long running issue).

Changelog:
[iOS][Fixed] - Fix cursor moving in iOS controlled single line TextInput on Autocorrection (New Arch)

Reviewed By: philIip

Differential Revision: D64121570
NickGerleman added a commit to NickGerleman/react-native that referenced this pull request Oct 15, 2024
…n controlled single line TextInput on iOS (New Arch) (facebook#46970)

Summary:

Fixes facebook#44157

This one is a bit of a doozy...

During auto-correct in UITextField (used for single line TextInput) iOS will mutate the buffer in two parts, non-atomically. After the first part, after iOS triggers `textFieldDidChange`, selection is in the wrong position. If we set full new AttributedText at this point, we propagate the incorrect cursor position, and it is never restored.

In the common case, where we are not mutating text in the controlled component, we shouldn't need to be setting AttributedString in the first place, and we do have an equality comparison there currently. But it is defeated because attributes are not identical. There are a few sources of that:
1. NSParagraphStyle is present in backing input, but not the AttributedString we are setting. 
2. Backing text has an NSShadow with no color (does not render) not in the AttributedText
3. Event emitter attributes change on each update, and new text does not inherit the attributes.

The first two are part of the backing input `typingAttributes`, even if we set a dictionary without them. To solve for them, we make attribute comparison insensitive to the attribute values in a default initialized control. There is code around here fully falling back to attribute insensitive comparison, which we would ideally fix to instead role into this "effective" attribute comparison.

The event emitter attributes being misaligned is a real problem. We fix in a couple ways.
1. We treat the attribute values as equal if the backing event emitter is the same
2. We set paragraph level event emitter as a default attribute so the first typed character receives it

After these fixes, scenario in facebook/react-native-website#4247 no longer repros in new arch. Typing in debug build also subjectively seems faster? (we are not doing second invalidation of the control on every keypress).

Changes which do mutate content may be susceptible to the same style of issue, though on web/`react-dom` in Chrome, this seems to not try to preserve selection at all if the selection is uncontrolled, so this seems like less of an issue. 

I haven't yet looked at old arch, but my guess is we have similar issues there, and could be fixed in similar ways (though, we've been trying to avoid changing it as much as possible, and 0.76+ has new arch as default, so not sure if worth fixing in old impl as well if this is very long running issue).

Changelog:
[iOS][Fixed] - Fix cursor moving in iOS controlled single line TextInput on Autocorrection (New Arch)

Reviewed By: philIip

Differential Revision: D64121570
NickGerleman added a commit to NickGerleman/react-native that referenced this pull request Oct 15, 2024
…n controlled single line TextInput on iOS (New Arch) (facebook#46970)

Summary:

Fixes facebook#44157

This one is a bit of a doozy...

During auto-correct in UITextField (used for single line TextInput) iOS will mutate the buffer in two parts, non-atomically. After the first part, after iOS triggers `textFieldDidChange`, selection is in the wrong position. If we set full new AttributedText at this point, we propagate the incorrect cursor position, and it is never restored.

In the common case, where we are not mutating text in the controlled component, we shouldn't need to be setting AttributedString in the first place, and we do have an equality comparison there currently. But it is defeated because attributes are not identical. There are a few sources of that:
1. NSParagraphStyle is present in backing input, but not the AttributedString we are setting. 
2. Backing text has an NSShadow with no color (does not render) not in the AttributedText
3. Event emitter attributes change on each update, and new text does not inherit the attributes.

The first two are part of the backing input `typingAttributes`, even if we set a dictionary without them. To solve for them, we make attribute comparison insensitive to the attribute values in a default initialized control. There is code around here fully falling back to attribute insensitive comparison, which we would ideally fix to instead role into this "effective" attribute comparison.

The event emitter attributes being misaligned is a real problem. We fix in a couple ways.
1. We treat the attribute values as equal if the backing event emitter is the same
2. We set paragraph level event emitter as a default attribute so the first typed character receives it

After these fixes, scenario in facebook/react-native-website#4247 no longer repros in new arch. Typing in debug build also subjectively seems faster? (we are not doing second invalidation of the control on every keypress).

Changes which do mutate content may be susceptible to the same style of issue, though on web/`react-dom` in Chrome, this seems to not try to preserve selection at all if the selection is uncontrolled, so this seems like less of an issue. 

I haven't yet looked at old arch, but my guess is we have similar issues there, and could be fixed in similar ways (though, we've been trying to avoid changing it as much as possible, and 0.76+ has new arch as default, so not sure if worth fixing in old impl as well if this is very long running issue).

Changelog:
[iOS][Fixed] - Fix cursor moving in iOS controlled single line TextInput on Autocorrection (New Arch)

Reviewed By: javache, philIip

Differential Revision: D64121570
facebook-github-bot pushed a commit to facebook/react-native that referenced this pull request Oct 15, 2024
…n controlled single line TextInput on iOS (New Arch) (#46970)

Summary:
Pull Request resolved: #46970

Fixes #44157

This one is a bit of a doozy...

During auto-correct in UITextField (used for single line TextInput) iOS will mutate the buffer in two parts, non-atomically. After the first part, after iOS triggers `textFieldDidChange`, selection is in the wrong position. If we set full new AttributedText at this point, we propagate the incorrect cursor position, and it is never restored.

In the common case, where we are not mutating text in the controlled component, we shouldn't need to be setting AttributedString in the first place, and we do have an equality comparison there currently. But it is defeated because attributes are not identical. There are a few sources of that:
1. NSParagraphStyle is present in backing input, but not the AttributedString we are setting.
2. Backing text has an NSShadow with no color (does not render) not in the AttributedText
3. Event emitter attributes change on each update, and new text does not inherit the attributes.

The first two are part of the backing input `typingAttributes`, even if we set a dictionary without them. To solve for them, we make attribute comparison insensitive to the attribute values in a default initialized control. There is code around here fully falling back to attribute insensitive comparison, which we would ideally fix to instead role into this "effective" attribute comparison.

The event emitter attributes being misaligned is a real problem. We fix in a couple ways.
1. We treat the attribute values as equal if the backing event emitter is the same
2. We set paragraph level event emitter as a default attribute so the first typed character receives it

After these fixes, scenario in facebook/react-native-website#4247 no longer repros in new arch. Typing in debug build also subjectively seems faster? (we are not doing second invalidation of the control on every keypress).

Changes which do mutate content may be susceptible to the same style of issue, though on web/`react-dom` in Chrome, this seems to not try to preserve selection at all if the selection is uncontrolled, so this seems like less of an issue.

I haven't yet looked at old arch, but my guess is we have similar issues there, and could be fixed in similar ways (though, we've been trying to avoid changing it as much as possible, and 0.76+ has new arch as default, so not sure if worth fixing in old impl as well if this is very long running issue).

Changelog:
[iOS][Fixed] - Fix cursor moving in iOS controlled single line TextInput on Autocorrection (New Arch)

Reviewed By: javache, philIip

Differential Revision: D64121570

fbshipit-source-id: 2b3bd8a3002c33b68af60ffabeffe01e25c7ccfe
@Guseyn
Copy link

Guseyn commented Oct 24, 2024

It's ironic how a framework designed to minimize re-renders causes a text input to re-render on every keystroke. Reactivity is a joke.

blakef pushed a commit to facebook/react-native that referenced this pull request Nov 12, 2024
…n controlled single line TextInput on iOS (New Arch) (#46970)

Summary:
Pull Request resolved: #46970

Fixes #44157

This one is a bit of a doozy...

During auto-correct in UITextField (used for single line TextInput) iOS will mutate the buffer in two parts, non-atomically. After the first part, after iOS triggers `textFieldDidChange`, selection is in the wrong position. If we set full new AttributedText at this point, we propagate the incorrect cursor position, and it is never restored.

In the common case, where we are not mutating text in the controlled component, we shouldn't need to be setting AttributedString in the first place, and we do have an equality comparison there currently. But it is defeated because attributes are not identical. There are a few sources of that:
1. NSParagraphStyle is present in backing input, but not the AttributedString we are setting.
2. Backing text has an NSShadow with no color (does not render) not in the AttributedText
3. Event emitter attributes change on each update, and new text does not inherit the attributes.

The first two are part of the backing input `typingAttributes`, even if we set a dictionary without them. To solve for them, we make attribute comparison insensitive to the attribute values in a default initialized control. There is code around here fully falling back to attribute insensitive comparison, which we would ideally fix to instead role into this "effective" attribute comparison.

The event emitter attributes being misaligned is a real problem. We fix in a couple ways.
1. We treat the attribute values as equal if the backing event emitter is the same
2. We set paragraph level event emitter as a default attribute so the first typed character receives it

After these fixes, scenario in facebook/react-native-website#4247 no longer repros in new arch. Typing in debug build also subjectively seems faster? (we are not doing second invalidation of the control on every keypress).

Changes which do mutate content may be susceptible to the same style of issue, though on web/`react-dom` in Chrome, this seems to not try to preserve selection at all if the selection is uncontrolled, so this seems like less of an issue.

I haven't yet looked at old arch, but my guess is we have similar issues there, and could be fixed in similar ways (though, we've been trying to avoid changing it as much as possible, and 0.76+ has new arch as default, so not sure if worth fixing in old impl as well if this is very long running issue).

Changelog:
[iOS][Fixed] - Fix cursor moving in iOS controlled single line TextInput on Autocorrection (New Arch)

Reviewed By: javache, philIip

Differential Revision: D64121570

fbshipit-source-id: 2b3bd8a3002c33b68af60ffabeffe01e25c7ccfe
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.