Fix cursor moving while typing quickly and autocorrection triggered in controlled single line TextInput on iOS (New Arch)#46970
Closed
NickGerleman wants to merge 1 commit into
Closed
Conversation
facebook-github-bot
added
the
CLA Signed
Contributor
|
This pull request was exported from Phabricator. Differential Revision: D64121570 |
NickGerleman
force-pushed
the
export-D64121570
branch
from
32dacc0 to
804bdb1
Compare
Contributor
|
This pull request was exported from Phabricator. Differential Revision: D64121570 |
NickGerleman
changed the title
attributedText from state
NickGerleman
changed the title
attributedText from state
NickGerleman
force-pushed
the
export-D64121570
branch
from
804bdb1 to
4bf3519
Compare
Contributor
|
This pull request was exported from Phabricator. Differential Revision: D64121570 |
NickGerleman
force-pushed
the
export-D64121570
branch
from
4bf3519 to
cb9e1fe
Compare
Contributor
|
This pull request was exported from Phabricator. Differential Revision: D64121570 |
NickGerleman
force-pushed
the
export-D64121570
branch
from
cb9e1fe to
86c90d9
Compare
Contributor
|
This pull request was exported from Phabricator. Differential Revision: D64121570 |
NickGerleman
force-pushed
the
export-D64121570
branch
from
86c90d9 to
fae06e7
Compare
Contributor
|
This pull request was exported from Phabricator. Differential Revision: D64121570 |
|
@NickGerleman hi, pleaaaaaaase try to fix Paper too, there are huge projects out there that will take some extra time to transition to Fabric. Thank you! |
|
@NickGerleman hey, I found the same thing, related with attributes, I created a PR to fix other cases, do you think this can fix those too? Expensify/App#17153, #27693, #29572, #42792 I know my proposal also fix this autocorrection: #39385 |
NickGerleman
force-pushed
the
export-D64121570
branch
from
fae06e7 to
43cff32
Compare
Contributor
|
This pull request was exported from Phabricator. Differential Revision: D64121570 |
NickGerleman
force-pushed
the
export-D64121570
branch
from
43cff32 to
f639240
Compare
Contributor
|
This pull request was exported from Phabricator. Differential Revision: D64121570 |
…n controlled single line TextInput on iOS (New Arch) (react#46970) Summary: Fixes react#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 react/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
NickGerleman
force-pushed
the
export-D64121570
branch
from
f639240 to
a78f7f3
Compare
Contributor
|
This pull request was exported from Phabricator. 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) (react#46970) Summary: Fixes react#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 react/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
Contributor
|
This pull request has been merged in 36fd553. |
Collaborator
|
This pull request was successfully merged by @NickGerleman in 36fd553 When will my fix make it into a release? | How to file a pick request? |
This was referenced Oct 16, 2024
6 tasks
This was referenced Oct 26, 2024
|
@NickGerleman hi, The RN version of expo51 used in our project is 0.74.5, and newArchEnabled is false. According to experience, it will take at least two months to upgrade to expo52 and RN0.76.0. |
blakef
pushed a commit
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 react/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
Collaborator
|
This pull request was successfully merged by @NickGerleman in 40093d9 When will my fix make it into a release? | How to file a pick request? |
This was referenced Apr 1, 2025
|
Our project is currently using Expo 52.0.47 + RN 0.77.3. We ultimately decided to disable the New Architecture because we encountered too many issues after turning it on. We attempted to fix this issue under the Old Architecture. While I am not entirely sure if this fixes the problem fundamentally, our users have not reported this issue again so far. If anyone else is in the same boat—using the Old Architecture and struggling with this issue—here is the patch we are currently using. We hope it helps! |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
6 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
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:
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.
After these fixes, scenario in react/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-domin 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