From d602c51996a1d9cf1ebdb51a943d89b337f76c00 Mon Sep 17 00:00:00 2001 From: Joshua Gross Date: Mon, 24 Aug 2020 11:57:11 -0700 Subject: [PATCH] Simplify TextInput measurements Summary: Simplify the TextInput measurement mechanism. Now, data only flows from JS->C++->Java and from Java->JS. C++ passes along AttributedStrings from JS if JS updates, and otherwise Java maintains the only source of truth. Previously we tried to keep all three in sync. This was complicated, slow, and even lead to some crashes. This feels a bit hacky but I believe it's the simplest way to achieve this short-term. Ideally, we would use something like `AttributedStringBox` and pass that to State from Java, but currently everything passed through the State system from Java must be serializable as `folly::dynamic`. So, instead, we just cache one Spannable per TextInput component and use ReactTag as the cache identifier for lookup. An interesting side-effect is that `measure` could race with TextInput updates, but the race condition favors measuring the latest text, not outdated values. Followups: - Can we do this without copying the EditText Spannable on every keystroke? Maybe this approach is too aggressive, but I don't want a background thread measuring a Spannable as it's being mutated. - Do we need to support measuring Attachments? - How can we clean up this API? It should work for now, but feels a little hacky. Changelog: [Internal] Reviewed By: mdvacca Differential Revision: D23290230 fbshipit-source-id: 832d2f397d30dfb17b77958af970d9c52a37e88b --- .../react/views/text/TextLayoutManager.java | 28 +++++- .../react/views/textinput/ReactEditText.java | 23 +++-- .../textinput/ReactTextInputManager.java | 99 ++----------------- .../AndroidTextInputShadowNode.cpp | 11 ++- .../AndroidTextInputState.cpp | 9 +- .../androidtextinput/AndroidTextInputState.h | 64 +++--------- .../textlayoutmanager/TextLayoutManager.cpp | 60 ++++++++++- .../textlayoutmanager/TextLayoutManager.h | 9 ++ 8 files changed, 149 insertions(+), 154 deletions(-) diff --git a/ReactAndroid/src/main/java/com/facebook/react/views/text/TextLayoutManager.java b/ReactAndroid/src/main/java/com/facebook/react/views/text/TextLayoutManager.java index 8bde57f62d7..ce32acc099d 100644 --- a/ReactAndroid/src/main/java/com/facebook/react/views/text/TextLayoutManager.java +++ b/ReactAndroid/src/main/java/com/facebook/react/views/text/TextLayoutManager.java @@ -21,6 +21,7 @@ import android.text.TextPaint; import android.util.LayoutDirection; import android.util.LruCache; import android.view.View; +import androidx.annotation.NonNull; import androidx.annotation.Nullable; import com.facebook.common.logging.FLog; import com.facebook.react.bridge.ReadableArray; @@ -33,6 +34,7 @@ import com.facebook.yoga.YogaMeasureMode; import com.facebook.yoga.YogaMeasureOutput; import java.util.ArrayList; import java.util.List; +import java.util.concurrent.ConcurrentHashMap; /** Class responsible of creating {@link Spanned} object for the JS representation of Text */ public class TextLayoutManager { @@ -57,7 +59,10 @@ public class TextLayoutManager { private static final String INCLUDE_FONT_PADDING_KEY = "includeFontPadding"; private static final String TEXT_BREAK_STRATEGY_KEY = "textBreakStrategy"; private static final String MAXIMUM_NUMBER_OF_LINES_KEY = "maximumNumberOfLines"; - private static LruCache sSpannableCache = new LruCache<>(spannableCacheSize); + private static final LruCache sSpannableCache = + new LruCache<>(spannableCacheSize); + private static final ConcurrentHashMap sTagToSpannableCache = + new ConcurrentHashMap<>(); public static boolean isRTL(ReadableMap attributedString) { ReadableArray fragments = attributedString.getArray("fragments"); @@ -70,6 +75,14 @@ public class TextLayoutManager { return false; } + public static void setCachedSpannabledForTag(int reactTag, @NonNull Spannable sp) { + sTagToSpannableCache.put(reactTag, sp); + } + + public static void deleteCachedSpannableForTag(int reactTag) { + sTagToSpannableCache.remove(reactTag); + } + private static void buildSpannableFromFragment( Context context, ReadableArray fragments, @@ -227,8 +240,17 @@ public class TextLayoutManager { // TODO(5578671): Handle text direction (see View#getTextDirectionHeuristic) TextPaint textPaint = sTextPaintInstance; - Spannable text = - getOrCreateSpannableForText(context, attributedString, reactTextViewManagerCallback); + Spannable text; + if (attributedString.hasKey("cacheId")) { + int cacheId = attributedString.getInt("cacheId"); + if (sTagToSpannableCache.containsKey(cacheId)) { + text = sTagToSpannableCache.get(attributedString.getInt("cacheId")); + } else { + return 0; + } + } else { + text = getOrCreateSpannableForText(context, attributedString, reactTextViewManagerCallback); + } int textBreakStrategy = TextAttributeProps.getTextBreakStrategy( diff --git a/ReactAndroid/src/main/java/com/facebook/react/views/textinput/ReactEditText.java b/ReactAndroid/src/main/java/com/facebook/react/views/textinput/ReactEditText.java index 5350aa5364a..cbda60c185b 100644 --- a/ReactAndroid/src/main/java/com/facebook/react/views/textinput/ReactEditText.java +++ b/ReactAndroid/src/main/java/com/facebook/react/views/textinput/ReactEditText.java @@ -17,6 +17,7 @@ import android.os.Build; import android.os.Bundle; import android.text.Editable; import android.text.InputType; +import android.text.SpannableString; import android.text.SpannableStringBuilder; import android.text.Spanned; import android.text.TextUtils; @@ -37,7 +38,6 @@ import androidx.appcompat.widget.AppCompatEditText; import androidx.core.view.AccessibilityDelegateCompat; import androidx.core.view.ViewCompat; import com.facebook.infer.annotation.Assertions; -import com.facebook.react.bridge.JavaOnlyMap; import com.facebook.react.bridge.ReactContext; import com.facebook.react.uimanager.FabricViewStateManager; import com.facebook.react.uimanager.UIManagerModule; @@ -46,6 +46,7 @@ import com.facebook.react.views.text.ReactTextUpdate; import com.facebook.react.views.text.ReactTypefaceUtils; import com.facebook.react.views.text.TextAttributes; import com.facebook.react.views.text.TextInlineImageSpan; +import com.facebook.react.views.text.TextLayoutManager; import com.facebook.react.views.view.ReactViewBackgroundManager; import java.util.ArrayList; @@ -100,7 +101,6 @@ public class ReactEditText extends AppCompatEditText private ReactViewBackgroundManager mReactBackgroundManager; - protected @Nullable JavaOnlyMap mAttributedString = null; private final FabricViewStateManager mFabricViewStateManager = new FabricViewStateManager(); protected boolean mDisableTextDiffing = false; @@ -152,6 +152,11 @@ public class ReactEditText extends AppCompatEditText }); } + @Override + protected void finalize() { + TextLayoutManager.deleteCachedSpannableForTag(getId()); + } + // After the text changes inside an EditText, TextView checks if a layout() has been requested. // If it has, it will not scroll the text to the end of the new text inserted, but wait for the // next layout() to be called. However, we do not perform a layout() after a requestLayout(), so @@ -481,16 +486,13 @@ public class ReactEditText extends AppCompatEditText return; } - if (reactTextUpdate.mAttributedString != null) { - mAttributedString = JavaOnlyMap.deepClone(reactTextUpdate.mAttributedString); - } - // The current text gets replaced with the text received from JS. However, the spans on the // current text need to be adapted to the new text. Since TextView#setText() will remove or // reset some of these spans even if they are set directly, SpannableStringBuilder#replace() is // used instead (this is also used by the keyboard implementation underneath the covers). SpannableStringBuilder spannableStringBuilder = new SpannableStringBuilder(reactTextUpdate.getText()); + manageSpans(spannableStringBuilder); mContainsImages = reactTextUpdate.containsImages(); @@ -516,6 +518,11 @@ public class ReactEditText extends AppCompatEditText setBreakStrategy(reactTextUpdate.getTextBreakStrategy()); } } + + // Update cached spans (in Fabric only) + if (this.getFabricViewStateManager() != null) { + TextLayoutManager.setCachedSpannabledForTag(getId(), spannableStringBuilder); + } } /** @@ -848,6 +855,10 @@ public class ReactEditText extends AppCompatEditText } } + if (getFabricViewStateManager() != null) { + TextLayoutManager.setCachedSpannabledForTag(getId(), new SpannableString(getText())); + } + onContentSizeChange(); } diff --git a/ReactAndroid/src/main/java/com/facebook/react/views/textinput/ReactTextInputManager.java b/ReactAndroid/src/main/java/com/facebook/react/views/textinput/ReactTextInputManager.java index c322d6e02f2..ec3bb9a4010 100644 --- a/ReactAndroid/src/main/java/com/facebook/react/views/textinput/ReactTextInputManager.java +++ b/ReactAndroid/src/main/java/com/facebook/react/views/textinput/ReactTextInputManager.java @@ -35,17 +35,13 @@ import com.facebook.common.logging.FLog; import com.facebook.infer.annotation.Assertions; import com.facebook.react.bridge.Dynamic; import com.facebook.react.bridge.JSApplicationIllegalArgumentException; -import com.facebook.react.bridge.JavaOnlyArray; -import com.facebook.react.bridge.JavaOnlyMap; import com.facebook.react.bridge.ReactContext; import com.facebook.react.bridge.ReactSoftException; import com.facebook.react.bridge.ReadableArray; import com.facebook.react.bridge.ReadableMap; import com.facebook.react.bridge.ReadableNativeMap; import com.facebook.react.bridge.ReadableType; -import com.facebook.react.bridge.WritableArray; import com.facebook.react.bridge.WritableMap; -import com.facebook.react.bridge.WritableNativeArray; import com.facebook.react.bridge.WritableNativeMap; import com.facebook.react.common.MapBuilder; import com.facebook.react.module.annotations.ReactModule; @@ -250,8 +246,6 @@ public class ReactTextInputManager extends BaseViewManager start + before - ? completeStr.substring(start + before) - : ""); - attributedString.putString("string", newCompleteStr); - - // Loop through all fragments and change them in-place - JavaOnlyArray fragments = (JavaOnlyArray) attributedString.getArray("fragments"); - int positionInAttributedString = 0; - boolean found = false; - for (int i = 0; i < fragments.size() && !found; i++) { - JavaOnlyMap fragment = (JavaOnlyMap) fragments.getMap(i); - String fragmentStr = fragment.getString("string"); - int positionBefore = positionInAttributedString; - positionInAttributedString += fragmentStr.length(); - if (positionInAttributedString < start) { - continue; - } - - int relativePosition = start - positionBefore; - found = true; - - // Does the change span multiple Fragments? - // If so, we put any new text entirely in the first - // Fragment that we edit. For example, if you select two words - // across Fragment boundaries, "one | two", and replace them with a - // character "x", the first Fragment will replace "one " with "x", and the - // second Fragment will replace "two" with an empty string. - int remaining = fragmentStr.length() - relativePosition; - - String newString = - fragmentStr.substring(0, relativePosition) - + changedText - + (fragmentStr.substring(relativePosition + Math.min(before, remaining))); - fragment.putString("string", newString); - - // If we're changing 10 characters (before=10) and remaining=3, - // we want to remove 3 characters from this fragment (`Math.min(before, remaining)`) - // and 7 from the next Fragment (`before = 10 - 3`) - if (remaining < before) { - changedText = ""; - start += remaining; - before = before - remaining; - found = false; - } - } - } - - // Fabric: communicate to C++ layer that text has changed - // We need to call `incrementAndGetEventCounter` here explicitly because this - // update may race with other updates. - // TODO: currently WritableNativeMaps/WritableNativeArrays cannot be reused so - // we must recreate these data structures every time. It would be nice to have a - // reusable data-structure to use for TextInput because constructing these and copying - // on every keystroke is very expensive. - if (mEditText.getFabricViewStateManager().hasStateWrapper() && attributedString != null) { + if (mEditText.getFabricViewStateManager().hasStateWrapper()) { + // Fabric: communicate to C++ layer that text has changed + // We need to call `incrementAndGetEventCounter` here explicitly because this + // update may race with other updates. + // We simply pass in the cache ID, which never changes, but UpdateState will still be called + // on the native side, triggering a measure. mEditText .getFabricViewStateManager() .setState( @@ -990,24 +927,8 @@ public class ReactTextInputManager extends BaseViewManagermeasureCachedSpannableById( + getStateData().cachedAttributedStringId, + getConcreteProps().paragraphAttributes, + layoutConstraints) + .size; + } + // Layout is called right after measure. // Measure is marked as `const`, and `layout` is not; so State can be updated // during layout, but not during `measure`. If State is out-of-date in layout, @@ -179,7 +188,7 @@ Size AndroidTextInputShadowNode::measureContent( attributedString = getPlaceholderAttributedString(); } - if (attributedString.isEmpty()) { + if (attributedString.isEmpty() && getStateData().mostRecentEventCount != 0) { return {0, 0}; } diff --git a/ReactCommon/react/renderer/components/textinput/androidtextinput/react/renderer/components/androidtextinput/AndroidTextInputState.cpp b/ReactCommon/react/renderer/components/textinput/androidtextinput/react/renderer/components/androidtextinput/AndroidTextInputState.cpp index d5106ef5fed..15683507999 100644 --- a/ReactCommon/react/renderer/components/textinput/androidtextinput/react/renderer/components/androidtextinput/AndroidTextInputState.cpp +++ b/ReactCommon/react/renderer/components/textinput/androidtextinput/react/renderer/components/androidtextinput/AndroidTextInputState.cpp @@ -18,9 +18,12 @@ folly::dynamic AndroidTextInputState::getDynamic() const { // Java doesn't need all fields, so we don't pass them along. folly::dynamic newState = folly::dynamic::object(); newState["mostRecentEventCount"] = mostRecentEventCount; - newState["attributedString"] = toDynamic(attributedString); - newState["paragraphAttributes"] = toDynamic(paragraphAttributes); - newState["hash"] = newState["attributedString"]["hash"]; + if (mostRecentEventCount != 0) { + newState["attributedString"] = toDynamic(attributedString); + newState["hash"] = newState["attributedString"]["hash"]; + } + newState["paragraphAttributes"] = + toDynamic(paragraphAttributes); // TODO: can we memoize this in Java? return newState; } #endif diff --git a/ReactCommon/react/renderer/components/textinput/androidtextinput/react/renderer/components/androidtextinput/AndroidTextInputState.h b/ReactCommon/react/renderer/components/textinput/androidtextinput/react/renderer/components/androidtextinput/AndroidTextInputState.h index 3f60eb0bbab..2c039ab8a3f 100644 --- a/ReactCommon/react/renderer/components/textinput/androidtextinput/react/renderer/components/androidtextinput/AndroidTextInputState.h +++ b/ReactCommon/react/renderer/components/textinput/androidtextinput/react/renderer/components/androidtextinput/AndroidTextInputState.h @@ -19,15 +19,21 @@ namespace facebook { namespace react { /* - * State for component. - * Represents what to render and how to render. + * State for component. */ class AndroidTextInputState final { public: int64_t mostRecentEventCount{0}; + /** + * Stores an opaque cache ID used on the Java side to refer to a specific + * AttributedString for measurement purposes only. + */ + int cachedAttributedStringId{0}; + /* * All content of component represented as an `AttributedString`. + * Only set if changed from the React tree's perspective. */ AttributedString attributedString{}; @@ -76,49 +82,6 @@ class AndroidTextInputState final { float defaultThemePaddingTop{NAN}; float defaultThemePaddingBottom{NAN}; -#ifdef ANDROID - AttributedString updateAttributedString( - TextAttributes const &defaultTextAttributes, - ShadowView const &defaultParentShadowView, - AttributedString const &original, - folly::dynamic const &data) { - if (data["textChanged"].empty()) { - return original; - } - - // TODO: parse other attributes besides just string? - // on the other hand, not much should be driven from Java - // TODO: it'd be really nice to treat these as operational transforms - // instead of having to pass the whole string across. - // Unfortunately we don't have a good way of communicating from Java to C++ - // *which* version of the State changes should be applied to; and if there's - // a conflict, we don't have any recourse of any way to bail out of a - // commit. - - auto str = AttributedString{}; - - int i = 0; - folly::dynamic fragments = data["textChanged"]["fragments"]; - for (auto const &fragment : original.getFragments()) { - str.appendFragment(AttributedString::Fragment{ - fragments.size() > i ? fragments[i]["string"].getString() : "", - fragment.textAttributes, - fragment.parentShadowView}); - i++; - } - - if (fragments.size() > original.getFragments().size()) { - for (; i < fragments.size(); i++) { - str.appendFragment( - AttributedString::Fragment{fragments[i]["string"].getString(), - defaultTextAttributes, - defaultParentShadowView}); - } - } - - return str; - } - AndroidTextInputState( int64_t mostRecentEventCount, AttributedString const &attributedString, @@ -132,6 +95,7 @@ class AndroidTextInputState final { float defaultThemePaddingTop, float defaultThemePaddingBottom) : mostRecentEventCount(mostRecentEventCount), + cachedAttributedStringId(0), attributedString(attributedString), reactTreeAttributedString(reactTreeAttributedString), paragraphAttributes(paragraphAttributes), @@ -150,11 +114,10 @@ class AndroidTextInputState final { "mostRecentEventCount", previousState.mostRecentEventCount) .getInt()), - attributedString(updateAttributedString( - previousState.defaultTextAttributes, - previousState.defaultParentShadowView, - previousState.attributedString, - data)), + cachedAttributedStringId( + data.getDefault("cacheId", previousState.cachedAttributedStringId) + .getInt()), + attributedString(previousState.attributedString), reactTreeAttributedString(previousState.reactTreeAttributedString), paragraphAttributes(previousState.paragraphAttributes), defaultTextAttributes(previousState.defaultTextAttributes), @@ -178,7 +141,6 @@ class AndroidTextInputState final { previousState.defaultThemePaddingBottom) .getDouble()){}; folly::dynamic getDynamic() const; -#endif }; } // namespace react diff --git a/ReactCommon/react/renderer/textlayoutmanager/platform/android/react/renderer/textlayoutmanager/TextLayoutManager.cpp b/ReactCommon/react/renderer/textlayoutmanager/platform/android/react/renderer/textlayoutmanager/TextLayoutManager.cpp index 9d7bc047540..86b26366874 100644 --- a/ReactCommon/react/renderer/textlayoutmanager/platform/android/react/renderer/textlayoutmanager/TextLayoutManager.cpp +++ b/ReactCommon/react/renderer/textlayoutmanager/platform/android/react/renderer/textlayoutmanager/TextLayoutManager.cpp @@ -36,6 +36,64 @@ TextMeasurement TextLayoutManager::measure( }); } +TextMeasurement TextLayoutManager::measureCachedSpannableById( + int cacheId, + ParagraphAttributes paragraphAttributes, + LayoutConstraints layoutConstraints) const { + const jni::global_ref &fabricUIManager = + contextContainer_->at>("FabricUIManager"); + + auto env = Environment::current(); + auto attachmentPositions = env->NewFloatArray(0); + + static auto measure = + jni::findClassStatic("com/facebook/react/fabric/FabricUIManager") + ->getMethod("measure"); + + auto minimumSize = layoutConstraints.minimumSize; + auto maximumSize = layoutConstraints.maximumSize; + + local_ref componentName = make_jstring("RCTText"); + folly::dynamic cacheIdMap; + cacheIdMap["cacheId"] = cacheId; + local_ref attributedStringRNM = + ReadableNativeMap::newObjectCxxArgs(cacheIdMap); + local_ref paragraphAttributesRNM = + ReadableNativeMap::newObjectCxxArgs(toDynamic(paragraphAttributes)); + + local_ref attributedStringRM = make_local( + reinterpret_cast(attributedStringRNM.get())); + local_ref paragraphAttributesRM = make_local( + reinterpret_cast(paragraphAttributesRNM.get())); + auto size = yogaMeassureToSize(measure( + fabricUIManager, + -1, // TODO: we should pass rootTag in + componentName.get(), + attributedStringRM.get(), + paragraphAttributesRM.get(), + nullptr, + minimumSize.width, + maximumSize.width, + minimumSize.height, + maximumSize.height, + attachmentPositions)); + + // TODO: currently we do not support attachments for cached IDs - should we? + auto attachments = TextMeasurement::Attachments{}; + + return TextMeasurement{size, attachments}; +} + TextMeasurement TextLayoutManager::doMeasure( AttributedString attributedString, ParagraphAttributes paragraphAttributes, @@ -82,7 +140,7 @@ TextMeasurement TextLayoutManager::doMeasure( reinterpret_cast(paragraphAttributesRNM.get())); auto size = yogaMeassureToSize(measure( fabricUIManager, - -1, + -1, // TODO: we should pass rootTag in componentName.get(), attributedStringRM.get(), paragraphAttributesRM.get(), diff --git a/ReactCommon/react/renderer/textlayoutmanager/platform/android/react/renderer/textlayoutmanager/TextLayoutManager.h b/ReactCommon/react/renderer/textlayoutmanager/platform/android/react/renderer/textlayoutmanager/TextLayoutManager.h index 0136e7084ba..926a9498f28 100644 --- a/ReactCommon/react/renderer/textlayoutmanager/platform/android/react/renderer/textlayoutmanager/TextLayoutManager.h +++ b/ReactCommon/react/renderer/textlayoutmanager/platform/android/react/renderer/textlayoutmanager/TextLayoutManager.h @@ -37,6 +37,15 @@ class TextLayoutManager { ParagraphAttributes paragraphAttributes, LayoutConstraints layoutConstraints) const; + /** + * Measures an AttributedString on the platform, as identified by some + * opaque cache ID. + */ + TextMeasurement measureCachedSpannableById( + int cacheId, + ParagraphAttributes paragraphAttributes, + LayoutConstraints layoutConstraints) const; + /* * Returns an opaque pointer to platform-specific TextLayoutManager. * Is used on a native views layer to delegate text rendering to the manager.