Mimimize EditText Spans 9/9: Remove addSpansForMeasurement() (#36575)

Summary:
Pull Request resolved: #36575

This is part of a series of changes to minimize the number of spans committed to EditText, as a mitigation for platform issues on Samsung devices. See this [GitHub thread]( #35936 (comment)) for greater context on the platform behavior.

D23670779 addedd a previous mechanism to add spans for measurement caching, like we needed to do as part of this change. It is called in more specific cases (e.g. when there is a text hint but no text), but it edits the live EditText spannable instead of the cache copy, and does not handle nested text at all.

We are already adding spans back to the input after this, behind everything else, and can replace it with the code we have been adding.

Changelog:
[Android][Fixed] - Mimimize EditText Spans 9/9: Remove `addSpansForMeasurement()`

Reviewed By: javache

Differential Revision: D44298159

fbshipit-source-id: 1af44a39de7550b7e66e45db9ebc3523ae9ff002

# Conflicts:
#	ReactAndroid/src/main/java/com/facebook/react/views/textinput/ReactTextInputManager.java

Author: NickGerleman

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

@@ -31,8 +31,6 @@ public class ReactTextUpdate {
   private final int mSelectionEnd;
   private final int mJustificationMode;
 
-  public boolean mContainsMultipleFragments;
-
   /**
    * @deprecated Use a non-deprecated constructor for ReactTextUpdate instead. This one remains
    *     because it's being used by a unit test that isn't currently open source.
@@ -142,13 +140,11 @@ public static ReactTextUpdate buildReactTextUpdateFromState(
       int jsEventCounter,
       int textAlign,
       int textBreakStrategy,
-      int justificationMode,
-      boolean containsMultipleFragments) {
+      int justificationMode) {
 
     ReactTextUpdate reactTextUpdate =
         new ReactTextUpdate(
             text, jsEventCounter, false, textAlign, textBreakStrategy, justificationMode);
-    reactTextUpdate.mContainsMultipleFragments = containsMultipleFragments;
     return reactTextUpdate;
   }
 

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

@@ -64,7 +64,6 @@
 import com.facebook.react.views.text.TextLayoutManager;
 import com.facebook.react.views.view.ReactViewBackgroundManager;
 import java.util.ArrayList;
-import java.util.List;
 import java.util.Objects;
 
 /**
@@ -89,7 +88,6 @@ public class ReactEditText extends AppCompatEditText
   // *TextChanged events should be triggered. This is less expensive than removing the text
   // listeners and adding them back again after the text change is completed.
   protected boolean mIsSettingTextFromJS;
-  protected boolean mIsSettingTextFromCacheUpdate = false;
   private int mDefaultGravityHorizontal;
   private int mDefaultGravityVertical;
 
@@ -369,7 +367,7 @@ protected void onSelectionChanged(int selStart, int selEnd) {
     }
 
     super.onSelectionChanged(selStart, selEnd);
-    if (!mIsSettingTextFromCacheUpdate && mSelectionWatcher != null && hasFocus()) {
+    if (mSelectionWatcher != null && hasFocus()) {
       mSelectionWatcher.onSelectionChanged(selStart, selEnd);
     }
   }
@@ -610,7 +608,7 @@ public void maybeSetText(ReactTextUpdate reactTextUpdate) {
     SpannableStringBuilder spannableStringBuilder =
         new SpannableStringBuilder(reactTextUpdate.getText());
 
-    manageSpans(spannableStringBuilder, reactTextUpdate.mContainsMultipleFragments);
+    manageSpans(spannableStringBuilder);
     stripStyleEquivalentSpans(spannableStringBuilder);
 
     mContainsImages = reactTextUpdate.containsImages();
@@ -639,7 +637,7 @@ public void maybeSetText(ReactTextUpdate reactTextUpdate) {
     }
 
     // Update cached spans (in Fabric only).
-    updateCachedSpannable(false);
+    updateCachedSpannable();
   }
 
   /**
@@ -648,8 +646,7 @@ public void maybeSetText(ReactTextUpdate reactTextUpdate) {
    * will adapt to the new text, hence why {@link SpannableStringBuilder#replace} never removes
    * them.
    */
-  private void manageSpans(
-      SpannableStringBuilder spannableStringBuilder, boolean skipAddSpansForMeasurements) {
+  private void manageSpans(SpannableStringBuilder spannableStringBuilder) {
     Object[] spans = getText().getSpans(0, length(), Object.class);
     for (int spanIdx = 0; spanIdx < spans.length; spanIdx++) {
       Object span = spans[spanIdx];
@@ -677,13 +674,6 @@ private void manageSpans(
         spannableStringBuilder.setSpan(span, spanStart, spanEnd, spanFlags);
       }
     }
-
-    // In Fabric only, apply necessary styles to entire span
-    // If the Spannable was constructed from multiple fragments, we don't apply any spans that could
-    // impact the whole Spannable, because that would override "local" styles per-fragment
-    if (!skipAddSpansForMeasurements) {
-      addSpansForMeasurement(getText());
-    }
   }
 
   // TODO: Replace with Predicate<T> and lambdas once Java 8 builds in OSS
@@ -785,10 +775,10 @@ private <T> void stripSpansOfKind(
   }
 
   /**
-   * Copy back styles represented as attributes to the underlying span, for later measurement
-   * outside the ReactEditText.
+   * Copy styles represented as attributes to the underlying span, for later measurement or other
+   * usage outside the ReactEditText.
    */
-  private void restoreStyleEquivalentSpans(SpannableStringBuilder workingText) {
+  private void addSpansFromStyleAttributes(SpannableStringBuilder workingText) {
     int spanFlags = Spannable.SPAN_INCLUSIVE_INCLUSIVE;
 
     // Set all bits for SPAN_PRIORITY so that this span has the highest possible priority
@@ -844,6 +834,11 @@ private void restoreStyleEquivalentSpans(SpannableStringBuilder workingText) {
           workingText.length(),
           spanFlags);
     }
+
+    float lineHeight = mTextAttributes.getEffectiveLineHeight();
+    if (!Float.isNaN(lineHeight)) {
+      workingText.setSpan(new CustomLineHeightSpan(lineHeight), 0, workingText.length(), spanFlags);
+    }
   }
 
   private static boolean sameTextForSpan(
@@ -862,73 +857,6 @@ private static boolean sameTextForSpan(
     return true;
   }
 
-  // This is hacked in for Fabric. When we delete non-Fabric code, we might be able to simplify or
-  // clean this up a bit.
-  private void addSpansForMeasurement(Spannable spannable) {
-    if (!mFabricViewStateManager.hasStateWrapper()) {
-      return;
-    }
-
-    boolean originalDisableTextDiffing = mDisableTextDiffing;
-    mDisableTextDiffing = true;
-
-    int start = 0;
-    int end = spannable.length();
-
-    // Remove duplicate spans we might add here
-    Object[] spans = spannable.getSpans(0, length(), Object.class);
-    for (Object span : spans) {
-      int spanFlags = spannable.getSpanFlags(span);
-      boolean isInclusive =
-          (spanFlags & Spanned.SPAN_INCLUSIVE_INCLUSIVE) == Spanned.SPAN_INCLUSIVE_INCLUSIVE
-              || (spanFlags & Spanned.SPAN_INCLUSIVE_EXCLUSIVE) == Spanned.SPAN_INCLUSIVE_EXCLUSIVE;
-      if (isInclusive
-          && span instanceof ReactSpan
-          && spannable.getSpanStart(span) == start
-          && spannable.getSpanEnd(span) == end) {
-        spannable.removeSpan(span);
-      }
-    }
-
-    List<TextLayoutManager.SetSpanOperation> ops = new ArrayList<>();
-
-    if (!Float.isNaN(mTextAttributes.getLetterSpacing())) {
-      ops.add(
-          new TextLayoutManager.SetSpanOperation(
-              start, end, new CustomLetterSpacingSpan(mTextAttributes.getLetterSpacing())));
-    }
-    ops.add(
-        new TextLayoutManager.SetSpanOperation(
-            start, end, new ReactAbsoluteSizeSpan((int) mTextAttributes.getEffectiveFontSize())));
-    if (mFontStyle != UNSET || mFontWeight != UNSET || mFontFamily != null) {
-      ops.add(
-          new TextLayoutManager.SetSpanOperation(
-              start,
-              end,
-              new CustomStyleSpan(
-                  mFontStyle,
-                  mFontWeight,
-                  null, // TODO: do we need to support FontFeatureSettings / fontVariant?
-                  mFontFamily,
-                  getReactContext(ReactEditText.this).getAssets())));
-    }
-    if (!Float.isNaN(mTextAttributes.getEffectiveLineHeight())) {
-      ops.add(
-          new TextLayoutManager.SetSpanOperation(
-              start, end, new CustomLineHeightSpan(mTextAttributes.getEffectiveLineHeight())));
-    }
-
-    int priority = 0;
-    for (TextLayoutManager.SetSpanOperation op : ops) {
-      // Actual order of calling {@code execute} does NOT matter,
-      // but the {@code priority} DOES matter.
-      op.execute(spannable, priority);
-      priority++;
-    }
-
-    mDisableTextDiffing = originalDisableTextDiffing;
-  }
-
   protected boolean showSoftKeyboard() {
     return mInputMethodManager.showSoftInput(this, 0);
   }
@@ -1210,7 +1138,7 @@ public FabricViewStateManager getFabricViewStateManager() {
    * TextLayoutManager.java with some very minor modifications. There's some duplication between
    * here and TextLayoutManager, so there might be an opportunity for refactor.
    */
-  private void updateCachedSpannable(boolean resetStyles) {
+  private void updateCachedSpannable() {
     // Noops in non-Fabric
     if (mFabricViewStateManager == null || !mFabricViewStateManager.hasStateWrapper()) {
       return;
@@ -1220,12 +1148,6 @@ private void updateCachedSpannable(boolean resetStyles) {
       return;
     }
 
-    if (resetStyles) {
-      mIsSettingTextFromCacheUpdate = true;
-      addSpansForMeasurement(getText());
-      mIsSettingTextFromCacheUpdate = false;
-    }
-
     Editable currentText = getText();
     boolean haveText = currentText != null && currentText.length() > 0;
 
@@ -1268,7 +1190,6 @@ private void updateCachedSpannable(boolean resetStyles) {
       // - android.app.Activity.dispatchKeyEvent (Activity.java:3447)
       try {
         sb.append(currentText.subSequence(0, currentText.length()));
-        restoreStyleEquivalentSpans(sb);
       } catch (IndexOutOfBoundsException e) {
         ReactSoftExceptionLogger.logSoftException(TAG, e);
       }
@@ -1284,11 +1205,9 @@ private void updateCachedSpannable(boolean resetStyles) {
         // Measure something so we have correct height, even if there's no string.
         sb.append("I");
       }
-
-      // Make sure that all text styles are applied when we're measurable the hint or "blank" text
-      addSpansForMeasurement(sb);
     }
 
+    addSpansFromStyleAttributes(sb);
     TextLayoutManager.setCachedSpannabledForTag(getId(), sb);
   }
 
@@ -1303,7 +1222,7 @@ void setEventDispatcher(@Nullable EventDispatcher eventDispatcher) {
   private class TextWatcherDelegator implements TextWatcher {
     @Override
     public void beforeTextChanged(CharSequence s, int start, int count, int after) {
-      if (!mIsSettingTextFromCacheUpdate && !mIsSettingTextFromJS && mListeners != null) {
+      if (!mIsSettingTextFromJS && mListeners != null) {
         for (TextWatcher listener : mListeners) {
           listener.beforeTextChanged(s, start, count, after);
         }
@@ -1317,23 +1236,20 @@ public void onTextChanged(CharSequence s, int start, int before, int count) {
             TAG, "onTextChanged[" + getId() + "]: " + s + " " + start + " " + before + " " + count);
       }
 
-      if (!mIsSettingTextFromCacheUpdate) {
-        if (!mIsSettingTextFromJS && mListeners != null) {
-          for (TextWatcher listener : mListeners) {
-            listener.onTextChanged(s, start, before, count);
-          }
+      if (!mIsSettingTextFromJS && mListeners != null) {
+        for (TextWatcher listener : mListeners) {
+          listener.onTextChanged(s, start, before, count);
         }
-
-        updateCachedSpannable(
-            !mIsSettingTextFromJS && !mIsSettingTextFromState && start == 0 && before == 0);
       }
 
+      updateCachedSpannable();
+
       onContentSizeChange();
     }
 
     @Override
     public void afterTextChanged(Editable s) {
-      if (!mIsSettingTextFromCacheUpdate && !mIsSettingTextFromJS && mListeners != null) {
+      if (!mIsSettingTextFromJS && mListeners != null) {
         for (TextWatcher listener : mListeners) {
           listener.afterTextChanged(s);
         }

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

@@ -1343,9 +1343,6 @@ public Object updateState(
         TextLayoutManager.getOrCreateSpannableForText(
             view.getContext(), attributedString, mReactTextViewManagerCallback);
 
-    boolean containsMultipleFragments =
-        attributedString.getArray("fragments").toArrayList().size() > 1;
-
     int textBreakStrategy =
         TextAttributeProps.getTextBreakStrategy(paragraphAttributes.getString("textBreakStrategy"));
 
@@ -1354,8 +1351,7 @@ public Object updateState(
         state.getInt("mostRecentEventCount"),
         TextAttributeProps.getTextAlignment(props, TextLayoutManager.isRTL(attributedString)),
         textBreakStrategy,
-        TextAttributeProps.getJustificationMode(props),
-        containsMultipleFragments);
+        TextAttributeProps.getJustificationMode(props, currentJustificationMode));
   }
 
   public Object getReactTextUpdate(ReactEditText view, ReactStylesDiffMap props, MapBuffer state) {
@@ -1376,9 +1372,6 @@ public Object getReactTextUpdate(ReactEditText view, ReactStylesDiffMap props, M
         TextLayoutManagerMapBuffer.getOrCreateSpannableForText(
             view.getContext(), attributedString, mReactTextViewManagerCallback);
 
-    boolean containsMultipleFragments =
-        attributedString.getMapBuffer(TextLayoutManagerMapBuffer.AS_KEY_FRAGMENTS).getCount() > 1;
-
     int textBreakStrategy =
         TextAttributeProps.getTextBreakStrategy(
             paragraphAttributes.getString(TextLayoutManagerMapBuffer.PA_KEY_TEXT_BREAK_STRATEGY));
@@ -1389,7 +1382,6 @@ public Object getReactTextUpdate(ReactEditText view, ReactStylesDiffMap props, M
         TextAttributeProps.getTextAlignment(
             props, TextLayoutManagerMapBuffer.isRTL(attributedString)),
         textBreakStrategy,
-        TextAttributeProps.getJustificationMode(props),
-        containsMultipleFragments);
+        TextAttributeProps.getJustificationMode(props, currentJustificationMode));
   }
 }