facebook / react-native

A framework for building native applications using React
https://reactnative.dev
MIT License
119.59k stars 24.37k forks source link

TextInput controlled selection broken on both ios and android. #29063

Open Gutyn opened 4 years ago

Gutyn commented 4 years ago

This issue is a continuation of the discussion: https://github.com/facebook/react-native/commit/dff490d140010913d3209a2f3e987914b9c4eee4#commitcomment-39332764 The link to the sample project that demonstrates the issues: https://github.com/Ginger-Labs/Input-bug

Description

Controlled selection seems to be broken on both ios and android, to demonstrate the issues I created a sample project (find the link above).

React Native version:

System: OS: macOS 10.15.4 CPU: (8) x64 Intel(R) Core(TM) i7-7820HQ CPU @ 2.90GHz Memory: 1.55 GB / 16.00 GB Shell: 5.7.1 - /bin/zsh Binaries: Node: 10.14.2 - /usr/local/bin/node Yarn: 1.13.0 - /usr/local/bin/yarn npm: 6.14.5 - ~/.npm-global/bin/npm Watchman: 4.9.0 - /usr/local/bin/watchman SDKs: iOS SDK: Platforms: iOS 13.5, DriverKit 19.0, macOS 10.15, tvOS 13.4, watchOS 6.2 Android SDK: API Levels: 23, 24, 25, 26, 27, 28, 29 Build Tools: 26.0.3, 28.0.3, 29.0.0, 30.0.0 System Images: android-28 | Google APIs Intel x86 Atom, android-28 | Google Play Intel x86 Atom, android-29 | Google APIs Intel x86 Atom, android-29 | Google Play Intel x86 Atom_64 Android NDK: 21.1.6352462 IDEs: Android Studio: 3.6 AI-192.7142.36.36.6392135 Xcode: 11.5/11E608c - /usr/bin/xcodebuild npmPackages: react: ~16.9.0 => 16.9.0 react-native: ~0.62.2 => 0.62.2 npmGlobalPackages: create-react-native-app: 3.4.0 react-native-app-id: 0.0.5 react-native-cli: 2.0.1

Steps To Reproduce

The reproduction steps are in the sample project's ReadMe file. For simplicity purposes, I will post them here as well:

SIM - iPhone 11 (13.4.1): 1) Click on the "Click Me" Button, set the cursor in the middle, add some text, click on the button again: Notice the text is set to needed one but the selection is not 10 2) Input text: "Hello world", move cursor in between words, click on "@": Expected: "Hello @Mihailworld" with the cursor at 13 Actual: "Hello @Mihailworld" with the cursor at the end of the whole string.

SIM - nexus 6P API28 1) Add text, click on enter (new line). Expected: The text stays and a new line is created with "-" in front. Actual: The first line becomes empty and the second line "-". 2) Press enter twice and you will get a: Exception in native call java.lang.IndexOutOfBoundsException: setSpan (6 ... 6) ends beyond length 3 3) Press "@" twice and observe the same bug above.

Expected Results

I expect the TextInput to work as intended (unless I am missing something conceptual).

Snack, code example, screenshot, or link to a repository:

https://github.com/Ginger-Labs/Input-bug

chrisglein commented 4 years ago

The text string is being programmatically replaced. The expectation stated here is that the cursor is stable as a result of how TextInput.selection is being managed. Relevant bits of code here:

function replace(previous, range, chars) {
  const text = previous.slice(0, range.start) + chars + previous.slice(range.end)
  const sel = range.start + chars.length
  return {
    text,
    selection: {start: sel, end: sel}
  }
}
  onSelectionChange = evt => {
    console.log('onSelectionChange: ', evt.nativeEvent)

    let nextSelection = evt.nativeEvent.selection
    if (!this.inputEvent) {
      return this.setState({selection: nextSelection})
    }

    const {previousText, text, range} = this.inputEvent
    let nextText = replace(this.state.text, range, text).text
    delete this.inputEvent

    function replaceInputWith(chars) {
      const result = replace(previousText, range, chars)
      nextText = result.text
      nextSelection = result.selection
    }

    switch (text) {
      case '@': {
        replaceInputWith('@Mihail')
        break
      }

      case '\n': {
        replaceInputWith('\n- ')
        break
      }
    }

    this.setState({
      text: nextText,
      selection: nextSelection
    })
  }
        <TextInput 
          selection={this.state.selection}
          value={this.state.text}
          placeholder={'Say Something'}
          onSelectionChange={this.onSelectionChange}
          onChange={this.onChange}
          onChangeText={this.onChangeText}
          onTextInput={this.onTextInput}
          onKeyPress={this.onKeyPress}
          multiline
          autoFocus
        />
kelset commented 4 years ago

cc @TheSavior I think this is relevant to your commit rewriting TextInput to hooks.

As far as I am aware, there is no available commit on master that would fix it via a cherry pick in a release - but I may be wrong.

fabOnReact commented 4 years ago

My superficial understanding.

The below Java API is called from JavaScript

https://github.com/facebook/react-native/blob/a9971b4ca542037699e5ce2f795c8fdae29a1aba/Libraries/Components/TextInput/TextInput.js#L1011

https://github.com/facebook/react-native/blob/f8bcb1063e3b7ff363203f6b1a19f7da512eaf20/ReactAndroid/src/main/java/com/facebook/react/views/textinput/ReactTextInputManager.java#L213-L214

text can be updated from State, from Javascript, but we also expose separate api to update the Selection using nativeProps

https://github.com/facebook/react-native/blob/f8bcb1063e3b7ff363203f6b1a19f7da512eaf20/ReactAndroid/src/main/java/com/facebook/react/views/textinput/ReactTextInputManager.java#L245

maybeSetText updates the text

https://github.com/facebook/react-native/blob/9263eb5d3864a42925b699343db2c09cc8934ed0/ReactAndroid/src/main/java/com/facebook/react/views/textinput/ReactEditText.java#L473

text is stored in a separate class ReactTextUpdate which includes his own attributes

https://github.com/facebook/react-native/blob/9263eb5d3864a42925b699343db2c09cc8934ed0/ReactAndroid/src/main/java/com/facebook/react/views/textinput/ReactEditText.java#L492

The mText state (the string we receive from Javascript) is stored in instance of class ReactTextUpdate attributes mText. This is where it is first set.

https://github.com/facebook/react-native/blob/9312313c3c6701490d728f912e0b0bbd16d91ad9/ReactAndroid/src/main/java/com/facebook/react/views/textinput/ReactTextInputShadowNode.java#L232-L249

This is the ReactTextUpdate instance constructor

https://github.com/facebook/react-native/blob/f8bcb1063e3b7ff363203f6b1a19f7da512eaf20/ReactAndroid/src/main/java/com/facebook/react/views/text/ReactTextUpdate.java#L115-L128

I did some research on this working on https://github.com/facebook/react-native/pull/29070#issuecomment-650127577, but I ended up giving up as it was relatively complex functionality.

My main problem when working on https://github.com/facebook/react-native/pull/29070 was that length() would be different then actual lenght of the string, causing duplicated letters.

Thanks a lot I wish you a good day Fabrizio Bertoglio

JoshuaGross commented 4 years ago

It looks like there could be some relevant commits missing from the 0.63 branch? Forgive me if I'm wrong, maybe some of these are already included but it doesn't look like it to me.

https://github.com/facebook/react-native/commit/027e8f9b1600c931aa4b826b905a67969733c6ea https://github.com/facebook/react-native/commit/e68f9bf76846dbbc767ec4dbcabc9d83adb5f0dd https://github.com/facebook/react-native/commit/b861782562080bf9c91afcac0f823d96088c2d8d

There were a lot of changes made to TextInput up through those diffs, roughly, so if some of them aren't included in the branch I would expect some TextInput issues.

garrettm commented 4 years ago

Here’s a rough outline of our hack workarounds:

It's pretty involved, has a bunch of hacks, and doesn't work all the time. If I was gonna do it again, I'd probably just fork the native code...

If anyone who is working on fixing this bug in RN core would like to chat about my experience with it or possible solutions, I’d love to, anytime.

fabOnReact commented 4 years ago

on Android onTextInput receives

{ 
  nativeEvent: { 
     previousText: ""
  } 
}

when the cursor is moved between the 2 words like this

Before
Large Text[CURSOR]
After
Large [CURSOR]Text

caused from this line. previousText is calculated here with .substring(start, before), but before has value of 0 so substring(6, 6 + 0) returns empty string.

https://github.com/facebook/react-native/blob/4f897336b6ce6c640a65f86b65c6d386ea55b944/ReactAndroid/src/main/java/com/facebook/react/views/textinput/ReactTextInputManager.java#L920

https://github.com/facebook/react-native/blob/4f897336b6ce6c640a65f86b65c6d386ea55b944/ReactAndroid/src/main/java/com/facebook/react/views/textinput/ReactEditText.java#L258-L266

https://github.com/facebook/react-native/blob/7485e9380799583450088fc41d09323c999de2f3/ReactAndroid/src/main/java/com/facebook/react/views/textinput/ReactTextInputManager.java#L1024

If I add the below text { start: 0, before: 10}

If I move middle of the word { start: 0, before: 10}

If I insert from the previous cursor position the a character { start: 6, before: 0}

onTextInput receives mPreviousText = empty string

https://github.com/facebook/react-native/blob/4f897336b6ce6c640a65f86b65c6d386ea55b944/ReactAndroid/src/main/java/com/facebook/react/views/textinput/ReactTextInputManager.java#L920

https://github.com/facebook/react-native/blob/ee3994530d4f76695d77c9a21a6b57429bac68aa/Libraries/Components/TextInput/TextInput.js#L821-L828

This issue is not very clear to me especially the example attached.. I unsubscribed from this issue and may not follow-up. Sorry

Additionally:

1) I click on Click me button, the state text value is set and onTextInput is not called 2) this.inputEvent is set inside onTextInput callback 3) additional conditions change the execution of the program.. Are this essential to reproduce the bug?

All this conditions are not relevant for reproducing this bug/issue and they are considerable amount of work for the OpenSource contributor that need to delete existing JavaScript logic to reproduce the issue with a minimal reproducible example

Minimal

The more code there is to go through, the less likely people can find your problem. Streamline your example in one of two ways:

Restart from scratch. Create a new program, adding in only what is needed to see the problem. Use simple, descriptive names for functions and variables – don’t copy the names you’re using in your existing code. Divide and conquer. If you’re not sure what the source of the problem is, start removing code a bit at a time until the problem disappears – then add the last part back.

Reproducible Eliminate any issues that aren't relevant to the problem. If your question isn’t about a compiler error, ensure that there are no compile-time errors. Use a program such as JSLint to validate interpreted languages. Validate any HTML or XML.

Sorry, Thanks a lot. Best Regards. Fabrizio Bertoglio

JoshuaGross commented 4 years ago

@garrettm @fabriziobertoglio1987 I would expect a variety of bugs until (at least) the three commits I mentioned above are merged into release.

garrettm commented 4 years ago

Is it expected that these three commits get into 63.2?

todorone commented 4 years ago

@TheSavior @kelset controlled selection is broken on both platforms since the component has been rewritten to hooks. Is there anything can be done to fix it as it's critical for some cases?

kelset commented 4 years ago

I thought the commits mentioned by Joshua were cherry picked in 0.63.2. Please submit a new repro if that still happens with latest. We are also working our way towards a 0.63.3 release which should have more fixes.

focux commented 4 years ago

For the record, the controlled selection is still broken in 0.63.3.

garrettm commented 4 years ago

We're on 0.63.2 and still having issues too

velidan commented 3 years ago

Have the same issue, unfortunately. I'd like to wonder if there is any update or suggestion of how to solve it? Thanks!

fabOnReact commented 3 years ago

I think it requires a big change in both ReactAndroid and ReactNative JS libraries in fact I gave up and search for a new issue to solve https://github.com/facebook/react-native/issues/30393#issuecomment-752676908

fabOnReact commented 3 years ago

Thanks a lot for the bug report. I tested your example with my pr https://github.com/facebook/react-native/pull/29070 and It solves this issue

I would experience the runtime on latest master, but after checking out and building my branch https://github.com/facebook/react-native/pull/29070 of react-native from source (see the below test), the error would not reproduce.

I pushed the branch tested below which includes your example in this commit https://github.com/fabriziobertoglio1987/react-native/commit/0660ac910da342574c2b7e6c9e8f5790f71312ec

Please thumbs up my PR, test, feedback is appreciated. Thanks :peace_symbol: :heart: :pray:

CLICK TO OPEN TESTS RESULTS

| **BEFORE** | **AFTER** | |:-------------------------:|:-------------------------:| | | |

jocoders commented 3 years ago

I have the same bag on Android on react-native 0.63.2:

Fatal Exception: java.lang.IndexOutOfBoundsException: setSpan (8 ... 8) ends beyond length 7
       at android.text.SpannableStringBuilder.checkRange(SpannableStringBuilder.java:1325)
       at android.text.SpannableStringBuilder.setSpan(SpannableStringBuilder.java:684)
       at android.text.SpannableStringBuilder.setSpan(SpannableStringBuilder.java:676)
       at android.text.Selection.setSelection(Selection.java:94)
       at android.text.Selection.setSelection(Selection.java:78)
       at android.widget.EditText.setSelection(EditText.java:129)
       at com.facebook.react.views.textinput.ReactEditText.setSelection(ReactEditText.java:308)
       at com.facebook.react.views.textinput.ReactEditText.maybeSetSelection(ReactEditText.java:302)
       at com.facebook.react.views.textinput.ReactTextInputManager.updateExtraData(ReactTextInputManager.java:291)
       at com.facebook.react.views.textinput.ReactTextInputManager.updateExtraData(ReactTextInputManager.java:84)
       at com.facebook.react.uimanager.NativeViewHierarchyManager.updateViewExtraData(NativeViewHierarchyManager.java:149)
       at com.facebook.react.uimanager.UIViewOperationQueue$UpdateViewExtraData.execute(UIViewOperationQueue.java:240)
       at com.facebook.react.uimanager.UIViewOperationQueue$1.run(UIViewOperationQueue.java:917)
       at com.facebook.react.uimanager.UIViewOperationQueue.flushPendingBatches(UIViewOperationQueue.java:1028)
       at com.facebook.react.uimanager.UIViewOperationQueue.access$2600(UIViewOperationQueue.java:48)
       at com.facebook.react.uimanager.UIViewOperationQueue$DispatchUIFrameCallback.doFrameGuarded(UIViewOperationQueue.java:1088)
       at com.facebook.react.uimanager.GuardedFrameCallback.doFrame(GuardedFrameCallback.java:29)
       at com.facebook.react.modules.core.ReactChoreographer$ReactChoreographerDispatcher.doFrame(ReactChoreographer.java:175)
       at com.facebook.react.modules.core.ChoreographerCompat$FrameCallback$1.doFrame(ChoreographerCompat.java:85)
       at android.view.Choreographer$CallbackRecord.run(Choreographer.java:965)
       at android.view.Choreographer.doCallbacks(Choreographer.java:791)
       at android.view.Choreographer.doFrame(Choreographer.java:722)
       at android.view.Choreographer$FrameDisplayEventReceiver.run(Choreographer.java:952)
       at android.os.Handler.handleCallback(Handler.java:883)
       at android.os.Handler.dispatchMessage(Handler.java:100)
       at android.os.Looper.loop(Looper.java:214)
       at android.app.ActivityThread.main(ActivityThread.java:7386)
       at java.lang.reflect.Method.invoke(Method.java)
       at com.android.internal.os.RuntimeInit$MethodAndArgsCaller.run(RuntimeInit.java:514)
       at com.android.internal.os.ZygoteInit.main(ZygoteInit.java:980)

Any ideas how to solve it?

kyoz commented 3 years ago

I'm working on an app need add customize words & emoji in Input and after a day got headache with selection, this is my solution. @@ Hope it help or inspire you guy to got another better solution

const inputRef = useRef();
const [text, setText] = useState('');
const [selection, setSelection] = useState({start: 0, end: 0});
const [blockSelection, setBlockSelection] = useState(false);

const addCustomText = (customText) => {
  setBlockSelection(true);

  const newText =
      text.substr(0, selection.start) +
      customText +
      text.substr(selection.end);

  setText(newText);

  // Change The Selection
  const newPos = selection.start + customText.length;

  setSelection({start: newPos, end: newPos});

  inputRef.current.setNativeProps({
    selection: {start: newPos, end: newPos},
  });
};

const handleTextChange = text => {
  setText(text);

  inputRef.current.setNativeProps({
    selection: {
      start: selection.start + 1,
      end: selection.start + 1,
    },
  });
};

const handleSelectionChange = position => {
  if (blockSelection) {
    setBlockSelection(false);
    return;
  }

  setSelection(position);
};

  ...
         <TextInput
            ...
            ref={inputRef}
            value={text}
            onChangeText={text => handleTextChange(text)}
            onSelectionChange={e => handleSelectionChange(e.nativeEvent.selection)}
            style={styles.input} />
        </View>
mir1198yusuf commented 3 years ago

inputRef.current.setNativeProps({ selection: { start: selection.start + 1, end: selection.start + 1, }, });

This works for me but setting selection prop from local state did not work while changing state in onSelectionChange.

Above comments is helpful

apfritts commented 3 years ago

I think I have another bug related to text input selection #31375. This doesn't seem directly related since it doesn't look like anyone is changing font style though.

Sanath91009 commented 2 years ago

https://stackoverflow.com/questions/73151416/cursor-not-changing-its-position-in-react-native

If I run the code in android it is working fine but IOS simulator is working differently (working as mentioned in the above question)

update : related to this issue https://github.com/facebook/react-native/issues/28865

ZComwiz commented 1 year ago

We are getting errors on Samsung devices for SpannableText:

[SpannableStringBuilder.java line 1123](https://notifications.googleapis.com/email/...someLongURL)
android.text.SpannableStringBuilder.siftDown

running "react-native": "0.71.1",

The issue appears to effect Samsung Phones more and when users change to using GBoard Keyboard, the error sometimes goes away? Super janky. Its a new issue in Android 13

joemun commented 1 year ago

Saw that the React Native team recently merged in this PR updating controlled selection logic for Android: https://github.com/facebook/react-native/pull/37424

kyoz commented 1 year ago

I'm working on an app need add customize words & emoji in Input and after a day got headache with selection, this is my solution. @@ Hope it help or inspire you guy to got another better solution

const inputRef = useRef();
const [text, setText] = useState('');
const [selection, setSelection] = useState({start: 0, end: 0});
const [blockSelection, setBlockSelection] = useState(false);

const addCustomText = (customText) => {
  setBlockSelection(true);

  const newText =
      text.substr(0, selection.start) +
      customText +
      text.substr(selection.end);

  setText(newText);

  // Change The Selection
  const newPos = selection.start + customText.length;

  setSelection({start: newPos, end: newPos});

  inputRef.current.setNativeProps({
    selection: {start: newPos, end: newPos},
  });
};

const handleTextChange = text => {
  setText(text);

  inputRef.current.setNativeProps({
    selection: {
      start: selection.start + 1,
      end: selection.start + 1,
    },
  });
};

const handleSelectionChange = position => {
  if (blockSelection) {
    setBlockSelection(false);
    return;
  }

  setSelection(position);
};

  ...
         <TextInput
            ...
            ref={inputRef}
            value={text}
            onChangeText={text => handleTextChange(text)}
            onSelectionChange={e => handleSelectionChange(e.nativeEvent.selection)}
            style={styles.input} />
        </View>

This is no longer work in RN 0.72.0. I still looking for solution, tried setSelection but it seem doesn't work

kyoz commented 1 year ago

Since my older solution doesn't work. Cause we can't set selection with setNativeProps anymore in RN 0.72. I just update my apps and find a new solution.

In RN 0.72. You can use value and selection properties to handle selection and input value. Like so:

const [inputValue, setInputValue] = useState('');
const [inputSelection, setInputSelection] = useState({start: 0, end: 0});
const [blockSelection, setBlockSelection] = useState(false);

const addCustomText = (customText) => {
  setBlockSelection(true);

  const newText =
      text.substr(0, selection.start) +
      customText +
      text.substr(selection.end);

  // Change The Selection
  const newPos = selection.start + customText.length;

  setInputValue(newText);
  setInputSelection({start: newPos, end: newPos});
};

const handleTextChange = text => {
  setInputValue(text);
  setInputSelection({
      start: selection.start + 1,
      end: selection.start + 1,
   });
};

const handleSelectionChange = position => {
  if (blockSelection) {
    setBlockSelection(false);
    return;
  }

  setInputSelection(position);
};

  ...
         <TextInput
            ...
            value={inputValue}
            selection={inputSelection}
            onChangeText={text => handleTextChange(text)}
            onSelectionChange={e => handleSelectionChange(e.nativeEvent.selection)}
            ... />
        </View>

But be aware that when using States. It will re-render the whole component. So put your input to isolate component or find your own solutions to keep good performance for the input.

Have tested on Android ans iOS. It's seem the selection properties work fine and accurate.

Hope it help.

fabOnReact commented 10 months ago

Do you still experience this issue?

I have four years of experience maintaining facebook/react-native and I specialize in the Text and TextInput components. I currently have 58 facebook/react-native PRs.

If you still experience this issue, I will prepare a patched release with the fix.

Thanks a lot

fabOnReact commented 10 months ago

This PR is included in the react-native-improved library:

react-native-improved

Set-up

In package.json

 "scripts": {
+  "postinstall": "yarn run react-native-patch"
 }

Then

npm

npm install react-native-improved --save-dev

yarn v1

yarn add react-native-improved --dev