From 1f912b9f31308b0e959ab54ee611f22ae2d3ca59 Mon Sep 17 00:00:00 2001 From: Adam Comella Date: Fri, 18 Jan 2019 11:31:24 -0800 Subject: [PATCH] Android TextInput: Fix updating of style props (#22994) Summary: For certain style props, each time any style prop changed, the previous version of the style would remain. For example, if you passed `"underline"` for `textDecorationLine` on a `TextInput` and then later passed `undefined` for `textDecorationLine`, the underline would remain. We solved this problem before in https://github.com/facebook/react-native/commit/de586bfa186289114974674bc2aece462f40393e. The fix was to use `manageSpans` to remove the old spans we added before adding the new spans. However, that fix hardcoded the list of spans to remove. Every span type that was introduced since that commit is affected by this bug: - CustomLetterSpacingSpan - CustomLineHeightSpan - CustomTextTransformSpan - ShadowStyleSpan - StrikethroughSpan - UnderlineSpan - TextInlineImageSpan The reason this bug was reintroduced is `ReactBaseTextShadowNode` is responsible for adding spans and `ReactEditText` is responsible for removing spans. These classes fell out of sync. This fix attempts a more robust solution. Every span that React Native adds to text now implements the `ReactSpan` interface. `manageSpans` deletes all spans that React Native adds by targeting the ones that implement `ReactSpan`. `ReactBaseTextShadowNode.SetSpanOperation` has been updated so that it's a compiler error to add a span that doesn't implement `ReactSpan`. Pull Request resolved: https://github.com/facebook/react-native/pull/22994 Differential Revision: D13727580 Pulled By: mdvacca fbshipit-source-id: 07b2eb08832efafb6c2806aa3329f0e9466b09fb --- .../views/text/CustomLetterSpacingSpan.java | 2 +- .../views/text/CustomLineHeightSpan.java | 2 +- .../react/views/text/CustomStyleSpan.java | 2 +- .../views/text/CustomTextTransformSpan.java | 2 +- .../views/text/ReactAbsoluteSizeSpan.java | 19 +++++++++++++++++ .../views/text/ReactBackgroundColorSpan.java | 19 +++++++++++++++++ .../views/text/ReactBaseTextShadowNode.java | 21 +++++++------------ .../views/text/ReactForegroundColorSpan.java | 19 +++++++++++++++++ .../facebook/react/views/text/ReactSpan.java | 15 +++++++++++++ .../views/text/ReactStrikethroughSpan.java | 16 ++++++++++++++ .../react/views/text/ReactTagSpan.java | 2 +- .../react/views/text/ReactUnderlineSpan.java | 16 ++++++++++++++ .../react/views/text/ShadowStyleSpan.java | 2 +- .../react/views/text/TextInlineImageSpan.java | 2 +- .../react/views/text/TextLayoutManager.java | 19 +++++++---------- .../react/views/textinput/ReactEditText.java | 13 ++---------- 16 files changed, 127 insertions(+), 44 deletions(-) create mode 100644 ReactAndroid/src/main/java/com/facebook/react/views/text/ReactAbsoluteSizeSpan.java create mode 100644 ReactAndroid/src/main/java/com/facebook/react/views/text/ReactBackgroundColorSpan.java create mode 100644 ReactAndroid/src/main/java/com/facebook/react/views/text/ReactForegroundColorSpan.java create mode 100644 ReactAndroid/src/main/java/com/facebook/react/views/text/ReactSpan.java create mode 100644 ReactAndroid/src/main/java/com/facebook/react/views/text/ReactStrikethroughSpan.java create mode 100644 ReactAndroid/src/main/java/com/facebook/react/views/text/ReactUnderlineSpan.java diff --git a/ReactAndroid/src/main/java/com/facebook/react/views/text/CustomLetterSpacingSpan.java b/ReactAndroid/src/main/java/com/facebook/react/views/text/CustomLetterSpacingSpan.java index cbe2c2e49b5..fe0c5ca6499 100644 --- a/ReactAndroid/src/main/java/com/facebook/react/views/text/CustomLetterSpacingSpan.java +++ b/ReactAndroid/src/main/java/com/facebook/react/views/text/CustomLetterSpacingSpan.java @@ -23,7 +23,7 @@ import com.facebook.infer.annotation.Assertions; * spans affecting font size. */ @TargetApi(Build.VERSION_CODES.LOLLIPOP) -public class CustomLetterSpacingSpan extends MetricAffectingSpan { +public class CustomLetterSpacingSpan extends MetricAffectingSpan implements ReactSpan { private final float mLetterSpacing; diff --git a/ReactAndroid/src/main/java/com/facebook/react/views/text/CustomLineHeightSpan.java b/ReactAndroid/src/main/java/com/facebook/react/views/text/CustomLineHeightSpan.java index 2205a04b673..c866e2e61b6 100644 --- a/ReactAndroid/src/main/java/com/facebook/react/views/text/CustomLineHeightSpan.java +++ b/ReactAndroid/src/main/java/com/facebook/react/views/text/CustomLineHeightSpan.java @@ -14,7 +14,7 @@ import android.text.style.LineHeightSpan; * We use a custom {@link LineHeightSpan}, because `lineSpacingExtra` is broken. Details here: * https://github.com/facebook/react-native/issues/7546 */ -public class CustomLineHeightSpan implements LineHeightSpan { +public class CustomLineHeightSpan implements LineHeightSpan, ReactSpan { private final int mHeight; CustomLineHeightSpan(float height) { diff --git a/ReactAndroid/src/main/java/com/facebook/react/views/text/CustomStyleSpan.java b/ReactAndroid/src/main/java/com/facebook/react/views/text/CustomStyleSpan.java index 9330243de9e..7b5f33cc32b 100644 --- a/ReactAndroid/src/main/java/com/facebook/react/views/text/CustomStyleSpan.java +++ b/ReactAndroid/src/main/java/com/facebook/react/views/text/CustomStyleSpan.java @@ -15,7 +15,7 @@ import android.graphics.Typeface; import android.text.TextPaint; import android.text.style.MetricAffectingSpan; -public class CustomStyleSpan extends MetricAffectingSpan { +public class CustomStyleSpan extends MetricAffectingSpan implements ReactSpan { /** * A {@link MetricAffectingSpan} that allows to change the style of the displayed font. diff --git a/ReactAndroid/src/main/java/com/facebook/react/views/text/CustomTextTransformSpan.java b/ReactAndroid/src/main/java/com/facebook/react/views/text/CustomTextTransformSpan.java index 20cc73cdad9..769ae9e909f 100644 --- a/ReactAndroid/src/main/java/com/facebook/react/views/text/CustomTextTransformSpan.java +++ b/ReactAndroid/src/main/java/com/facebook/react/views/text/CustomTextTransformSpan.java @@ -12,7 +12,7 @@ import android.graphics.Paint; import android.text.style.ReplacementSpan; import java.text.BreakIterator; -public class CustomTextTransformSpan extends ReplacementSpan { +public class CustomTextTransformSpan extends ReplacementSpan implements ReactSpan { /** * A {@link ReplacementSpan} that allows declarative changing of text casing. diff --git a/ReactAndroid/src/main/java/com/facebook/react/views/text/ReactAbsoluteSizeSpan.java b/ReactAndroid/src/main/java/com/facebook/react/views/text/ReactAbsoluteSizeSpan.java new file mode 100644 index 00000000000..5d88e99d9e4 --- /dev/null +++ b/ReactAndroid/src/main/java/com/facebook/react/views/text/ReactAbsoluteSizeSpan.java @@ -0,0 +1,19 @@ +/** + * Copyright (c) Facebook, Inc. and its affiliates. + * + * This source code is licensed under the MIT license found in the + * LICENSE file in the root directory of this source tree. + */ + +package com.facebook.react.views.text; + +import android.text.style.AbsoluteSizeSpan; + +/* + * Wraps {@link AbsoluteSizeSpan} as a {@link ReactSpan}. + */ +public class ReactAbsoluteSizeSpan extends AbsoluteSizeSpan implements ReactSpan { + public ReactAbsoluteSizeSpan(int size) { + super(size); + } +} diff --git a/ReactAndroid/src/main/java/com/facebook/react/views/text/ReactBackgroundColorSpan.java b/ReactAndroid/src/main/java/com/facebook/react/views/text/ReactBackgroundColorSpan.java new file mode 100644 index 00000000000..2472443b936 --- /dev/null +++ b/ReactAndroid/src/main/java/com/facebook/react/views/text/ReactBackgroundColorSpan.java @@ -0,0 +1,19 @@ +/** + * Copyright (c) Facebook, Inc. and its affiliates. + * + * This source code is licensed under the MIT license found in the + * LICENSE file in the root directory of this source tree. + */ + +package com.facebook.react.views.text; + +import android.text.style.BackgroundColorSpan; + +/* + * Wraps {@link BackgroundColorSpan} as a {@link ReactSpan}. + */ +public class ReactBackgroundColorSpan extends BackgroundColorSpan implements ReactSpan { + public ReactBackgroundColorSpan(int color) { + super(color); + } +} diff --git a/ReactAndroid/src/main/java/com/facebook/react/views/text/ReactBaseTextShadowNode.java b/ReactAndroid/src/main/java/com/facebook/react/views/text/ReactBaseTextShadowNode.java index b9f012e2abd..dd1db9681ec 100644 --- a/ReactAndroid/src/main/java/com/facebook/react/views/text/ReactBaseTextShadowNode.java +++ b/ReactAndroid/src/main/java/com/facebook/react/views/text/ReactBaseTextShadowNode.java @@ -12,11 +12,6 @@ import android.os.Build; import android.text.Layout; import android.text.Spannable; import android.text.SpannableStringBuilder; -import android.text.style.AbsoluteSizeSpan; -import android.text.style.BackgroundColorSpan; -import android.text.style.ForegroundColorSpan; -import android.text.style.StrikethroughSpan; -import android.text.style.UnderlineSpan; import android.view.Gravity; import com.facebook.react.bridge.JSApplicationIllegalArgumentException; import com.facebook.react.bridge.ReadableMap; @@ -24,8 +19,6 @@ import com.facebook.react.uimanager.IllegalViewOperationException; import com.facebook.react.uimanager.LayoutShadowNode; import com.facebook.react.uimanager.PixelUtil; import com.facebook.react.uimanager.ReactShadowNode; -import com.facebook.react.uimanager.UIManagerModule; -import com.facebook.react.uimanager.ViewDefaults; import com.facebook.react.uimanager.ViewProps; import com.facebook.react.uimanager.annotations.ReactProp; import com.facebook.yoga.YogaDirection; @@ -60,9 +53,9 @@ public abstract class ReactBaseTextShadowNode extends LayoutShadowNode { private static class SetSpanOperation { protected int start, end; - protected Object what; + protected ReactSpan what; - SetSpanOperation(int start, int end, Object what) { + SetSpanOperation(int start, int end, ReactSpan what) { this.start = start; this.end = end; this.what = what; @@ -122,12 +115,12 @@ public abstract class ReactBaseTextShadowNode extends LayoutShadowNode { int end = sb.length(); if (end >= start) { if (textShadowNode.mIsColorSet) { - ops.add(new SetSpanOperation(start, end, new ForegroundColorSpan(textShadowNode.mColor))); + ops.add(new SetSpanOperation(start, end, new ReactForegroundColorSpan(textShadowNode.mColor))); } if (textShadowNode.mIsBackgroundColorSet) { ops.add( new SetSpanOperation( - start, end, new BackgroundColorSpan(textShadowNode.mBackgroundColor))); + start, end, new ReactBackgroundColorSpan(textShadowNode.mBackgroundColor))); } if (Build.VERSION.SDK_INT >= Build.VERSION_CODES.LOLLIPOP) { float effectiveLetterSpacing = textAttributes.getEffectiveLetterSpacing(); @@ -143,7 +136,7 @@ public abstract class ReactBaseTextShadowNode extends LayoutShadowNode { if (// `getEffectiveFontSize` always returns a value so don't need to check for anything like // `Float.NaN`. parentTextAttributes == null || parentTextAttributes.getEffectiveFontSize() != effectiveFontSize) { - ops.add(new SetSpanOperation(start, end, new AbsoluteSizeSpan(effectiveFontSize))); + ops.add(new SetSpanOperation(start, end, new ReactAbsoluteSizeSpan(effectiveFontSize))); } if (textShadowNode.mFontStyle != UNSET || textShadowNode.mFontWeight != UNSET @@ -159,10 +152,10 @@ public abstract class ReactBaseTextShadowNode extends LayoutShadowNode { textShadowNode.getThemedContext().getAssets()))); } if (textShadowNode.mIsUnderlineTextDecorationSet) { - ops.add(new SetSpanOperation(start, end, new UnderlineSpan())); + ops.add(new SetSpanOperation(start, end, new ReactUnderlineSpan())); } if (textShadowNode.mIsLineThroughTextDecorationSet) { - ops.add(new SetSpanOperation(start, end, new StrikethroughSpan())); + ops.add(new SetSpanOperation(start, end, new ReactStrikethroughSpan())); } if ( ( diff --git a/ReactAndroid/src/main/java/com/facebook/react/views/text/ReactForegroundColorSpan.java b/ReactAndroid/src/main/java/com/facebook/react/views/text/ReactForegroundColorSpan.java new file mode 100644 index 00000000000..44383d2f831 --- /dev/null +++ b/ReactAndroid/src/main/java/com/facebook/react/views/text/ReactForegroundColorSpan.java @@ -0,0 +1,19 @@ +/** + * Copyright (c) Facebook, Inc. and its affiliates. + * + * This source code is licensed under the MIT license found in the + * LICENSE file in the root directory of this source tree. + */ + +package com.facebook.react.views.text; + +import android.text.style.ForegroundColorSpan; + +/* + * Wraps {@link ForegroundColorSpan} as a {@link ReactSpan}. + */ +public class ReactForegroundColorSpan extends ForegroundColorSpan implements ReactSpan { + public ReactForegroundColorSpan(int color) { + super(color); + } +} diff --git a/ReactAndroid/src/main/java/com/facebook/react/views/text/ReactSpan.java b/ReactAndroid/src/main/java/com/facebook/react/views/text/ReactSpan.java new file mode 100644 index 00000000000..dc3787936db --- /dev/null +++ b/ReactAndroid/src/main/java/com/facebook/react/views/text/ReactSpan.java @@ -0,0 +1,15 @@ +/** + * Copyright (c) Facebook, Inc. and its affiliates. + * + * This source code is licensed under the MIT license found in the + * LICENSE file in the root directory of this source tree. + */ + +package com.facebook.react.views.text; + +/* + * Enables us to distinguish between spans that were added by React Native and spans that were + * added by something else. All spans that React Native adds should implement this interface. + */ +public interface ReactSpan { +} diff --git a/ReactAndroid/src/main/java/com/facebook/react/views/text/ReactStrikethroughSpan.java b/ReactAndroid/src/main/java/com/facebook/react/views/text/ReactStrikethroughSpan.java new file mode 100644 index 00000000000..b881f7d2eb2 --- /dev/null +++ b/ReactAndroid/src/main/java/com/facebook/react/views/text/ReactStrikethroughSpan.java @@ -0,0 +1,16 @@ +/** + * Copyright (c) Facebook, Inc. and its affiliates. + * + * This source code is licensed under the MIT license found in the + * LICENSE file in the root directory of this source tree. + */ + +package com.facebook.react.views.text; + +import android.text.style.StrikethroughSpan; + +/* + * Wraps {@link StrikethroughSpan} as a {@link ReactSpan}. + */ +public class ReactStrikethroughSpan extends StrikethroughSpan implements ReactSpan { +} diff --git a/ReactAndroid/src/main/java/com/facebook/react/views/text/ReactTagSpan.java b/ReactAndroid/src/main/java/com/facebook/react/views/text/ReactTagSpan.java index 3ce01845189..48b08aa5005 100644 --- a/ReactAndroid/src/main/java/com/facebook/react/views/text/ReactTagSpan.java +++ b/ReactAndroid/src/main/java/com/facebook/react/views/text/ReactTagSpan.java @@ -11,7 +11,7 @@ package com.facebook.react.views.text; * Instances of this class are used to place reactTag information of nested text react nodes * into spannable text rendered by single {@link TextView} */ -public class ReactTagSpan { +public class ReactTagSpan implements ReactSpan { private final int mReactTag; diff --git a/ReactAndroid/src/main/java/com/facebook/react/views/text/ReactUnderlineSpan.java b/ReactAndroid/src/main/java/com/facebook/react/views/text/ReactUnderlineSpan.java new file mode 100644 index 00000000000..375f7dfcad1 --- /dev/null +++ b/ReactAndroid/src/main/java/com/facebook/react/views/text/ReactUnderlineSpan.java @@ -0,0 +1,16 @@ +/** + * Copyright (c) Facebook, Inc. and its affiliates. + * + * This source code is licensed under the MIT license found in the + * LICENSE file in the root directory of this source tree. + */ + +package com.facebook.react.views.text; + +import android.text.style.UnderlineSpan; + +/* + * Wraps {@link UnderlineSpan} as a {@link ReactSpan}. + */ +public class ReactUnderlineSpan extends UnderlineSpan implements ReactSpan { +} diff --git a/ReactAndroid/src/main/java/com/facebook/react/views/text/ShadowStyleSpan.java b/ReactAndroid/src/main/java/com/facebook/react/views/text/ShadowStyleSpan.java index 4eface3f687..18c8be6ff4d 100644 --- a/ReactAndroid/src/main/java/com/facebook/react/views/text/ShadowStyleSpan.java +++ b/ReactAndroid/src/main/java/com/facebook/react/views/text/ShadowStyleSpan.java @@ -11,7 +11,7 @@ package com.facebook.react.views.text; import android.text.TextPaint; import android.text.style.CharacterStyle; -public class ShadowStyleSpan extends CharacterStyle { +public class ShadowStyleSpan extends CharacterStyle implements ReactSpan { private final float mDx, mDy, mRadius; private final int mColor; diff --git a/ReactAndroid/src/main/java/com/facebook/react/views/text/TextInlineImageSpan.java b/ReactAndroid/src/main/java/com/facebook/react/views/text/TextInlineImageSpan.java index 18a971e77ce..c986d7348c4 100644 --- a/ReactAndroid/src/main/java/com/facebook/react/views/text/TextInlineImageSpan.java +++ b/ReactAndroid/src/main/java/com/facebook/react/views/text/TextInlineImageSpan.java @@ -18,7 +18,7 @@ /** * Base class for inline image spans. */ - public abstract class TextInlineImageSpan extends ReplacementSpan { + public abstract class TextInlineImageSpan extends ReplacementSpan implements ReactSpan { /** * For TextInlineImageSpan we need to update the Span to know that the window is attached and 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 5ff6c26dc4e..1a219f43090 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 @@ -18,11 +18,6 @@ import android.text.SpannableStringBuilder; import android.text.Spanned; import android.text.StaticLayout; import android.text.TextPaint; -import android.text.style.AbsoluteSizeSpan; -import android.text.style.BackgroundColorSpan; -import android.text.style.ForegroundColorSpan; -import android.text.style.StrikethroughSpan; -import android.text.style.UnderlineSpan; import android.util.LruCache; import com.facebook.react.bridge.ReactContext; import com.facebook.react.bridge.ReadableArray; @@ -88,12 +83,12 @@ public class TextLayoutManager { int end = sb.length(); if (end >= start) { if (textAttributes.mIsColorSet) { - ops.add(new SetSpanOperation(start, end, new ForegroundColorSpan(textAttributes.mColor))); + ops.add(new SetSpanOperation(start, end, new ReactForegroundColorSpan(textAttributes.mColor))); } if (textAttributes.mIsBackgroundColorSet) { ops.add( new SetSpanOperation( - start, end, new BackgroundColorSpan(textAttributes.mBackgroundColor))); + start, end, new ReactBackgroundColorSpan(textAttributes.mBackgroundColor))); } if (Build.VERSION.SDK_INT >= Build.VERSION_CODES.LOLLIPOP) { if (!Float.isNaN(textAttributes.mLetterSpacing)) { @@ -105,7 +100,7 @@ public class TextLayoutManager { } ops.add( new SetSpanOperation( - start, end, new AbsoluteSizeSpan(textAttributes.mFontSize))); + start, end, new ReactAbsoluteSizeSpan(textAttributes.mFontSize))); if (textAttributes.mFontStyle != UNSET || textAttributes.mFontWeight != UNSET || textAttributes.mFontFamily != null) { @@ -120,10 +115,10 @@ public class TextLayoutManager { context.getAssets()))); } if (textAttributes.mIsUnderlineTextDecorationSet) { - ops.add(new SetSpanOperation(start, end, new UnderlineSpan())); + ops.add(new SetSpanOperation(start, end, new ReactUnderlineSpan())); } if (textAttributes.mIsLineThroughTextDecorationSet) { - ops.add(new SetSpanOperation(start, end, new StrikethroughSpan())); + ops.add(new SetSpanOperation(start, end, new ReactStrikethroughSpan())); } if (textAttributes.mTextShadowOffsetDx != 0 || textAttributes.mTextShadowOffsetDy != 0) { ops.add( @@ -324,9 +319,9 @@ public class TextLayoutManager { private static class SetSpanOperation { protected int start, end; - protected Object what; + protected ReactSpan what; - SetSpanOperation(int start, int end, Object what) { + SetSpanOperation(int start, int end, ReactSpan what) { this.start = start; this.end = end; this.what = what; 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 df3476801fb..05886aae2b6 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 @@ -20,9 +20,6 @@ import android.text.TextWatcher; import android.text.TextUtils; import android.text.method.KeyListener; import android.text.method.QwertyKeyListener; -import android.text.style.AbsoluteSizeSpan; -import android.text.style.BackgroundColorSpan; -import android.text.style.ForegroundColorSpan; import android.util.TypedValue; import android.view.Gravity; import android.view.KeyEvent; @@ -34,10 +31,8 @@ import android.view.inputmethod.InputMethodManager; import android.widget.EditText; import com.facebook.infer.annotation.Assertions; import com.facebook.react.bridge.ReactContext; -import com.facebook.react.uimanager.PixelUtil; import com.facebook.react.uimanager.UIManagerModule; -import com.facebook.react.views.text.CustomStyleSpan; -import com.facebook.react.views.text.ReactTagSpan; +import com.facebook.react.views.text.ReactSpan; import com.facebook.react.views.text.ReactTextUpdate; import com.facebook.react.views.text.TextAttributes; import com.facebook.react.views.text.TextInlineImageSpan; @@ -402,11 +397,7 @@ public class ReactEditText extends EditText { Object[] spans = getText().getSpans(0, length(), Object.class); for (int spanIdx = 0; spanIdx < spans.length; spanIdx++) { // Remove all styling spans we might have previously set - if (ForegroundColorSpan.class.isInstance(spans[spanIdx]) || - BackgroundColorSpan.class.isInstance(spans[spanIdx]) || - AbsoluteSizeSpan.class.isInstance(spans[spanIdx]) || - CustomStyleSpan.class.isInstance(spans[spanIdx]) || - ReactTagSpan.class.isInstance(spans[spanIdx])) { + if (spans[spanIdx] instanceof ReactSpan) { getText().removeSpan(spans[spanIdx]); }