Skip to content

Commit d4f6cf1

Browse files
janicduplessisfacebook-github-bot
authored andcommitted
Fix copy / paste menu and simplify controlled text selection on Android (facebook#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: facebook#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
1 parent a449291 commit d4f6cf1

File tree

8 files changed

+24
-146
lines changed

8 files changed

+24
-146
lines changed

packages/react-native/Libraries/Components/TextInput/AndroidTextInputNativeComponent.js

Lines changed: 0 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -692,7 +692,6 @@ export const __INTERNAL_VIEW_CONFIG: PartialViewConfig = {
692692
fontStyle: true,
693693
textShadowOffset: true,
694694
selectionColor: {process: require('../../StyleSheet/processColor').default},
695-
selection: true,
696695
placeholderTextColor: {
697696
process: require('../../StyleSheet/processColor').default,
698697
},

packages/react-native/Libraries/Components/TextInput/TextInput.js

Lines changed: 5 additions & 20 deletions
Original file line numberDiff line numberDiff line change
@@ -1066,27 +1066,19 @@ function InternalTextInput(props: Props): React.Node {
10661066
accessibilityState,
10671067
id,
10681068
tabIndex,
1069+
selection: propsSelection,
10691070
...otherProps
10701071
} = props;
10711072

10721073
const inputRef = useRef<null | React.ElementRef<HostComponent<mixed>>>(null);
10731074

1074-
// Android sends a "onTextChanged" event followed by a "onSelectionChanged" event, for
1075-
// the same "most recent event count".
1076-
// For controlled selection, that means that immediately after text is updated,
1077-
// a controlled component will pass in the *previous* selection, even if the controlled
1078-
// component didn't mean to modify the selection at all.
1079-
// Therefore, we ignore selections and pass them through until the selection event has
1080-
// been sent.
1081-
// Note that this mitigation is NOT needed for Fabric.
1082-
// discovered when upgrading react-hooks
10831075
// eslint-disable-next-line react-hooks/exhaustive-deps
1084-
let selection: ?Selection =
1085-
props.selection == null
1076+
const selection: ?Selection =
1077+
propsSelection == null
10861078
? null
10871079
: {
1088-
start: props.selection.start,
1089-
end: props.selection.end ?? props.selection.start,
1080+
start: propsSelection.start,
1081+
end: propsSelection.end ?? propsSelection.start,
10901082
};
10911083

10921084
const [mostRecentEventCount, setMostRecentEventCount] = useState<number>(0);
@@ -1098,12 +1090,6 @@ function InternalTextInput(props: Props): React.Node {
10981090
|}>({selection, mostRecentEventCount});
10991091

11001092
const lastNativeSelection = lastNativeSelectionState.selection;
1101-
const lastNativeSelectionEventCount =
1102-
lastNativeSelectionState.mostRecentEventCount;
1103-
1104-
if (lastNativeSelectionEventCount < mostRecentEventCount) {
1105-
selection = null;
1106-
}
11071093

11081094
let viewCommands;
11091095
if (AndroidTextInputCommands) {
@@ -1503,7 +1489,6 @@ function InternalTextInput(props: Props): React.Node {
15031489
onScroll={_onScroll}
15041490
onSelectionChange={_onSelectionChange}
15051491
placeholder={placeholder}
1506-
selection={selection}
15071492
style={style}
15081493
text={text}
15091494
textBreakStrategy={props.textBreakStrategy}

packages/react-native/ReactAndroid/src/main/java/com/facebook/react/views/text/ReactTextUpdate.java

Lines changed: 3 additions & 47 deletions
Original file line numberDiff line numberDiff line change
@@ -27,8 +27,6 @@ public class ReactTextUpdate {
2727
private final float mPaddingBottom;
2828
private final int mTextAlign;
2929
private final int mTextBreakStrategy;
30-
private final int mSelectionStart;
31-
private final int mSelectionEnd;
3230
private final int mJustificationMode;
3331

3432
/**
@@ -55,35 +53,7 @@ public ReactTextUpdate(
5553
paddingBottom,
5654
textAlign,
5755
Layout.BREAK_STRATEGY_HIGH_QUALITY,
58-
Layout.JUSTIFICATION_MODE_NONE,
59-
-1,
60-
-1);
61-
}
62-
63-
public ReactTextUpdate(
64-
Spannable text,
65-
int jsEventCounter,
66-
boolean containsImages,
67-
float paddingStart,
68-
float paddingTop,
69-
float paddingEnd,
70-
float paddingBottom,
71-
int textAlign,
72-
int textBreakStrategy,
73-
int justificationMode) {
74-
this(
75-
text,
76-
jsEventCounter,
77-
containsImages,
78-
paddingStart,
79-
paddingTop,
80-
paddingEnd,
81-
paddingBottom,
82-
textAlign,
83-
textBreakStrategy,
84-
justificationMode,
85-
-1,
86-
-1);
56+
Layout.JUSTIFICATION_MODE_NONE);
8757
}
8858

8959
public ReactTextUpdate(
@@ -103,9 +73,7 @@ public ReactTextUpdate(
10373
UNSET,
10474
textAlign,
10575
textBreakStrategy,
106-
justificationMode,
107-
-1,
108-
-1);
76+
justificationMode);
10977
}
11078

11179
public ReactTextUpdate(
@@ -118,9 +86,7 @@ public ReactTextUpdate(
11886
float paddingBottom,
11987
int textAlign,
12088
int textBreakStrategy,
121-
int justificationMode,
122-
int selectionStart,
123-
int selectionEnd) {
89+
int justificationMode) {
12490
mText = text;
12591
mJsEventCounter = jsEventCounter;
12692
mContainsImages = containsImages;
@@ -130,8 +96,6 @@ public ReactTextUpdate(
13096
mPaddingBottom = paddingBottom;
13197
mTextAlign = textAlign;
13298
mTextBreakStrategy = textBreakStrategy;
133-
mSelectionStart = selectionStart;
134-
mSelectionEnd = selectionEnd;
13599
mJustificationMode = justificationMode;
136100
}
137101

@@ -187,12 +151,4 @@ public int getTextBreakStrategy() {
187151
public int getJustificationMode() {
188152
return mJustificationMode;
189153
}
190-
191-
public int getSelectionStart() {
192-
return mSelectionStart;
193-
}
194-
195-
public int getSelectionEnd() {
196-
return mSelectionEnd;
197-
}
198154
}

packages/react-native/ReactAndroid/src/main/java/com/facebook/react/views/textinput/ReactEditText.java

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -117,6 +117,7 @@ public class ReactEditText extends AppCompatEditText
117117
private int mFontStyle = UNSET;
118118
private boolean mAutoFocus = false;
119119
private boolean mDidAttachToWindow = false;
120+
private @Nullable String mPlaceholder = null;
120121

121122
private ReactViewBackgroundManager mReactBackgroundManager;
122123

@@ -497,6 +498,13 @@ public void setInputType(int type) {
497498
setKeyListener(mKeyListener);
498499
}
499500

501+
public void setPlaceholder(@Nullable String placeholder) {
502+
if (!Objects.equals(placeholder, mPlaceholder)) {
503+
mPlaceholder = placeholder;
504+
setHint(placeholder);
505+
}
506+
}
507+
500508
public void setFontFamily(String fontFamily) {
501509
mFontFamily = fontFamily;
502510
mTypefaceDirty = true;

packages/react-native/ReactAndroid/src/main/java/com/facebook/react/views/textinput/ReactTextInputManager.java

Lines changed: 7 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -328,21 +328,19 @@ public void receiveCommand(
328328

329329
if (!args.isNull(1)) {
330330
String text = args.getString(1);
331-
reactEditText.maybeSetTextFromJS(
332-
getReactTextUpdate(text, mostRecentEventCount, start, end));
331+
reactEditText.maybeSetTextFromJS(getReactTextUpdate(text, mostRecentEventCount));
333332
}
334333
reactEditText.maybeSetSelection(mostRecentEventCount, start, end);
335334
break;
336335
}
337336
}
338337

339-
private ReactTextUpdate getReactTextUpdate(
340-
String text, int mostRecentEventCount, int start, int end) {
338+
private ReactTextUpdate getReactTextUpdate(String text, int mostRecentEventCount) {
341339
SpannableStringBuilder sb = new SpannableStringBuilder();
342340
sb.append(TextTransform.apply(text, TextTransform.UNSET));
343341

344342
return new ReactTextUpdate(
345-
sb, mostRecentEventCount, false, 0, 0, 0, 0, Gravity.NO_GRAVITY, 0, 0, start, end);
343+
sb, mostRecentEventCount, false, 0, 0, 0, 0, Gravity.NO_GRAVITY, 0, 0);
346344
}
347345

348346
@Override
@@ -373,9 +371,9 @@ public void updateExtraData(ReactEditText view, Object extraData) {
373371

374372
// Ensure that selection is handled correctly on text update
375373
boolean isCurrentSelectionEmpty = view.getSelectionStart() == view.getSelectionEnd();
376-
int selectionStart = update.getSelectionStart();
377-
int selectionEnd = update.getSelectionEnd();
378-
if ((selectionStart == UNSET || selectionEnd == UNSET) && isCurrentSelectionEmpty) {
374+
int selectionStart = UNSET;
375+
int selectionEnd = UNSET;
376+
if (isCurrentSelectionEmpty) {
379377
// if selection is not set by state, shift current selection to ensure constant gap to
380378
// text end
381379
int textLength = view.getText() == null ? 0 : view.getText().length();
@@ -507,7 +505,7 @@ public void setAllowFontScaling(ReactEditText view, boolean allowFontScaling) {
507505

508506
@ReactProp(name = "placeholder")
509507
public void setPlaceholder(ReactEditText view, String placeholder) {
510-
view.setHint(placeholder);
508+
view.setPlaceholder(placeholder);
511509
}
512510

513511
@ReactProp(name = "placeholderTextColor", customType = "Color")

packages/react-native/ReactAndroid/src/main/java/com/facebook/react/views/textinput/ReactTextInputShadowNode.java

Lines changed: 1 addition & 31 deletions
Original file line numberDiff line numberDiff line change
@@ -19,7 +19,6 @@
1919
import com.facebook.common.logging.FLog;
2020
import com.facebook.infer.annotation.Assertions;
2121
import com.facebook.react.R;
22-
import com.facebook.react.bridge.ReadableMap;
2322
import com.facebook.react.common.ReactConstants;
2423
import com.facebook.react.common.annotations.VisibleForTesting;
2524
import com.facebook.react.uimanager.Spacing;
@@ -46,13 +45,10 @@ public class ReactTextInputShadowNode extends ReactBaseTextShadowNode
4645

4746
@VisibleForTesting public static final String PROP_TEXT = "text";
4847
@VisibleForTesting public static final String PROP_PLACEHOLDER = "placeholder";
49-
@VisibleForTesting public static final String PROP_SELECTION = "selection";
5048

5149
// Represents the {@code text} property only, not possible nested content.
5250
private @Nullable String mText = null;
5351
private @Nullable String mPlaceholder = null;
54-
private int mSelectionStart = UNSET;
55-
private int mSelectionEnd = UNSET;
5652

5753
public ReactTextInputShadowNode(
5854
@Nullable ReactTextViewManagerCallback reactTextViewManagerCallback) {
@@ -167,18 +163,6 @@ public void setMostRecentEventCount(int mostRecentEventCount) {
167163
@ReactProp(name = PROP_TEXT)
168164
public void setText(@Nullable String text) {
169165
mText = text;
170-
if (text != null) {
171-
// The selection shouldn't be bigger than the length of the text
172-
if (mSelectionStart > text.length()) {
173-
mSelectionStart = text.length();
174-
}
175-
if (mSelectionEnd > text.length()) {
176-
mSelectionEnd = text.length();
177-
}
178-
} else {
179-
mSelectionStart = UNSET;
180-
mSelectionEnd = UNSET;
181-
}
182166
markUpdated();
183167
}
184168

@@ -196,18 +180,6 @@ public void setPlaceholder(@Nullable String placeholder) {
196180
return mPlaceholder;
197181
}
198182

199-
@ReactProp(name = PROP_SELECTION)
200-
public void setSelection(@Nullable ReadableMap selection) {
201-
mSelectionStart = mSelectionEnd = UNSET;
202-
if (selection == null) return;
203-
204-
if (selection.hasKey("start") && selection.hasKey("end")) {
205-
mSelectionStart = selection.getInt("start");
206-
mSelectionEnd = selection.getInt("end");
207-
markUpdated();
208-
}
209-
}
210-
211183
@Override
212184
public void setTextBreakStrategy(@Nullable String textBreakStrategy) {
213185
if (Build.VERSION.SDK_INT < Build.VERSION_CODES.M) {
@@ -247,9 +219,7 @@ public void onCollectExtraUpdates(UIViewOperationQueue uiViewOperationQueue) {
247219
getPadding(Spacing.BOTTOM),
248220
mTextAlign,
249221
mTextBreakStrategy,
250-
mJustificationMode,
251-
mSelectionStart,
252-
mSelectionEnd);
222+
mJustificationMode);
253223
uiViewOperationQueue.enqueueUpdateExtraData(getReactTag(), reactTextUpdate);
254224
}
255225
}

packages/react-native/ReactCommon/react/renderer/components/textinput/androidtextinput/react/renderer/components/androidtextinput/AndroidTextInputProps.cpp

Lines changed: 0 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -134,8 +134,6 @@ AndroidTextInputProps::AndroidTextInputProps(
134134
"selectionColor",
135135
sourceProps.selectionColor,
136136
{})),
137-
selection(CoreFeatures::enablePropIteratorSetter? sourceProps.selection :
138-
convertRawProp(context, rawProps, "selection", sourceProps.selection, {})),
139137
value(CoreFeatures::enablePropIteratorSetter? sourceProps.value : convertRawProp(context, rawProps, "value", sourceProps.value, {})),
140138
defaultValue(CoreFeatures::enablePropIteratorSetter? sourceProps.defaultValue : convertRawProp(context, rawProps,
141139
"defaultValue",
@@ -349,7 +347,6 @@ void AndroidTextInputProps::setProp(
349347
RAW_SET_PROP_SWITCH_CASE_BASIC(placeholderTextColor);
350348
RAW_SET_PROP_SWITCH_CASE_BASIC(secureTextEntry);
351349
RAW_SET_PROP_SWITCH_CASE_BASIC(selectionColor);
352-
RAW_SET_PROP_SWITCH_CASE_BASIC(selection);
353350
RAW_SET_PROP_SWITCH_CASE_BASIC(defaultValue);
354351
RAW_SET_PROP_SWITCH_CASE_BASIC(selectTextOnFocus);
355352
RAW_SET_PROP_SWITCH_CASE_BASIC(submitBehavior);
@@ -449,7 +446,6 @@ folly::dynamic AndroidTextInputProps::getDynamic() const {
449446
props["placeholderTextColor"] = toAndroidRepr(placeholderTextColor);
450447
props["secureTextEntry"] = secureTextEntry;
451448
props["selectionColor"] = toAndroidRepr(selectionColor);
452-
props["selection"] = toDynamic(selection);
453449
props["value"] = value;
454450
props["defaultValue"] = defaultValue;
455451
props["selectTextOnFocus"] = selectTextOnFocus;

packages/react-native/ReactCommon/react/renderer/components/textinput/androidtextinput/react/renderer/components/androidtextinput/AndroidTextInputProps.h

Lines changed: 0 additions & 34 deletions
Original file line numberDiff line numberDiff line change
@@ -26,32 +26,6 @@
2626

2727
namespace facebook::react {
2828

29-
struct AndroidTextInputSelectionStruct {
30-
int start;
31-
int end;
32-
};
33-
34-
static inline void fromRawValue(
35-
const PropsParserContext &context,
36-
const RawValue &value,
37-
AndroidTextInputSelectionStruct &result) {
38-
auto map = (butter::map<std::string, RawValue>)value;
39-
40-
auto start = map.find("start");
41-
if (start != map.end()) {
42-
fromRawValue(context, start->second, result.start);
43-
}
44-
auto end = map.find("end");
45-
if (end != map.end()) {
46-
fromRawValue(context, end->second, result.end);
47-
}
48-
}
49-
50-
static inline std::string toString(
51-
const AndroidTextInputSelectionStruct &value) {
52-
return "[Object AndroidTextInputSelectionStruct]";
53-
}
54-
5529
struct AndroidTextInputTextShadowOffsetStruct {
5630
double width;
5731
double height;
@@ -86,13 +60,6 @@ inline folly::dynamic toDynamic(
8660
dynamicValue["height"] = value.height;
8761
return dynamicValue;
8862
}
89-
90-
inline folly::dynamic toDynamic(const AndroidTextInputSelectionStruct &value) {
91-
folly::dynamic dynamicValue = folly::dynamic::object();
92-
dynamicValue["start"] = value.start;
93-
dynamicValue["end"] = value.end;
94-
return dynamicValue;
95-
}
9663
#endif
9764

9865
class AndroidTextInputProps final : public ViewProps, public BaseTextProps {
@@ -137,7 +104,6 @@ class AndroidTextInputProps final : public ViewProps, public BaseTextProps {
137104
SharedColor placeholderTextColor{};
138105
bool secureTextEntry{false};
139106
SharedColor selectionColor{};
140-
AndroidTextInputSelectionStruct selection{};
141107
std::string value{};
142108
std::string defaultValue{};
143109
bool selectTextOnFocus{false};

0 commit comments

Comments
 (0)