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

[Android] Fix issue#11068 of duplicating characters when replacing letters to lowercase or uppercase in TextInput (Adjusted) #38649

Conversation

fknives
Copy link
Contributor

@fknives fknives commented Jul 27, 2023

Summary:

These changes are intended to resolve #11068.
This pull request is an adjustment to #35929 which had issues with it's implementation and had to be reverted. (commit: c1eec4c)

Changelog:

[Android] [Fixed] - Fix letters duplication when using autoCapitalize

Test Plan:

I have added an example to the TextInputSharedExamples.js which can demonstration the fix.

Here is a short video showing the following:

  • 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
short3.mov

Note: I have tested with latest main, 0.72.3, commit before mine: 3c6dbec
Note2: I have tested on Emulator where I can reproduce the original issue. On real device (Samsung A52s 5G) I could reproduce it using Samsung Keyboard, I could not reproduce it on the same device with Gboard.

Here is a longer version which goes through the TextInput examples: typing, clearing, selection, cut, paste etc.
So to ensure this indeed does not cause the same issues as #35929 did.
Uploaded to youtube, because it was too big: https://youtu.be/5IEzXYpfa7w

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.

@facebook-github-bot facebook-github-bot added 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. labels Jul 27, 2023
@fknives fknives force-pushed the fix-issue#11068-android-letter-duplication-adjusted branch from e567413 to 7f47644 Compare July 27, 2023 07:52
@analysis-bot
Copy link

analysis-bot commented Jul 27, 2023

Platform Engine Arch Size (bytes) Diff
android hermes arm64-v8a 17,649,282 +112
android hermes armeabi-v7a n/a --
android hermes x86 n/a --
android hermes x86_64 n/a --
android jsc arm64-v8a 21,026,750 +41
android jsc armeabi-v7a n/a --
android jsc x86 n/a --
android jsc x86_64 n/a --

Base commit: 0239776
Branch: main

@lunaleaps
Copy link
Contributor

lunaleaps commented Aug 3, 2023

Is it possible to put this fix behind a flag? You can use ReactFeatureFlag and create a new flag. I can hook it up to our fb infra to test out and of course every React Native app can override these feature flags.

That way, I can roll out the fix and if it regresses, simply turn off the feature flag vs. needing to revert etc.

Edit: also thanks @fknives for the re-attempt!

Copy link
Contributor

@lunaleaps lunaleaps left a comment

Choose a reason for hiding this comment

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

See comment around flagging this

@fknives fknives force-pushed the fix-issue#11068-android-letter-duplication-adjusted branch from e728ea2 to f32f142 Compare August 4, 2023 10:13
@fknives
Copy link
Contributor Author

fknives commented Aug 4, 2023

@lunaleaps I think Feature Flagging makes sense, I've added the new Flag by default enabled.
Modified my changes to happen only if the new flag is true.

I hope I understood correctly and that's what you meant.

Thank you!

Note: Also had to rebase, because the ReactFeatureFlags.java has been modified since the PR was initially opened.

@fknives fknives requested a review from lunaleaps August 6, 2023 11:46
@fknives fknives force-pushed the fix-issue#11068-android-letter-duplication-adjusted branch from 1e3f017 to 8fde589 Compare August 17, 2023 06:40
@fknives
Copy link
Contributor Author

fknives commented Aug 17, 2023

^ rebase to resolve conflicts

@fknives fknives force-pushed the fix-issue#11068-android-letter-duplication-adjusted branch from 8fde589 to 586cf14 Compare October 12, 2023 20:11
@fknives
Copy link
Contributor Author

fknives commented Oct 12, 2023

^ rebased to resolve conflicts within ReactFeatureFlags.java

Ran rn-tester again, attached video still applies.

@fknives fknives force-pushed the fix-issue#11068-android-letter-duplication-adjusted branch from 586cf14 to 2114952 Compare October 22, 2023 19:49
@fknives
Copy link
Contributor Author

fknives commented Oct 22, 2023

^ rebased to resolve conflicts within ReactFeatureFlags.java

Copy link

This PR is stale because it has been open 180 days with no activity. Remove stale label or comment or this will be closed in 7 days.

@github-actions github-actions bot added the Stale There has been a lack of activity on this issue and it may be closed soon. label Apr 20, 2024
Copy link

This PR was closed because it has been stalled for 7 days with no activity.

@github-actions github-actions bot closed this Apr 27, 2024
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. Stale There has been a lack of activity on this issue and it may be closed soon.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Duplicated letters when autoCapitalize="characters" on android
4 participants