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.

Sign up for GitHub

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

Jump to bottom

[0.76] Fix cursor moving while typing quickly and autocorrection triggered in controlled single line TextInput on iOS (New Arch) #595

Closed
mhoran opened this issue Oct 26, 2024 · 8 comments
Closed

[0.76] Fix cursor moving while typing quickly and autocorrection triggered in controlled single line TextInput on iOS (New Arch) #595

mhoran opened this issue Oct 26, 2024 · 8 comments
Labels
Type: Pick Request

Comments

@mhoran
Copy link

mhoran commented Oct 26, 2024 •
edited
Loading

Target Branch(es)

0.76

Link to commit or PR to be picked

facebook/react-native#46970
facebook/react-native@557e344
and facebook/react-native#47269.

Description

A long standing bug with TextInput has been fixed in New Arch in the aforementioned PR. The bug manifested as many different issues, requiring various workarounds. As of 0.76, at least one of these workarounds no longer works in New Arch (facebook/react-native#29572 (comment)).

Related issues:
facebook/react-native#29572
facebook/react-native#42792
facebook/react-native#27693
... and likely others.
@mhoran
Copy link
Author

mhoran commented Oct 27, 2024

Unfortunately after some testing it seems that the aforementioned issues are not fixed by this patch, though it is still likely worth pulling in.

@cipolleschi
Copy link
Collaborator

cipolleschi commented Oct 28, 2024 •
edited
Loading

@mhoran could you provide a repro with this template to show that it is not fixed?

@cipolleschi
Copy link
Collaborator

We might also need facebook/react-native@557e344

@blakef
Copy link
Collaborator

blakef commented Oct 28, 2024

Holding on this for 0.76.1, waiting for a reproducer.

@mhoran
Copy link
Author

mhoran commented Oct 28, 2024 •
edited
Loading

Here is a repro with both patches applied: https://github.com/mhoran/new-arch-autocorrect-repro.

I am in fact able to reproduce all of the issues that were reportedly fixed with facebook/react-native#46970 -- so perhaps the fix is incomplete.

  1. If I type incredibly fast with a hardware keyboard in a simulator, I see the cursor move incorrectly (Discourage using controlled inputs in React Native because they are broken facebook/react-native-website#4247).
  2. Issues 27693 and 42792, which were both closed when Fix cursor moving while typing quickly and autocorrection triggered in controlled single line TextInput on iOS (New Arch) facebook/react-native#46970 was merged, can be reproduced (type a word, then space twice, and "." will not be entered.)
  3. The undo button flickers when typing and otherwise does not work (iOS TextInput - text undo/redo doesn't work if value, defaultValue, or onChange is set facebook/react-native#29572).

Perhaps there's another missing patch?

@mhoran mhoran mentioned this issue Oct 28, 2024
Closed
@NickGerleman
Copy link
Collaborator

Let me take a look at these, to see if something might have broke since the initial iteration of the change and what was merged. I'm also seeing this not fixed, which wasn't the case earlier.

Once that is settled, I do think we should pick the collection of changes. I was holding out until this week, since as of this Wednesday, the change will have been in major production apps for about a week.

@NickGerleman NickGerleman mentioned this issue Oct 29, 2024
Closed
NickGerleman added a commit to NickGerleman/react-native that referenced this issue Oct 29, 2024
@NickGerleman @facebook-github-bot
Fix missing event emitters on iOS TextInput when controlled component…
a26baae 
… value specified using `value` instead of `children`

Summary:
There were [reports](reactwg/react-native-releases#595) that patching in the fixes for iOS controlled input did not work as expected.

I think tracked this down to a difference in how I tested, where the controlled component I used passed value as a child of the `TextInput`, instead of via `value`. Passing via `value` triggers a secondary bug, where we don't correctly pass a reference to correct ShadowView when creating attributedstring, specifically in the iOS TextInputShadowNode impl. This was first exposed in D52589303 which enabled `-Wextra`, but there, we went with same behavior of passing empty ShadowView, instead of the correct behavior (like Android impl) of passing a ShadowView of the current ShadowNode.

After fixing this, we now correctly create event emitters in the passed attributedstring, which matches expectations for pargraph-level eventemitter now in typing attributes. We don't seem actually use this on iOS for TextInput right now (just Text), but this is likely the right foundation for events regardless.

Changelog:
[iOS][Fixed] - Fix missing event emitters on iOS TextInput when controlled component value specified using `value` instead of `children`

Differential Revision: D65108163
@NickGerleman
Copy link
Collaborator

@mhoran I think I tracked this down. Would you be able to try also applying the change in facebook/react-native#47269 ?

The gist is that, the controlled component I used in testing accepted value as child instead of via value prop, and the case of accepting by value may have a separate bug causing inequality.

@mhoran
Copy link
Author

mhoran commented Oct 29, 2024

facebook/react-native#47269 works for me! All the issues mentioned in #595 (comment) are resolved with the three patches discussed:
facebook/react-native#46970
facebook/react-native@557e344
and facebook/react-native#47269.

Thanks for taking a look at this so quickly!

This was referenced Oct 29, 2024
Open
Open
NickGerleman added a commit to NickGerleman/react-native that referenced this issue Oct 29, 2024
@NickGerleman @facebook-github-bot
Fix missing emitter attributes on iOS TextInput when controlled compo…
9389dd6 
…nent value specified using `value` instead of `children` (facebook#47269)

Summary:

There were [reports](reactwg/react-native-releases#595) that patching in the fixes for iOS controlled input did not work as expected.

I think tracked this down to a difference in how I tested, where the controlled component I used passed value as a child of the `TextInput`, instead of via `value`. Passing via `value` triggers a secondary bug, where we don't correctly pass a reference to correct ShadowView when creating attributedstring, specifically in the iOS TextInputShadowNode impl.

We previously passed nothing for the ShadowView (only the first two struct fields). This was exposed in D52589303 which enabled `-Wextra`, but there, I went with same behavior of passing empty ShadowView, instead of the correct behavior (like Android impl) of passing a ShadowView of the current ShadowNode.

After fixing this, we now correctly create event emitters in the passed attributedstring, which matches expectations for pargraph-level eventemitter now in typing attributes. We don't seem actually use this on iOS for TextInput right now (just Text), but this is likely the right foundation for events regardless.

Changelog:
[iOS][Fixed] - Fix missing emitter attributes on iOS TextInput when controlled component value specified using `value` instead of `children`

Differential Revision: D65108163
facebook-github-bot pushed a commit to facebook/react-native that referenced this issue Oct 29, 2024
@NickGerleman @facebook-github-bot
Fix missing emitter attributes on iOS TextInput when controlled compo…
52cdedb 
…nent value specified using `value` instead of `children` (#47269)

Summary:
Pull Request resolved: #47269

There were [reports](reactwg/react-native-releases#595) that patching in the fixes for iOS controlled input did not work as expected.

I think tracked this down to a difference in how I tested, where the controlled component I used passed value as a child of the `TextInput`, instead of via `value`. Passing via `value` triggers a secondary bug, where we don't correctly pass a reference to correct ShadowView when creating attributedstring, specifically in the iOS TextInputShadowNode impl.

We previously passed nothing for the ShadowView (only the first two struct fields). This was exposed in D52589303 which enabled `-Wextra`, but there, I went with same behavior of passing empty ShadowView, instead of the correct behavior (like Android impl) of passing a ShadowView of the current ShadowNode.

After fixing this, we now correctly create event emitters in the passed attributedstring, which matches expectations for pargraph-level eventemitter now in typing attributes. We don't seem actually use this on iOS for TextInput right now (just Text), but this is likely the right foundation for events regardless.

Changelog:
[iOS][Fixed] - Fix missing emitter attributes on iOS TextInput when controlled component value specified using `value` instead of `children`

Reviewed By: cipolleschi

Differential Revision: D65108163

fbshipit-source-id: 499fe28439fabd2579eca6ded7fd13fd8ea2e43e
@blakef blakef moved this from Inbox to Done / Picked in React Native 0.76 Releases Nov 12, 2024
@blakef blakef closed this as completed by moving to Done / Picked in React Native 0.76 Releases Nov 12, 2024
blakef pushed a commit to facebook/react-native that referenced this issue Nov 12, 2024
@NickGerleman @blakef
Fix missing emitter attributes on iOS TextInput when controlled compo…
fbe38bb 
…nent value specified using `value` instead of `children` (#47269)

Summary:
Pull Request resolved: #47269

There were [reports](reactwg/react-native-releases#595) that patching in the fixes for iOS controlled input did not work as expected.

I think tracked this down to a difference in how I tested, where the controlled component I used passed value as a child of the `TextInput`, instead of via `value`. Passing via `value` triggers a secondary bug, where we don't correctly pass a reference to correct ShadowView when creating attributedstring, specifically in the iOS TextInputShadowNode impl.

We previously passed nothing for the ShadowView (only the first two struct fields). This was exposed in D52589303 which enabled `-Wextra`, but there, I went with same behavior of passing empty ShadowView, instead of the correct behavior (like Android impl) of passing a ShadowView of the current ShadowNode.

After fixing this, we now correctly create event emitters in the passed attributedstring, which matches expectations for pargraph-level eventemitter now in typing attributes. We don't seem actually use this on iOS for TextInput right now (just Text), but this is likely the right foundation for events regardless.

Changelog:
[iOS][Fixed] - Fix missing emitter attributes on iOS TextInput when controlled component value specified using `value` instead of `children`

Reviewed By: cipolleschi

Differential Revision: D65108163

fbshipit-source-id: 499fe28439fabd2579eca6ded7fd13fd8ea2e43e
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
Type: Pick Request
Projects
React Native 0.76 Releases
Status: Done / Picked
Milestone
No milestone
Development

No branches or pull requests
4 participants
@mhoran @blakef @NickGerleman @cipolleschi