Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Turn iOS compose box uncontrolled; dedupe with Android #2886

Closed

Conversation

borisyankov
Copy link
Contributor

A fix for the iOS blocking issue is nowhere in sight, but a cool and simple workaround was discovered that allows us to implement the fix now, and not in an unspecified time in future.

Details are given in the commit messages.

@gnprice gnprice mentioned this pull request Aug 16, 2018
4 tasks
@gnprice
Copy link
Member

gnprice commented Aug 16, 2018

Thanks! Glad to get these together again :)

A few questions:

if (text.length === 0) {
  // ...
} else {
  textInput.setNativeProps({ text });
}
  • In these comments that mention different RN bugs, let's definitely have links -- either to the upstream bugs, or to our own issue that describes the RN bug when that's a better reference.
  • Also, this isn't an "iOS bug" as far as I know -- it's an RN bug specific to iOS. Similarly "Android" doesn't have a bug here. You know that, but I think it's important to be clear about it in the code for other readers -- not only to keep an accurate mental tally of which platforms are responsible for bugs that affect us 😛 , but also because that framing can make a big difference when following the link and reading more details to understand what the story is.
  • How confident are you / how are you confident that the double-set trick consistently avoids the issue? The setTimeout in particular makes me wonder if there might be a race where sometimes the relevant bit of RN code runs in between the two sets, and other times it doesn't and the trick doesn't work -- like maybe that bit of RN code runs once per frame, and if we get an update just after that code ran for one frame, and the phone is nice and fast and not busy, the second set happens within the >=8ms before it runs for the next frame.
  • On the other side of things, I wonder if a legitimate update could get squeezed in between the two sets, and get clobbered -- like if the user has just one letter there, and hits delete and then a letter in quick succession, and maybe if it's an old and overloaded phone so the JS engine takes a while to get around to the setTimeout'd second set.

@borisyankov
Copy link
Contributor Author

Indeed it is the first commit that solves #2434, I will reword that.

Hmm, while the code I did commit works correctly, I seem to have missed to change the updateTextInput function completely as intended. I totally wanted to do

  if (text.length === 0) {
    clearTextInput(textInput);
  } else {
    textInput.setNativeProps({ text });
}

@borisyankov
Copy link
Contributor Author

The setTimeout concern is pretty good to raise! I am researching to see how likely it is to cause this issue.

Remove the iOS code which was our old 'controlled' version and
keep the new Android 'uncontrolled' version.

While iOS does not have the extreme performance issues the Android
app had because of bugs in React Native, it is still faster to not
have the component controlled - a controlled compoennt would block
any time the UI thread blocks.

Another big benefit is removing the need to support two versions.

Making the component uncontrolled fixes zulip#2434
Fixes: zulip#2434

There is a simple hack/fix for the iOS-specific bug that was a
blocker for us migrating the iOS version.

If we set the text via `setNativeProps` twice in a row, it will
indeed reset the contents, so the first time we use a space `' '`
that looks empty but is different than the second time when we
set it to an actually empty string '``'. We use `setTimeout` in
order to give React Native a chance to update the native value
before running the second time.

For Android we don't need to call `setNativeProps` as the
`TextInputReset.resetKeyboardInput` call does that. Also compared
to the previous version we don't need to check if `TextInputReset`
is defined as it isn't only for iOS and we `if` for that.
@gnprice
Copy link
Member

gnprice commented Sep 12, 2018

Quick update:

@gnprice gnprice changed the title Turn iOS compose box uncontrolled Turn iOS compose box uncontrolled; dedupe with Android Sep 24, 2018
@gnprice
Copy link
Member

gnprice commented Oct 2, 2018

I think this makes most sense to track as an issue at this point rather than a PR; when we come back to it I think we'll basically repeat the boring part of this PR and (happily!) skip the more interesting part. So I went and filed #3017 for that.

@gnprice gnprice closed this Oct 2, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants