Skip to content

Commit 6524e61

Browse files
JoshuaGrossfacebook-github-bot
authored andcommitted
Fix TextInput measurement caching
Summary: This fixes TextInput measurement caching. Previously we were not setting the correct Spannable in the cache; we needed to additional Spans to it to indicate font size and other attributes. This brings Fabric closer to how non-Fabric was measuring Spannables for TextInputs (see ReactTextInputShadowNode.java). This should fix a few crashes and will be most noticeable with dynamically-sized multiline textinputs where the number of lines changes over time. This also allows us to transmit less data from C++ to Java in the majority of cases. Changelog: [Internal] Differential Revision: D23670779 fbshipit-source-id: cf9b8c848b9e0c2619e01766b72b074248466825
1 parent 0b3f46b commit 6524e61

File tree

7 files changed

+197
-45
lines changed

7 files changed

+197
-45
lines changed

ReactAndroid/src/main/java/com/facebook/react/views/text/CustomLineHeightSpan.java

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -17,7 +17,7 @@
1717
public class CustomLineHeightSpan implements LineHeightSpan, ReactSpan {
1818
private final int mHeight;
1919

20-
CustomLineHeightSpan(float height) {
20+
public CustomLineHeightSpan(float height) {
2121
this.mHeight = (int) Math.ceil(height);
2222
}
2323

ReactAndroid/src/main/java/com/facebook/react/views/text/ReactTextUpdate.java

Lines changed: 5 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -11,8 +11,6 @@
1111

1212
import android.text.Layout;
1313
import android.text.Spannable;
14-
import androidx.annotation.Nullable;
15-
import com.facebook.react.bridge.ReadableMap;
1614

1715
/**
1816
* Class that contains the data needed for a text update. Used by both <Text/> and <TextInput/>
@@ -33,7 +31,7 @@ public class ReactTextUpdate {
3331
private final int mSelectionEnd;
3432
private final int mJustificationMode;
3533

36-
public @Nullable ReadableMap mAttributedString = null;
34+
public boolean mContainsMultipleFragments;
3735

3836
/**
3937
* @deprecated Use a non-deprecated constructor for ReactTextUpdate instead. This one remains
@@ -145,14 +143,13 @@ public static ReactTextUpdate buildReactTextUpdateFromState(
145143
int textAlign,
146144
int textBreakStrategy,
147145
int justificationMode,
148-
ReadableMap attributedString) {
146+
boolean containsMultipleFragments) {
149147

150-
ReactTextUpdate textUpdate =
148+
ReactTextUpdate reactTextUpdate =
151149
new ReactTextUpdate(
152150
text, jsEventCounter, false, textAlign, textBreakStrategy, justificationMode);
153-
154-
textUpdate.mAttributedString = attributedString;
155-
return textUpdate;
151+
reactTextUpdate.mContainsMultipleFragments = containsMultipleFragments;
152+
return reactTextUpdate;
156153
}
157154

158155
public Spannable getText() {

ReactAndroid/src/main/java/com/facebook/react/views/text/TextLayoutManager.java

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -268,7 +268,7 @@ public static long measureText(
268268
if (attributedString.hasKey("cacheId")) {
269269
int cacheId = attributedString.getInt("cacheId");
270270
if (sTagToSpannableCache.containsKey(cacheId)) {
271-
text = sTagToSpannableCache.get(attributedString.getInt("cacheId"));
271+
text = sTagToSpannableCache.get(cacheId);
272272
} else {
273273
return 0;
274274
}
@@ -501,13 +501,13 @@ public static class SetSpanOperation {
501501
protected int start, end;
502502
protected ReactSpan what;
503503

504-
SetSpanOperation(int start, int end, ReactSpan what) {
504+
public SetSpanOperation(int start, int end, ReactSpan what) {
505505
this.start = start;
506506
this.end = end;
507507
this.what = what;
508508
}
509509

510-
public void execute(SpannableStringBuilder sb, int priority) {
510+
public void execute(Spannable sb, int priority) {
511511
// All spans will automatically extend to the right of the text, but not the left - except
512512
// for spans that start at the beginning of the text.
513513
int spanFlags = Spannable.SPAN_EXCLUSIVE_INCLUSIVE;

ReactAndroid/src/main/java/com/facebook/react/views/textinput/ReactEditText.java

Lines changed: 165 additions & 25 deletions
Original file line numberDiff line numberDiff line change
@@ -8,6 +8,7 @@
88
package com.facebook.react.views.textinput;
99

1010
import static com.facebook.react.uimanager.UIManagerHelper.getReactContext;
11+
import static com.facebook.react.views.text.TextAttributeProps.UNSET;
1112

1213
import android.content.Context;
1314
import android.graphics.Rect;
@@ -17,7 +18,7 @@
1718
import android.os.Bundle;
1819
import android.text.Editable;
1920
import android.text.InputType;
20-
import android.text.SpannableString;
21+
import android.text.Spannable;
2122
import android.text.SpannableStringBuilder;
2223
import android.text.Spanned;
2324
import android.text.TextUtils;
@@ -41,6 +42,10 @@
4142
import com.facebook.react.bridge.ReactContext;
4243
import com.facebook.react.uimanager.FabricViewStateManager;
4344
import com.facebook.react.uimanager.UIManagerModule;
45+
import com.facebook.react.views.text.CustomLetterSpacingSpan;
46+
import com.facebook.react.views.text.CustomLineHeightSpan;
47+
import com.facebook.react.views.text.CustomStyleSpan;
48+
import com.facebook.react.views.text.ReactAbsoluteSizeSpan;
4449
import com.facebook.react.views.text.ReactSpan;
4550
import com.facebook.react.views.text.ReactTextUpdate;
4651
import com.facebook.react.views.text.ReactTypefaceUtils;
@@ -49,6 +54,7 @@
4954
import com.facebook.react.views.text.TextLayoutManager;
5055
import com.facebook.react.views.view.ReactViewBackgroundManager;
5156
import java.util.ArrayList;
57+
import java.util.List;
5258

5359
/**
5460
* A wrapper around the EditText that lets us better control what happens when an EditText gets
@@ -70,6 +76,7 @@ public class ReactEditText extends AppCompatEditText
7076
// *TextChanged events should be triggered. This is less expensive than removing the text
7177
// listeners and adding them back again after the text change is completed.
7278
protected boolean mIsSettingTextFromJS;
79+
protected boolean mIsSettingTextFromCacheUpdate = false;
7380
private int mDefaultGravityHorizontal;
7481
private int mDefaultGravityVertical;
7582

@@ -325,7 +332,7 @@ public void setSelection(int start, int end) {
325332
@Override
326333
protected void onSelectionChanged(int selStart, int selEnd) {
327334
super.onSelectionChanged(selStart, selEnd);
328-
if (mSelectionWatcher != null && hasFocus()) {
335+
if (!mIsSettingTextFromCacheUpdate && mSelectionWatcher != null && hasFocus()) {
329336
mSelectionWatcher.onSelectionChanged(selStart, selEnd);
330337
}
331338
}
@@ -502,7 +509,7 @@ public void maybeSetText(ReactTextUpdate reactTextUpdate) {
502509
SpannableStringBuilder spannableStringBuilder =
503510
new SpannableStringBuilder(reactTextUpdate.getText());
504511

505-
manageSpans(spannableStringBuilder);
512+
manageSpans(spannableStringBuilder, reactTextUpdate.mContainsMultipleFragments);
506513
mContainsImages = reactTextUpdate.containsImages();
507514

508515
// When we update text, we trigger onChangeText code that will
@@ -528,10 +535,8 @@ public void maybeSetText(ReactTextUpdate reactTextUpdate) {
528535
}
529536
}
530537

531-
// Update cached spans (in Fabric only)
532-
if (this.getFabricViewStateManager() != null) {
533-
TextLayoutManager.setCachedSpannabledForTag(getId(), spannableStringBuilder);
534-
}
538+
// Update cached spans (in Fabric only).
539+
updateCachedSpannable(false);
535540
}
536541

537542
/**
@@ -540,30 +545,42 @@ public void maybeSetText(ReactTextUpdate reactTextUpdate) {
540545
* will adapt to the new text, hence why {@link SpannableStringBuilder#replace} never removes
541546
* them.
542547
*/
543-
private void manageSpans(SpannableStringBuilder spannableStringBuilder) {
548+
private void manageSpans(
549+
SpannableStringBuilder spannableStringBuilder, boolean skipAddSpansForMeasurements) {
544550
Object[] spans = getText().getSpans(0, length(), Object.class);
545551
for (int spanIdx = 0; spanIdx < spans.length; spanIdx++) {
552+
Object span = spans[spanIdx];
553+
int spanFlags = getText().getSpanFlags(span);
554+
boolean isExclusiveExclusive =
555+
(spanFlags & Spanned.SPAN_EXCLUSIVE_EXCLUSIVE) == Spanned.SPAN_EXCLUSIVE_EXCLUSIVE;
556+
546557
// Remove all styling spans we might have previously set
547-
if (spans[spanIdx] instanceof ReactSpan) {
548-
getText().removeSpan(spans[spanIdx]);
558+
if (span instanceof ReactSpan) {
559+
getText().removeSpan(span);
549560
}
550561

551-
if ((getText().getSpanFlags(spans[spanIdx]) & Spanned.SPAN_EXCLUSIVE_EXCLUSIVE)
552-
!= Spanned.SPAN_EXCLUSIVE_EXCLUSIVE) {
562+
// We only add spans back for EXCLUSIVE_EXCLUSIVE spans
563+
if (!isExclusiveExclusive) {
553564
continue;
554565
}
555-
Object span = spans[spanIdx];
556-
final int spanStart = getText().getSpanStart(spans[spanIdx]);
557-
final int spanEnd = getText().getSpanEnd(spans[spanIdx]);
558-
final int spanFlags = getText().getSpanFlags(spans[spanIdx]);
566+
567+
final int spanStart = getText().getSpanStart(span);
568+
final int spanEnd = getText().getSpanEnd(span);
559569

560570
// Make sure the span is removed from existing text, otherwise the spans we set will be
561571
// ignored or it will cover text that has changed.
562-
getText().removeSpan(spans[spanIdx]);
572+
getText().removeSpan(span);
563573
if (sameTextForSpan(getText(), spannableStringBuilder, spanStart, spanEnd)) {
564574
spannableStringBuilder.setSpan(span, spanStart, spanEnd, spanFlags);
565575
}
566576
}
577+
578+
// In Fabric only, apply necessary styles to entire span
579+
// If the Spannable was constructed from multiple fragments, we don't apply any spans that could
580+
// impact the whole Spannable, because that would override "local" styles per-fragment
581+
if (!skipAddSpansForMeasurements) {
582+
addSpansForMeasurement(getText());
583+
}
567584
}
568585

569586
private static boolean sameTextForSpan(
@@ -582,6 +599,75 @@ private static boolean sameTextForSpan(
582599
return true;
583600
}
584601

602+
// This is hacked in for Fabric. When we delete non-Fabric code, we might be able to simplify or
603+
// clean this up a bit.
604+
private void addSpansForMeasurement(Spannable spannable) {
605+
if (!mFabricViewStateManager.hasStateWrapper()) {
606+
return;
607+
}
608+
609+
boolean originalDisableTextDiffing = mDisableTextDiffing;
610+
mDisableTextDiffing = true;
611+
612+
int start = 0;
613+
int end = spannable.length();
614+
615+
// Remove duplicate spans we might add here
616+
Object[] spans = spannable.getSpans(0, length(), Object.class);
617+
for (Object span : spans) {
618+
int spanFlags = spannable.getSpanFlags(span);
619+
boolean isInclusive =
620+
(spanFlags & Spanned.SPAN_INCLUSIVE_INCLUSIVE) == Spanned.SPAN_INCLUSIVE_INCLUSIVE
621+
|| (spanFlags & Spanned.SPAN_INCLUSIVE_EXCLUSIVE) == Spanned.SPAN_INCLUSIVE_EXCLUSIVE;
622+
if (isInclusive
623+
&& span instanceof ReactSpan
624+
&& spannable.getSpanStart(span) == start
625+
&& spannable.getSpanEnd(span) == end) {
626+
spannable.removeSpan(span);
627+
}
628+
}
629+
630+
List<TextLayoutManager.SetSpanOperation> ops = new ArrayList<>();
631+
632+
if (Build.VERSION.SDK_INT >= Build.VERSION_CODES.LOLLIPOP) {
633+
if (!Float.isNaN(mTextAttributes.getLetterSpacing())) {
634+
ops.add(
635+
new TextLayoutManager.SetSpanOperation(
636+
start, end, new CustomLetterSpacingSpan(mTextAttributes.getLetterSpacing())));
637+
}
638+
}
639+
ops.add(
640+
new TextLayoutManager.SetSpanOperation(
641+
start, end, new ReactAbsoluteSizeSpan((int) mTextAttributes.getEffectiveFontSize())));
642+
if (mFontStyle != UNSET || mFontWeight != UNSET || mFontFamily != null) {
643+
ops.add(
644+
new TextLayoutManager.SetSpanOperation(
645+
start,
646+
end,
647+
new CustomStyleSpan(
648+
mFontStyle,
649+
mFontWeight,
650+
null, // TODO: do we need to support FontFeatureSettings / fontVariant?
651+
mFontFamily,
652+
getReactContext(ReactEditText.this).getAssets())));
653+
}
654+
if (!Float.isNaN(mTextAttributes.getEffectiveLineHeight())) {
655+
ops.add(
656+
new TextLayoutManager.SetSpanOperation(
657+
start, end, new CustomLineHeightSpan(mTextAttributes.getEffectiveLineHeight())));
658+
}
659+
660+
int priority = 0;
661+
for (TextLayoutManager.SetSpanOperation op : ops) {
662+
// Actual order of calling {@code execute} does NOT matter,
663+
// but the {@code priority} DOES matter.
664+
op.execute(spannable, priority);
665+
priority++;
666+
}
667+
668+
mDisableTextDiffing = originalDisableTextDiffing;
669+
}
670+
585671
protected boolean showSoftKeyboard() {
586672
return mInputMethodManager.showSoftInput(this, 0);
587673
}
@@ -842,14 +928,67 @@ public FabricViewStateManager getFabricViewStateManager() {
842928
return mFabricViewStateManager;
843929
}
844930

931+
/**
932+
* Update the cached Spannable used in TextLayoutManager to measure the text in Fabric. This is
933+
* mostly copied from ReactTextInputShadowNode.java (the non-Fabric version) and
934+
* TextLayoutManager.java with some very minor modifications. There's some duplication between
935+
* here and TextLayoutManager, so there might be an opportunity for refactor.
936+
*/
937+
private void updateCachedSpannable(boolean resetStyles) {
938+
// Noops in non-Fabric
939+
if (getFabricViewStateManager() == null) {
940+
return;
941+
}
942+
// If this view doesn't have an ID yet, we don't have a cache key, so bail here
943+
if (getId() == -1) {
944+
return;
945+
}
946+
947+
if (resetStyles) {
948+
mIsSettingTextFromCacheUpdate = true;
949+
addSpansForMeasurement(getText());
950+
mIsSettingTextFromCacheUpdate = false;
951+
}
952+
953+
Editable currentText = getText();
954+
boolean haveText = currentText != null && currentText.length() > 0;
955+
956+
SpannableStringBuilder sb = new SpannableStringBuilder();
957+
958+
// A note of caution: appending currentText to sb appends all the spans of currentText - not
959+
// copies of the Spans, but the actual span objects. Any modifications to sb after that point
960+
// can modify the spans of sb/currentText, impact the text or spans visible on screen, and
961+
// also call the TextChangeWatcher methods.
962+
if (haveText) {
963+
sb.append(currentText);
964+
}
965+
966+
// If we don't have text, make sure we have *something* to measure.
967+
// Hint has the same dimensions - the only thing that's different is background or foreground
968+
// color
969+
if (!haveText) {
970+
if (getHint() != null && getHint().length() > 0) {
971+
sb.append(getHint());
972+
} else {
973+
// Measure something so we have correct height, even if there's no string.
974+
sb.append("I");
975+
}
976+
977+
// Make sure that all text styles are applied when we're measurable the hint or "blank" text
978+
addSpansForMeasurement(sb);
979+
}
980+
981+
TextLayoutManager.setCachedSpannabledForTag(getId(), sb);
982+
}
983+
845984
/**
846985
* This class will redirect *TextChanged calls to the listeners only in the case where the text is
847986
* changed by the user, and not explicitly set by JS.
848987
*/
849988
private class TextWatcherDelegator implements TextWatcher {
850989
@Override
851990
public void beforeTextChanged(CharSequence s, int start, int count, int after) {
852-
if (!mIsSettingTextFromJS && mListeners != null) {
991+
if (!mIsSettingTextFromCacheUpdate && !mIsSettingTextFromJS && mListeners != null) {
853992
for (TextWatcher listener : mListeners) {
854993
listener.beforeTextChanged(s, start, count, after);
855994
}
@@ -858,22 +997,23 @@ public void beforeTextChanged(CharSequence s, int start, int count, int after) {
858997

859998
@Override
860999
public void onTextChanged(CharSequence s, int start, int before, int count) {
861-
if (!mIsSettingTextFromJS && mListeners != null) {
862-
for (TextWatcher listener : mListeners) {
863-
listener.onTextChanged(s, start, before, count);
1000+
if (!mIsSettingTextFromCacheUpdate) {
1001+
if (!mIsSettingTextFromJS && mListeners != null) {
1002+
for (TextWatcher listener : mListeners) {
1003+
listener.onTextChanged(s, start, before, count);
1004+
}
8641005
}
865-
}
8661006

867-
if (getFabricViewStateManager() != null) {
868-
TextLayoutManager.setCachedSpannabledForTag(getId(), new SpannableString(getText()));
1007+
updateCachedSpannable(
1008+
!mIsSettingTextFromJS && !mIsSettingTextFromState && start == 0 && before == 0);
8691009
}
8701010

8711011
onContentSizeChange();
8721012
}
8731013

8741014
@Override
8751015
public void afterTextChanged(Editable s) {
876-
if (!mIsSettingTextFromJS && mListeners != null) {
1016+
if (!mIsSettingTextFromCacheUpdate && !mIsSettingTextFromJS && mListeners != null) {
8771017
for (TextWatcher listener : mListeners) {
8781018
listener.afterTextChanged(s);
8791019
}

0 commit comments

Comments
 (0)