[iOS] Resubmit #24714 and fixes kvo crash

[zhongwuzw]

Summary

Resubmit #24714.
From the #24714 (comment), seems it would crash sometimes. @JoshuaGross Hi :) I added a method to let UITextField call the cleanup explicitly. Please check again, thanks!

Changelog

[iOS] [Fixed] - Fixes selection of single line text input

Test Plan

Create a single line textInput, then do some selection, onSelectionChange can be called.

[JoshuaGross]

Hi @zhongwuzw, thanks for working on this. Do you think it's possible to do this without KVO, or without KVO on unowned objects? KVO has a lot of pitfalls and this use-case is tricky. Could we, for example, have the RCTUITextField observe itself and call delegate methods when relevant?

[zhongwuzw]

@JoshuaGross Agree! 👍 Here we go! no KVO and override setSelectedTextRange instead.

[JoshuaGross]

Can you squash your commits?

[JoshuaGross]

Hmm, why do we need these pragmas? This seems like a sign that this might not be the right solution.

[zhongwuzw]

Yeah, we need it to suppress warning, warning because we mark setSelectedTextRange as unavailable, please see :

react-native/Libraries/Text/TextInput/RCTBackedTextInputViewProtocol.h

Lines 30 to 36 in 7a7eb11

	// This protocol disallows direct access to `selectedTextRange` property because
	// unwise usage of it can break the `delegate` behavior. So, we always have to
	// explicitly specify should `delegate` be notified about the change or not.
	// If the change was initiated programmatically, we must NOT notify the delegate.
	// If the change was a result of user actions (like typing or touches), we MUST notify the delegate.
	- (void)setSelectedTextRange:(nullable UITextRange *)selectedTextRange NS_UNAVAILABLE;
	- (void)setSelectedTextRange:(nullable UITextRange *)selectedTextRange notifyDelegate:(BOOL)notifyDelegate;

We disallow access setSelectedTextRange directly, but Apple framework would call it, and this is how KVO works :).

[JoshuaGross]

Should we try to call delegates in the same order? In the method below this one, we call the delegate method and then we call super; here, it's reversed.

[zhongwuzw]

🤔 Emm, may not have much difference IMO, but I can consolidate them.

[zhongwuzw]

@JoshuaGross I reverted to this change, we need to call super first, otherwise, onSelectionChange would be called with last args, not latest args.

[JoshuaGross]

Thanks again for doing this! I have some specific concerns but I'm excited to get this in once we know we're doing everything the right way :)

[facebook-github-bot]

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

[JoshuaGross]

Hi @zhongwuzw, I'm testing this PR with the following example and seeing that selection changes are happening, but not really getting useful information about the selection changes that I can tell. Is this what you would expect? Is there a better test-case for this?

/**
 * Copyright 2004-present Facebook. All Rights Reserved.
 *
 * @flow strict-local
 * @format
 */

'use strict';
import React, { Component } from 'react';
import {StyleSheet, Text, View} from 'react-native';
import { TextInput } from 'react-native';

class Playground extends Component {
  constructor(props) {
    super(props);
    this.state = { text: 'Useless Placeholder', selectionChange: null };
  }

  render() {
    return (
      <View style={styles.container}>
        <TextInput
          style={{height: 40, borderColor: 'gray', borderWidth: 1}}
          onChangeText={(text) => this.setState({...this.state, text}) && console.log('onChangeText: ', text)}
          onSelectionChange={(...args) => this.setState({...this.state, selectionChange: args}) && console.log('onSelectionChange: ', args)}
          value={this.state.text}
        />
        <Text>Text: {this.state.text}</Text>
        <Text>Selection: {JSON.stringify((this.state.selectionChange))}</Text>
      </View>
    );
  }
}

const styles = StyleSheet.create({
  container: {
    backgroundColor: 'white',
    padding: 10,
    paddingTop: 30,
  },
});

export default Playground;

[zhongwuzw]

@JoshuaGross I amended the code based yours, please see:

  constructor(props) {
    super(props);
    this.state = { text: 'Useless Placeholder', selectionChange: null };
  }

  onSelectionChange({nativeEvent: {selection}}) {
    this.setState({selection});
  }

  render() {
    return (
      <View style={styles.container}>
        <TextInput
          style={{height: 40, borderColor: 'gray', borderWidth: 1}}
          onChangeText={(text) => this.setState({...this.state, text}) && console.log('onChangeText: ', text)}
          onSelectionChange={this.onSelectionChange.bind(this)}
          value={this.state.text}
        />
        <Text>Text: {this.state.text}</Text>
        <Text>selection = {JSON.stringify(this.state.selection)}</Text>
      </View>
    );
  }
}

[facebook-github-bot]

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

[react-native-bot]

This pull request was successfully merged by @zhongwuzw in c38f167.

^{When will my fix make it into a release? | Upcoming Releases}

[MrXyfir]

Would be nice to update the docs that multiline={true} is no longer required for onSelectionChange.