Skip to content

Implement TextInput autoFocus natively on iOS - #27803

Closed
janicduplessis wants to merge 2 commits into
react:masterfrom
janicduplessis:autofocus-native
Closed

Implement TextInput autoFocus natively on iOS#27803
janicduplessis wants to merge 2 commits into
react:masterfrom
janicduplessis:autofocus-native

Conversation

@janicduplessis

@janicduplessisjanicduplessis commented Jan 18, 2020

Copy link
Copy Markdown
Contributor

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-botfacebook-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
Copy Markdown
ContributorAuthor

cc @TheSavior

@react-native-botreact-native-bot added Component: TextInput Related to the TextInput component. Platform: iOS iOS applications. Bug labels Jan 18, 2020
@elicwhite

Copy link
Copy Markdown
Contributor

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
Copy Markdown
ContributorAuthor

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

@elicwhite

Copy link
Copy Markdown
Contributor

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

janicduplessis commented Jan 18, 2020

Copy link
Copy Markdown
ContributorAuthor

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.

https://github.com/facebook/react-native/blob/022e79c5f8cce70d7f4fa32dc0f64f053d2da1f0/Libraries/Components/TextInput/TextInput.js#L712

https://github.com/facebook/react-native/blob/022e79c5f8cce70d7f4fa32dc0f64f053d2da1f0/Libraries/Components/TextInput/TextInput.js#L1005

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.

@elicwhite

Copy link
Copy Markdown
Contributor

Ah, onFocus, right

@janicduplessis

Copy link
Copy Markdown
ContributorAuthor

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

@elicwhite

Copy link
Copy Markdown
Contributor

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

janicduplessis commented Jan 31, 2020

Copy link
Copy Markdown
ContributorAuthor

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.

@elicwhite

Copy link
Copy Markdown
Contributor

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

@janicduplessis

Copy link
Copy Markdown
ContributorAuthor

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.

@elicwhite

Copy link
Copy Markdown
Contributor

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.

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

Copy link
Copy Markdown
Contributor

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
Copy Markdown
ContributorAuthor

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
Copy Markdown
Collaborator

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

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

@react-native-botreact-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 react#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: react#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 react#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: react#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 react#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: react#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 react#27803 and react#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: react#27923
Test Plan: Test that focus works in RN Tester with this, react#27803 and react#27924
Differential Revision: D19956373
Pulled By: TheSavior
fbshipit-source-id: 5d99ead55011663b3edaf499ac7616765a24cb50
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

BugCLA SignedThis label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed.Component: TextInputRelated to the TextInput component.ContributorA React Native contributor.MergedThis PR has been merged.Platform: iOSiOS applications.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants

@janicduplessis@elicwhite@react-native-bot@facebook-github-bot