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

[Android] Fix issue#11068 of duplicating characters when replacing letters to lowercase or uppercase in TextInput #35929

Closed
wants to merge 2 commits into from
Closed

[Android] Fix issue#11068 of duplicating characters when replacing letters to lowercase or uppercase in TextInput #35929

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
b499bd9
Issue#11068 [Android] Fix issue of duplicating characters when replac…
fknives Jan 21, 2023
539e1b1
Issue#11068 PR#35929 [Android] Unify naming of introduced variables
fknives Jul 7, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
53 changes: 47 additions & 6 deletions ...t-native/ReactAndroid/src/main/java/com/facebook/react/views/textinput/ReactEditText.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -632,10 +632,13 @@ public void maybeSetText(ReactTextUpdate reactTextUpdate) {
if (reactTextUpdate.getText().length() == 0) {
setText(null);
} else {
// 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 shouldRestoreComposingSpans = length() == spannableStringBuilder.length();

getText().replace(0, length(), spannableStringBuilder);

if (shouldRestoreComposingSpans) {
restoreComposingSpansToTextFrom(spannableStringBuilder);
}
}
mDisableTextDiffing = false;

Expand All @@ -650,10 +653,13 @@ public void maybeSetText(ReactTextUpdate reactTextUpdate) {
}

/**
* Remove and/or add {@link Spanned.SPAN_EXCLUSIVE_EXCLUSIVE} spans, since they should only exist
* as long as the text they cover is the same. All other spans will remain the same, since they
* will adapt to the new text, hence why {@link SpannableStringBuilder#replace} never removes
* Remove and/or add {@link Spanned#SPAN_EXCLUSIVE_EXCLUSIVE} spans, since they should only exist
* as long as the text they cover is the same unless they are {@link Spanned#SPAN_COMPOSING}.
* All other spans will remain the same, since they will adapt to the new text, hence why {@link SpannableStringBuilder#replace} never removes
* them.
* Keep copy of {@link Spanned#SPAN_COMPOSING} Spans in {@param spannableStringBuilder}, because they are important for
* keyboard suggestions. Without keeping these Spans, suggestions default to be put after the current selection position,
* possibly resulting in letter duplication.
*/
private void manageSpans(SpannableStringBuilder spannableStringBuilder) {
Object[] spans = getText().getSpans(0, length(), Object.class);
Expand All @@ -662,6 +668,8 @@ private void manageSpans(SpannableStringBuilder spannableStringBuilder) {
int spanFlags = getText().getSpanFlags(span);
boolean isExclusiveExclusive =
(spanFlags & Spanned.SPAN_EXCLUSIVE_EXCLUSIVE) == Spanned.SPAN_EXCLUSIVE_EXCLUSIVE;
boolean isComposing =
(spanFlags & Spanned.SPAN_COMPOSING) == Spanned.SPAN_COMPOSING;

// Remove all styling spans we might have previously set
if (span instanceof ReactSpan) {
Expand All @@ -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);
Copy link
Contributor

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

Copy link
Contributor

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?

Copy link
Contributor Author

@fknives fknives Jul 25, 2023 •
edited
Loading

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?

Copy link
Contributor

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

Copy link

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?

continue;
}

// Make sure the span is removed from existing text, otherwise the spans we set will be
// ignored or it will cover text that has changed.
getText().removeSpan(span);
Expand Down Expand Up @@ -803,6 +817,33 @@ private void addSpansFromStyleAttributes(SpannableStringBuilder workingText) {
}
}

/**
* Attaches the {@link Spanned#SPAN_COMPOSING} from {@param spannableStringBuilder} to {@link ReactEditText#getText}
*
* See {@link ReactEditText#manageSpans} for more details.
* Also https://github.com/facebook/react-native/issues/11068
*/
private void restoreComposingSpansToTextFrom(SpannableStringBuilder spannableStringBuilder) {
Editable text = getText();
if (text == null) {
return;
}
Object[] spans = spannableStringBuilder.getSpans(0, length(), Object.class);
for (Object span : spans) {
int spanFlags = spannableStringBuilder.getSpanFlags(span);
boolean isComposing = (spanFlags & Spanned.SPAN_COMPOSING) == Spanned.SPAN_COMPOSING;

if (!isComposing) {
continue;
}

final int spanStart = spannableStringBuilder.getSpanStart(span);
final int spanEnd = spannableStringBuilder.getSpanEnd(span);

text.setSpan(span, spanStart, spanEnd, spanFlags);
}
}

private static boolean sameTextForSpan(
final Editable oldText,
final SpannableStringBuilder newText,
Expand Down