Fix Android ScrollView not responding to Keyboard events when nested inside a KeyboardAvoidingView #38728
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
facebook-github-bot
added
CLA Signed
Base commit: 7437990 |
|
@ryancat has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator. |
|
This pull request was successfully merged by @andreacassani in c616148. When will my fix make it into a release? | Upcoming Releases |
lunaleaps
pushed a commit
that referenced
this pull request
Aug 7, 2023
…inside a KeyboardAvoidingView (#38728) Summary: Starting from RN 0.72.0, when we nest a ScrollView inside a KeyboardAvoidingView, the ScrollView doesn't respond properly to the Keyboard on Android. https://github.com/facebook/react-native/assets/32062066/a62b5a42-6817-4093-91a2-7cc9e4a315bb This issue is due to a change made in #36104, which was added to fix #32235. That commit changed this line of code to abort the Scroller animation if a new call to the `scrollTo` method was made: https://github.com/facebook/react-native/blob/aab52859a447a8257b106fe307008af218322e3d/packages/react-native/ReactAndroid/src/main/java/com/facebook/react/views/scroll/ReactScrollView.java#L1073 Apparently, this is the same method that scrolls the ScrollView in response to the Keyboard opening on Android. So, here comes my proposal for a fix that doesn't break #36104 and fixes #38152. When we open the Keyboard, the call stack is as follows: - InputMethodManager - AndroidIME - InsetsController - `ReactScrollView.scrollTo` gets called When we use the ScrollView method `scrollTo` directly from the UI, the call stack is different as it goes through: - ReactScrollViewCommandHelper - ReactScrollViewManager - `ReactScrollView.scrollTo` gets called We can move `mScroller.abortAnimation();` from `ReactScrollView.scrollTo` to the `ReactScrollViewManager.scrollTo` method so that it gets called only when we call `scrollTo` from the UI and not when the `scrollTo` method is called by other sources. https://github.com/facebook/react-native/assets/32062066/9c10ded3-08e5-48e0-9a85-0987d62de011 ## Changelog: [ANDROID] [FIXED] - Fixed ScrollView not responding to Keyboard events when nested inside a KeyboardAvoidingView Pull Request resolved: #38728 Test Plan: You can see the issue and the proposed fixes in this repo: [kav-test-android](https://github.com/andreacassani/kav-test-android). Please refer to the `kav_test_fix` folder and to the [readme](https://github.com/andreacassani/kav-test-android/blob/main/README.md). Reviewed By: NickGerleman Differential Revision: D47972445 Pulled By: ryancat fbshipit-source-id: e58758d4b3d5318b947b42a88a56ad6ae69a539c
This was referenced Aug 10, 2023
Kudo
pushed a commit
to expo/react-native
that referenced
this pull request
Aug 21, 2023
…inside a KeyboardAvoidingView (facebook#38728) Summary: Starting from RN 0.72.0, when we nest a ScrollView inside a KeyboardAvoidingView, the ScrollView doesn't respond properly to the Keyboard on Android. https://github.com/facebook/react-native/assets/32062066/a62b5a42-6817-4093-91a2-7cc9e4a315bb This issue is due to a change made in facebook#36104, which was added to fix facebook#32235. That commit changed this line of code to abort the Scroller animation if a new call to the `scrollTo` method was made: https://github.com/facebook/react-native/blob/aab52859a447a8257b106fe307008af218322e3d/packages/react-native/ReactAndroid/src/main/java/com/facebook/react/views/scroll/ReactScrollView.java#L1073 Apparently, this is the same method that scrolls the ScrollView in response to the Keyboard opening on Android. So, here comes my proposal for a fix that doesn't break facebook#36104 and fixes facebook#38152. When we open the Keyboard, the call stack is as follows: - InputMethodManager - AndroidIME - InsetsController - `ReactScrollView.scrollTo` gets called When we use the ScrollView method `scrollTo` directly from the UI, the call stack is different as it goes through: - ReactScrollViewCommandHelper - ReactScrollViewManager - `ReactScrollView.scrollTo` gets called We can move `mScroller.abortAnimation();` from `ReactScrollView.scrollTo` to the `ReactScrollViewManager.scrollTo` method so that it gets called only when we call `scrollTo` from the UI and not when the `scrollTo` method is called by other sources. https://github.com/facebook/react-native/assets/32062066/9c10ded3-08e5-48e0-9a85-0987d62de011 ## Changelog: [ANDROID] [FIXED] - Fixed ScrollView not responding to Keyboard events when nested inside a KeyboardAvoidingView Pull Request resolved: facebook#38728 Test Plan: You can see the issue and the proposed fixes in this repo: [kav-test-android](https://github.com/andreacassani/kav-test-android). Please refer to the `kav_test_fix` folder and to the [readme](https://github.com/andreacassani/kav-test-android/blob/main/README.md). Reviewed By: NickGerleman Differential Revision: D47972445 Pulled By: ryancat fbshipit-source-id: e58758d4b3d5318b947b42a88a56ad6ae69a539c
facebook-github-bot
pushed a commit
that referenced
this pull request
Sep 20, 2023
…zontal scrollviews (#39529) Summary: ### Motivation My main motivation for this is using nested horizontal `Flashlists` inside a vertical `Flashlist`. Like a `RecyclerView`, since my horizontal lists get recycled, if I scroll say the first horizontal list and scroll down, then when this list gets recycled it continues scrolling even if the content is new:  To handle this, I want to call `scrollTo` everytime a new row appears to reset the scroll offset, however I've realized this doesn't stop the scroll momentum ### The bug When scrolling and calling `scrollTo`, scroll momentum should be stopped and we should scroll to where `scrollTo` asked for. All credit goes to tomekzaw for #36104 who fixed it for vertical scrollviews I realized we had the same issue for - horizontal scroll views - when calling `scrollToEnd` | Vertical scrollview (working ✅) | Horizontal scrollview (before fix, not stopping ❌) | Horizontal scrollview (after fix ✅)| |--------|--------|--| | ||| Based on #38728 I kept all those calls to `abortAnimation` on the View Manager ## Changelog: [ANDROID] [FIXED] - Fixed horizontal ScrollView momentum not stopping when calling scrollTo programmatically [ANDROID] [FIXED] - Fixed ScrollView momentum not stopping when calling scrollToEnd programmatically Pull Request resolved: #39529 Test Plan: My test code is [this](https://gist.github.com/Almouro/3ffcfbda8e2239e64bee93985e243000) Basically: - a scrollview with a few elements - some buttons to trigger a `scrollTo` To reproduce the bug, I scroll then click one of the buttons triggering a `scrollTo` I added `react-native@nightly` to my project, and copy pasted `packages/react-native/ReactAndroid/src/main/java/com/facebook/react/views/scroll` folder from this branch to try out my fixes Then tested the following scenarios on Android: | List layout | Method | Not animated | Animated| |--------|--------|--|--| | horizontal | scrollTo | |  | | horizontal | scrollToEnd | | | | vertical | scrollTo | | | | vertical | scrollToEnd |  | | Reviewed By: javache Differential Revision: D49437886 Pulled By: ryancat fbshipit-source-id: 358c9b0eed7dabcbc9b87a15d1a757b414ef514b
ShevO27
pushed a commit
to ShevO27/react-native
that referenced
this pull request
Sep 26, 2023
…zontal scrollviews (facebook#39529) Summary: ### Motivation My main motivation for this is using nested horizontal `Flashlists` inside a vertical `Flashlist`. Like a `RecyclerView`, since my horizontal lists get recycled, if I scroll say the first horizontal list and scroll down, then when this list gets recycled it continues scrolling even if the content is new:  To handle this, I want to call `scrollTo` everytime a new row appears to reset the scroll offset, however I've realized this doesn't stop the scroll momentum ### The bug When scrolling and calling `scrollTo`, scroll momentum should be stopped and we should scroll to where `scrollTo` asked for. All credit goes to tomekzaw for facebook#36104 who fixed it for vertical scrollviews I realized we had the same issue for - horizontal scroll views - when calling `scrollToEnd` | Vertical scrollview (working ✅) | Horizontal scrollview (before fix, not stopping ❌) | Horizontal scrollview (after fix ✅)| |--------|--------|--| | ||| Based on facebook#38728 I kept all those calls to `abortAnimation` on the View Manager ## Changelog: [ANDROID] [FIXED] - Fixed horizontal ScrollView momentum not stopping when calling scrollTo programmatically [ANDROID] [FIXED] - Fixed ScrollView momentum not stopping when calling scrollToEnd programmatically Pull Request resolved: facebook#39529 Test Plan: My test code is [this](https://gist.github.com/Almouro/3ffcfbda8e2239e64bee93985e243000) Basically: - a scrollview with a few elements - some buttons to trigger a `scrollTo` To reproduce the bug, I scroll then click one of the buttons triggering a `scrollTo` I added `react-native@nightly` to my project, and copy pasted `packages/react-native/ReactAndroid/src/main/java/com/facebook/react/views/scroll` folder from this branch to try out my fixes Then tested the following scenarios on Android: | List layout | Method | Not animated | Animated| |--------|--------|--|--| | horizontal | scrollTo | |  | | horizontal | scrollToEnd | | | | vertical | scrollTo | | | | vertical | scrollToEnd |  | | Reviewed By: javache Differential Revision: D49437886 Pulled By: ryancat fbshipit-source-id: 358c9b0eed7dabcbc9b87a15d1a757b414ef514b
This was referenced Jan 4, 2024
This was referenced Feb 28, 2024
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Summary:
Starting from RN 0.72.0, when we nest a ScrollView inside a KeyboardAvoidingView, the ScrollView doesn't respond properly to the Keyboard on Android.
issue.mp4
This issue is due to a change made in #36104, which was added to fix #32235.
That commit changed this line of code to abort the Scroller animation if a new call to the
scrollTomethod was made:react-native/packages/react-native/ReactAndroid/src/main/java/com/facebook/react/views/scroll/ReactScrollView.java
Line 1073 in aab5285
Apparently, this is the same method that scrolls the ScrollView in response to the Keyboard opening on Android.
So, here comes my proposal for a fix that doesn't break #36104 and fixes #38152.
When we open the Keyboard, the call stack is as follows:
ReactScrollView.scrollTogets calledWhen we use the ScrollView method
scrollTodirectly from the UI, the call stack is different as it goes through:ReactScrollView.scrollTogets calledWe can move
mScroller.abortAnimation();fromReactScrollView.scrollToto theReactScrollViewManager.scrollTomethod so that it gets called only when we callscrollTofrom the UI and not when thescrollTomethod is called by other sources.fix.mp4
Changelog:
[ANDROID] [FIXED] - Fixed ScrollView not responding to Keyboard events when nested inside a KeyboardAvoidingView
Test Plan:
You can see the issue and the proposed fixes in this repo: kav-test-android. Please refer to the
kav_test_fixfolder and to the readme.