Skip to content

fix(android): ignore scrim taps once the sheet is closing - #80

Open
giaBaoJS wants to merge 1 commit into
software-mansion-labs:mainfrom
giaBaoJS:fix/android-scrim-dismiss-guard
Open

fix(android): ignore scrim taps once the sheet is closing#80
giaBaoJS wants to merge 1 commit into
software-mansion-labs:mainfrom
giaBaoJS:fix/android-scrim-dismiss-guard

Conversation

@giaBaoJS

@giaBaoJS giaBaoJS commented Sep 4, 2026

Copy link
Copy Markdown
Contributor

Summary

On Android, tapping the scrim of a modal sheet that is already animating to
its closed detent starts the dismissal over again.

onTouchEvent snaps to scrimDismissIndex on every scrim release, without
checking whether that detent is already targetIndex:

https://github.com/software-mansion-labs/react-native-bottom-sheet/blob/249b2cf/android/src/main/java/com/swmansion/reactnativebottomsheet/BottomSheetHostView.kt#L1185-L1194

The scrim stays visible for the whole close animation: updateScrim only
forces it hidden once the sheet is settled at the closed detent
(isTargetingClosedDetent && activeAnimation == null && !isPanning), so
isScrimVisible() is still true mid-close and every further tap is treated
as a fresh dismissal. Each one calls snapToIndex(closeIndex, 0f), which
emits onIndexChange and cancels the in-flight spring to start a new one
from zero velocity, so the sheet also loses the momentum it had.

iOS does not have this: handleScrimPress bails when the closed detent is
already the target.

https://github.com/software-mansion-labs/react-native-bottom-sheet/blob/249b2cf/ios/BottomSheetHostingView.swift#L717-L728

Fix

Add the same closeIndex != targetIndex guard to the Android handler.
A settled closed sheet is unaffected, because its scrim is already hidden and
shouldDismiss is false there.

Repro

Modal sheet with detents={[0, 300]}, index={1} and a scrim, tapping the
scrim three times in quick succession:

onIndexChange onSettle
before 0, 0, 0 0
after 0 0

Test plan

  • New android/src/test/.../BottomSheetViewScrimDismissTest.kt covers both
    the single tap (still dismisses) and the repeated taps (one index change).
  • Reverted the one-line guard and confirmed the repeated-tap test fails with
    expected:<[0]> but was:<[0, 0, 0]> while the single-tap test still passes,
    then restored it and confirmed the whole test:android:unit suite is green.
  • bun run lint, bun run typecheck, ktfmt --google-style.

A scrim tap dismisses the sheet by snapping to the closed detent, but the
Android handler never checks whether that detent is already the target. The
scrim stays visible while the close animates, so every further tap runs
snapToIndex again: it emits another onIndexChange and cancels the running
spring, restarting the close from zero velocity. Tapping the scrim three
times reports three index changes for one dismissal and stalls the sheet.

iOS already guards this in handleScrimPress with targetIndex != closedIndex.
Apply the same guard on Android.
@giaBaoJS
giaBaoJS force-pushed the fix/android-scrim-dismiss-guard branch from 8e7380d to 5939dc1 Compare September 5, 2026 01:32
@giaBaoJS

giaBaoJS commented Sep 5, 2026

Copy link
Copy Markdown
Contributor Author

Fixed the unit test that failed on the previous CI run. Both cases asserted onSettle, which needs the closing spring to finish. androidx.dynamicanimation caches its AnimationHandler per thread, so once another Robolectric view test class has run in the same sandbox the cached Choreographer belongs to a torn-down environment and the spring never advances a frame. Running the class alone passed, running it in the full suite did not, and Gradle's class ordering made it look intermittent.

The tests now assert only the synchronous onIndexChange emissions, which is what the guard changes. Settling behaviour stays covered by the instrumented tests.

Verified locally with bun run test:android:unit: 67 tests, 0 failed on 5 consecutive forced runs. Reverting the one-line guard turns further scrim taps during the close snap emit no extra index change red with expected:<[0]> but was:<[0, 0, 0]> while the single-tap control stays green.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant