[Web] Fix handling of enabled prop

[m-bert]

Description

Right now, if one changes enabled property of handlers to false, they can never activate again. In the following example:

const [enabled, setEnabled] = useState(false);
const pan = Gesture.Pan().enabled(enabled)

gesture will never become active, even if enabled changes to true. Same happens when enabled is set to true and then changed to false and true again.

This happens because we set enabled to true by default in setGestureConfig, thus the following check:

if (config.enabled !== undefined && this.enabled !== config.enabled) { ... }

passes only when config.enabled is false.

To fix this, I've set enabled to null by default, and then if config.enabled is defined, it is assigned given value. Else default will be true.

Alternative approach would be to follow Android implementation and always react to changes in enabled even in cases like false $\rightarrow$ false and true $\rightarrow$ true.

Test plan

Tested on the following code:

import {
  GestureHandlerRootView,
  Gesture,
  NativeDetector,
  useGesture,
  SingleGestureName,
  GestureDetector,
} from 'react-native-gesture-handler';
import { Pressable, StyleSheet, View, Text } from 'react-native';
import { useState } from 'react';
import { useSharedValue } from 'react-native-reanimated';

export default function App() {
  const [stateEnabled, setStateEnabled] = useState(false);
  const svEnabled = useSharedValue(false);

  const oldPan = Gesture.Pan()
    .enabled(stateEnabled)
    .onUpdate(() => {
      console.log('Old API');
    });

  const newPan = useGesture(SingleGestureName.Pan, {
    enabled: svEnabled,

    onUpdate: () => {
      'worklet';
      console.log('New API');
    },
  });

  return (
    <GestureHandlerRootView style={[styles.container, styles.center]}>
      <View style={[styles.boxContainer, styles.center]}>
        <Pressable
          onPress={() => setStateEnabled((prev) => !prev)}
          style={[styles.button, styles.center]}>
          <Text>Toggle old API</Text>
        </Pressable>
        <GestureDetector gesture={oldPan}>
          <View style={[styles.box, styles.center]} />
        </GestureDetector>
      </View>
      <View style={[styles.boxContainer, styles.center]}>
        <Pressable
          onPress={() => {
            svEnabled.value = !svEnabled.value;
          }}
          style={[styles.button, styles.center]}>
          <Text>Toggle new API</Text>
        </Pressable>
        <NativeDetector gesture={newPan}>
          <View style={[styles.box, styles.center]} />
        </NativeDetector>
      </View>
    </GestureHandlerRootView>
  );
}

const styles = StyleSheet.create({
  center: {
    display: 'flex',
    justifyContent: 'space-around',
    alignItems: 'center',
  },
  container: {
    flex: 1,
  },
  boxContainer: {
    gap: 20,
  },
  button: {
    width: 150,
    height: 40,
    borderRadius: 20,
    backgroundColor: 'pink',
  },
  box: {
    width: 150,
    height: 150,
    backgroundColor: 'lightblue',
    borderRadius: 20,
  },
});

[j-piasecki]

If I understand correctly, the issue is that we reset the config here, but if the gesture was disabled before, we don't reattach the event listeners. Shouldn't we also try to attach listeners when resetting the config? At first glance, it looks like it could simplify this a lot (especially if it were to be done in a setter).

[m-bert]

Shouldn't we also try to attach listeners when resetting the config?

But in that case if we have config that has enabled set to false we will first attach and then immediately detach listeners. Is that something that we want?

[j-piasecki]

Can we reverse that and start with detached listeners which are then attached when config has enabled !== false?

[m-bert]

So instead of attaching listeners in resetConfig, we should detach them, and then attach if enabled !== false?

[m-bert]

If I understand correctly, the issue is that we reset the config here, but if the gesture was disabled before, we don't reattach the event listeners.

Also, I don't know whether we will have any other logic that depends on enabled change, but I think that we should say that what the issue really is is that in current setup it is impossible to detect that enabled has changed to true*. This may (or may not) cause other troubles in the future.

* If we go through resetConfig first, with SharedValue it should be fine.

[m-bert]

If I understand correctly, you want to react only to config.enabled, without comparing it to previous value is that true? If so, what in situation where two subsequent updateGestureConfig will have enabled: true? I guess we could check if listeners are already attached and do nothing in that case, but I think it solves only part of the problem.

Right now, onEnabledChange in GestureHandlerWebDelegate does some other things, i.e.:

    this.setUserSelect();
    this.setTouchAction();
    this.setContextMenu();

So even if we solve problem with listeners, we still have issues where those functions won't be called because we can't detect when enabled value changes to true.

[j-piasecki]

If I understand correctly, you want to react only to config.enabled, without comparing it to previous value is that true?

No - IIRC, currently, we start every gesture with enabled: true and listeners attached. We could start every gesture with enabled: false and listeners detached. Then compare config.enabled ?? true with the previous value and attach/detach if needed.

[m-bert]

But wouldn't it make it the same problem, but with false instead of true? If we make false default value, and then in updateGestureConfig we receive config.enabled = false, we won't trigger onEnabledChanged, even if user changed it from true.

[j-piasecki]

I'm not sure I see it. Could you do a step-by-step on what needs to happen to get that?

[m-bert]

We could start every gesture with enabled: false and listeners detached.

I assume this means that when we reset handler, we set this.enabled into false. Let's say that we have this example:

const [enabled, setEnabled] = useState(true);
const pan = Gesture.Pan().enabled(enabled)

The flow here is:

1. Reset this.enabled to false (via calling setGestureConfig)
2. Compare config.enabled with this.enabled

At first this works fine, we compare config.enabled which is true with this.enabled which is false. But now, when we change enabled to false, we have the same comparison, but both values are false, so onEnabledChange will not be called as we did not detect change from true to false - we lost that information in resetConfig.