feat(Android, FormSheet v5): Add support for fractional detents#4251
feat(Android, FormSheet v5): Add support for fractional detents#4251t0maboro wants to merge 17 commits into
Conversation
There was a problem hiding this comment.
Pull request overview
Adds fractional detent support for the new Android FormSheet (gamma) by mapping a JS detents array into BottomSheetBehavior configuration, while also addressing visual/insets issues (enter-animation flicker on API < 30 and non-edge-to-edge dialog layout).
Changes:
- Plumbs
detentsfrom the RN prop layer (FormSheetHostViewManager/FormSheetHost) intoFormSheetConfigandFormSheetDialogManager. - Introduces
FormSheetDetents+FormSheetBehaviorControllerto validate detent input and apply correspondingBottomSheetBehaviorparams (peek/half-expanded/expanded/maxHeight) with a forced re-layout. - Forces edge-to-edge rendering in
FormSheetDialogand disables Material’s insets animation callback to prevent translationY conflicts with the custom enter animation.
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| android/src/main/java/com/swmansion/rnscreens/gamma/modals/formsheet/FormSheetHostViewManager.kt | Implements detents prop parsing from ReadableArray into a Kotlin list. |
| android/src/main/java/com/swmansion/rnscreens/gamma/modals/formsheet/FormSheetHost.kt | Stores detents on the host and passes them into FormSheetConfig. |
| android/src/main/java/com/swmansion/rnscreens/gamma/modals/formsheet/FormSheetDialogManager.kt | Resolves detents, triggers reconfiguration on open/config change, updates container height, and disables Material insets animation callback. |
| android/src/main/java/com/swmansion/rnscreens/gamma/modals/formsheet/FormSheetDialog.kt | Forces edge-to-edge drawing for consistent dimming + layout across API levels. |
| android/src/main/java/com/swmansion/rnscreens/gamma/modals/formsheet/FormSheetDetents.kt | Adds detent validation + helpers for computing pixel heights and container sizing. |
| android/src/main/java/com/swmansion/rnscreens/gamma/modals/formsheet/FormSheetConfig.kt | Extends config with detents. |
| android/src/main/java/com/swmansion/rnscreens/gamma/modals/formsheet/FormSheetBehaviorController.kt | Stateless mapper from resolved detents to BottomSheetBehavior properties. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
kkafar
left a comment
There was a problem hiding this comment.
Great job. I think that the abstractions you've introduced make sense.
I have series of remarks regarding some details. Let's iron them out before proceeding.
| isFitToContents = true | ||
| peekHeight = detents.firstHeight(sheetAvailableSpace) | ||
| maxHeight = detents.maxAllowedHeight(sheetAvailableSpace) | ||
| // TODO: @t0maboro - in v4 impl the state was passed as a param, consider the same approach |
There was a problem hiding this comment.
Yeah, it might be useful to support initialDetentIndex or whatever prop.
There was a problem hiding this comment.
Not needed in this very moment though. Looks good. You might modify that on is-needed basis.
There was a problem hiding this comment.
There was also some logic in v4 SheetDelegate impl. around the keyboard handling - I haven't verified whether it still applies in the new implementation, so leaving myself a note to revisit this line
| hideNativeDimmingView(window) | ||
| disableNativeWindowAnimation(window) |
There was a problem hiding this comment.
These lines have not beed modified in this PR, but I notice them here for the first time.
They are risky. AFAIK the window object you modify in their implementation is shared between FormSheetDialog view hierarchy, "original view hierarchy" (where react root view resides), and any other dialog opened before/after the form sheet.
Therefore modifying global window settings will affect behaviour of not only the formsheet, but also all other dialogs.
E.g. if you disable the dimming view this way, my prediction is that the <Modal /> component from react-native won't have dimming view anymore. Similarly the animations will be disabled.
Please verify this. In case I'm right (let's hope I'm wrong).
There was a problem hiding this comment.
Okay! We did some research together with @t0maboro and it turns out that each Dialog on Android creates it's own window instance! That's great then.
There was a problem hiding this comment.
https://cs.android.com/android/platform/superproject/+/android-16.0.0_r4:frameworks/base/core/java/android/app/Dialog.java;l=208-209 pasting here a link to the source code if we need to revisit it for some reason
| private fun forceEdgeToEdge() { | ||
| val window = window ?: return | ||
| WindowCompat.setDecorFitsSystemWindows(window, false) | ||
| findViewById<View>(com.google.android.material.R.id.container)?.fitsSystemWindows = false | ||
| findViewById<View>(com.google.android.material.R.id.coordinator)?.fitsSystemWindows = false | ||
| } |
There was a problem hiding this comment.
This is an important assumption. We MUST emphasise this in the component description that right now we enforce / assume edge-to-edge mode being enabled.
| @@ -81,12 +126,27 @@ class FormSheetDialogManager( | |||
| override fun onPreDraw(): Boolean { | |||
| view.viewTreeObserver.removeOnPreDrawListener(this) | |||
| view.translationY = view.height.toFloat() | |||
There was a problem hiding this comment.
Why do we set the view.translationY here? You already do that in runEnterAnimation.
There was a problem hiding this comment.
I've removed this line & it seems to be working just fine. Please see to it.
There was a problem hiding this comment.
I've noticed this, because setting translationY for animation purposes in onPreDraw seems rather out of place.
| override fun onPreDraw(): Boolean { | ||
| view.viewTreeObserver.removeOnPreDrawListener(this) | ||
| view.translationY = view.height.toFloat() | ||
| disableMaterialInsetsAnimationCallback(view) |
There was a problem hiding this comment.
Also this one: why do we do it in onPreDraw? Can't we do it much earlier, e.g. after the BottomSheetBehavior of the formsheet is initialized for the very first time? When does BottomSheetBehavior / BottomSheetDialog register this callback?
There was a problem hiding this comment.
it's registered on the first onLayoutChild call here: https://github.com/material-components/material-components-android/blob/release-1.13/lib/java/com/google/android/material/bottomsheet/BottomSheetBehavior.java#L556
There was a problem hiding this comment.
I'm already changing it in the followup PR to use doOnLayout
| private fun updateNativeContainerHeight() { | ||
| val dialogDecorHeight = dialog.window?.decorView?.height ?: 0 | ||
|
|
There was a problem hiding this comment.
Just a style guide -> if there is nothing to do if dialogDecorHeight == 0 then just early return and remove one level of indentation from the rest of the code.
There was a problem hiding this comment.
This would make more sense to take null as any other API in Android, instead of "empty callback".
| } | ||
|
|
||
| companion object { | ||
| private const val LARGE_DETENT = 1.0 |
There was a problem hiding this comment.
| private const val LARGE_DETENT = 1.0 | |
| private const val LARGE_DETENT_FRACTION = 1.0 |
| // TODO: @t0maboro | ||
| // - a dedicated presentation manager should be introduced as on iOS, | ||
| // - invalidation flags logic should be implemented following other components convention | ||
| val isOpening = newConfig.isOpen && !formSheetConfig.isOpen |
There was a problem hiding this comment.
You already test for that earlier in this function. Please move this value to the top of the scope and reuse it.
Description
Adds configurable detents to the new FormSheets on Android. Due to the native limitations, it allows passing 0-3 detents from JS, which are parsed to BottomSheetBehavior params (peek/half-expanded/expanded/max-height).
I am introducing a
FormSheetBehaviorControllerwhich is a stateless mapper from a resolved detent config toBottomSheetBehavior, forcing a fresh layout pass so the behavior re-settles to the new detent configuration.Additionally, I am making some bugfixes related to or exposed by introducing the sheet detents mechanism.
BottomSheetBehaviorregisters aWindowInsetsAnimationCallbackwhich updatestranslationYand is causing the sheet to jump during the enter animation (I am modifying translation on a custom slide-in animation). We clear that callback after the first layout, because we manage insets ourselves via a fixed container height, preventing the conflict.BottomSheetDialogedge-to-edge when the theme opts in, and the nav bar is translucent; otherwise, the CoordinatorLayout (and our full-bleed dimming view) is inset below the status bar. We now force edge-to-edge on the dialog window on every API level.Closes: https://github.com/software-mansion/react-native-screens-labs/issues/1551
Changes
FormSheetBehaviorControllerresponsible for mapping the raw JS detents array into specificBottomSheetBehaviorproperties.WindowInsetsAnimationCallback, unblocking our customtranslationYenter-animation.onAttachedToWindow()inFormSheetDialogon both the window decor and the internal Material containers (R.id.container,R.id.coordinator), ensuring the dimming view covers the status bar area and the FormSheetContainer space is predictable across different API levels.Before & after - visual documentation
detents.mov
Test plan
Tested on the base example &
Checklist