Skip to content

Conversation

akwasniewski
Copy link
Contributor

Description

In NativeDetector native gesture failed to reattach when child changed.

Test plan

Add a logging message in attach/detach in android/.../react/RNGestureHandlerRegistry.kt and apple/RNGestureHandlerRegistry.m
`

import * as React from 'react';
import { StyleSheet, Text, View, ScrollView, Button } from 'react-native';
import {
  GestureHandlerRootView,
  NativeDetector,
  useGesture,
} from 'react-native-gesture-handler';

export default function App() {
  const items = Array.from({ length: 5 }, (_, index) => `Item ${index + 1}`);
  const [enabled, setEnabled] = React.useState(true);
  const gesture = useGesture('NativeViewGestureHandler', {
    onBegin: (e) => {
      console.log('onBegin');
    },
    onStart: (e) => {
      console.log('onStart');
    }
  });

  const SV1 = () => (
    <ScrollView style={styles.scrollView1}>
      {items.map((item, index) => (
        <View key={index} style={styles.item}>
          <Text style={styles.text}>{item}</Text>
        </View>
      ))}
    </ScrollView>
  );

  const SV2 = () => (
    <ScrollView style={styles.scrollView2}>
      {items.map((item, index) => (
        <View key={index} style={styles.item}>
          <Text style={styles.text}>{item}</Text>
        </View>
      ))}
    </ScrollView>
  );

  return (
    <GestureHandlerRootView
      style={{ flex: 1, backgroundColor: 'white', paddingTop: 100 }}>
      <Button
        title="Swap the child"
        onPress={() => {
          setEnabled(!enabled);
        }}
      />
      <NativeDetector gesture={gesture}>
        {enabled ? <SV1 /> : <SV2 />}
      </NativeDetector >
    </GestureHandlerRootView >
  );
}

const styles = StyleSheet.create({
  scrollView1: {
    backgroundColor: 'pink',
    marginHorizontal: 20,
  },
  scrollView2: {
    backgroundColor: 'lightblue',
    marginHorizontal: 20,
  },

  item: {
    flexDirection: 'row',
    justifyContent: 'center',
    alignItems: 'center',
    padding: 20,
    margin: 2,
    backgroundColor: 'white',
    borderRadius: 10,
  },
  text: {
    fontSize: 20,
    color: 'black',
  },
});

@akwasniewski akwasniewski marked this pull request as ready for review August 12, 2025 09:46
@akwasniewski akwasniewski changed the title @akwasniewski/fix native gesture reattach Fix native gesture reattach Aug 12, 2025
@akwasniewski akwasniewski requested a review from m-bert August 12, 2025 10:36
Copy link
Contributor

@m-bert m-bert left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can see that android got a bit "out of sync" with iOS, can we make it so that one is analogous to another?

@akwasniewski akwasniewski requested a review from m-bert August 18, 2025 11:16
Comment on lines 178 to 179
if (!self.subviews[0])
return;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Correct me if I'm wrong, but this should never happen with NativeHandler and if it does, I believe we would like to throw an error instead of silent fail.

Copy link
Contributor Author

@akwasniewski akwasniewski Aug 19, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That was my attempt to synchronise android and iOS as on Android we had this, on Android there is a comment that explains it. I'll move it to the right place

It might happen that attachHandlers will be called before children are added into view hierarchy

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, you are right it can really only happen in the attachHandlers, thus it's better to have a check only there. Fixed in 4916ba5.

Comment on lines -78 to -79
// It might happen that `attachHandlers` will be called before children are added into view hierarchy. In that case we cannot
// attach `NativeViewGestureHandlers` here and we have to do it in `addView` method.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't really see the point of moving this comment and removing part of its information. It was written in this place so that when you see adding to set instead of attaching handler you know why it happens.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I thought, that the set would have been used regardless as we need it for reattach, so it wasn't the place for this comment. But ok, I don't have a strong opinion thus I restored it and synced ios, c4f18a7d

Co-authored-by: Michał Bert <[email protected]>
Copy link
Contributor

@m-bert m-bert left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good job! 🚀

@akwasniewski akwasniewski merged commit cd27bfd into next Aug 19, 2025
6 checks passed
@akwasniewski akwasniewski deleted the @akwasniewski/fix-native-gesture-reattach branch August 19, 2025 10:58
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.

2 participants