Compose box uncontrolled

[borisyankov]

Fixes #2589

When having 'controlled' TextInput components we run the risk of
JS blocking the updates to these components while you type.

This is generally not an issue in web React and rarely in React Native
(communication through the bridge is fast but slower than not having it at all)

Some users have experienced unusually slow input in the compose box.
This is likely related to facebook/react-native#19126

Instead of using the topic and message inputs' property value
we can short-circuit this and update native properties ourselves.

This was an improvement I wanted to implement previously even before the
issues were reported, but back then it wasn't worth the effort.

[gnprice]

Thanks @borisyankov !

Applying the 'clear text input' patch to React Native is a superior solution.

Do you consider this PR ready to merge, and then ship in a release, without doing that?

If not, we shouldn't merge it until we have that ready too. How much work do you believe that will be?

[borisyankov]

Without the patch, when sending a message the text remains in the compose box.
With the patch, I'll need to handle few specific cases, and the placeholder disappears because it contains a character.

[gnprice]

Without the patch, when sending a message the text remains in the compose box.

OK. That does not sound ready to me -- I expect that will (very reasonably) generate a lot of user complaints.

With the patch, I'll need to handle few specific cases, and the placeholder disappears because it contains a character.

Wait, I was replying to what you said about "the 'clear text input' patch to React Native", which I take to mean this upstream PR. Are you referring to that here, or are you talking now about the zero-width-space hack?

How much work do you believe it will be to apply the patch (i.e. the PR I just linked) to React Native and ship a version of the app using that?

[borisyankov]

'Not too long' is my guess, few hours. But I haven't kept a fork of RN so, not sure.

[borisyankov]

After reading the guide:
https://facebook.github.io/react-native/docs/building-from-source.html
Possibly few hours to half a day.

[gnprice]

OK, thanks. Given the uncertainty in our estimates of how much work that will be, I think I would rather not have it block getting a release out that fixes this issue. I do want to set up that infrastructure because it will help us on this and other issues, but I'd rather do it without that pressure.

I'm also not 100% confident that PR is good to apply -- the review feedback is cryptic, but it sounds like it may not be minor -- like the reviewer may think it's the wrong approach. I'd want us to either do some QA, or do some study of the context in the code to understand what the reviewer is saying (in particular whether it's a potential live issue, or a concern about code clarity which we can overlook for a temporary local patch), before we applied it.

This PR won't be ready to merge until we've done those two pieces of work, but let's leave it open because it's likely we will want to merge it then. (Basically, unless RN upstream suddenly responds to and fixes the original issue this works around -- and maybe even if they do, given the potential performance improvement.)

[gnprice]

In a chat thread, @borisyankov pointed out another option: we could release a version with this patch only on Android. The RN bug it works around only appears there, and the RN bug it triggers only appears on iOS.

That seems likely to be a good immediate solution. I have to go AFK for a bit, but I'll review this code later today, and if it looks good I think I'll do that.

[gnprice]

Some comments from reading through this branch:

• I'm having a hard time understanding some of what it's doing. Part of this is because it seems like a lot of the commits don't make sense on their own, and leave things in a broken intermediate state. For example, the first commit in the series removes the value prop from the inputs, without doing anything yet to replace its functionality. And the third commit moves a bunch of logic from methods of the component (which do get called) into updateTextInput, which was just introduced in the second commit and isn't called from anywhere yet. Would you try to reorganize these as a series of coherent commits?
• One of the commit messages says "React Native has problems with some custom Android keyboards. It fails to reset the TextInput contents." What issue are you referring to there? A link or issue number would help.

None of this would stop me from shipping it as a temporary fix, in a stable branch, for a high-priority issue like #2589 -- I'll be OK with some empirical testing. But before we merge a change into master I definitely want something that's clearer to understand.

[borisyankov]

The custom keyboard fix is not a new line of code, but I was clarifying.
It is this one:

TextInputReset.resetKeyboardInput(findNodeHandle(textInput));

[gnprice]

This is a very confusing way to tell a story.

Imagine deleting a file in one commit, then adding it back with a new name in a later commit -- it'd be much better to squash those two so that there's one commit that moves the file. That's much clearer to understand.

Doubly so if there's some other code that refers to the file, so the first commit just leaves that code broken. That's very confusing, and it's much better to have one commit that both renames the file and updates the references to it.

Here, what we're moving is a piece of logic that's a function within a file, instead of a whole file, but basically the exact same thing applies. These first two commits are impossible to understand without going and reading the third commit too; so it'd be better even to just squash the three of them together.

There's kind of a lot going on in that third commit, though. So better still would be to split it up in some other direction, in a way where each of the pieces makes sense.

Here's a version of this that I think would be a lot clearer:

• First, factor out updateTextInput. This is a lot like the current second commit -- but the places that call clearMessageInput should turn that into updateTextInput, so that they keep working right.
• Then, switch just the message input to be uncontrolled; including the necessary fix to the autocomplete logic.
• Then, do the same for the topic input.

Would you please try doing that?

[borisyankov]

I did went over the commits once more.

• Fixed the formatting issue (that was introduced then fixed)
• Made sure several messages are more logical or clear with the commits differing from before
• I did split the last change to ComposeBox and left for last the making of the component to 'uncontrolled'

[gnprice]

@borisyankov and I had a detailed discussion about these changes out of band, and he's posted a revised version as #2738 .

Closing this one because it's superseded by that.