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

Fix for: You cannot blur a text-input if you tab to it. #27038

Closed
wants to merge 2 commits into from

Conversation

fat
Copy link
Contributor

@fat fat commented Oct 28, 2019

Summary

I've been working on a new iOS experience with lots of text inputs and this has been driving me a bit nuts…

If you're in a scrollview with keyboardShouldPersistTaps="handled" and you tab through your text-inputs, you aren't able to tap outside of a given text-input to blur it (and dismiss the keyboard).

I wrote up a quick explanation and some repo steps here: https://snack.expo.io/BJBcKgrqB

The patch i came up with, after poking around for a little bit seems terrifying - so almost certainly not it. But if it's helpful at all - decided to just got ahead and submit it.

Changelog

[iOS] [Fixed] - TextInput blur when tabbing in iOS simulator.

Test Plan

I tried to think of a way to test this in jest… but i didn't get very far sorry 😢

I did create a snack here so you can demo the issue: https://snack.expo.io/BJBcKgrqB

I also created two videos…

Here's the text input not working when i try to blur it after tabbing in simulator
ezgif-1-dc85b405c760

Here's it working after I applied this patch
ezgif-1-ed9f6b19653d

Thanks!

@facebook-github-bot
Copy link
Contributor

Hi fat! Thank you for your pull request and welcome to our community. We require contributors to sign our Contributor License Agreement, and we don't seem to have you on file.In order for us to review and merge your code, please sign at https://code.facebook.com/cla. If you are contributing on behalf of someone else (eg your employer), the individual CLA may not be sufficient and your employer may need to sign the corporate CLA.

If you have received this in error or have any questions, please contact us at cla@fb.com. Thanks!

@fat
Copy link
Contributor Author

fat commented Oct 28, 2019

Signed, thanks 🙏

@react-native-bot react-native-bot added Bug Platform: iOS iOS applications. labels Oct 28, 2019
@facebook-github-bot facebook-github-bot added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label Oct 29, 2019
@facebook-github-bot
Copy link
Contributor

Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Facebook open source project. Thanks!

@shergin
Copy link
Contributor

shergin commented Oct 30, 2019

I would love to see the explanation of why it works and why it should be implemented this way.

@fat
Copy link
Contributor Author

fat commented Oct 30, 2019

I originally was trying to figure out why _onPress (

_onPress: function(event: PressEvent) {
if (this.props.editable || this.props.editable === undefined) {
this.focus();
}
},
) was working, while _onFocus wasn't (specifically when _onFocus is triggered by something like a tab event. That lead me to _onPress calling the this.focus method exposed by the NativeMethodsMIxin.

In trying to figure out what focus() did - I got as far as this code: (

__SECRET_INTERNALS_DO_NOT_USE_OR_YOU_WILL_BE_FIRED,
) and then kinda threw my hands up (unclear to me what is happening under the hood by calling focus? I might just not be familiar enough with the project to know where that code lives, but from my quick look - seemed like it was something obfuscated.

Because of that, the first thing I thought to do then was… "what if i just called this focus mixin method when _onFocus is triggered" (such as by a tab event). This seems to work for my usecase - although again - unclear if this is the way this issue should be resolved.

@fat
Copy link
Contributor Author

fat commented Oct 30, 2019

Hope that's helpful 🤷‍♀

@elicwhite
Copy link
Member

elicwhite commented Oct 31, 2019

Thanks for highlighting this bug...and sending a PR too!

Okay, here is what I think is happening. For context, here is a diagram I have of how focus and blur propagates through the system. This might be interesting to refer back to as you go through the rest of my explanation.

graphviz (12)

ScrollView's scrollResponder is responsible for blurring text inputs when a touch occurs in the ScrollView but outside of the currently focused TextInput. The code for that is here:

// By default scroll views will unfocus a textField
// if another touch occurs outside of it
const currentlyFocusedTextInput = TextInputState.currentlyFocusedField();
if (
this.props.keyboardShouldPersistTaps !== true &&
this.props.keyboardShouldPersistTaps !== 'always' &&
currentlyFocusedTextInput != null &&
e.target !== currentlyFocusedTextInput &&
!this.state.observedScrollSinceBecomingResponder &&
!this.state.becameResponderWhileAnimating
) {
this.props.onScrollResponderKeyboardDismissed &&
this.props.onScrollResponderKeyboardDismissed(e);
TextInputState.blurTextInput(currentlyFocusedTextInput);

This happens on scrollResponderHandleResponderRelease aka, touch up.

It checks for what the currently focused textinput is by calling TextInputState.currentlyFocusedField().

That function is a JS variable that is being updated by calls to TextInputState.focusTextInput and TextInputState.blurTextInput:

function focusTextInput(textFieldID: ?number) {
if (currentlyFocusedID !== textFieldID && textFieldID !== null) {
currentlyFocusedID = textFieldID;
if (Platform.OS === 'ios') {
UIManager.focus(textFieldID);
} else if (Platform.OS === 'android') {
UIManager.dispatchViewManagerCommand(
textFieldID,
UIManager.getViewManagerConfig('AndroidTextInput').Commands
.focusTextInput,
null,
);
}
}
}
/**
* @param {number} textFieldID id of the text field to unfocus
* Unfocuses the specified text field
* noop if it wasn't focused
*/
function blurTextInput(textFieldID: ?number) {
if (currentlyFocusedID === textFieldID && textFieldID !== null) {
currentlyFocusedID = null;
if (Platform.OS === 'ios') {
UIManager.blur(textFieldID);
} else if (Platform.OS === 'android') {
UIManager.dispatchViewManagerCommand(
textFieldID,
UIManager.getViewManagerConfig('AndroidTextInput').Commands
.blurTextInput,
null,
);
}
}
}

I added some console logs to those methods to see which ones are being called when running your repro (thanks for the repro!). This is without your fix

Click on and off:

// Click on input 1
focusTextInput input1
TextInput's _onFocus called

// Click on blank space
scrollResponderHandleResponderRelease blur input1
blurTextInput input1
TextInput's _onBlur called

Click on input1, then input 2, then off

// Click on input 1
focusTextInput input1
TextInput's _onFocus called for input1

// Click on input 2
focusTextInput input2
TextInput's _onBlur called for input1 
TextInput's _onFocus called for input2

// Click on blank space
scrollResponderHandleResponderRelease blur input2
blurTextInput input2
TextInput's _onBlur called for input2

And now for the bug. Click on input 1, tab to 2, then off

// Click on input 1
focusTextInput input1
TextInput's _onFocus called for input1

// Tab to input 2
TextInput's _onBlur called for input1 
TextInput's _onFocus called for input2

// Click on blank space
scrollResponderHandleResponderRelease blur input1
blurTextInput input1

Notice how focusTextInput was never called with input2 in the last example. Since this is the function that sets the currentlyFocusedField when we click on the blank space RN is trying to blur the first input instead of the second.

The root cause

We are tracking the state of which field is focused in JS which has to stay in sync with what native knows is focused. We listen to _onPress and call TextInputState.focusTextInput in that handler. However, we don't currently have anything listening to other ways for an input to become focused (like tabbing) so it doesn't end up updating the currentlyFocusedField.

We have the same problem with blur that we actually fixed the same way you did here in this PR:

_onBlur: function(event: BlurEvent) {
// This is a hack to fix https://fburl.com/toehyir8
// @todo(rsnara) Figure out why this is necessary.
this.blur();
if (this.props.onBlur) {
this.props.onBlur(event);
}
},

If you look back at my diagram at the beginning of this post, you'll notice the missing edge from TextInput._onFocus to TextInputState.focusTextInput. That's the problem. :)

The reason this solution works is because this function is the notification from native that an input was focused or blurred. This solution is fine because this updates the currentlyFocusedID but isn't great because it both sets that value and calls the native code to focus or blur again. Luckily the native code doesn't send an event back to JS if you try to blur an already blurred TextInput otherwise we'd have an infinite loop.

The correct solution

The correct thing would probably be to have all of this tracking in native code and not in JavaScript code. That's a pretty big change though and very out of scope. Something for our team to keep in mind for the future.

A shorterm term solution would be to refactor focusTextInput and blurTextInput to pull out the part that sets the currentlyFocusedID that we could call from TextInput directly from _onFocus and _onBlur

Thank you

Thank you for highlighting this bug and giving a really solid repro for me to investigate. I don't think you need to take any action on these "correct" solutions in this PR but I wanted to share all the context back with you that I can. This is also helpful documentation for our team as we work on these components in the future.

@@ -1098,6 +1098,7 @@ const TextInput = createReactClass({
if (this.props.onFocus) {
this.props.onFocus(event);
}
this.focus();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you flip the order and put this above the if statement to be consistent with _onBlur?

_onBlur: function(event: BlurEvent) {
// This is a hack to fix https://fburl.com/toehyir8
// @todo(rsnara) Figure out why this is necessary.
this.blur();
if (this.props.onBlur) {
this.props.onBlur(event);
}
},

I think it's important because if someone checks if they are the currently focused input in the onFocus prop they pass in it should be true.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually, I'm able to just edit the branch on your remote through the GitHub UI. Neat!

Flip the order in `_onFocus`
Copy link
Contributor

@facebook-github-bot facebook-github-bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@TheSavior is landing this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

@react-native-bot
Copy link
Collaborator

This pull request was successfully merged by Jacob Thornton in a743771.

When will my fix make it into a release? | Upcoming Releases

@react-native-bot react-native-bot added the Merged This PR has been merged. label Nov 1, 2019
@RSNara
Copy link
Contributor

RSNara commented Nov 1, 2019

So inside a ScrollView, when you click outside the TextInput, we grab the currently focused element and unfocus it. All that code is in JS, and we keep a track of which TextInput is focused inside TextInputState. In TextInput, that JS state is updated only by calls to this.blur() and this.focus(). When Native changes focus or blur, it calls TextInput._onBlur and TextInput._onFocus. So, we have to call this.blur() and this.focus(). Otherwise, we won't update TextInputState, and the ScrollView (and JS in general) won't know which element is currently focused/blurred.

Is that right, @TheSavior?

@elicwhite
Copy link
Member

Yes! Exactly!

Well, kinda exactly. The only part that I'd want to clarify from your comment is:

In TextInput, that JS state is updated only by calls to this.blur() and this.focus()

All NativeComponents (host component, component using NativeMethodsMixin, or component extending ReactNative.NativeComponent) have a .focus and .blur method which updates TextInputState.

That means that if you have a <Switch> you can call .focus on it, which will set it to be the currently focused field in TextInputState and then when you click somewhere else in a ScrollView, ScrollResponder will tell TextInputState to blur that <Switch>.

Even though it isn't a TextInput it is still gets focus and blur set on it. But to make it worse, since we don't listen to onFocus and onBlur in the Switch JS code to update TextInputState as well then if native focuses off a Switch, the JS might still think it is focused. That's the same problem this PR is fixing for TextInput.

Doesn't this whole thing just make you feel terrible? 😁

facebook-github-bot pushed a commit that referenced this pull request Nov 4, 2019
Summary:
I wrote up a bunch of context for this in response to #27038 by fat. That comment is reproduced here in this commit message. You can see it in it's original contxt here: #27038

Okay, here is what I think is happening. For context, here is a diagram I have of how focus and blur propagates through the system. This might be interesting to refer back to as you go through the rest of my explanation.

![graphviz (12)](https://user-images.githubusercontent.com/249164/67992345-982c9d80-fbf9-11e9-96ea-b091210dddbe.png)

ScrollView's scrollResponder is responsible for blurring text inputs when a touch occurs in the ScrollView but outside of the currently focused TextInput. The code for that is here:
https://github.com/facebook/react-native/blob/6ba2769f0f92ca75fb0eb60ccb8337920a9c31eb/Libraries/Components/ScrollResponder.js#L301-L314

This happens on `scrollResponderHandleResponderRelease` aka, touch up.

It checks for what the currently focused textinput is by calling `TextInputState.currentlyFocusedField()`.

That function is a JS variable that is being updated by calls to `TextInputState.focusTextInput` and `TextInputState.blurTextInput`:

https://github.com/facebook/react-native/blob/6ba2769f0f92ca75fb0eb60ccb8337920a9c31eb/Libraries/Components/TextInput/TextInputState.js#L36-L71

I added some console logs to those methods to see which ones are being called when running your repro (thanks for the repro!). **This is without your fix**

Click on and off:
```
// Click on input 1
focusTextInput input1
TextInput's _onFocus called

// Click on blank space
scrollResponderHandleResponderRelease blur input1
blurTextInput input1
TextInput's _onBlur called
```

Click on input1, then input 2, then off
```
// Click on input 1
focusTextInput input1
TextInput's _onFocus called for input1

// Click on input 2
focusTextInput input2
TextInput's _onBlur called for input1
TextInput's _onFocus called for input2

// Click on blank space
scrollResponderHandleResponderRelease blur input2
blurTextInput input2
TextInput's _onBlur called for input2
```

And now for the bug. Click on input 1, tab to 2, then off
```
// Click on input 1
focusTextInput input1
TextInput's _onFocus called for input1

// Tab to input 2
TextInput's _onBlur called for input1
TextInput's _onFocus called for input2

// Click on blank space
scrollResponderHandleResponderRelease blur input1
blurTextInput input1
```

Notice how `focusTextInput` was never called with input2 in the last example. Since this is the function that sets the `currentlyFocusedField` when we click on the blank space RN is trying to blur the first input instead of the second.

# The root cause
We are tracking the state of which field is focused in JS which has to stay in sync with what native knows is focused. We [listen to _onPress](https://github.com/facebook/react-native/blob/6ba2769f0f92ca75fb0eb60ccb8337920a9c31eb/Libraries/Components/TextInput/TextInput.js#L1103-L1107) and call `TextInputState.focusTextInput` in that handler. However, we don't currently have anything listening to other ways for an input to become focused (like tabbing) so it doesn't end up updating the `currentlyFocusedField`.

We have the same problem with blur that we actually fixed the same way you did here in this PR:
https://github.com/facebook/react-native/blob/6ba2769f0f92ca75fb0eb60ccb8337920a9c31eb/Libraries/Components/TextInput/TextInput.js#L1182-L1189

If you look back at my diagram at the beginning of this post, you'll notice the missing edge from `TextInput._onFocus` to `TextInputState.focusTextInput`. That's the problem. :)

The reason this solution works is because this function **is** the notification from native that an input was focused or blurred. This solution is *fine* because this updates the `currentlyFocusedID` but isn't great because it both sets that value and **calls the native code to focus or blur again**. Luckily the native code doesn't send an event back to JS if you try to blur an already blurred TextInput otherwise we'd have an infinite loop.

# The correct solution
The correct thing would probably be to have all of this tracking in native code and not in JavaScript code. That's a pretty big change though and very out of scope. Something for our team to keep in mind for the future.

A short term term solution would be to refactor `focusTextInput` and `blurTextInput` to pull out the part that sets the `currentlyFocusedID` that we could call from `TextInput` directly from `_onFocus` and `_onBlur`.

# ^This short term term solution is what this commit is doing.

Changelog:
[General][Changed] TextInput no longer does an extra round trip to native on focus/blur

Reviewed By: RSNara

Differential Revision: D18278359

fbshipit-source-id: 417566f25075a847b0f4bac2888f92fbac934096
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. Merged This PR has been merged. Platform: iOS iOS applications.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants