-
Notifications
You must be signed in to change notification settings - Fork 24.3k
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
[Android] Fix issue#11068 of duplicating characters when replacing letters to lowercase or uppercase in TextInput #35929
Conversation
Base commit: f071616 |
Hello @fknives, If you have this kind of error: You should install cmake '3.22.1' in Android Studio: It worked for me! |
hello @MaeIg, I get the following error when calling > Task :ReactAndroid:generateCodegenSchemaFromJavaScript FAILED
/{..}/react-native/node_modules/flow-parser/flow_parser.js:807
throw a}function
^
Error: ENOENT: no such file or directory, lstat 'android'
ENOENT: no such file or directory, lstat 'android'
// later
Node.js v19.4.0
Execution failed for task ':ReactAndroid:generateCodegenSchemaFromJavaScript'.
> Process 'command 'node'' finished with non-zero exit value 7 When trying to build packages.rn-tester.android.app to a device/emulator, the error is same as above (second), plus: > Task :ReactAndroid:prepareBoost
FAILURE: Build completed with 3 failures.
// first
1: Task failed with an exception.
-----------
* What went wrong:
Execution failed for task ':packages:rn-tester:android:app:generateCodegenSchemaFromJavaScript'.
> Process 'command 'node'' finished with non-zero exit value 7
// second is the same as for ./gradlew assemble :ReactAndroid:assemble
// third
3: Task failed with an exception.
-----------
* What went wrong:
Execution failed for task ':ReactAndroid:hermes-engine:configureBuildForHermes'.
> A problem occurred starting process 'command 'cmake''
* Exception is:
org.gradle.api.tasks.TaskExecutionException: Execution failed for task ':ReactAndroid:hermes-engine:configureBuildForHermes'.
Caused by: net.rubygrapefruit.platform.NativeException: Could not start 'cmake'
at net.rubygrapefruit.platform.internal.DefaultProcessLauncher.start(DefaultProcessLauncher.java:25)
... 9 more
Caused by: java.io.IOException: error=2, No such file or directory
at java.base/java.lang.ProcessImpl.forkAndExec(Native Method)
at java.base/java.lang.ProcessImpl.<init>(ProcessImpl.java:340)
Caused by: java.io.IOException: Cannot run program "cmake" (in directory "{..}/react-native/sdks/hermes"): error=2, No such file or directory But apparently no issues on CI, so must be just some local config issue on my machine |
@MaeIg Sorry, if I am spamming you, I would just like to ask, what suppose to happen now? |
Hello, For your problem, I've never had this problem so I don't know how to help you. Sorry :/ |
0c25b83
to
d18163a
Compare
^ Had to rebase because of merge conflict |
Hey folks, is this PR affecting this PR? |
Hello, it should have no correlation. This PR is working with Spans with The SPAN_COMPOSING flags should come only from input methods for temporary styling. |
@fknives thanks, I've just wanted to bring this to your attention since the other PR is a critical fix. Congrats for fixing this, it's been around for ages. |
Thank you for pointing me to that PR, it was good idea to have me check out for potential conflicts 👍. It is only fixed if it gets merged :), but I am hopeful. One question if I may, the |
@fknives I have no clue about the ci, I'm not a contributor, sorry. Don't worry about them tho, Meta team will take care of them. |
d18163a
to
6ea4a2b
Compare
6ea4a2b
to
7c3c3ae
Compare
If this solves #11068 we are seeing history in the making. I'm having goosebumps already and its not even merged. |
7c3c3ae
to
e9845c0
Compare
@lunaleaps 👋 I see you have self-assigned #11068, so I thought maybe you could take a look at this PR? I would greatly appreciate it. |
Astonishing that this issue has existed for at least 7 years, and yet a PR which fixes it has gone unnoticed! |
@lunaleaps has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator. |
@fknives Sorry for the delay and thank you so much for the fix! Importing and testing it |
getText().replace(0, length(), spannableStringBuilder); | ||
|
||
if (shouldKeepCompositeSpan) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When I read through this at first, the name composite confused me, as meaning multiple spans together (e.g. nested spans or something else).
Could we consistently use composing instead?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A second question, can there be multiple spans? It looks like we iterate through all of them, so maybe this should be named something like shouldRestoreComposingSpans
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, that makes a lot of sense, I should have used composing, I will adjust!
During my testing I haven't seen multiple composing spans, but it's added by auto complete, so I wouldn't take that as granted. So yes, I agree, the naming would make more sense.
I will adjust and reply on this thread once that's done.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@NickGerleman Adjusted naming as suggested, thank you!
#11068 describes an issue where The repro describes here is around RewriteExample, which happens on the JS side via a controlled component, so a different mechanism. Are these addressing the same issue? |
// When we update text, we trigger onChangeText code that will | ||
// try to update state if the wrapper is available. Temporarily disable | ||
// to prevent an infinite loop. | ||
boolean shouldKeepCompositeSpan = length() == spannableStringBuilder.length(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This means that we only retain composing spans if JS wants to set a new string, and the string is the same length as the one currently held in native.
Is this a heuristic to try to preserve location of the spans? I could imagine some real-world cases where this would bail (e.g. we add a dash in a phone number), where in the problem described, we may still want to preserve the composing span. But I understand this gets a bit hairy.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, it's a heuristic way of preserving the span.
I agree this could be improved to handle more cases, I just tried to keep focus on this specific issue since I am not that familiar with the codebase.
Note that, even if this solution loses the span, the heuristic fails, the code will be working just as it is working currently without the changes introduced
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My overall comment here is that the PR may be addressing a different issue than the one described in the linked issue, since controlled text updates, and autoComplete
, go through different mechanisms.
Because this PR was made targeting 0.67, there have been quite a few changes to this logic over time. So it would be helpful to understand:
- Does duplicated text repro for controlled inputs which make modifications?
- Does this resolve or interact with the text duplication from
autoCapitalize
Hello @NickGerleman, first of all, let me thank you for looking into this, I appreciate this a lot!
I might misunderstand the linked issue, but as I see it:
I created a new project via
And letter duplication still happens if we try to write lowercase letter into this input, and a character after that.
Some comments regarding this on the linked issue, seems to point to that too: |
Thank you for the clarification. I must admit I read the issue title, and not the full description. I think your solution here makes sense with the context. |
…ing letters to lowercase or uppercase in TextInput
Use Composing name everywhere instead of composite to ensure the intent is clear and its not confusing two different things together.
e3fe96e
to
539e1b1
Compare
^ rebased, because a lot has changed on main since the PR was opened |
@lunaleaps has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator. |
Hi @fknives, I'm trying to reproduce the issue on React Native 72 and verify the fix but I'm not able to reproduce the issue. See #11068 (comment) I've also tried copying and pasting the Rewrite Example like so:
Have you been reproducing on 0.72? I've also tried with the New Architecture enabled and disabled. Both don't reproduce for me |
@lunaleaps Hello, thank you for looking into this. I've created a new react-native project with Changed the App.tsx function to:
I try to type "Almost" Screen.Recording.2023-07-13.at.06.03.48.movEdit: |
Ah I see, I am able to reproduce on the emulator! Okay let me verify the fix |
This pull request was successfully merged by @fknives in ab3c00d. When will my fix make it into a release? | Upcoming Releases |
…owercase or uppercase in TextInput (facebook#35929) Summary: These changes are intended to resolve facebook#11068. ## Changelog: [Android] [Fixed] - Fix letters duplication when using autoCapitalize Pull Request resolved: facebook#35929 Test Plan: I took the `RewriteExample` from `TextInputSharedExamples.js` duplicated it, updated the labels, attached to the same screen. Modified its `onChangeText` function, from `text = text.replace(/ /g, '_');` to `text = text.toLowerCase();` then tested via rn-tester as shown in the video: - No duplicate characters - Characters are updated to be lowercase - Long pressing delete properly deletes, doesn’t stop after deleting one character - Suggestions (selected from keyboard) work and are updated to lowercase when it becomes part of the input text - Moving the cursor and typing works, cursor position is kept as it should - Moving the cursor and deleting works - Selection portion and deleting it works, cursor position is kept as it should https://user-images.githubusercontent.com/14225329/213890296-2f194e21-2cf9-493f-a516-5e0212ed070e.mp4 Note: I have tested manually with 0.67.4, because later versions failed on my machine with cmake and argument errors when building the rn-tester from Android Studio to any device. Help regarding that would be appreciated. ## Possible Future Improvements As it can be seen the video, the letter duplication is resolved, however since the lowercase modification happens on the Javascript side, it takes a couple milliseconds and the Uppercase can still be shown momentarily while typing. ## Technical details, why the solution works I've debugged a simple AppCompatEditText with calling the same `getText().replace` in `doAfterTextChanged` with a bit of delay and noticed a difference to the `ReactEditText`. The ReactEditText removes the `android.view.inputmethod.ComposingText` Span in `manageSpans` before calling replace (ComposingText is `Spanned.SPAN_EXCLUSIVE_EXCLUSIVE`). That `ComposingText` Span is used in `android.view.inputmethod.BaseInputConnection` `private replaceText` to find from what point the text should be replaced from when applying suggestions or typing new letters. Without that Span, it defaults to the selection position, which is usually the end of the text causing duplication of the old "word". **In simple terms, while typing with suggestions on the keyboard, each new letter is handled similarly as clicking a suggestion would be, aka replacing the current "word" with the new "word". (let's say "Ar" word with "Are" word)** Another way to describe it: While typing with suggestions, with the ComposingText Span the TextView keeps track of what word completions are suggested for on the keyboard UI. When receiving a new letter input, it replaces the current "word" with a new "word", and without the Span, it replaces nothing at the end (selection point) with the new word which results in character duplication. It also seems to make sense then why without suggestions (like password-visible and secureTextEntry) the issue hasn't occurred. ### Examples How the issue happened: > - User types: A (ComposingText Span becomes (0,1), composingWord: "A") > - Javascript replaced A with a, ComposingText Span was removed from getText() > - User types a new character: r (ComposingText, defaulting to selection, from selection, empty string is replaced with word "Ar") > => Complete text: aAr => letter duplication. How it works with the changes applied: > - User types: A (ComposingText Span becomes (0,1), composingWord: "A") > - Javascript replaces A with a, (ComposingText Span (0,1) is re-set after replace) > - User types a new character: r (ComposingText (0,1), "a" word is replaced with word "Ar". ComposingText becomes (0,2) "Ar") > - Javascript replaced Ar with ar, (ComposingText Span (0,2) is re-set after replace) > => Complete text: ar => no letter duplication > - User selects suggestion "Are" (ComposingText Span (0,2) "ar" is replaced with new word and space "Are ") > - CompleteText: "Are " > - Javascript replaces "Are " with "are " (ComposingText Span doesn't exist, no string after space " ") Note: the Editable.replace also removes the ComposingText, if it doesn't cover the whole text, that's why we need to re-setSpan them even if we wouldn't remove them in `manageSpans`. ## Note This is my first attempt to contribute so if I missed something or messed up, please point out and I will try to adapt. Reviewed By: NickGerleman Differential Revision: D47243817 Pulled By: lunaleaps fbshipit-source-id: 5f3551d17466f2c7cd1aa89ffb09af50e065b52e
@@ -676,6 +684,12 @@ private void manageSpans(SpannableStringBuilder spannableStringBuilder) { | |||
final int spanStart = getText().getSpanStart(span); | |||
final int spanEnd = getText().getSpanEnd(span); | |||
|
|||
// We keep a copy of Composing spans | |||
if (isComposing) { | |||
spannableStringBuilder.setSpan(span, spanStart, spanEnd, spanFlags); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @fknives,
After merging this, we're getting an exception
java.lang.IndexOutOfBoundsException: setSpan (0 ... 3) ends beyond length 0
which is thrown from here:
https://cs.android.com/android/platform/superproject/main/+/refs/heads/main:frameworks/base/core/java/android/text/SpannableStringBuilder.java;l=1324;drc=3d4951806618b5119ec60a12d80c026bf7ae4153;bpv=0;bpt=1
Here is the relevant stack trace:
- android.text.SpannableStringBuilder.checkRange [SpannableStringBuilder.java:1326]
- android.text.SpannableStringBuilder.setSpan [SpannableStringBuilder.java:685]
- android.text.SpannableStringBuilder.setSpan [SpannableStringBuilder.java:677]
- com.facebook.react.views.textinput.ReactEditText.manageSpans [ReactEditText.java:688]
[inlined]
I'm wondering if somehow the spannableStringBuilder state gets out of sync. Do you have any ideas? cc @NickGerleman as well. Here is the internal task: T159263581
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looking at how this is called, I think this may be applying a span at the location of a previous string, which may not exist in the new one.
The other usage of these span indices is when the text is the same as previous. Maybe we should use the heuristic here that we use elsewhere in the PR of applying the span when the new text length is the same as before?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@lunaleaps Ohh, wow, I am so sorry, I should have caught that.
I agree with @NickGerleman, I think what I messed up, is that the length check should have been sooner.
Since the Composing spans are only "restored" if the new and old texts are the same length, it does make sense to only "keep" them on the same condition.
It should have been something like:
private void manageSpans(SpannableStringBuilder spannableStringBuilder) {
boolean shouldKeepComposing = length() == spannableStringBuilder.length(); // new line
Object[] spans = getText().getSpans(0, length(), Object.class);
for (int spanIdx = 0; spanIdx < spans.length; spanIdx++) {
// ...
// We keep a copy of Composing spans
if (shouldKeepComposing && isComposing) { // changed line
spannableStringBuilder.setSpan(span, spanStart, spanEnd, spanFlags);
continue;
}
// ...
I think that should resolve the IndexOutOfBoundsException or similar issue, because with this check the "new" text has the space to host the Composing Span.
Again, I am sorry I haven't caught this, is there any way I could help?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Got it, let me try to make these changes. I'm not sure I totally get the source of the issue. Let me read up on ComposingSpans
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @fknives and @lunaleaps, thanks for investigating this issue. I wonder can we prioritize to fix this issue today as it has a fairly large crash rate and it is blocking our app release for this week? If no action taken, how about we revert this PR to unblock and re-merge it later?
…tters to lowercase or uppercase in TextInput" Summary: Changelog: [Internal] - Back out #35929 due to #35929 (comment) Original Phabricator Diff: D47243817 Reviewed By: sunzhaoyuan Differential Revision: D47785787 fbshipit-source-id: 81598be3f0b4e0e753dfbee4cee781673f900827
@lunaleaps I see you have reverted the changes, I think that was a good decision considering the issues it caused, again I am sorry I have not caught that. |
…tters to lowercase or uppercase in TextInput" Summary: Changelog: [Internal] - Back out facebook#35929 due to facebook#35929 (comment) Original Phabricator Diff: D47243817 Reviewed By: sunzhaoyuan Differential Revision: D47785787 fbshipit-source-id: 81598be3f0b4e0e753dfbee4cee781673f900827
…tters to lowercase or uppercase in TextInput" Summary: Changelog: [Internal] - Back out facebook#35929 due to facebook#35929 (comment) Original Phabricator Diff: D47243817 Reviewed By: sunzhaoyuan Differential Revision: D47785787 fbshipit-source-id: 81598be3f0b4e0e753dfbee4cee781673f900827
…tters to lowercase or uppercase in TextInput" Summary: Changelog: [Internal] - Back out facebook#35929 due to facebook#35929 (comment) Original Phabricator Diff: D47243817 Reviewed By: sunzhaoyuan Differential Revision: D47785787 fbshipit-source-id: 81598be3f0b4e0e753dfbee4cee781673f900827
Summary
These changes are intended to resolve #11068.
Changelog:
[Android] [Fixed] - Fix letters duplication when using autoCapitalize
Test Plan
I took the
RewriteExample
fromTextInputSharedExamples.js
duplicated it, updated the labels, attached to the same screen. Modified itsonChangeText
function, fromtext = text.replace(/ /g, '_');
totext = text.toLowerCase();
then tested via rn-tester as shown in the video:demo_of_lowercase_no_duplicates.mp4
Note: I have tested manually with 0.67.4, because later versions failed on my machine with cmake and argument errors when building the rn-tester from Android Studio to any device.
Help regarding that would be appreciated.
Possible Future Improvements
As it can be seen the video, the letter duplication is resolved, however since the lowercase modification happens on the Javascript side, it takes a couple milliseconds and the Uppercase can still be shown momentarily while typing.
Technical details, why the solution works
I've debugged a simple AppCompatEditText with calling the same
getText().replace
indoAfterTextChanged
with a bit of delay and noticed a difference to theReactEditText
.The ReactEditText removes the
android.view.inputmethod.ComposingText
Span inmanageSpans
before calling replace (ComposingText isSpanned.SPAN_EXCLUSIVE_EXCLUSIVE
).That
ComposingText
Span is used inandroid.view.inputmethod.BaseInputConnection
private replaceText
to find from what point the text should be replaced from when applying suggestions or typing new letters. Without that Span, it defaults to the selection position, which is usually the end of the text causing duplication of the old "word".In simple terms, while typing with suggestions on the keyboard, each new letter is handled similarly as clicking a suggestion would be, aka replacing the current "word" with the new "word". (let's say "Ar" word with "Are" word)
Another way to describe it:
While typing with suggestions, with the ComposingText Span the TextView keeps track of what word completions are suggested for on the keyboard UI. When receiving a new letter input, it replaces the current "word" with a new "word", and without the Span, it replaces nothing at the end (selection point) with the new word which results in character duplication.
It also seems to make sense then why without suggestions (like password-visible and secureTextEntry) the issue hasn't occurred.
Examples
How the issue happened:
How it works with the changes applied:
Note: the Editable.replace also removes the ComposingText, if it doesn't cover the whole text, that's why we need to re-setSpan them even if we wouldn't remove them in
manageSpans
.Note
This is my first attempt to contribute so if I missed something or messed up, please point out and I will try to adapt.