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.

Sign up for GitHub

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

Select text inside TextInput only once from onLayout #47902

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open

Select text inside TextInput only once from onLayout #47902

Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
d457fb8
Select only once
j-piasecki Nov 22, 2024
c2f3e3a
Rename field
j-piasecki Nov 26, 2024
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
6 changes: 4 additions & 2 deletions ...t-native/ReactAndroid/src/main/java/com/facebook/react/views/textinput/ReactEditText.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -134,6 +134,7 @@ public class ReactEditText extends AppCompatEditText {
private boolean mContextMenuHidden = false;
private boolean mDidAttachToWindow = false;
private boolean mSelectTextOnFocus = false;
private boolean hasSelectedTextOnFocus = false;
private @Nullable String mPlaceholder = null;
private Overflow mOverflow = Overflow.VISIBLE;

Expand Down Expand Up @@ -250,11 +251,11 @@ public boolean isLayoutRequested() {
@Override
protected void onLayout(boolean changed, int left, int top, int right, int bottom) {
onContentSizeChange();
if (mSelectTextOnFocus && isFocused()) {
if (mSelectTextOnFocus && isFocused() && !hasSelectedTextOnFocus) {
// Explicitly call this method to select text when layout is drawn
selectAll();
// Prevent text on being selected for next layout pass
mSelectTextOnFocus = false;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are there other cases in Reanimated where props are reapplied multiple times? There are other places where that will cause problems, e.g. for ScrollView setting the contentOffset prop.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's not strictly Reanimated reapplying props - it's this part of the code:

react-native/packages/react-native/ReactCommon/react/renderer/core/ShadowNode.cpp

Lines 56 to 62 in d86412d

if (!hasBeenMounted && sourceNodeHasRawProps && props) {
auto& castedProps = const_cast<Props&>(*props);
castedProps.rawProps = mergeDynamicProps(
sourceShadowNode.getProps()->rawProps,
props->rawProps,
NullValueStrategy::Override);
return props;

which relies on the previous node being mounted to apply only the props diff. This assumption is broken by Reanimated at the moment, but it's going to be fixed in the future. I think wrong behavior here was caused by the same field being used to store a prop and the view state at the same time.

hasSelectedTextOnFocus = true;
}
}

Expand Down Expand Up @@ -630,6 +631,7 @@ public void maybeUpdateTypeface() {
// VisibleForTesting from {@link TextInputEventsTestCase}.
public void requestFocusFromJS() {
requestFocusInternal();
hasSelectedTextOnFocus = true;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why do we avoid the onLayout focus here?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The original PR added this behavior to fix autofocus and selectTextOnFocus not working when used together. Since the focus logic was added to onLayout it may trigger in different cases also, you can see that in the first video (Before in the table), where the input is focused from JS (with no layout) and then layout change causes the text to be selected even though the focus didn't change.

}

/* package */ void clearFocusFromJS() {
Expand Down
Loading