Skip to content

Commit

Permalink
Mimimize EditText Spans 9/9: Remove addSpansForMeasurement() (#36575)
Browse files Browse the repository at this point in the history 
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
  • Loading branch information
@NickGerleman @facebook-github-bot
NickGerleman authored and facebook-github-bot committed Mar 25, 2023
1 parent ef4ae32 commit 92b8981
Show file tree
Hide file tree
Showing 3 changed files with 24 additions and 120 deletions.
6 changes: 1 addition & 5 deletions ...eact-native/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 @@ -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.
Expand Down Expand Up @@ -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;
}

Expand Down
126 changes: 21 additions & 105 deletions ...t-native/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 @@ -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;

/**
Expand All @@ -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;

Expand Down Expand Up @@ -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);
}
}
Expand Down Expand Up @@ -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();
Expand Down Expand Up @@ -639,7 +637,7 @@ public void maybeSetText(ReactTextUpdate reactTextUpdate) {
}

// Update cached spans (in Fabric only).
updateCachedSpannable(false);
updateCachedSpannable();
}

/**
Expand All @@ -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];
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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(
Expand All @@ -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);
}
Expand Down Expand Up @@ -1215,7 +1143,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;
Expand All @@ -1225,12 +1153,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;

Expand Down Expand Up @@ -1273,7 +1195,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);
}
Expand All @@ -1289,11 +1210,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);
}

Expand All @@ -1308,7 +1227,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);
}
Expand All @@ -1322,23 +1241,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);
}
Expand Down
12 changes: 2 additions & 10 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 @@ -1350,9 +1350,6 @@ public Object updateState(
TextLayoutManager.getOrCreateSpannableForText(
view.getContext(), attributedString, mReactTextViewManagerCallback);

boolean containsMultipleFragments =
attributedString.getArray("fragments").toArrayList().size() > 1;

int textBreakStrategy =
TextAttributeProps.getTextBreakStrategy(
paragraphAttributes.getString(ViewProps.TEXT_BREAK_STRATEGY));
Expand All @@ -1365,8 +1362,7 @@ public Object updateState(
TextAttributeProps.getTextAlignment(
props, TextLayoutManager.isRTL(attributedString), view.getGravityHorizontal()),
textBreakStrategy,
TextAttributeProps.getJustificationMode(props, currentJustificationMode),
containsMultipleFragments);
TextAttributeProps.getJustificationMode(props, currentJustificationMode));
}

public Object getReactTextUpdate(ReactEditText view, ReactStylesDiffMap props, MapBuffer state) {
Expand All @@ -1387,9 +1383,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));
Expand All @@ -1402,7 +1395,6 @@ public Object getReactTextUpdate(ReactEditText view, ReactStylesDiffMap props, M
TextAttributeProps.getTextAlignment(
props, TextLayoutManagerMapBuffer.isRTL(attributedString), view.getGravityHorizontal()),
textBreakStrategy,
TextAttributeProps.getJustificationMode(props, currentJustificationMode),
containsMultipleFragments);
TextAttributeProps.getJustificationMode(props, currentJustificationMode));
}
}

0 comments on commit 92b8981

Please sign in to comment.