fix: keep a sheet presented when snapToPosition targets a temporary position - #2738
Open
KurnosovNikita wants to merge 1 commit into
Open
fix: keep a sheet presented when snapToPosition targets a temporary position#2738KurnosovNikita wants to merge 1 commit into
KurnosovNikita wants to merge 1 commit into
Conversation
…osition `animateToPosition` resolves the index of its target by looking the position up in the detents array, and a `snapToPosition` target is deliberately not one of them. The resulting -1 is written to `nextIndex`, and on completion `animateToPositionCompleted` fires `onClose` for that index — which on a `BottomSheetModal` reaches `unmount()` and `onDismiss`. So `snapToPosition` dismisses a modal and destroys its content, unless the keyboard happens to be SHOWN and the recovery above catches it first. Add the same recovery for a temporary position: report the index the sheet already holds, mirroring `animatedIndex`, which returns `animatedCurrentIndex` rather than -1 while `isInTemporaryPosition`. Reporting the current index also keeps the reposition invisible to `onChange`, which fires only when the index changed. The branch cannot swallow a real dismissal: every close — `close()`, `forceClose()`, pan-down — animates to `closedDetentPosition`, which is excluded, and gesture, keyboard, snap-point-change and mount animations carry a source other than USER. A sheet already at -1 is left alone. Keyed on `source` rather than on `isInTemporaryPosition` because `handleSnapToPosition` sets that flag wherever it is called from — often the JS thread — and then schedules `animateToPosition` on the UI thread; `source` is an argument and is already there. Fixes gorhom#2736
This file contains hidden or 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
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.
Motivation
Fixes #2739 —
snapToPosition()dismisses aBottomSheetModal.Repro Snack: https://snack.expo.dev/Bfj1FfiQhXKRNduNipoJN — present the modal, bump the counter,
tap Reposition 1px;
onChange index=-1thenonDismiss, and the sheet's state is gone.animateToPositionresolves the index of its target by looking the position up in the detentsarray:
A
snapToPosition()target is by definition not a detent — that is what the API is for, andhandleSnapToPositionmarks it withisInTemporaryPosition.value = true. The-1is written tonextIndex, and when the animation completes:On a
BottomSheetModalthat ishandleBottomSheetOnClose→ statusDISMISSED→unmount()→onDismiss. The sheet unmounts and everything in its content is destroyed, with nothing havingasked for a dismissal. The keyboard recovery immediately above is the only thing that ever
prevents it, and only while
KEYBOARD_STATUS.SHOWN.We hit this in production: an Android-only repair that nudges a presented sheet a pixel with
snapToPosition()after returning from the system photo picker. Every nudge dismissed the sheet —five for five in one user's diagnostic bundle, each
onChange {index: -1}landing at exactly thenudged position, taking a filled-in form with it.
The change
One branch, next to the keyboard recovery it mirrors:
A temporary position is not part of the detents array either, and unlike the closed position it is
not a dismissal. Reporting the index the sheet already holds mirrors
animatedIndex, whichreturns
animatedCurrentIndexrather than-1whileisInTemporaryPosition— and it keeps thereposition invisible to
onChangeas well, sinceanimateToPositionCompletedfires it only whenthe index actually changed.
It cannot swallow a genuine dismissal. Every close —
close(),forceClose(), pan-down —animates to
closedDetentPosition, which the branch excludes; gesture, keyboard,snap-point-change and mount animations carry a source other than
USER; and a sheet already at-1is left alone. The gesture path already makes exactly this distinction(
nextPositionIndex === -1 && !isInTemporaryPosition.value), so this only bringsanimateToPositionin line with it.Why
sourceand notisInTemporaryPosition.handleSnapToPositionsets that flag wherever itis called from — commonly the JS thread — and then schedules
animateToPositionon the UI threadvia
runOnUI. A fix depending on that write landing first would be silently ineffective if theordering ever changed.
sourceis an argument: it is already on the UI thread when the branchreads it. Happy to switch to the flag (or to both) if you'd rather keep the concept in one place.
animatedCurrentIndexis added to the callback's dependency list in the same edit.Test plan
yarn typescript— clean.yarn lint— the changed file is clean; the one warning biome reports(
src/hooks/useBoundingClientRect.ts:54,useOptionalChain) is pre-existing onmasteranduntouched here. There is no
testscript at the root, so nothing else to run.patch-packagepatch on 5.2.14: applies clean, that app's suite is green (195 files / 1774 tests), and its own
guard asserts the branch is present in both shipped entry points.
so I have not personally watched a repositioned sheet stay presented on hardware. The evidence I
do have is the source path, the Snack, and five-for-five dismissals in a production diagnostic
bundle that stop being possible with this branch in place. I'll report back here once the device
pass is done — and if you'd rather wait for that before reviewing, that's fair.
I can add the Snack's repro to the example app if you'd like one in the tree — tell me where you'd want it.