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

[Android] Fix non selectable Text in FlatList #28952

Closed

Conversation

fabOnReact
Copy link
Contributor

@fabOnReact fabOnReact commented May 22, 2020

Summary

This issue fixes #26264 fixes #27107
Text is not selectable inside a FlatList on Android. The solution is to invalidate the ReactTextView after a change of the selectable prop. If the view is visible, onDraw(android.graphics.Canvas) will be called at some point in the future and make the Text selectable.

Changelog

[Android] [Fixed] - Fix non selectable Text in FlatList

Test Plan

CLICK TO OPEN TESTS RESULTS

The issue was demonstrated in the following snack (more info in issue #26264).

The solution is:

  1. Calling invalidate() from setSelectableText after changing the selectable prop and mSelectableText value. invalidate() triggers the onDraw callback.
  1. calling setTextIsSelectable(mSelectableText); from the onDraw callback

The example below is availabe in RNTester FlatList example. Two options (onPressDisabled and textSelectable) have been added to test the functionality inside a FlatList.

@facebook-github-bot facebook-github-bot added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label May 22, 2020
@react-native-bot react-native-bot added Bug Platform: Android Android applications. labels May 22, 2020
@analysis-bot
Copy link

analysis-bot commented May 22, 2020

Platform Engine Arch Size (bytes) Diff
ios - universal n/a --

Base commit: 8a8a532

@analysis-bot
Copy link

analysis-bot commented May 22, 2020

Platform Engine Arch Size (bytes) Diff
android hermes arm64-v8a 9,189,062 +57
android hermes armeabi-v7a 8,714,683 +45
android hermes x86 9,630,923 +44
android hermes x86_64 9,598,667 +51
android jsc arm64-v8a 10,825,421 +204
android jsc armeabi-v7a 9,742,320 +207
android jsc x86 10,862,701 +214
android jsc x86_64 11,471,939 +203

Base commit: 8a8a532

Copy link

@analysis-bot analysis-bot left a comment

Choose a reason for hiding this comment

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

Code analysis results:

@lunaleaps lunaleaps self-assigned this Jul 29, 2021
@facebook-github-bot
Copy link
Contributor

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

@ShikaSD
Copy link
Contributor

ShikaSD commented Aug 2, 2021

Hey, just noticed this PR in the internal review queue :)

Not sure doing this in each onDraw here is a correct solution (for performance and otherwise).
If the underlying reason for this change was to delay the update, doing it in postOnAnimation()/post() (whatever works) is more correct approach. If it was caused by some other issues (e.g. cleared on view removal), maybe we can use onAttachedToWindow or some other callbacks to re-apply the value?
E.g.:

public void setSelectableText(boolean value) {
  postOnAnimation(new Runnable() {
    @Override
    public void run() {
      setTextSelectable(value)    
    }
  });
}

@fabOnReact fabOnReact marked this pull request as draft August 4, 2021 06:06
Code Review from ShikaSD facebook#28952 (comment)
A similar solution was merged to master for a similar problem with TextInput inside FlatList facebook#28852
@fabOnReact
Copy link
Contributor Author

fabOnReact commented Aug 4, 2021

Thanks a lot ShikaSD for the code review.

after some investigation I notice that

The change was already applied with commit 89340f1 and then reverted later with commit 862136a

https://github.com/facebook/react-native/commits/main?after=363a8fb08cef8326bba53cd3ea250bde02444126+34&branch=main&path%5B%5D=ReactAndroid&path%5B%5D=src&path%5B%5D=main&path%5B%5D=java&path%5B%5D=com&path%5B%5D=facebook&path%5B%5D=react&path%5B%5D=views&path%5B%5D=text&path%5B%5D=ReactTextView.java

The title of the revert Revert the suspicious text input change for now seems to indicate a problem with TextInput and not Text component, I will further analyse in the upcoming days if this change introduces regressions.

@github-actions github-actions bot added Needs: Attention Issues where the author has responded to feedback. and removed Needs: Author Feedback labels Aug 4, 2021
The fix was reverted due to a regression with commit facebook@862136a.
I will investigate this change and verify that no regression are
introduced
@fabOnReact fabOnReact marked this pull request as ready for review August 10, 2021 23:42
@facebook-github-bot
Copy link
Contributor

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

@facebook-github-bot
Copy link
Contributor

@lunaleaps merged this pull request in c360b1d.

@facebook-github-bot facebook-github-bot added the Merged This PR has been merged. label Sep 1, 2021
fabOnReact added a commit to fabOnReact/react-native that referenced this pull request Mar 12, 2022
calling setError does not display an error if the TextInput is a
controlled component.

https://reactnative.dev/docs/textinput#value
>The value to show for the text input. TextInput is a controlled component, which means the native value will be forced to match this value prop if provided. For most uses, this works great, but in some cases this may cause flickering - one common cause is preventing edits by keeping value the same. In addition to setting the same value, either set editable={false}, or set/update maxLength to prevent unwanted edits without flicker.

```javascript
function ErrorExample(): React.Node {
  const [text, setText] = React.useState('');
  const [error, setError] = React.useState(null);
  return (
    <TextInput
      errorMessage={error}
      onChangeText={newText => {
        setText(newText);
        setError(newText === 'error' ? 'this input is invalid' : null);
      }}
      value={text}
    />
  );
}
```

The solution from pr facebook#28952
fixes this issue and forces the update by invalidating the TextInput
instance which triggers onAttachedToWindow()

To be noticed that there is logic to trigger this updates in the
ReactTextInputManager
https://github.com/fabriziobertoglio1987/react-native/blob/60b6c9be8e811241039a6db5dc906a0e88e6ba82/ReactAndroid/src/main/java/com/facebook/react/views/textinput/ReactTextInputManager.java#L1291-L1292

The PR was previously accepted and could be an acceptable solution for
this issue
@Zmwang622
Copy link

Zmwang622 commented Aug 28, 2022

@fabriziobertoglio1987 I'm on version 0.64.3, and Text still isn't selectable in a FlatList. Any ideas? Setting removeClippedSubviews to false works but I don't want to sacrifice performance.

This is my code:

const MyClass = ({data}) => {
  const renderItem = useCallback(
      (item: key) => {
        return (
          <View>
              <Text selectable> {key} </Text>
              <Text selectable> {data[key]} </Text>
          </View>
        )
      }
   ,  [data])
  return <FlatList data={Object.keys(data)} renderItem={renderItem}
} 

@fabOnReact
Copy link
Contributor Author

@Zmwang622
Copy link

@fabriziobertoglio1987 Ah okay got it, thanks for the quick response!

@markedwards
Copy link

I'm using react-native 0.72.4 and still need removeClippedSubviews={false} to enable text selection on Android.

@Code-Victor
Copy link

Please, this issue isn't fixed yet.
I just spent over an hour trying to fix this issue, thank God I found this.
#26264

@LA-Johan
Copy link

Reopened the issue here: #46999

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. Merged This PR has been merged. Needs: Attention Issues where the author has responded to feedback. Platform: Android Android applications.
Projects
None yet
10 participants