Expensify
[Tracking] [Performance] Investigate creating our own KeyboardAvoidingView
Coming from https://expensify.slack.com/archives/C01GTK53T8Q/p1659722402266779?thread_ts=1659347778.519049&cid=C01GTK53T8Q
Related Issues on HOLD
- FIXED https://github.com/Expensify/App/issues/11118
- FIXED https://github.com/Expensify/App/issues/10905
- NOT FIXED https://github.com/Expensify/App/issues/10609
- NOT FIXED https://github.com/Expensify/App/issues/11701
- NOT FIXED https://github.com/Expensify/App/issues/10670
- NOT FIXED https://github.com/Expensify/App/issues/11094
- NOT FIXED https://github.com/Expensify/App/issues/11057
- NOT FIXED https://github.com/Expensify/App/issues/10610
- NOT FIXED https://github.com/Expensify/App/issues/11277
- NOT FIXED https://github.com/Expensify/App/issues/11087
- NOT FIXED https://github.com/Expensify/App/issues/11722
- NOT FIXED https://github.com/Expensify/App/issues/12114
- https://github.com/Expensify/App/issues/11776
- https://github.com/Expensify/App/issues/11858
- https://github.com/Expensify/App/issues/7572
Problem
First a few observations:
- Dealing with KeyboardAvoidingViews (different behaviors) is always kind of confusing imo
- Furthermore, we've got our own wrapper component for KeyboardAvoidingView that isn't used anywhere in the codebase
- We've also got this LoginKeyboardAvoidingView component. The native implementation is just the KeyboardAvoidingView from React Native, and the web/desktop implementation seems kind of funky to me. It just passes a style and contentContainerStyle to the style prop of the React Native implementation, which seems unnecessary since the RN component has both of those props.
- We also use this abandoned KeyboardSpacer component that is not very different from KeyboardAvoidingView AFAICT
- EdIt: We now have our own customer KeyboardSpacer component
All that is to say, it feels like our usage of keyboard-avoiding stuff is a bit inconsistent/all-over-the-place, and maybe it's a good time to step back and see if we can consolidate some of this and make it a bit more consistent.
Moreover, we've noticed issues such as this one where the keyboard-avoiding stuff isn't necessarily working as expected. There have been other similar bugs in the past. For example, we have some screens where we use timeouts to wait for navigation transitions to finish before opening the keyboard, etc... Furthermore, many of these bugs have been sort of inconsistent (maybe suggesting that they are caused by race conditions). Lastly, I've noticed that the keyboard is just really sluggish and slow to open on iOS.
Solution
Now this part is more of a hunch, but I have a theory that many of these bugs are caused because Reanimated (the animation library used by react-navigation) and the React Native KeyboardAvoidingView/KeyboardSpacer components aren't playing nice together. Why? Because Reanimated runs animations synchronously on the UI thread, while the KeyboardAvoidingView components will update asynchronously between the UI thread and the layout/render thread. This makes it hard to keep them in sync / behaving as expected.
I did a bit of research and noticed that Reanimated 3 is set to introduce a useAnimatedKeyboard hook. This would allow us to create a simple KeyboardAvoidingView of our own that is animated synchronously on the UI thread. I think this would be better for performance (which I have noticed is lackluster on iOS staging+production), and hopefully make opening/closing the keyboard much snappier. Furthermore, since (all?) our animations would be running in Reanimated, I have a hunch that many of these inconsistent bugs might go away.
So my proposal (or at minimum, an idea that I think is worth looking into) is that we:
- upgrade to Reanimated 3 (after this PR is merged)
- create our own KeyboardAvoidingView component using the useAnimatedKeyboard hook from Reanimated
- Use our component everywhere. No more use of the RN built-in KeyboardAvoidingView or the KeyboardSpacer library.
Thanks Rory, the solution part of this is really fascinating and it sounds like maybe there are some big performance gains hiding away in Reanimated 3.
I am having a little trouble with the "problem" section of this and think it needs refining. Or at a minimum break concerns into separate issues to tackle one at a time. I'm admittedly having trouble believing that we aren't just holding something wrong (as that's usually the case).
Dealing with KeyboardAvoidingViews (different behaviors) is always kind of confusing imo
Can we unlock this somehow? There must be some basic cases where the behaviors should be used and it seems possible to articulate what they do and develop some best practices and guidance?
Furthermore, we've got our own wrapper component for KeyboardAvoidingView that isn't used anywhere in the codebase
Could we figure out if we can delete this and replace it with a regular KeyboardAvoidingView ? That seems like a good cleanup task.
We've also got this LoginKeyboardAvoidingView component. The native implementation is just the KeyboardAvoidingView from React Native, and the web/desktop implementation seems kind of funky to me. It just passes a style and contentContainerStyle to the style prop of the React Native implementation, which seems unnecessary since the RN component has both of those props.
This too?
We also use this abandoned KeyboardSpacer component that is not very different from KeyboardAvoidingView AFAICT EdIt: We now have our own customer KeyboardSpacer component
And this?
maybe it's a good time to step back and see if we can consolidate some of this and make it a bit more consistent
Agree, but what does it have to do with rolling our own KeyboardAvoidingView? Is it possible to articulate what it is exactly about the component that isn't working for us? I think a lot of the changes you are mentioning might have been implemented when many of us were very naively approaching react-native and trying to get things done quickly.
For example, we have some screens where we use timeouts to wait for navigation transitions to finish before opening the keyboard
What's an example of this? Is it something we have or can create issues to address/improve?
Furthermore, many of these bugs have been sort of inconsistent (maybe suggesting that they are caused by race conditions)
Could use an example here as well.
I've noticed that the keyboard is just really sluggish and slow to open on iOS.
And it's because of the KeyboardAvoidingView ? Do we have an open issue for this?
One thing learned from today is that barely anyone has a proper understanding of how this component works. 😂 So I'd love to keep exploring KeyboardAvoidingView and see if it is the cause of these woes or if we can make it work for us or not.
Looks like something related to react-navigation may have been mentioned in this issue discussion.
As a reminder, please make sure that all proposals are not workarounds and that any and all attempt to fix the issue holistically have been made before proceeding with a solution. Proposals to change our DeprecatedCustomActions.js files should not be accepted.
Feel free to drop a note in #expensify-open-source with any questions.
![melvin-bot[bot] avatar](https://avatars.githubusercontent.com/in/179547?v=4 "Published by a pub.dev verified publisher"){kind=small}
⚠️ Looks like this issue was linked to a possible regression on PRODUCTION here
If you are the assigned CME please investigate whether the linked PR caused a regression and leave a comment with the results.
If a production regression has occurred a Root Cause Analysis is required. Please follow the instructions here.
If a regression has occurred and you are the assigned CM follow the instructions here.
If this regression could have been avoided please consider also proposing a recommendation to the PR checklist so that we can avoid it in the future.
⚠️ Looks like this issue was linked to a possible regression on PRODUCTION here
If you are the assigned CME please investigate whether the linked PR caused a regression and leave a comment with the results.
If a production regression has occurred a Root Cause Analysis is required. Please follow the instructions here.
If a regression has occurred and you are the assigned CM follow the instructions here.
If this regression could have been avoided please consider also proposing a recommendation to the PR checklist so that we can avoid it in the future.
⚠️ Looks like this issue was linked to a possible regression on PRODUCTION here
If you are the assigned CME please investigate whether the linked PR caused a regression and leave a comment with the results.
If a production regression has occurred a Root Cause Analysis is required. Please follow the instructions here.
If a regression has occurred and you are the assigned CM follow the instructions here.
If this regression could have been avoided please consider also proposing a recommendation to the PR checklist so that we can avoid it in the future.
No update here yet, but looking forward to working on this. Fortunately useAnimatedKeyboard was released in 2.10 so upgrading to a 3.0 beta release is no longer a prerequisite of doing this
⚠️ Looks like this issue was linked to a possible regression on PRODUCTION here
If you are the assigned CME please investigate whether the linked PR caused a regression and leave a comment with the results.
If a production regression has occurred a Root Cause Analysis is required. Please follow the instructions here.
If a regression has occurred and you are the assigned CM follow the instructions here.
If this regression could have been avoided please consider also proposing a recommendation to the PR checklist so that we can avoid it in the future.
⚠️ Looks like this issue was linked to a possible regression on PRODUCTION here
If you are the assigned CME please investigate whether the linked PR caused a regression and leave a comment with the results.
If a production regression has occurred a Root Cause Analysis is required. Please follow the instructions here.
If a regression has occurred and you are the assigned CM follow the instructions here.
If this regression could have been avoided please consider also proposing a recommendation to the PR checklist so that we can avoid it in the future.
⚠️ Looks like this issue was linked to a possible regression on PRODUCTION here
If you are the assigned CME please investigate whether the linked PR caused a regression and leave a comment with the results.
If a production regression has occurred a Root Cause Analysis is required. Please follow the instructions here.
If a regression has occurred and you are the assigned CM follow the instructions here.
If this regression could have been avoided please consider also proposing a recommendation to the PR checklist so that we can avoid it in the future.
⚠️ Looks like this issue was linked to a possible regression on PRODUCTION here
If you are the assigned CME please investigate whether the linked PR caused a regression and leave a comment with the results.
If a production regression has occurred a Root Cause Analysis is required. Please follow the instructions here.
If a regression has occurred and you are the assigned CM follow the instructions here.
If this regression could have been avoided please consider also proposing a recommendation to the PR checklist so that we can avoid it in the future.
⚠️ Looks like this issue was linked to a possible regression on PRODUCTION here
If you are the assigned CME please investigate whether the linked PR caused a regression and leave a comment with the results.
If a production regression has occurred a Root Cause Analysis is required. Please follow the instructions here.
If a regression has occurred and you are the assigned CM follow the instructions here.
If this regression could have been avoided please consider also proposing a recommendation to the PR checklist so that we can avoid it in the future.
Cool, thanks! Just FYI, we have already upgraded to Reanimated 2.10, which includes the useAnimatedKeyboard hook.
@tgolen We have a bunch of issues we are holding on for this one. I'm thinking of linking them all in the OP, for clarify on the scope of what keyboard issues/bugs we're talking about. Is that ok with you?
I'm making pretty good progress on this. I've mostly spent my time ensuring that I can test everything and get a handle on what's broken and what's not.
I've spent a fair amount of time just refactoring our code to use the React-Native KeyboardAvoidingView everywhere consistently. Once it's used everywhere, I will go through all the open issues and see what's reproducible and what isn't. Then, see about digging into the bugs and see if they can be fixed with the RN component, or if we feel like building our own basic component (based off the useAnimatedKeyboard hook) is a route to go.
Circling back to check how things have been going since your last update?
⚠️ Looks like this issue was linked to a possible regression on PRODUCTION here
If you are the assigned CME please investigate whether the linked PR caused a regression and leave a comment with the results.
If a production regression has occurred a Root Cause Analysis is required. Please follow the instructions here.
If a regression has occurred and you are the assigned CM follow the instructions here.
If this regression could have been avoided please consider also proposing a recommendation to the PR checklist so that we can avoid it in the future.
Ah, yes. I had been waiting for some changes from Jules to be merged in another related PR https://github.com/Expensify/App/pull/10648 which prevents me from doing any definitive testing on this feature. This is currently on HOLD until that PR is merged.
I tested this today against my native fix @tgolen, and unfortunately, it's still occurring. I'm going to have to hold my PR again while focusing on higher-priority work. But I'd be happy to test this PR again.
Also, I noticed this bug on the login page, the Android status bar seems to have been hidden (physical device: Nexus 5x, Android 8.0):
What are the next steps on this issue? I peek at the referenced PR and that's still in the works. Does our plan change at all in light of @Julesssss comment above?
OK, I got a little confused by the comments, but I think I understand what's going on. TLDR; I think this should still be on HOLD regardless of the specific issue mentioned in Jules's last comment and regardless of this PR not fixing this problem in Jules's PR.
The issue is that I can't reliably test my changes without having https://github.com/Expensify/App/pull/10648 completed. Once https://github.com/Expensify/App/pull/10648 is merged, it's going to cause me to re-evaluate every single bug reported on this issue. The same thing will happen vice-versa (if my PR is merged before Jules, it will cause all of his code changes to be re-evaluated again).
I prefer having Jules's PR merged first because it is a change to the underlying foundation and my changes are higher up in the view hierarchy, so I think Jules's changes will have a big impact on consistently trying to reproduce all these issues.
Cool, that makes sense to me, but FYI I'm unlikely to be able to prioritize my PR anytime soon. It might be worth prioritizing this PR over mine, on the assumption that my change could be on hold for months.
Ah, that's really good to know. But, why months? I can move forward with mine if that's the case