Skip to content

fix(android): declare gesture simultaneousness bidirectionally between content pan and scroll#2675

Open
ronnymajani wants to merge 1 commit into
gorhom:masterfrom
ronnymajani:fix/android-bidirectional-gesture-simultaneousness
Open

fix(android): declare gesture simultaneousness bidirectionally between content pan and scroll#2675
ronnymajani wants to merge 1 commit into
gorhom:masterfrom
ronnymajani:fix/android-bidirectional-gesture-simultaneousness

Conversation

@ronnymajani
Copy link
Copy Markdown

Motivation

On Android, the inner scrollable inside a BottomSheet cannot be scrolled when the sheet is first opened. Dragging vertically inside the scrollable does nothing — the content pan gesture cancels the native scroll on every touch. Curiously, focusing then blurring a TextInput inside the sheet "fixes" the problem for the remainder of that session, after which the scroll works normally.

Diagnostic instrumentation showed the symptom precisely: onScrollBeginDrag fires in succession with zero onScroll events between them — i.e. RNGH keeps starting the scroll and immediately canceling it in favor of the pan.

Root cause

BottomSheetDraggableView creates the content pan gesture with:

gesture = gesture.simultaneousWithExternalGesture(simultaneousHandlers as never);

…where simultaneousHandlers is built from nativeGestureRef, refreshControlGestureRef, and any user-provided handlers. It does not include the inner scrollable's Gesture.Native().

Meanwhile, createBottomSheetScrollableComponent does declare the relation on the scroll side:

Gesture.Native()
  .simultaneousWithExternalGesture(draggableGesture)   // declares pan as simultaneous
  .shouldCancelWhenOutside(false)

So the simultaneousness is only declared on one side. RNGH on Android requires the declaration on both sides — on iOS the one-sided declaration appears to be enough, which is why this bug only manifests on Android.

The "focus then blur an input" workaround presumably forces RNGH to re-resolve its internal gesture tree, at which point the declarations end up coordinated.

What this PR does

Introduces a NativeScrollGestureContext that lets the inner scrollable register its Gesture.Native() back up to the outer BottomSheetDraggableView. The pan then includes that gesture in its simultaneousWithExternalGesture(...) array, completing the bidirectional declaration.

Three notable implementation choices, each captured in inline JSDoc on the changed files:

1. State, not ref

simultaneousWithExternalGesture resolves its argument at gesture-handler registration time. A ref whose .current is null at that moment results in no relation being established, and subsequent ref mutations are not picked up — RNGH does not lazily re-resolve.

A useState is therefore required: when the inner scrollable mounts and registers its gesture, the resulting state update re-renders the BottomSheet, recomputes the pan's useMemo, and re-attaches its GestureDetector with the relation now in place.

2. Setter is idempotent (first non-null wins)

Without a guard, the setter would loop:

set scroll → state update → pan re-creates → new pan flows into the scrollable via BottomSheetDraggableContext → scrollable's useMemo rebuilds scrollableGesture → effect re-fires with the new identity → set scroll → …

(Reproducible: yields a hard Maximum update depth exceeded after ~50 iterations.)

Once a non-null gesture is registered, subsequent non-null registrations are no-ops. The bidirectional relation only needs to be established once; further updates would only churn the gesture tree without changing semantics.

3. Registration effect has no deps-change cleanup; unmount cleanup lives in its own effect

For the same loop-avoidance reason: nulling the registration on every deps change would re-render the pan without the scroll, then re-register, then re-render the pan with the scroll, ad infinitum. The deps-change branch of the cleanup is dropped; a separate unmount-only effect (using a ref to capture the latest context) still clears the registration when the scrollable fully unmounts (e.g. when the sheet closes).

Files changed

File Change
src/contexts/gesture.ts Adds NativeScrollGestureContext with {nativeScrollGesture, setNativeScrollGesture}. JSDoc explains the why.
src/components/bottomSheet/BottomSheet.tsx Provides the context with a useState-backed value and an idempotent setter.
src/components/bottomSheetDraggableView/BottomSheetDraggableView.tsx Consumes the context and adds nativeScrollGesture to the pan's simultaneousHandlers.
src/components/bottomSheetScrollable/createBottomSheetScrollableComponent.tsx Registers its scrollableGesture via the context on mount; clears on unmount.

No public API changes. No type changes. No behavior change for any platform/scenario where the scroll already worked.

Verification

  • yarn typescript — clean
  • yarn lint --error-on-warnings — clean on the four modified files (the pre-existing warning in src/hooks/useBoundingClientRect.ts is on master and untouched here)
  • Manual test on a physical Android device with the example app and with my own production app: scroll inside the sheet now works on first open, no need for the focus/blur reset. iOS behavior unchanged.

Related

This bug is the underlying cause for several long-standing reports about scroll/pan conflicts in BottomSheet content on Android (notably the "scroll only works after focusing an input" pattern).

Happy to break this into smaller commits, adjust the comments, or rework the context shape if you'd prefer a different approach.

…n content pan and scroll

On Android, the inner scrollable cannot be scrolled on first sheet open
because the content pan gesture cancels the native scroll gesture. The
existing code only declares simultaneousness on the scroll side
(`Gesture.Native().simultaneousWithExternalGesture(pan)`); the pan does
not declare `simultaneousWithExternalGesture(scroll)`. RNGH on Android
requires both sides to declare the relation.

This change wires the inner scrollable's native gesture back up to
`BottomSheetDraggableView` via a new `NativeScrollGestureContext`, so
the pan can include it in its `simultaneousHandlers` array. State (not
ref) is required, and the setter is made idempotent to avoid a
re-render loop.
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