From dfc64d5bcc50c25bab40ba853af9f7b0c1c46d7a Mon Sep 17 00:00:00 2001 From: Janic Duplessis Date: Wed, 24 May 2023 15:24:16 -0700 Subject: [PATCH] Fix copy / paste menu and simplify controlled text selection on Android (#37424) Summary: Currently when using a TextInput with a controlled selection prop the Copy / Paste menu is constantly getting dismissed and is impossible to use. This is because Android dismisses it when certain method that affect the input text are called (https://cs.android.com/android/platform/superproject/+/refs/heads/master:frameworks/base/core/java/android/widget/Editor.java;l=1667;drc=7346c436e5a11ce08f6a80dcfeb8ef941ca30176?q=Editor, https://cs.android.com/android/platform/superproject/+/refs/heads/master:frameworks/base/core/java/android/widget/TextView.java;l=6792;drc=7346c436e5a11ce08f6a80dcfeb8ef941ca30176). The solution to fix this is to avoid calling those methods when only the selection changes. I also noticed there are a lot of differences on how selection is handled in old vs new arch and a lot of the selection handling can actually be removed as it is partially the cause of this issue. This implements 2 mitigations to avoid the issue: - Unify selection handling via commands for old arch, like fabric. Selection is currently a prop in the native component, but it is completely ignored in fabric and selection is set using commands. I removed the selection prop from the native component on Android so now it is exclusively handled with commands like it is currently for fabric. This makes it so that when the selection prop changes the native component no longer re-renders which helps mitigate this issue. More specifically for the old arch we no longer handle the `selection` prop in `ReactTextInputShadowNode`, which used to invalidate the shadow node and cause the text to be replaced and the copy / paste menu to close. - Only set placeholder if the text value changed. Calling `EditText.setHint` also causes the copy / paste menu to be dismissed. Fabric will call all props handlers when a single prop changed, so if the `selection` prop changed the `placeholder` prop handler would be called too. To fix this we can check that the value changed before calling `setHint`. ## Changelog: [ANDROID] [FIXED] - Fix copy / paste menu and simplify controlled text selection on Android Pull Request resolved: https://github.com/facebook/react-native/pull/37424 Test Plan: Tested on new and old arch in RNTester example. Before: https://github.com/facebook/react-native/assets/2677334/a915b62a-dd79-4adb-9d95-2317780431cf After: https://github.com/facebook/react-native/assets/2677334/0dd475ed-8981-410c-8908-f00998dcc425 Reviewed By: cortinico Differential Revision: D45958425 Pulled By: NickGerleman fbshipit-source-id: 7b90c1270274f6621303efa60b5398b1a49276ca # Conflicts: # packages/react-native/ReactAndroid/src/main/java/com/facebook/react/views/textinput/ReactTextInputShadowNode.java --- .../AndroidTextInputNativeComponent.js | 1 - .../Components/TextInput/TextInput.js | 25 ++-------- .../react/views/text/ReactTextUpdate.java | 50 ++----------------- .../react/views/textinput/ReactEditText.java | 8 +++ .../textinput/ReactTextInputManager.java | 16 +++--- .../textinput/ReactTextInputShadowNode.java | 32 +----------- .../AndroidTextInputProps.cpp | 4 -- .../androidtextinput/AndroidTextInputProps.h | 34 ------------- 8 files changed, 24 insertions(+), 146 deletions(-) diff --git a/packages/react-native/Libraries/Components/TextInput/AndroidTextInputNativeComponent.js b/packages/react-native/Libraries/Components/TextInput/AndroidTextInputNativeComponent.js index e7c28912a07..55b770d26a3 100644 --- a/packages/react-native/Libraries/Components/TextInput/AndroidTextInputNativeComponent.js +++ b/packages/react-native/Libraries/Components/TextInput/AndroidTextInputNativeComponent.js @@ -692,7 +692,6 @@ export const __INTERNAL_VIEW_CONFIG: PartialViewConfig = { fontStyle: true, textShadowOffset: true, selectionColor: {process: require('../../StyleSheet/processColor').default}, - selection: true, placeholderTextColor: { process: require('../../StyleSheet/processColor').default, }, diff --git a/packages/react-native/Libraries/Components/TextInput/TextInput.js b/packages/react-native/Libraries/Components/TextInput/TextInput.js index dce846b5936..df890974ece 100644 --- a/packages/react-native/Libraries/Components/TextInput/TextInput.js +++ b/packages/react-native/Libraries/Components/TextInput/TextInput.js @@ -1066,27 +1066,19 @@ function InternalTextInput(props: Props): React.Node { accessibilityState, id, tabIndex, + selection: propsSelection, ...otherProps } = props; const inputRef = useRef>>(null); - // Android sends a "onTextChanged" event followed by a "onSelectionChanged" event, for - // the same "most recent event count". - // For controlled selection, that means that immediately after text is updated, - // a controlled component will pass in the *previous* selection, even if the controlled - // component didn't mean to modify the selection at all. - // Therefore, we ignore selections and pass them through until the selection event has - // been sent. - // Note that this mitigation is NOT needed for Fabric. - // discovered when upgrading react-hooks // eslint-disable-next-line react-hooks/exhaustive-deps - let selection: ?Selection = - props.selection == null + const selection: ?Selection = + propsSelection == null ? null : { - start: props.selection.start, - end: props.selection.end ?? props.selection.start, + start: propsSelection.start, + end: propsSelection.end ?? propsSelection.start, }; const [mostRecentEventCount, setMostRecentEventCount] = useState(0); @@ -1098,12 +1090,6 @@ function InternalTextInput(props: Props): React.Node { |}>({selection, mostRecentEventCount}); const lastNativeSelection = lastNativeSelectionState.selection; - const lastNativeSelectionEventCount = - lastNativeSelectionState.mostRecentEventCount; - - if (lastNativeSelectionEventCount < mostRecentEventCount) { - selection = null; - } let viewCommands; if (AndroidTextInputCommands) { @@ -1503,7 +1489,6 @@ function InternalTextInput(props: Props): React.Node { onScroll={_onScroll} onSelectionChange={_onSelectionChange} placeholder={placeholder} - selection={selection} style={style} text={text} textBreakStrategy={props.textBreakStrategy} diff --git a/packages/react-native/ReactAndroid/src/main/java/com/facebook/react/views/text/ReactTextUpdate.java b/packages/react-native/ReactAndroid/src/main/java/com/facebook/react/views/text/ReactTextUpdate.java index 82c9f5d75ac..0802ec3622a 100644 --- a/packages/react-native/ReactAndroid/src/main/java/com/facebook/react/views/text/ReactTextUpdate.java +++ b/packages/react-native/ReactAndroid/src/main/java/com/facebook/react/views/text/ReactTextUpdate.java @@ -27,8 +27,6 @@ public class ReactTextUpdate { private final float mPaddingBottom; private final int mTextAlign; private final int mTextBreakStrategy; - private final int mSelectionStart; - private final int mSelectionEnd; private final int mJustificationMode; /** @@ -55,35 +53,7 @@ public class ReactTextUpdate { paddingBottom, textAlign, Layout.BREAK_STRATEGY_HIGH_QUALITY, - Layout.JUSTIFICATION_MODE_NONE, - -1, - -1); - } - - public ReactTextUpdate( - Spannable text, - int jsEventCounter, - boolean containsImages, - float paddingStart, - float paddingTop, - float paddingEnd, - float paddingBottom, - int textAlign, - int textBreakStrategy, - int justificationMode) { - this( - text, - jsEventCounter, - containsImages, - paddingStart, - paddingTop, - paddingEnd, - paddingBottom, - textAlign, - textBreakStrategy, - justificationMode, - -1, - -1); + Layout.JUSTIFICATION_MODE_NONE); } public ReactTextUpdate( @@ -103,9 +73,7 @@ public class ReactTextUpdate { UNSET, textAlign, textBreakStrategy, - justificationMode, - -1, - -1); + justificationMode); } public ReactTextUpdate( @@ -118,9 +86,7 @@ public class ReactTextUpdate { float paddingBottom, int textAlign, int textBreakStrategy, - int justificationMode, - int selectionStart, - int selectionEnd) { + int justificationMode) { mText = text; mJsEventCounter = jsEventCounter; mContainsImages = containsImages; @@ -130,8 +96,6 @@ public class ReactTextUpdate { mPaddingBottom = paddingBottom; mTextAlign = textAlign; mTextBreakStrategy = textBreakStrategy; - mSelectionStart = selectionStart; - mSelectionEnd = selectionEnd; mJustificationMode = justificationMode; } @@ -187,12 +151,4 @@ public class ReactTextUpdate { public int getJustificationMode() { return mJustificationMode; } - - public int getSelectionStart() { - return mSelectionStart; - } - - public int getSelectionEnd() { - return mSelectionEnd; - } } diff --git a/packages/react-native/ReactAndroid/src/main/java/com/facebook/react/views/textinput/ReactEditText.java b/packages/react-native/ReactAndroid/src/main/java/com/facebook/react/views/textinput/ReactEditText.java index 290bd23f2e0..1b5e0f4ed35 100644 --- a/packages/react-native/ReactAndroid/src/main/java/com/facebook/react/views/textinput/ReactEditText.java +++ b/packages/react-native/ReactAndroid/src/main/java/com/facebook/react/views/textinput/ReactEditText.java @@ -116,6 +116,7 @@ public class ReactEditText extends AppCompatEditText private int mFontStyle = UNSET; private boolean mAutoFocus = false; private boolean mDidAttachToWindow = false; + private @Nullable String mPlaceholder = null; private ReactViewBackgroundManager mReactBackgroundManager; @@ -496,6 +497,13 @@ public class ReactEditText extends AppCompatEditText setKeyListener(mKeyListener); } + public void setPlaceholder(@Nullable String placeholder) { + if (!Objects.equals(placeholder, mPlaceholder)) { + mPlaceholder = placeholder; + setHint(placeholder); + } + } + public void setFontFamily(String fontFamily) { mFontFamily = fontFamily; mTypefaceDirty = true; diff --git a/packages/react-native/ReactAndroid/src/main/java/com/facebook/react/views/textinput/ReactTextInputManager.java b/packages/react-native/ReactAndroid/src/main/java/com/facebook/react/views/textinput/ReactTextInputManager.java index 4c9ba17785b..b27ace40cc9 100644 --- a/packages/react-native/ReactAndroid/src/main/java/com/facebook/react/views/textinput/ReactTextInputManager.java +++ b/packages/react-native/ReactAndroid/src/main/java/com/facebook/react/views/textinput/ReactTextInputManager.java @@ -328,21 +328,19 @@ public class ReactTextInputManager extends BaseViewManager text.length()) { - mSelectionStart = text.length(); - } - if (mSelectionEnd > text.length()) { - mSelectionEnd = text.length(); - } - } else { - mSelectionStart = UNSET; - mSelectionEnd = UNSET; - } markUpdated(); } @@ -194,18 +178,6 @@ public class ReactTextInputShadowNode extends ReactBaseTextShadowNode return mPlaceholder; } - @ReactProp(name = PROP_SELECTION) - public void setSelection(@Nullable ReadableMap selection) { - mSelectionStart = mSelectionEnd = UNSET; - if (selection == null) return; - - if (selection.hasKey("start") && selection.hasKey("end")) { - mSelectionStart = selection.getInt("start"); - mSelectionEnd = selection.getInt("end"); - markUpdated(); - } - } - @Override public void setTextBreakStrategy(@Nullable String textBreakStrategy) { if (Build.VERSION.SDK_INT < Build.VERSION_CODES.M) { @@ -245,9 +217,7 @@ public class ReactTextInputShadowNode extends ReactBaseTextShadowNode getPadding(Spacing.BOTTOM), mTextAlign, mTextBreakStrategy, - mJustificationMode, - mSelectionStart, - mSelectionEnd); + mJustificationMode); uiViewOperationQueue.enqueueUpdateExtraData(getReactTag(), reactTextUpdate); } } diff --git a/packages/react-native/ReactCommon/react/renderer/components/textinput/androidtextinput/react/renderer/components/androidtextinput/AndroidTextInputProps.cpp b/packages/react-native/ReactCommon/react/renderer/components/textinput/androidtextinput/react/renderer/components/androidtextinput/AndroidTextInputProps.cpp index 19cb290a332..9953e2228ed 100644 --- a/packages/react-native/ReactCommon/react/renderer/components/textinput/androidtextinput/react/renderer/components/androidtextinput/AndroidTextInputProps.cpp +++ b/packages/react-native/ReactCommon/react/renderer/components/textinput/androidtextinput/react/renderer/components/androidtextinput/AndroidTextInputProps.cpp @@ -134,8 +134,6 @@ AndroidTextInputProps::AndroidTextInputProps( "selectionColor", sourceProps.selectionColor, {})), - selection(CoreFeatures::enablePropIteratorSetter? sourceProps.selection : - convertRawProp(context, rawProps, "selection", sourceProps.selection, {})), value(CoreFeatures::enablePropIteratorSetter? sourceProps.value : convertRawProp(context, rawProps, "value", sourceProps.value, {})), defaultValue(CoreFeatures::enablePropIteratorSetter? sourceProps.defaultValue : convertRawProp(context, rawProps, "defaultValue", @@ -349,7 +347,6 @@ void AndroidTextInputProps::setProp( RAW_SET_PROP_SWITCH_CASE_BASIC(placeholderTextColor); RAW_SET_PROP_SWITCH_CASE_BASIC(secureTextEntry); RAW_SET_PROP_SWITCH_CASE_BASIC(selectionColor); - RAW_SET_PROP_SWITCH_CASE_BASIC(selection); RAW_SET_PROP_SWITCH_CASE_BASIC(defaultValue); RAW_SET_PROP_SWITCH_CASE_BASIC(selectTextOnFocus); RAW_SET_PROP_SWITCH_CASE_BASIC(submitBehavior); @@ -449,7 +446,6 @@ folly::dynamic AndroidTextInputProps::getDynamic() const { props["placeholderTextColor"] = toAndroidRepr(placeholderTextColor); props["secureTextEntry"] = secureTextEntry; props["selectionColor"] = toAndroidRepr(selectionColor); - props["selection"] = toDynamic(selection); props["value"] = value; props["defaultValue"] = defaultValue; props["selectTextOnFocus"] = selectTextOnFocus; diff --git a/packages/react-native/ReactCommon/react/renderer/components/textinput/androidtextinput/react/renderer/components/androidtextinput/AndroidTextInputProps.h b/packages/react-native/ReactCommon/react/renderer/components/textinput/androidtextinput/react/renderer/components/androidtextinput/AndroidTextInputProps.h index 7652984ce15..ba39ebb7623 100644 --- a/packages/react-native/ReactCommon/react/renderer/components/textinput/androidtextinput/react/renderer/components/androidtextinput/AndroidTextInputProps.h +++ b/packages/react-native/ReactCommon/react/renderer/components/textinput/androidtextinput/react/renderer/components/androidtextinput/AndroidTextInputProps.h @@ -27,32 +27,6 @@ namespace facebook { namespace react { -struct AndroidTextInputSelectionStruct { - int start; - int end; -}; - -static inline void fromRawValue( - const PropsParserContext &context, - const RawValue &value, - AndroidTextInputSelectionStruct &result) { - auto map = (butter::map)value; - - auto start = map.find("start"); - if (start != map.end()) { - fromRawValue(context, start->second, result.start); - } - auto end = map.find("end"); - if (end != map.end()) { - fromRawValue(context, end->second, result.end); - } -} - -static inline std::string toString( - const AndroidTextInputSelectionStruct &value) { - return "[Object AndroidTextInputSelectionStruct]"; -} - struct AndroidTextInputTextShadowOffsetStruct { double width; double height; @@ -87,13 +61,6 @@ inline folly::dynamic toDynamic( dynamicValue["height"] = value.height; return dynamicValue; } - -inline folly::dynamic toDynamic(const AndroidTextInputSelectionStruct &value) { - folly::dynamic dynamicValue = folly::dynamic::object(); - dynamicValue["start"] = value.start; - dynamicValue["end"] = value.end; - return dynamicValue; -} #endif class AndroidTextInputProps final : public ViewProps, public BaseTextProps { @@ -138,7 +105,6 @@ class AndroidTextInputProps final : public ViewProps, public BaseTextProps { SharedColor placeholderTextColor{}; bool secureTextEntry{false}; SharedColor selectionColor{}; - AndroidTextInputSelectionStruct selection{}; std::string value{}; std::string defaultValue{}; bool selectTextOnFocus{false};