[FIX] Rich Text loop when autocompleting text on iOS

[dratwas]

Fixes #
Gutenberg PR: WordPress/gutenberg#19240
Fixes: #1696

In this PR I created a local variable for focus state instead of using TextInputState, thanks to that we do not call focus() in onFocus which was a bit weird. I also pass the latest native value to onBlur event to update JS value if that is not correct.

PR submission checklist:

• I have considered adding unit tests where possible.
• I have considered if this change warrants user-facing release notes and have added them to RELEASE-NOTES.txt if necessary.

[mchowning]

Do we have two PRs fixing the same issue? cc: @koke

#1698

[pinarol]

Yes we do @mchowning, we are trying both approaches :)

[pinarol]

I realized that this PR is breaking auto-scroll on iOS, because keyboard-aware-scroll-view trusts TextInputState to find out the currently focused TextInput and then it adjusts the scroll position accordingly.

This is PR is making the global TextInputState not trustable anymore so it can have lots of other side effects as well.

So how about we just add onBlur fix onto @koke’s PR?

[dratwas]

Yeah, right we need the TextInputState. However, I still think we should change the way we use it.

The paint point is that there is no method setCurrentlyFocusedID and we need to call TextInputState.focusTextInput in _onFocus -> _onPress instead.
https://github.com/wordpress-mobile/gutenberg-mobile/blob/develop/react-native-aztec/src/AztecView.js#L144
The TextInputState.focusTextInput sets currentlyFocusedID for us but also calls UIManager.focus which tries to focus our input.
https://github.com/facebook/react-native/blob/master/Libraries/Components/TextInput/TextInputState.js#L52
We do the same for _onBlur. We call TextInputState.blurTextInput which clears the currentlyFocusedID but also tries to blur the input.
https://github.com/facebook/react-native/blob/master/Libraries/Components/TextInput/TextInputState.js#L73

My point is I think there is no reason to call focus in _onFocus callback and blur in _onBlur callback. We should only set the correct ID since we already know that the input is focused/blurred.

Let's imagine this kind of scenario:

• Focus input and write something as we did with autocorrection (with different length)
• Click on "Move up" button - blur() is called
• onSelectionChange() is called because suggestion has a different length than original text - focus() is called
• _onBlur() is called - remember that in the meantime the focus() was called and input is focused and in the current implementation we call blur() in _onBlur()
• _onFocus() is called - like above - the blur() was called in _onBlur() so input is blurred and we call focus() in _onFocus()
• _onBlur() is called - input is focused - we call one more time blur()
  -_onFocus() is called - input is blurred - we call focus()

And the loop starts. It's because we call blur() in _onBlur() and focus() in onFocus() instead of setting the correct currentlyFocusedID w/o focusing/bluring in TextInputState.

I added this function to TextInputState and used it just to check if it will fix these issues and seems like yes.

function setCurrentlyFocusedFieldID(textFieldID: number) {
  if (textFieldID != currentlyFocusedID) {
    currentlyFocusedID = textFieldID;
  }
}

Good news is that there are actually functions blurField and focusField on the master branch of react-native and they do what we need 🙂 https://github.com/facebook/react-native/blob/master/Libraries/Components/TextInput/TextInputState.js#L31-L41

[koke]

Those new methods in react-native are nice and will likely make this more robust, but I think that today's problem lies in here:

onSelectionChange() is called because suggestion has a different length than original text - focus() is called

A change on selection shouldn't trigger a change in focus. This kind of makes sense on the web since selection is global across the whole document, but on native, selection is specific to each view, so that's making things confusing:

• The selection changes in the text view because of the autocorrection
• onSelectionChange() is called on RichText because the selection has changed
• Normally, RichText would propagate that to the store with this.props.onSelectionChange, which is the other kind of selection global to the document. The store then sees the new selection and understands that the selection is back on the text view, which ends up in it requesting focus again.

We still want to receive the first onSelectionChange() to keep the internal state of RichText updated, but we don't want to update the global selection if that's not the currently selected view.

[dratwas]

Make sense. I reverted those changes related to the TextInputState and added registerInput and unregisterInput. I also cherry-picked @koke's changes in gutenberg PR WordPress/gutenberg#19240

[pinarol]

Could you explain a bit how this registerInput/unregisterInput works and how do they contribute to the solution?

[dratwas]

TBH I don't think that they are needed in that solution but I added it because I realized that we don't do that at this moment. They add or remove fieldID to/from Set of inputs in TextInputState
https://github.com/facebook/react-native/blob/master/Libraries/Components/TextInput/TextInputState.js#L85-L95

W/o this the false will be always returned when isTextInput is called with Aztec field ID
https://github.com/facebook/react-native/blob/master/Libraries/Components/TextInput/TextInputState.js#L93-L95

[pinarol]

This sounds really nice but I am a bit afraid about consequences. We don't know who is calling isTextInput and what could be done differently if it turns true this time. So I'd avoid adding it unless we have enough context. I am just trying to be extra cautious since this has been a pretty fragile component so far.

[dratwas]

Ok, I understand your worries so I will remove it then :)

[pinarol]

the new solution looks a lot better but I come across with a keyboard close button issue. It is not working in certain scenarios:

• create a new post
• write something
• tap keyboard close button

• open a post with media text
• type something on text area
• delete the text
• tap delete button once again so the paragraph block is also deleted
• focus on text area again
• tap keyboard close button

[pinarol]

But the issues related to focus loop on auto correct is fixed.

I tested both cases:

text from autocorrection has the same length as written text:

• type something that auto-correct wants to correct
• make sure suggested text has same length
• tap move down
• see that auto-correct made the change
• switch to html mode
• verify text is matching ✅

text from autocorrection has a different length as written text

• type something that auto-correct wants to correct
• make sure suggested text has different length
• tap the image
• see that auto-correct made the change
• verify there's no focus loop ✅

[pinarol]

The issue about keyboard close button is fixed! 🎉

[pinarol]

LGTM!
Tested with steps here on both WPAndroid and WPiOS.