Skip to content

Commit

Permalink
TextInput: keep C++ state in-sync with updated AttributedStrings in Java
Browse files Browse the repository at this point in the history 
Summary:
When the TextInput is updated on the Java side, make sure C++ State gets updated. We do this by making sure that the AttributedString data-structured in mirrored in Java and in C++.

In practice, the AttributedString is copied into Java a few times during initialization, and then after then, 99% of the time Java is writing without receiving updates from C++. This means that we should optimize the Java-to-C++ update path most aggressively in the future.

However, it turns out that for now, at least, we can't reuse NativeWritableMaps/NativeWritableArrays because they're consumed on the C++ side and can't be modified after that. This is a perf improvement for the future.

This allows us the user to edit any fragments, and the changes will flow through C++ State. This also allows us to edit across multiple Fragments.

Changelog: [Internal]

Reviewed By: mdvacca

Differential Revision: D18785960

fbshipit-source-id: 97b283ec411081eca4d2d7a4cce2b31b5e237c42
  • Loading branch information
@JoshuaGross @facebook-github-bot
JoshuaGross authored and facebook-github-bot committed Dec 5, 2019
1 parent 2a46980 commit 0bae474
Show file tree
Hide file tree
Showing 3 changed files with 158 additions and 18 deletions.
21 changes: 21 additions & 0 deletions ReactAndroid/src/main/java/com/facebook/react/views/text/ReactTextUpdate.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,8 @@

import android.text.Layout;
import android.text.Spannable;
import androidx.annotation.Nullable;
import com.facebook.react.bridge.ReadableMap;

/**
* Class that contains the data needed for a text update. Used by both <Text/> and <TextInput/>
Expand All @@ -31,6 +33,8 @@ public class ReactTextUpdate {
private final int mSelectionEnd;
private final int mJustificationMode;

public @Nullable ReadableMap mAttributedString = null;

/**
* @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.
Expand Down Expand Up @@ -135,6 +139,23 @@ public ReactTextUpdate(
mJustificationMode = justificationMode;
}

public static ReactTextUpdate buildReactTextUpdateFromState(
Spannable text,
int jsEventCounter,
boolean containsImages,
int textAlign,
int textBreakStrategy,
int justificationMode,
ReadableMap attributedString) {

ReactTextUpdate textUpdate =
new ReactTextUpdate(
text, jsEventCounter, containsImages, textAlign, textBreakStrategy, justificationMode);

textUpdate.mAttributedString = attributedString;
return textUpdate;
}

public Spannable getText() {
return mText;
}
Expand Down
49 changes: 35 additions & 14 deletions ReactAndroid/src/main/java/com/facebook/react/views/textinput/ReactEditText.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -35,9 +35,8 @@
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.bridge.WritableMap;
import com.facebook.react.bridge.WritableNativeMap;
import com.facebook.react.uimanager.StateWrapper;
import com.facebook.react.uimanager.UIManagerModule;
import com.facebook.react.views.text.ReactSpan;
Expand Down Expand Up @@ -72,8 +71,13 @@ public class ReactEditText extends EditText {
private boolean mShouldAllowFocus;
private int mDefaultGravityHorizontal;
private int mDefaultGravityVertical;

/** A count of events sent to JS or C++. */
protected int mNativeEventCount;

/** The most recent event number acked by JavaScript. Should only be updated from JS, not C++. */
protected int mMostRecentEventCount;

private @Nullable ArrayList<TextWatcher> mListeners;
private @Nullable TextWatcherDelegator mTextWatcherDelegator;
private int mStagedInputType;
Expand All @@ -95,7 +99,11 @@ public class ReactEditText extends EditText {

private ReactViewBackgroundManager mReactBackgroundManager;

protected @Nullable JavaOnlyMap mAttributedString = null;
protected @Nullable StateWrapper mStateWrapper = null;
protected boolean mDisableTextDiffing = false;

protected boolean mIsSettingTextFromState = false;

private static final KeyListener sKeyListener = QwertyKeyListener.getInstanceForFullKeyboard();

Expand Down Expand Up @@ -279,17 +287,7 @@ public void setContentSizeWatcher(ContentSizeWatcher contentSizeWatcher) {
}

public void setMostRecentEventCount(int mostRecentEventCount) {
if (mMostRecentEventCount == mostRecentEventCount) {
return;
}

mMostRecentEventCount = mostRecentEventCount;

if (mStateWrapper != null) {
WritableMap map = new WritableNativeMap();
map.putInt("mostRecentEventCount", mMostRecentEventCount);
mStateWrapper.updateState(map);
}
}

public void setScrollWatcher(ScrollWatcher scrollWatcher) {
Expand Down Expand Up @@ -453,6 +451,18 @@ public int incrementAndGetEventCounter() {
return ++mNativeEventCount;
}

public void maybeSetTextFromJS(ReactTextUpdate reactTextUpdate) {
mIsSettingTextFromJS = true;
maybeSetText(reactTextUpdate);
mIsSettingTextFromJS = false;
}

public void maybeSetTextFromState(ReactTextUpdate reactTextUpdate) {
mIsSettingTextFromState = true;
maybeSetText(reactTextUpdate);
mIsSettingTextFromState = false;
}

// VisibleForTesting from {@link TextInputEventsTestCase}.
public void maybeSetText(ReactTextUpdate reactTextUpdate) {
if (isSecureText() && TextUtils.equals(getText(), reactTextUpdate.getText())) {
Expand All @@ -465,6 +475,10 @@ public void maybeSetText(ReactTextUpdate reactTextUpdate) {
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
Expand All @@ -473,17 +487,24 @@ public void maybeSetText(ReactTextUpdate reactTextUpdate) {
new SpannableStringBuilder(reactTextUpdate.getText());
manageSpans(spannableStringBuilder);
mContainsImages = reactTextUpdate.containsImages();
mIsSettingTextFromJS = true;

// When we update text, we trigger onChangeText code that will
// try to update state if the wrapper is available. Temporarily disable
// to prevent an (asynchronous) infinite loop.
mDisableTextDiffing = true;

// On some devices, when the text is cleared, buggy keyboards will not clear the composing
// text so, we have to set text to null, which will clear the currently composing text.
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.
getText().replace(0, length(), spannableStringBuilder);
}
mDisableTextDiffing = false;

mIsSettingTextFromJS = false;
if (Build.VERSION.SDK_INT >= Build.VERSION_CODES.M) {
if (getBreakStrategy() != reactTextUpdate.getTextBreakStrategy()) {
setBreakStrategy(reactTextUpdate.getTextBreakStrategy());
Expand Down
106 changes: 102 additions & 4 deletions ReactAndroid/src/main/java/com/facebook/react/views/textinput/ReactTextInputManager.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -28,11 +28,17 @@
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.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;
import com.facebook.react.uimanager.BaseViewManager;
Expand Down Expand Up @@ -225,7 +231,8 @@ public void receiveCommand(

// TODO: construct a ReactTextUpdate and use that with maybeSetText
// instead of calling setText, etc directly - doing that will definitely cause bugs.
reactEditText.maybeSetText(getReactTextUpdate(text, mostRecentEventCount, start, end));
reactEditText.maybeSetTextFromJS(
getReactTextUpdate(text, mostRecentEventCount, start, end));
}
break;
}
Expand Down Expand Up @@ -257,7 +264,7 @@ public void updateExtraData(ReactEditText view, Object extraData) {
Spannable spannable = update.getText();
TextInlineImageSpan.possiblyUpdateInlineImageSpans(spannable, view);
}
view.maybeSetText(update);
view.maybeSetTextFromState(update);
if (update.getSelectionStart() != UNSET && update.getSelectionEnd() != UNSET)
view.setSelection(update.getSelectionStart(), update.getSelectionEnd());
}
Expand Down Expand Up @@ -842,6 +849,10 @@ public void beforeTextChanged(CharSequence s, int start, int count, int after) {

@Override
public void onTextChanged(CharSequence s, int start, int before, int count) {
if (mEditText.mDisableTextDiffing) {
return;
}

// Rearranging the text (i.e. changing between singleline and multiline attributes) can
// also trigger onTextChanged, call the event in JS only when the text actually changed
if (count == 0 && before == 0) {
Expand All @@ -856,6 +867,92 @@ public void onTextChanged(CharSequence s, int start, int before, int count) {
return;
}

// Fabric: update representation of AttributedString
JavaOnlyMap attributedString = mEditText.mAttributedString;
if (attributedString != null && attributedString.hasKey("fragments")) {
String changedText = s.subSequence(start, start + count).toString();

String completeStr = attributedString.getString("string");
String newCompleteStr =
completeStr.substring(0, start)
+ changedText
+ (completeStr.length() > 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.mStateWrapper != null && attributedString != null) {
WritableMap map = new WritableNativeMap();
WritableMap newAttributedString = new WritableNativeMap();

WritableArray fragments = new WritableNativeArray();

for (int i = 0; i < attributedString.getArray("fragments").size(); i++) {
ReadableMap readableFragment = attributedString.getArray("fragments").getMap(i);
WritableMap fragment = new WritableNativeMap();
fragment.putDouble("reactTag", readableFragment.getInt("reactTag"));
fragment.putString("string", readableFragment.getString("string"));
fragments.pushMap(fragment);
}

newAttributedString.putString("string", attributedString.getString("string"));
newAttributedString.putArray("fragments", fragments);

map.putInt("mostRecentEventCount", mEditText.incrementAndGetEventCounter());
map.putMap("textChanged", newAttributedString);

mEditText.mStateWrapper.updateState(map);
}

// The event that contains the event counter and updates it must be sent first.
// TODO: t7936714 merge these events
mEventDispatcher.dispatchEvent(
Expand Down Expand Up @@ -1116,12 +1213,13 @@ public Object updateState(

view.mStateWrapper = stateWrapper;

return new ReactTextUpdate(
return ReactTextUpdate.buildReactTextUpdateFromState(
spanned,
state.getInt("mostRecentEventCount"),
false, // TODO add this into local Data
textViewProps.getTextAlign(),
textBreakStrategy,
justificationMode);
justificationMode,
attributedString);
}
}

0 comments on commit 0bae474

Please sign in to comment.