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

Remove requestAnimationFrame when focusing input on mount #27217

Closed
wants to merge 1 commit into from

Conversation

janicduplessis
Copy link
Contributor

Summary

When using react-native-screen which uses native view controller animations for navigation TextInput with autoFocus causes a weird animation glitch.

Removing the requestAnimationFrame will cause the focus command to be sent in the same batch as starting screen transitions which fixes the issue.

It is unclear why the rAF was added in the first place as it was part of the initial RN open source commit. If someone at facebook has more context that would be great to make sure it doesn't cause unintended side effects.

Credits to @kmagiera for figuring out this

Changelog

[General] [Fixed] - Remove requestAnimationFrame when focusing input on mount

Test Plan

  • Tested in an app using react-native-screen to make sure the animation glitch is fixed
  • Tested in RNTester to make sure it doesn't cause other issues when not using react-native-screens

Before:

1

After:

2

@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 Nov 13, 2019
@elicwhite
Copy link
Member

I am pretty sure I remember this being needed in order to work in RNTester, although you mention explicitly that it worked there.

It very likely could be a race condition resolved with rAF as calling focus goes into native which has to cross the async bridge which might require certain things on the native side to have run before t he OS can give the input focus.

Im not exactly sure how we get confidence in this in the meantime. Maybe we just merge and see if we get any issues? I would like to take a closer look though when I get a chance, likely on Friday.

I’m surprised that this has such a negative effect on Screens. From my uninformed view it kinda seems like the fixes for these problems should be separate

@janicduplessis
Copy link
Contributor Author

@TheSavior I actually did not test android, will do that too.

Looking at the iOS code I don't see how a race could happen since it calls UIManager.focus which then uses addUIBlock to focus the input. All the operation should happen in order so the new input will always be created and attached before focus is called. On android it uses dispatchViewManagerCommand which uses UIViewOperationQueue so it will also be properly synced with view creating operations.

Might have been a workaround for an old race condition bug that was since then fixed.

It seems like a bug with UIKit, It must assume keyboard focus will happen in the same event loop as the transition. Normally you'd call becomeFirstResponder in viewWillAppear. This will cause the keyboard to slide in from right to left smoothly with the new screen animation. When removing the raF it will cause pretty much the same thing to happen.

@elicwhite
Copy link
Member

elicwhite commented Nov 20, 2019

Did you get a chance to test this on Android? Thanks for the digging. I think we should just merge this and see if we get any issues on it.

@janicduplessis
Copy link
Contributor Author

@TheSavior Just tested on Android and it causes the keyboard to open then close immediately :(

This explains why this was added. What about we document the need for this rAF and add a platform check so we only do it on Android?

@janicduplessis
Copy link
Contributor Author

Updated the diff, let me know if that seems like a reasonable fix.

Copy link

@analysis-bot analysis-bot left a comment

Choose a reason for hiding this comment

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

Code analysis results:

  • eslint found some issues. Run yarn lint --fix to automatically fix problems.

Libraries/Components/TextInput/TextInput.js Outdated Show resolved Hide resolved
Libraries/Components/TextInput/TextInput.js Outdated Show resolved Hide resolved
@elicwhite
Copy link
Member

elicwhite commented Nov 20, 2019

Since this is adding a platform check we probably need to check if this change is valid for React-Native Web and Windows / MacOS. Can you try Web and I'll see if someone from Microsoft can check Windows / Mac?

Edit: Since Windows has a forked JS file for TextInput this change might not impact anyways.

Edit Edit: Looks like Web's TextInput.js is forked too. Let's ship this.

Edit Edit Edit: I'm currently deep in a pretty major refactor to TextInput.js to rewrite it with Hooks. I'd like to hold off landing this and instead make the change on top of my refactor to avoid merge conflicts if that is okay with you

@janicduplessis
Copy link
Contributor Author

@TheSavior hahaha yeah you can land this on top of your text input changes.

@elicwhite
Copy link
Member

I'd like to keep this open for tracking if you don't mind so we don't forget about this

@elicwhite elicwhite reopened this Nov 20, 2019
@elicwhite
Copy link
Member

Alright, my refactor to hooks has landed and we haven't heard reports yet of a problem internally.

Would you like to reapply these changes on top? I'm fine with the platform check

@janicduplessis
Copy link
Contributor Author

@TheSavior 👍 Updated the PR

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 @janicduplessis in 5798cf2.

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 23, 2019
@janicduplessis janicduplessis deleted the focus-raf branch January 18, 2020 06:59
facebook-github-bot pushed a commit that referenced this pull request Feb 1, 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
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
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:
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
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. Contributor A React Native contributor. Merged This PR has been merged.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants