Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add numberOfPointers to LongPress #3043

Merged
merged 15 commits into from
Aug 23, 2024
Merged

Conversation

m-bert
Copy link
Contributor

@m-bert m-bert commented Aug 13, 2024

Description

LongPressGestureHandler doesn't support multiple pointers. Since default value of numberOfTouchesRequired on iOS is 1, handler gets cancelled if you put more than 1 pointer on it.

This PR adds numberOfPointers modifier for LongPress so that now users can specify how many pointers are needed for LongPress to activate.

Web

On web you could see similar problem, which was probably caused by calling checkDistanceFail inside onPointerMove - if you added another pointer, start position was still at the first pointer position, not in the midpoint. Therefore it was easy to accidentally cancel handler.

macOS

I'm not sure if this feature is required on macOS. In many places we just assume that handlers use only 1 pointer (or 2 in case of pinch/rotation). I've added required property to recognizer, but I have not tested that 😅

Test plan

Tested on slightly modified LongPress example from macOS:

Test code
import { StyleSheet, View } from 'react-native';
import { Gesture, GestureDetector } from 'react-native-gesture-handler';
import Animated, {
  interpolateColor,
  useAnimatedStyle,
  useSharedValue,
  withTiming,
} from 'react-native-reanimated';

const Durations = {
  LongPress: 750,
  Reset: 350,
  Scale: 120,
};

const Colors = {
  Initial: '#0a2688',
  Loading: '#6fcef5',
  Success: '#32a852',
  Fail: '#b02525',
};

export default function LongPressExample() {
  const isPressed = useSharedValue(false);
  const colorProgress = useSharedValue(0);
  const color1 = useSharedValue(Colors.Initial);
  const color2 = useSharedValue(Colors.Loading);

  const animatedStyles = useAnimatedStyle(() => {
    const backgroundColor = interpolateColor(
      colorProgress.value,
      [0, 1],
      [color1.value, color2.value]
    );

    return {
      transform: [
        {
          scale: withTiming(isPressed.value ? 1.2 : 1, {
            duration: Durations.Scale,
          }),
        },
      ],
      backgroundColor,
    };
  });

  const g = Gesture.LongPress()
    .onBegin(() => {
      console.log('onBegin');

      isPressed.value = true;
      colorProgress.value = withTiming(1, {
        duration: Durations.LongPress,
      });
    })
    .onStart(() => console.log('onStart'))
    .onEnd(() => console.log('onEnd'))
    .onFinalize((_, success) => {
      console.log('onFinalize', success);

      isPressed.value = false;

      color1.value = Colors.Initial;
      color2.value = success ? Colors.Success : Colors.Fail;

      colorProgress.value = withTiming(
        0,
        {
          duration: Durations.Reset,
        },
        () => {
          color2.value = Colors.Loading;
        }
      );
    })
    .onTouchesDown(() => console.log('onTouchesDown'))
    .onTouchesMove(() => console.log('onTouchesMove'))
    .onTouchesUp(() => console.log('onTouchesUp'))
    .minDuration(Durations.LongPress)
    .numberOfPointers(2)
    .maxDistance(100);

  return (
    <View style={styles.container}>
      <GestureDetector gesture={g}>
        <Animated.View style={[styles.pressBox, animatedStyles]} />
      </GestureDetector>
    </View>
  );
}

const styles = StyleSheet.create({
  container: {
    flex: 1,
    justifyContent: 'space-around',
    alignItems: 'center',
  },

  pressBox: {
    width: 250,
    height: 250,
    borderRadius: 20,
  },
});

@m-bert m-bert marked this pull request as ready for review August 13, 2024 11:35
Copy link
Contributor

@latekvo latekvo left a comment

Choose a reason for hiding this comment

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

One minor comment, looks great overall 👍

apple/Handlers/RNLongPressHandler.m Show resolved Hide resolved
j-piasecki
j-piasecki previously approved these changes Aug 14, 2024
@j-piasecki j-piasecki dismissed their stale review August 19, 2024 10:14

Does this work?

Copy link
Contributor

@latekvo latekvo left a comment

Choose a reason for hiding this comment

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

LongPress permanently stops working after being cancelled by placing additional finger down while activated.

Tested on Android with numberOfPointers(2), minDuration(2000) and maxDistance(99999999)

Screen.Recording.2024-08-19.at.12.47.29.mov

@m-bert
Copy link
Contributor Author

m-bert commented Aug 19, 2024

LongPress permanently stops working after being cancelled by placing additional finger down while activated.

That's strange, I've just tested it with parameters that you've provided and eI can't see this problem. What I do see though, is that we get onEnd and onFinalize with false triggered, I'll check what's going on.

Are you sure that it permanently stops working?

@latekvo
Copy link
Contributor

latekvo commented Aug 19, 2024

Are you sure that it permanently stops working?

Tested it again, it stops working for the very next to-be activation, but works again after that.

Here's an updated video:

Video

Screen.Recording.2024-08-19.at.14.03.28.mov

And here's the full repro:

Code
import React from 'react';
import { StyleSheet, View } from 'react-native';
import {
  Gesture,
  GestureDetector,
  GestureHandlerRootView,
} from 'react-native-gesture-handler';

export default function EmptyExample() {
  const outer = Gesture.LongPress()
    .numberOfPointers(2)
    .minDuration(1000)
    .maxDistance(99999999)
    // .hitSlop(-100)
    // .onTouchesUp(() => console.log('UP'))
    .onTouchesDown(() => console.log('DOWN'))
    .onStart(() => console.log('START'))
    .onBegin(() => console.log('BEGIN'))
    .onEnd(() => console.log('END'))
    .onFinalize(() => console.log('FINALIZE'));

  return (
    <GestureHandlerRootView>
      <View style={styles.container}>
        <GestureDetector gesture={outer}>
          <View style={styles.nested}>
            {/* <View style={styles.slopper} /> */}
          </View>
        </GestureDetector>
      </View>
    </GestureHandlerRootView>
  );
}

const styles = StyleSheet.create({
  container: {
    flex: 1,
    justifyContent: 'center',
    alignItems: 'center',
    backgroundColor: '#F5FCFF',
  },
  text: {
    width: 300,
    height: 200,
    backgroundColor: 'tomato',
  },
  nested: {
    width: 300,
    height: 300,
    backgroundColor: 'plum',
  },
  slopper: {
    width: 100,
    height: 100,
    margin: 'auto',
    backgroundColor: 'tomato',
  },
});

@m-bert

@latekvo
Copy link
Contributor

latekvo commented Aug 19, 2024

Another possibly related issue - given the following code:

Code
import React from 'react';
import { StyleSheet, View } from 'react-native';
import { Gesture, GestureDetector } from 'react-native-gesture-handler';

export default function EmptyExample() {
  const outer = Gesture.LongPress()
    .numberOfPointers(2)
    .minDuration(1000)
    .maxDistance(99999999)
    .onTouchesUp(() => console.log('UP'))
    .onTouchesDown(() => console.log('DOWN'))
    .onStart(() => console.log('START'))
    .onBegin(() => console.log('BEGIN'))
    .onEnd(() => console.log('END'))
    .onFinalize(() => console.log('FINALIZE'));

  return (
    <View style={styles.container}>
      <GestureDetector gesture={outer}>
        <View style={styles.nested} />
      </GestureDetector>
    </View>
  );
}

const styles = StyleSheet.create({
  container: {
    flex: 1,
    justifyContent: 'center',
    alignItems: 'center',
    backgroundColor: '#F5FCFF',
  },
  text: {
    width: 300,
    height: 200,
    backgroundColor: 'tomato',
  },
  nested: {
    width: 300,
    height: 300,
    backgroundColor: 'plum',
  },
});

When gesture is cancelled by dragging the finger out-of-bounds, the next 2 attempts to activate it fail, before the 3rd one is successful.

Issue only occurs on Android

To reproduce:

  1. press down with one finger, swipe out of bounds
    • 🟢 cancels as anticipated
  2. press down with 2 fingers
    • 🔴 gesture immediately failed
    • instead it should have activated after 1000ms
  3. press down with 2 fingers
    • 🔴 gesture never activates
    • instead it should have activated after 1000ms
  4. press down with 2 fingers
    • 🟢 gesture activates after 1000ms as anticipated
    • successfully activates only after 3rd attempt since the out-of-bounds cancelation

Reproduction video:

Screen.Recording.2024-08-19.at.14.14.09.mov

@m-bert
Copy link
Contributor Author

m-bert commented Aug 19, 2024

Tested it again, it stops working for the very next to-be activation, but works again after that.

Everything works fine in my case. @j-piasecki, could you check it if you can reproduce it?

@m-bert
Copy link
Contributor Author

m-bert commented Aug 19, 2024

When gesture is cancelled by dragging the finger out-of-bounds, the next 2 attempts to activate it fail, before the 3rd one is successful.

In my case only the first attempt fails, second one activates as expected. I'll check that.

@m-bert
Copy link
Contributor Author

m-bert commented Aug 19, 2024

In my case only the first attempt fails, second one activates as expected. I'll check that.

Okay, as I thought, shouldCancelWhenOutside follows different logic. If handler is to be cancelled by this flag, onHandle method is not called (well, it is, but in GestureHandler, not in the specific one). Because of that, currentPointers were not reseted.

To avoid much changes in general logic, I've set currentPointers to 1 when we first start gesture (i.e. it is in UNDETERMINED state).

Let me know if that satisfies you, @j-piasecki @latekvo.

Changes were made in this commit.

@latekvo
Copy link
Contributor

latekvo commented Aug 20, 2024

I confirm all previously failing test cases now behave correctly on my end 👍

@j-piasecki
Copy link
Member

To avoid much changes in general logic, I've set currentPointers to 1 when we first start gesture (i.e. it is in UNDETERMINED state).

Wouldn't onReset be a better pick here?

Copy link
Member

@j-piasecki j-piasecki left a comment

Choose a reason for hiding this comment

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

No idea if this looks better. Feel free to ignore it if you want.

@m-bert
Copy link
Contributor Author

m-bert commented Aug 20, 2024

Wouldn't onReset be a better pick here?

You're right. Changed in d61ed7e.

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.

3 participants