You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
{{ message }}
Repository navigation
Remove unused enabled listener from useJSResponderHandler - #4663
#4158 added a SharedValue listener to useJSResponderHandler: whenever a SharedValue passed as enabled changed, it bumped a state counter so that the responder handler was re-created. The handler reads enabled when the touch starts (maybeUnpackValue returns .value), and it already did so when the listener was added, so the counter never changed the result. It only cost a state, a ref and an effect with UI-thread scheduling for every detector, plus a re-render of the detector on every flip.
This PR removes the listener and the now unused getEnabledSharedValues. The hook is now a context read and a single callback, and without a ScrollView above it returns a stable no-op instead of a new function on every render. This also applies to NativeDetector, which has run the listener since #4158.
Test plan
New Jest test that flips a SharedValue enabled without a re-render and checks that the marking follows it. It fails if enabled is evaluated during render.
Android emulator and iOS simulator, identical results before and after:
the Fix handling keyboard should persists taps #4158 example (Keyboard Should Persist Taps), all three modes with GH Pressable, Touchable and useTapGesture: press with the keyboard open, then a tap on the blank area,
the code below, with enabled flipped from JS and from the UI thread: disabled closes the keyboard and the tap doesn't fire, enabled keeps the keyboard open and the tap fires.
Tested on the following code:
importReact,{memo,useCallback,useEffect,useState}from'react';import{Keyboard,Pressable,StyleSheet,Text,TextInput,View,}from'react-native';import{GestureDetector,ScrollView,useTapGesture,VirtualGestureDetector,}from'react-native-gesture-handler';import{typeSharedValue,useSharedValue}from'react-native-reanimated';import{scheduleOnRN,scheduleOnUI}from'react-native-worklets';// Rows are memoized and never re-render. Flip `enabled` from JS or the UI// thread, focus the input, tap a row. Disabled: keyboard closes, tap doesn't// fire. Enabled: keyboard stays open, tap fires.typeRowProps={label: string;virtual: boolean;enabled: SharedValue<boolean>;onTap: ()=>void;};constRow=memo(functionRow({ label, virtual, enabled, onTap }: RowProps){consttap=useTapGesture({ enabled,runOnJS: true,onDeactivate: onTap});constDetector=virtual ? VirtualGestureDetector : GestureDetector;return(<Detectorgesture={tap}><Viewstyle={styles.row}><Text>{label}</Text></View></Detector>);});exportdefaultfunctionEmptyExample(){const[keyboardVisible,setKeyboardVisible]=useState(false);const[taps,setTaps]=useState({native: 0,virtual: 0});const[mirror,setMirror]=useState({native: true,virtual: true});constnativeEnabled=useSharedValue(true);constvirtualEnabled=useSharedValue(true);useEffect(()=>{constshow=Keyboard.addListener('keyboardDidShow',()=>setKeyboardVisible(true));consthide=Keyboard.addListener('keyboardDidHide',()=>setKeyboardVisible(false));return()=>{show.remove();hide.remove();};},[]);constonNativeTap=useCallback(()=>setTaps((t)=>({ ...t,native: t.native+1})),[]);constonVirtualTap=useCallback(()=>setTaps((t)=>({ ...t,virtual: t.virtual+1})),[]);constreportMirror=(key: 'native'|'virtual',value: boolean)=>setMirror((m)=>({ ...m,[key]: value}));constsetFromJS=(key: 'native'|'virtual',value: boolean)=>{(key==='native' ? nativeEnabled : virtualEnabled).value=value;reportMirror(key,value);};constsetFromUI=(key: 'native'|'virtual',value: boolean)=>{constsv=key==='native' ? nativeEnabled : virtualEnabled;scheduleOnUI(()=>{'worklet';sv.value=value;scheduleOnRN(reportMirror,key,value);});};constcontrols: [string,()=>void][]=[['JS N off',()=>setFromJS('native',false)],['JS N on',()=>setFromJS('native',true)],['UI N off',()=>setFromUI('native',false)],['UI N on',()=>setFromUI('native',true)],['JS V off',()=>setFromJS('virtual',false)],['JS V on',()=>setFromJS('virtual',true)],['UI V off',()=>setFromUI('virtual',false)],['UI V on',()=>setFromUI('virtual',true)],];return(<Viewstyle={styles.container}><Textstyle={styles.status}testID="status">
keyboard: {keyboardVisible ? 'OPEN' : 'closed'} | N
{mirror.native ? 'on' : 'off'} taps {taps.native} | V
{mirror.virtual ? 'on' : 'off'} taps {taps.virtual}</Text><Viewstyle={styles.controls}>{controls.map(([label,action])=>(<Pressablekey={label}style={styles.control}onPress={action}testID={label}><Textstyle={styles.controlText}>{label}</Text></Pressable>))}</View><ScrollViewkeyboardShouldPersistTaps="handled"><TextInputstyle={styles.input}placeholder="focus me"testID="input"/><Rowlabel="N: GestureDetector Tap"virtual={false}enabled={nativeEnabled}onTap={onNativeTap}/><Rowlabel="V: VirtualGestureDetector Tap"virtualenabled={virtualEnabled}onTap={onVirtualTap}/></ScrollView></View>);}conststyles=StyleSheet.create({container: {flex: 1},status: {fontSize: 14,padding: 6,textAlign: 'center'},controls: {flexDirection: 'row',flexWrap: 'wrap',gap: 6,paddingHorizontal: 8,},control: {paddingVertical: 6,paddingHorizontal: 8,borderRadius: 6,backgroundColor: '#ccd',},controlText: {fontSize: 12},input: {marginHorizontal: 16,marginTop: 8,padding: 8,borderWidth: 1,borderColor: '#888',borderRadius: 8,},row: {height: 44,marginHorizontal: 16,marginTop: 8,backgroundColor: '#cde',borderRadius: 8,justifyContent: 'center',alignItems: 'center',},});
No actionable comments were generated in the recent review.
Summary
Summary by CodeRabbit
Bug Fixes
Gesture responder handling now reflects changes to a gesture’s enabled state when a touch starts, even if the hook hasn’t re-rendered.
The responder handler now checks gesture support when a touch starts. The change removes shared-value listener tracking and adds a test for enabled-value changes without a hook re-render.
The handler checks gesture support at touch-start time and uses a shared no-op handler when the context is absent. The shared-value listener helper is removed. The test checks the responder result as the enabled value changes without a hook re-render.
Included review availability: This review used your included allowance. Your plan provides up to 8 included reviews per hour; 7 remain after this review.
The reason will be displayed to describe this comment to others. Learn more.
🟡 Changes recommended
Hard-coding native-gesture handling to false in the public InterceptingGestureDetector can cause enclosing ScrollViews to dismiss the keyboard after handled native-control taps; the opt-out should be limited to the ScrollView wrapper.
This PR simplifies v3 gesture responder handling by removing SharedValue listeners and relying on touch-time enabled reads, reducing detector state and re-renders.
Changes:
Removes listener bookkeeping and the unused getEnabledSharedValues helper; uses a stable no-op outside responder context.
Consolidates the intercepting detector and changes its native gestures to never mark taps as handled.
Adds coverage for SharedValue changes without re-rendering and updates intercepting-detector expectations.
The reason will be displayed to describe this comment to others. Learn more.
Out of scope for this PR (the line comes from #4530), and intentional there. On main InterceptingGestureDetector never marked taps, so this isn't a regression. We tried counting its Native gesture by default and it kept the keyboard open on every tap on pagers wrapped in an InterceptingGestureDetector (verified on Android and iOS), with no way for users to opt out. Its Native gesture belongs to the container hosting virtual children; native tap targets should use GestureDetector, which still marks them.
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
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.
Description
#4158 added a SharedValue listener to
useJSResponderHandler: whenever a SharedValue passed asenabledchanged, it bumped a state counter so that the responder handler was re-created. The handler readsenabledwhen the touch starts (maybeUnpackValuereturns.value), and it already did so when the listener was added, so the counter never changed the result. It only cost a state, a ref and an effect with UI-thread scheduling for every detector, plus a re-render of the detector on every flip.This PR removes the listener and the now unused
getEnabledSharedValues. The hook is now a context read and a single callback, and without aScrollViewabove it returns a stable no-op instead of a new function on every render. This also applies toNativeDetector, which has run the listener since #4158.Test plan
enabledwithout a re-render and checks that the marking follows it. It fails ifenabledis evaluated during render.Pressable,TouchableanduseTapGesture: press with the keyboard open, then a tap on the blank area,enabledflipped from JS and from the UI thread: disabled closes the keyboard and the tap doesn't fire, enabled keeps the keyboard open and the tap fires.Tested on the following code: