-
-
Notifications
You must be signed in to change notification settings - Fork 595
fix(Android, Stack): Moving formsheet above keyboard #3248
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
Conversation
39c6b3e to
bda8134
Compare
|
Moving back to draft for a while, because I noticed some issues with autofocus and textinput below API 30 |
kligarski
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I had already started the review before you moved the PR to draft so I'll finish it.
This looks great! I have some remarks/questions:
- Just to be sure, this is not a regression, right? (not respecting navigation bar insets)
Screen_recording_20250929_140451.mp4
- Do you think we can do something about the gap between keyboard and formsheet when they're both hiding at the same time? Should we create a ticket to investigate it in the future?
Screen_recording_20250929_135928.mp4
- We should probably revisit #2925 to fix this, right?
Screen_recording_20250929_140440.mp4
android/src/main/java/com/swmansion/rnscreens/bottomsheet/SheetDelegate.kt
Outdated
Show resolved
Hide resolved
|
@kligarski answering your questions:
|
I'm not sure if we're talking about the same thing: I meant the text input being placed behind navigation bar (3-button navigation at the bottom) in first
|
Sorry, I read all together before answering and idk why I started thinking that 1 and 3 are connected. Good that we have an alignment on this one, but we should rethink whether this is expected now |
kkafar
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm reviewing only code right now. I'll check the runtime tomorrow.
android/src/main/java/com/swmansion/rnscreens/ScreenStackFragment.kt
Outdated
Show resolved
Hide resolved
android/src/main/java/com/swmansion/rnscreens/ScreenStackFragment.kt
Outdated
Show resolved
Hide resolved
android/src/main/java/com/swmansion/rnscreens/ScreenStackFragment.kt
Outdated
Show resolved
Hide resolved
android/src/main/java/com/swmansion/rnscreens/ScreenStackFragment.kt
Outdated
Show resolved
Hide resolved
android/src/main/java/com/swmansion/rnscreens/ScreenStackFragment.kt
Outdated
Show resolved
Hide resolved
android/src/main/java/com/swmansion/rnscreens/ScreenStackFragment.kt
Outdated
Show resolved
Hide resolved
| val startValueCallback = { _: Number? -> screen.height.toFloat() } | ||
| val evaluator = ExternalBoundaryValuesEvaluator(startValueCallback, { 0f }) | ||
|
|
||
| return ValueAnimator.ofObject(evaluator, screen.height.toFloat(), 0f).apply { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reading this for the first time & I don't get why on slide in animator the start value is height and target value is 0, at least passed here. I also see that there is an evaluator.
I'll update the comment if I get this
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wrote this code intitially, but I do not remember that. 😄
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh, is it because the sheet has already target position & we translate it back to the animation start position? That might be it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The initial sheet position is when the sheet bottom edge is equal to device's bottom edge, we're translating Y with a positive value to hide the sheet under the bottom edge of the device
| val detents = screen.sheetDetents | ||
| if (detents.isEmpty()) { | ||
| throw IllegalStateException("[RNScreens] Cannot determine sheet detent - detents list is empty") | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should have separate data structure for detents, making sure of this invariant. Let's create ticket for this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| throw IllegalStateException("[RNScreens] Cannot determine sheet detent - detents list is empty") | ||
| } | ||
|
|
||
| val detentValue = detents[detents.size - 1].coerceIn(0.0, 1.0) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't get this part. Why do you take value of the largest detent here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This method is used only when there is keyboard present, is that right? If so, let's name it appropriately, cause right now it's not obvious at all.
e.g. computeSheetOffsetYWithIMEPresent or something.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
also this place deserves its own comment, because it really defines the behaviour of the sheet -> that it expands to max detent when the keyboard shows, right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes, the sheet expands to its max detent; the purpose of this code is to determine whether we're able to show the full sheet or if we need to cover it partially with the keyboard, definitely deserves some description, giving the information about the final offset from bottom, to which we should animate
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
|
What about this behaviour? It goes up, even if there is no IME present? Can we detect such case? Screen.Recording.2025-11-06.at.11.10.01.mov |
|
Also this is messed up a bit. Is this particular behaviour handled in the safe area view PR? Screen.Recording.2025-11-06.at.11.12.16.mov |
kkafar
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just last code remarks.
As discussed internally, let's proceed with that in the PR with FormSheet - SAV fixes |
## Description Followup for: #3248 (comment) Note: @kkafar I'm aware that it differs from what we discussed internally, but imo it may make sense to do it in the way I'm proposing here, let me know what do you think Fixes software-mansion/react-native-screens-labs#537 ## Changes - Extracted sheet animation code to a separate class ## Test code and steps to reproduce Regression testing on `Test3248` ## Checklist - [x] Included code example that can be used to test this change - [x] Ensured that CI passes
kkafar
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good. Great job. Thank you!



Description
This PR improves the way the BottomSheet on Android handles the on-screen keyboard (IME). Previously, the keyboard was not respected properly - opening the keyboard could cause content to be partially or fully obscured due to incorrect handling of insets.
With this update:
There are two key touchpoints where we react to IME inset changes:
onProgresscallback: This listens to dynamic IME inset changes outside of enter/exit animations — e.g. when the keyboard resizes or changes without the BottomSheet visibility changing.Priority is given to the animation layer for transforming the component's position, while the onProgress callback serves as a fallback when animations are not active. This prioritization enables us to maintain a smooth and consistent visual appearance across all lifecycle scenarios of the BottomSheet.
Fixes: #3181
Changes
KeyboardDidHidestate forfitToContentsonProgresscallback from insets listener.Screenshots / GIFs
Here you can add screenshots / GIFs documenting your change.
You can add before / after section if you're changing some behavior.
Before
test3248-main.mov
After
test3248.mov
Test code and steps to reproduce
Tested on android with
Test3248Tested with API levels 29 or higher, because I noticed another issue regarding bottom sheets on 28: https://github.com/software-mansion/react-native-screens-labs/issues/480
Checklist