fix(android): update text natively only if needed (fixes selection menu missing)

[hannojg]

Summary

Problem

There exists a bug where on android when having a controlled TextInput using the selection prop the selection menu would not show up (the first TextInput is uncontrolled and there you can see its working):

Screen.Recording.2023-03-27.at.18.48.27.mov

There has also been an issue, which was closed as stale:

• [Android] Using TextInput with 'selection' prop causes context menu (copy/paste) to not display #33697

The reason for this behaviour is the following:

• The user moves the selection cursor in the native EditText view
• The native onSelectionChange listeners fires, which will call the JS onSelectionChange callback
• The JS onSelectionChange callback will update the react state lastNativeSelectionState
• When the TextInput is uncontrolled the selection that is passed to the native view will stay null
• When the TextInput is controlled however a new object gets set as state for lastNativeSelectionState, causing the component to re-render.
• The re-render causes the props to get applied, which then leads to a maybeSetText call (e.g. updating the text of the native view)
• maybeSetText doesn't check if the text is the same, and updates the texts natively
• This update causes the selection menu to disappear. There is no way to prevent this.

Solution

We don't want to update the text natively if it hasn't changed. There is already a check present in maybeSetText, but it only checks for text changes when the TextInput is a secure one (e.g. a password input).

I propose that we always check if the text has changed, and only update the text natively if it really did.
These changes can be seen in this PR.

With these changes you can see that in the tester app the text selection also works for controlled inputs:

Screen.Recording.2023-03-27.at.19.16.49.mov

Changelog

[ANDROID] [FIXED] - Selection menu missing when TextInput uses controlled selection prop

Test Plan

• I see that there are some references to a test file called TextInputEventsTestCase, however I can't find it anywhere in the code. I would like to run those tests and confirm everything is still working as expected
• Manual testing for the TextInput in the rn tester app

[analysis-bot]

Platform	Engine	Arch	Size (bytes)	Diff
android	hermes	arm64-v8a	8,519,307	-105
android	hermes	armeabi-v7a	7,835,586	-121
android	hermes	x86	8,998,436	-115
android	hermes	x86_64	8,854,608	-107
android	jsc	arm64-v8a	9,140,603	+39
android	jsc	armeabi-v7a	8,332,819	+42
android	jsc	x86	9,194,267	+33
android	jsc	x86_64	9,453,358	+48

Base commit: 04df252
Branch: main

[javache]

Comment from @mdvacca when this logic was first added:

reactTextUpdate is a Spannable and it might contain an HTML format, comparing only the text might create a regression when updating only the format of the content of a ReactEditText.
I believe that comparing text and format for each text update can be too expensive, so I would make this comparison ONLY when the secureTextEntry property is enabled (since I don't believe using html format into a password makes much sense)

[hannojg]

@javache Thx for clarifying that. And yes it's right I missed that we work with spannables here. So this code would introduce issues where the native text wouldn't update.

Example without this change:

Screen.Recording.2023-03-31.at.13.25.26.mov

Example with this change (broken):

Screen.Recording.2023-03-31.at.13.26.37.mov

---

I feel like the best solution to address this issue then is to avoid the re-render / the update of the native view when JS updates the selection state to the same as it is in native already.

[javache]

I feel like the best solution to address this issue then is to avoid the re-render / the update of the native view when JS updates the selection state to the same as it is in native already.

Makes sense. Do you want to try and update maybeSetSelection to bail out early if the selection doesn't change to see if that works?

[hannojg]

Hm, unfortunately I don't think that would help, as maybeSetText would get executed anyway from the call in updateExtraData:

react-native/packages/react-native/ReactAndroid/src/main/java/com/facebook/react/views/textinput/ReactTextInputManager.java

Lines 387 to 388 in de2f01d

	view.maybeSetTextFromState(update);
	view.maybeSetSelection(update.getJsEventCounter(), selectionStart, selectionEnd);

And the text being replaced is the reason for the selection menu to disappear. I am not sure if there is a feasible solution. We just can't really tell if two Spannables are identical from their content or not (we can, but the solution would be too expensive / complex to run on every re-render).

So it feels like the solution to keep the selection menu while manually controlling a TextInput is to use the imperative approach...

[hannojg]

Closed in favour of #37424 as it successfully fixed the issue without introducing regressions!