Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix and simplify controlled text selection on Android #55

Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -699,7 +699,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,
},
Expand Down
25 changes: 5 additions & 20 deletions packages/react-native/Libraries/Components/TextInput/TextInput.js
Original file line number Diff line number Diff line change
Expand Up @@ -1095,27 +1095,19 @@ function InternalTextInput(props: Props): React.Node {
tabIndex,
rows,
numberOfLines,
selection: propsSelection,
...otherProps
} = props;

const inputRef = useRef<null | React.ElementRef<HostComponent<mixed>>>(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<number>(0);
Expand All @@ -1127,12 +1119,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) {
Expand Down Expand Up @@ -1533,7 +1519,6 @@ function InternalTextInput(props: Props): React.Node {
onScroll={_onScroll}
onSelectionChange={_onSelectionChange}
placeholder={placeholder}
selection={selection}
style={style}
text={text}
textBreakStrategy={props.textBreakStrategy}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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;

/**
Expand All @@ -55,35 +53,7 @@ public 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(
Expand All @@ -103,9 +73,7 @@ public ReactTextUpdate(
UNSET,
textAlign,
textBreakStrategy,
justificationMode,
-1,
-1);
justificationMode);
}

public ReactTextUpdate(
Expand All @@ -118,9 +86,7 @@ public ReactTextUpdate(
float paddingBottom,
int textAlign,
int textBreakStrategy,
int justificationMode,
int selectionStart,
int selectionEnd) {
int justificationMode) {
mText = text;
mJsEventCounter = jsEventCounter;
mContainsImages = containsImages;
Expand All @@ -130,8 +96,6 @@ public ReactTextUpdate(
mPaddingBottom = paddingBottom;
mTextAlign = textAlign;
mTextBreakStrategy = textBreakStrategy;
mSelectionStart = selectionStart;
mSelectionEnd = selectionEnd;
mJustificationMode = justificationMode;
}

Expand Down Expand Up @@ -187,12 +151,4 @@ public int getTextBreakStrategy() {
public int getJustificationMode() {
return mJustificationMode;
}

public int getSelectionStart() {
return mSelectionStart;
}

public int getSelectionEnd() {
return mSelectionEnd;
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -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;

Expand Down Expand Up @@ -502,6 +503,13 @@ public void setInputType(int type) {
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;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -330,20 +330,20 @@ public void receiveCommand(
if (!args.isNull(1)) {
String text = args.getString(1);
reactEditText.maybeSetTextFromJS(
getReactTextUpdate(text, mostRecentEventCount, start, end));
getReactTextUpdate(text, mostRecentEventCount));
}
reactEditText.maybeSetSelection(mostRecentEventCount, start, end);
break;
}
}

private ReactTextUpdate getReactTextUpdate(
String text, int mostRecentEventCount, int start, int end) {
String text, int mostRecentEventCount) {
SpannableStringBuilder sb = new SpannableStringBuilder();
sb.append(TextTransform.apply(text, TextTransform.UNSET));

return new ReactTextUpdate(
sb, mostRecentEventCount, false, 0, 0, 0, 0, Gravity.NO_GRAVITY, 0, 0, start, end);
sb, mostRecentEventCount, false, 0, 0, 0, 0, Gravity.NO_GRAVITY, 0, 0);
}

@Override
Expand Down Expand Up @@ -374,9 +374,9 @@ public void updateExtraData(ReactEditText view, Object extraData) {

// Ensure that selection is handled correctly on text update
boolean isCurrentSelectionEmpty = view.getSelectionStart() == view.getSelectionEnd();
int selectionStart = update.getSelectionStart();
int selectionEnd = update.getSelectionEnd();
if ((selectionStart == UNSET || selectionEnd == UNSET) && isCurrentSelectionEmpty) {
int selectionStart = UNSET;
int selectionEnd = UNSET;
if (isCurrentSelectionEmpty) {
// if selection is not set by state, shift current selection to ensure constant gap to
// text end
int textLength = view.getText() == null ? 0 : view.getText().length();
Expand Down Expand Up @@ -508,7 +508,7 @@ public void setAllowFontScaling(ReactEditText view, boolean allowFontScaling) {

@ReactProp(name = "placeholder")
public void setPlaceholder(ReactEditText view, String placeholder) {
view.setHint(placeholder);
view.setPlaceholder(placeholder);
}

@ReactProp(name = "placeholderTextColor", customType = "Color")
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -44,13 +44,10 @@ public class ReactTextInputShadowNode extends ReactBaseTextShadowNode

@VisibleForTesting public static final String PROP_TEXT = "text";
@VisibleForTesting public static final String PROP_PLACEHOLDER = "placeholder";
@VisibleForTesting public static final String PROP_SELECTION = "selection";

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

public ReactTextInputShadowNode(
@Nullable ReactTextViewManagerCallback reactTextViewManagerCallback) {
Expand Down Expand Up @@ -169,18 +166,6 @@ public void setMostRecentEventCount(int mostRecentEventCount) {
@ReactProp(name = PROP_TEXT)
public void setText(@Nullable String text) {
mText = text;
if (text != null) {
// The selection shouldn't be bigger than the length of the text
if (mSelectionStart > text.length()) {
mSelectionStart = text.length();
}
if (mSelectionEnd > text.length()) {
mSelectionEnd = text.length();
}
} else {
mSelectionStart = UNSET;
mSelectionEnd = UNSET;
}
markUpdated();
}

Expand All @@ -198,18 +183,6 @@ public void setPlaceholder(@Nullable String placeholder) {
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) {
Expand Down Expand Up @@ -249,9 +222,7 @@ public void onCollectExtraUpdates(UIViewOperationQueue uiViewOperationQueue) {
getPadding(Spacing.BOTTOM),
mTextAlign,
mTextBreakStrategy,
mJustificationMode,
mSelectionStart,
mSelectionEnd);
mJustificationMode);
uiViewOperationQueue.enqueueUpdateExtraData(getReactTag(), reactTextUpdate);
}
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -138,8 +138,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",
Expand Down Expand Up @@ -361,7 +359,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);
Expand Down Expand Up @@ -462,7 +459,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;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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<std::string, RawValue>)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;
Expand Down Expand Up @@ -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 {
Expand Down Expand Up @@ -139,7 +106,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};
Expand Down