fix: textinput onchange event order fix

[shubhamguptadream11]

Summary:

Fixes the first issue mentioned here: #45018

Changelog:

[IOS] [FIXED]

Issue details?
In iOS for the multiline textinput, for the first keypress onSelectionChange fires first, but for later key presses onChangeText/onChange fire first.

What's the expected behaviour?
Ideally onChange should be before onSelectionChange for every keyPress.

Why this is happening?

• We are handling onChange/onSelectionChange events in this file:
  https://github.com/facebook/react-native/blob/52dc7a832666532b457bda736c5fe4095f31f731/packages/react-native/Libraries/Text/TextInput/RCTBackedTextInputDelegateAdapter.mm#L269C2-L278C1

- (void)textViewDidChangeSelection:(__unused UITextView *)textView
{
  if (_lastStringStateWasUpdatedWith && ![_lastStringStateWasUpdatedWith isEqual:_backedTextInputView.attributedText]) {
    [self textViewDidChange:_backedTextInputView];
    _ignoreNextTextInputCall = YES;
  }
  _lastStringStateWasUpdatedWith = _backedTextInputView.attributedText;
  [self textViewProbablyDidChangeSelection];
}

This function is called when we do onKeyPress but since _lastStringStateWasUpdatedWith is nil initially so it didn't goes under if case and calls textViewProbablyDidChangeSelection which internally calls onSelectionChange event through this method textInputDidChangeSelection. After this textInputDidChange is called. Due to this flow order of event is reversed.

Solution
By removing _lastStringStateWasUpdatedWith check from if case we are making sure that this flow of methods become same as per other keypress where events are in correct flow.
So this will trigger textViewDidChange which will trigger onChange event correctly
Now since just after this call we are setting _ignoreNextTextInputCall=YES this will prevent any further calls to textViewDidChange and after this if block is finished we are already calling textViewProbablyDidChangeSelection which will call onSelectionChange, preserving the correct flow.

Also, If we check implementation of this in Fabric here in this file:
https://github.com/facebook/react-native/blob/main/packages/react-native/React/Fabric/Mounting/ComponentViews/TextInput/RCTTextInputComponentView.mm#L392C2-L405C2

We can clearly see that there isn't any check applied for _lastStringStateWasUpdatedWith. Triggering the correct flow.

Test Plan:

Logs coming on first keypress after fix:

onChange
onChangeText
onSelectionChange

[analysis-bot]

Platform	Engine	Arch	Size (bytes)	Diff
android	hermes	arm64-v8a	21,285,123	+701
android	hermes	armeabi-v7a	n/a	--
android	hermes	x86	n/a	--
android	hermes	x86_64	n/a	--
android	jsc	arm64-v8a	24,482,001	+593
android	jsc	armeabi-v7a	n/a	--
android	jsc	x86	n/a	--
android	jsc	x86_64	n/a	--

Base commit: 52dc7a8
Branch: main

[facebook-github-bot]

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

[shubhamguptadream11]

@arushikesarwani94 Any update here?

[robhogan]

Hi @shubhamguptadream11 - this looks to effectively revert #36930, which fixed #36494. Can you check whether this change reintroduces that bug? If so we might need a different approach.

[dmytrorykun]

Hi @shubhamguptadream11 thank you for this PR. I see that it is Old Architecture only. I'd like to point out that the desired behaviour is what we have in text input components in the New Architecture. Could you please validate that your changes are consistent with what we have in the New Architecture?
Also, if you provide a distilled concise explanation of what is wrong with the current event sequence and why, it will help reviewing a great deal.

[shubhamguptadream11]

@robhogan @dmytrorykun Thanks for you suggestion.
I am trying to map flow of Old arch with new arch. Will share my findings here with detailed flow in a while.

[shubhamguptadream11]

I did some debugging around this and trying to map the flow between old and new architecture.

In both the cases textViewDidChangeSelection function is called from this file: RCTTextInputDelegateAdapter.

- (void)textViewDidChangeSelection:(__unused UITextView *)textView
{
  if (_lastStringStateWasUpdatedWith && ![_lastStringStateWasUpdatedWith isEqual:_backedTextInputView.attributedText]) {
    [self textViewDidChange:_backedTextInputView];
    _ignoreNextTextInputCall = YES;
  }
  _lastStringStateWasUpdatedWith = _backedTextInputView.attributedText;
  [self textViewProbablyDidChangeSelection];
}

Now since for the first KeyPress _lastStringStateWasUpdatedWith is nil so if condition failed and textViewProbablyDidChangeSelection is called in both (New and Old).

This will further call: textInputDidChangeSelection
Now textInputDidChangeSelection have different Implementation in Old and New Architecture.

In New Architecture(fileName: RCTTextInputComponentView) we are using 2 flags to control the flow: _comingFromJS, _ignoreNextTextInputCall which is responsible to call textInputDidChange and onSelectionChange event.

- (void)textInputDidChangeSelection
{
  if (_comingFromJS) {
    return;
  }
  const auto &props = static_cast<const TextInputProps &>(*_props);
  if (props.traits.multiline && ![_lastStringStateWasUpdatedWith isEqual:_backedTextInputView.attributedText]) {
    [self textInputDidChange];
    _ignoreNextTextInputCall = YES;
  }

  if (_eventEmitter) {
    static_cast<const TextInputEventEmitter &>(*_eventEmitter).onSelectionChange([self _textInputMetrics]);
  }
}

In Old Architecture(fileName: RCTBaseTextInputView) this is only responsible to call onSelectionChange event, without utilising any flags.

- (void)textInputDidChangeSelection
{
  if (!_onSelectionChange) {
    return;
  }

  RCTTextSelection *selection = self.selection;

  _onSelectionChange(@{
    @"selection" : @{
      @"start" : @(selection.start),
      @"end" : @(selection.end),
    },
  });
}

Note: textInputDidChange is responsible for emitting onChange event.

Possible Solution: We should utilise comingFromJs and _ignoreNextTextInputCall flags to match the flow in Old Arch.

Apart from above mentioned changes there are changes in other functions as well like textInputDidChange

[react-native-bot]

This PR is waiting for author's feedback since 24 days. Please provide the requested feedback or this will be closed in 7 days

[react-native-bot]

This PR is waiting for author's feedback since 24 days. Please provide the requested feedback or this will be closed in 7 days