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

Implement TextInput autoFocus natively on iOS #27803

Closed

Conversation

janicduplessis
Copy link
Contributor

@janicduplessis janicduplessis commented Jan 18, 2020

Summary

This implement the autoFocus functionality natively instead of calling the focus method from JS on mount. This is needed to properly fix the issue described in #27217, where when using native navigation (UINavigationController) text input focus needs to happen in the same frame transition starts or it leads to an animation bug in UIKit.

My previous attempt fixed the problem only partially and the bug could still happen since there is no guaranty code executed in useEffect will end up in the same frame as the native view being created and attached.

To fix this I added an autoFocus prop to the native component on iOS and in didAttachToWindow we focus the input if it is set. This makes sure the focus is set in the same frame as the view hierarchy containing the input is created.

Changelog

[iOS] [Fixed] - Add native support for TextInput autoFocus on iOS

Test Plan

  • Tested that the UI glitch when transitionning to a screen with an input with autofocus no longer happens in my app.
  • Tested that autofocus still works in RNTester
  • Made sure that onFocus does get called and TextInputState is updated properly

@facebook-github-bot facebook-github-bot added CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. Contributor A React Native contributor. labels Jan 18, 2020
@janicduplessis
Copy link
Contributor Author

cc @TheSavior

@react-native-bot react-native-bot added Component: TextInput Related to the TextInput component. Platform: iOS iOS applications. Bug labels Jan 18, 2020
@TheSavior
Copy link
Member

This needs integration with TextInputState, that is what lets dismissKeyboard and ScrollResponder properly blur the currently focused element.

All focus and blur work currently happens in JS, which isn’t ideal, but it is at least consistent, so it makes me a little nervous to split it up and have one piece implemented natively.

@janicduplessis
Copy link
Contributor Author

@TheSavior Oh I forgot to include it in the test plan but I tested this and onFocus gets called so TextInputState gets updated properly.

@TheSavior
Copy link
Member

Maybe I’m reading the code wrong, but it looks like you early return on iOS, so focus only gets called for autoFocus on Android. What is the code path that is keeping it calling into TextInputState?

@janicduplessis
Copy link
Contributor Author

janicduplessis commented Jan 18, 2020

The JS autoFocus code path doesn't actually call into TextInputState directly. It calls the focus method on the native input component which will focus the input and eventually call its onFocus callback that sets focus target in TextInputState.

TextInputState.focusField(ReactNative.findNodeHandle(inputRef.current));

The focus management is already set up in a way that no matter how focus is triggered (could be tapping the input or programmatically) TextInputState is updated through the onFocus and onBlur props of the native input component.

@TheSavior
Copy link
Member

Ah, onFocus, right

@janicduplessis
Copy link
Contributor Author

@TheSavior Anything I can do to help moving this forward?

@TheSavior
Copy link
Member

Ah whoops, I forgot to look at this when I got into the office the next day. Thanks for the ping.

Can you split this into two different PRs? One with the native change and one with the JS change? I'll need to land the native change to support this behavior first and separately than the JS change that depends on it.

@janicduplessis
Copy link
Contributor Author

janicduplessis commented Jan 31, 2020

This should work. Actually calling focus from JS if the Input is already focused from native will just result in a noop. We can land the JS change separately.

@TheSavior
Copy link
Member

Also, have you investigated implementing this natively on Android as well to make the implementation consistent?

@janicduplessis
Copy link
Contributor Author

I haven't tried but it should be possible. I can test focusing the input in onAttachedToWindow which would be similar to what this does on iOS.

@TheSavior
Copy link
Member

I'm going to run this by the people working on TextInput for Fabric as they will need to implement this behavior for Fabric as well now.

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.

@janicduplessis
Copy link
Contributor Author

Yay that was easy! #27924

facebook-github-bot pushed a commit that referenced this pull request Feb 1, 2020
Summary:
Follow up to #27803. To keep consistency between the implementations this also implements autoFocus natively on Android. This will allow removing the JS auto focus logic completely.

## Changelog

[Android] [Fixed] - Implement native TextInput autoFocus on Android
Pull Request resolved: #27924

Test Plan: Test using the TextInput example in RN tester.

Differential Revision: D19674506

Pulled By: TheSavior

fbshipit-source-id: 9cbb517fc69bccd11f5292a1ccb4acea2e3713e9
@react-native-bot
Copy link
Collaborator

This pull request was successfully merged by @janicduplessis in 6adba40.

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 Feb 1, 2020
alloy pushed a commit that referenced this pull request Feb 25, 2020
Summary:
This implement the autoFocus functionality natively instead of calling the focus method from JS on mount. This is needed to properly fix the issue described in #27217, where when using native navigation (UINavigationController) text input focus needs to happen in the same frame transition starts or it leads to an animation bug in UIKit.

My previous attempt fixed the problem only partially and the bug could still happen since there is no guaranty code executed in useEffect will end up in the same frame as the native view being created and attached.

To fix this I added an autoFocus prop to the native component on iOS and in didAttachToWindow we focus the input if it is set. This makes sure the focus is set in the same frame as the view hierarchy containing the input is created.

## Changelog

[iOS] [Fixed] - Add native support for TextInput autoFocus on iOS
Pull Request resolved: #27803

Test Plan:
- Tested that the UI glitch when transitionning to a screen with an input with autofocus no longer happens in my app.
- Tested that autofocus still works in RNTester
- Made sure that onFocus does get called and TextInputState is updated properly

Differential Revision: D19673369

Pulled By: TheSavior

fbshipit-source-id: 14d8486ac635901622ca667c0e61c75fb446e493
facebook-github-bot pushed a commit that referenced this pull request Mar 2, 2020
Summary:
Follow up to #27803 and #27924. We no longer need to call focus on mount from JS as both iOS and Android implements it natively now.

## Changelog

[General] [Fixed] - Remove JS autoFocus implementation
Pull Request resolved: #27923

Test Plan: Test that focus works in RN Tester with this, #27803 and #27924

Differential Revision: D19956373

Pulled By: TheSavior

fbshipit-source-id: 5d99ead55011663b3edaf499ac7616765a24cb50
xvonabur pushed a commit to xvonabur/react-native that referenced this pull request Mar 4, 2020
Summary:
This implement the autoFocus functionality natively instead of calling the focus method from JS on mount. This is needed to properly fix the issue described in facebook#27217, where when using native navigation (UINavigationController) text input focus needs to happen in the same frame transition starts or it leads to an animation bug in UIKit.

My previous attempt fixed the problem only partially and the bug could still happen since there is no guaranty code executed in useEffect will end up in the same frame as the native view being created and attached.

To fix this I added an autoFocus prop to the native component on iOS and in didAttachToWindow we focus the input if it is set. This makes sure the focus is set in the same frame as the view hierarchy containing the input is created.

[iOS] [Fixed] - Add native support for TextInput autoFocus on iOS
Pull Request resolved: facebook#27803

Test Plan:
- Tested that the UI glitch when transitionning to a screen with an input with autofocus no longer happens in my app.
- Tested that autofocus still works in RNTester
- Made sure that onFocus does get called and TextInputState is updated properly

Differential Revision: D19673369

Pulled By: TheSavior

fbshipit-source-id: 14d8486ac635901622ca667c0e61c75fb446e493
osdnk pushed a commit to osdnk/react-native that referenced this pull request Mar 9, 2020
Summary:
Follow up to facebook#27803. To keep consistency between the implementations this also implements autoFocus natively on Android. This will allow removing the JS auto focus logic completely.

## Changelog

[Android] [Fixed] - Implement native TextInput autoFocus on Android
Pull Request resolved: facebook#27924

Test Plan: Test using the TextInput example in RN tester.

Differential Revision: D19674506

Pulled By: TheSavior

fbshipit-source-id: 9cbb517fc69bccd11f5292a1ccb4acea2e3713e9
osdnk pushed a commit to osdnk/react-native that referenced this pull request Mar 9, 2020
Summary:
This implement the autoFocus functionality natively instead of calling the focus method from JS on mount. This is needed to properly fix the issue described in facebook#27217, where when using native navigation (UINavigationController) text input focus needs to happen in the same frame transition starts or it leads to an animation bug in UIKit.

My previous attempt fixed the problem only partially and the bug could still happen since there is no guaranty code executed in useEffect will end up in the same frame as the native view being created and attached.

To fix this I added an autoFocus prop to the native component on iOS and in didAttachToWindow we focus the input if it is set. This makes sure the focus is set in the same frame as the view hierarchy containing the input is created.

## Changelog

[iOS] [Fixed] - Add native support for TextInput autoFocus on iOS
Pull Request resolved: facebook#27803

Test Plan:
- Tested that the UI glitch when transitionning to a screen with an input with autofocus no longer happens in my app.
- Tested that autofocus still works in RNTester
- Made sure that onFocus does get called and TextInputState is updated properly

Differential Revision: D19673369

Pulled By: TheSavior

fbshipit-source-id: 14d8486ac635901622ca667c0e61c75fb446e493
osdnk pushed a commit to osdnk/react-native that referenced this pull request Mar 9, 2020
Summary:
Follow up to facebook#27803 and facebook#27924. We no longer need to call focus on mount from JS as both iOS and Android implements it natively now.

## Changelog

[General] [Fixed] - Remove JS autoFocus implementation
Pull Request resolved: facebook#27923

Test Plan: Test that focus works in RN Tester with this, facebook#27803 and facebook#27924

Differential Revision: D19956373

Pulled By: TheSavior

fbshipit-source-id: 5d99ead55011663b3edaf499ac7616765a24cb50
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. Component: TextInput Related to the TextInput component. Contributor A React Native contributor. Merged This PR has been merged. Platform: iOS iOS applications.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants