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

Select text inside TextInput only once from onLayout #47902

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

j-piasecki
Copy link
Collaborator

@j-piasecki j-piasecki commented Nov 22, 2024

Summary:

The behavior used for selectTextOnFocus changed in #45004. The new behavior relies on the field being set as a prop to keep track whether the text should be focused in onLayout or not. This causes a problem when autofocus is not used - when the layout of the text input changes, the text will get selected even if the focus doesn't change.

This also presents itself when the prop is set multiple times. I don't think this can happen in bare React Native, but third-party libraries (such as Reanimated) can cause this path to run:

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

by cloning the node before it's committed. This behavior (cloning in Reanimated) is not ideal and a replacement is in the works, but in the meantime, it's causing a similar problem where the text will be selected on every layout change.

Changelog:

[ANDROID] [FIXED] - Fixed text being selected multiple times when selectTextOnFocus is used on a TextInput

Test Plan:

Code used in the videos
import React, {useRef, useState} from 'react';
import {Button, SafeAreaView, TextInput, View} from 'react-native';

export default function A() {
    const [state, setState] = useState('Hello');
    const [visible, setVisible] = useState(false);
    const input = useRef(null);

    return (
        <SafeAreaView style={{flex: 1, backgroundColor: 'white', paddingTop: 100}}>
            <View style={{flexDirection: 'row'}}>
              <TextInput
                  ref={input}
                  accessibilityLabel="Text input field"
                  value={state}
                  underlineColorAndroid={'red'}
                  onChangeText={(a) => {
                      setState(a);
                  }}
                  selectTextOnFocus
                  style={{flex: 1}}
              />
              {visible && <View style={{width: 50, height: 50, backgroundColor: 'red'}} />}
            </View>
            <Button onPress={() => input.current.focus()} title="Focus" />
            <Button onPress={() => setVisible(!visible)} title="Toggle view" />
        </SafeAreaView>
    );
}

The behavior from the videos doesn't require Reanimated to be reproducible.

Before Before - autofocus
Screen.Recording.2024-11-22.at.10.33.43.mov
Screen.Recording.2024-11-22.at.10.33.22.mov
After After - autofocus
Screen.Recording.2024-11-22.at.10.15.33.mov
Screen.Recording.2024-11-22.at.10.18.29.mov

@facebook-github-bot facebook-github-bot added CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. p: Software Mansion Partner: Software Mansion Partner Shared with Meta Applied via automation to indicate that an Issue or Pull Request has been shared with the team. labels Nov 22, 2024
@facebook-github-bot
Copy link
Contributor

@dmytrorykun has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

@@ -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;
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: please follow existing conventions

Suggested change
private boolean hasSelectedTextOnFocus = false;
private boolean mDidSelectTextOnFocus = false;

// 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.

@@ -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?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. p: Software Mansion Partner: Software Mansion Partner Shared with Meta Applied via automation to indicate that an Issue or Pull Request has been shared with the team.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants