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

Swipes starting at edges (x or y === 0) are disregarded #182

Closed
merrywhether opened this issue Jun 25, 2020 · 2 comments · Fixed by #198 or #331
Closed

Swipes starting at edges (x or y === 0) are disregarded #182

merrywhether opened this issue Jun 25, 2020 · 2 comments · Fixed by #198 or #331
Assignees
Labels
bug 🐛 good first issue hopefully straightforward fix

Comments

@merrywhether
Copy link

merrywhether commented Jun 25, 2020

Describe the bug
Swipes originating from coordinates with x = 0 (left edge) or y = 0 (top edge) will not be captured. I only found this by writing tests against my usage of this library as I believe this behavior is really hard to replicate in real world use cases. It's caused by a improper falsy check on this line:
https://github.com/FormidableLabs/react-swipeable/blob/master/src/index.js#L73

I think it should be:

if (state.xy[0] === undefined || state.xy[1] === undefined || ...

Steps or Sandbox to reproduce
In my tests I have

    act(() => {
      // swipe down
      document.dispatchEvent(
        new TouchEvent('touchstart', {
          touches: [
            {
              clientX: 1,
              clientY: 1,
            } as any,
          ],
        }),
      );
      document.dispatchEvent(
        new TouchEvent('touchmove', {
          touches: [
            {
              clientX: 1,
              clientY: 20,
            } as any,
          ],
        }),
      );
      document.dispatchEvent(
        new TouchEvent('touchend', {
          touches: [
            {
              clientX: 1,
              clientY: 20,
            } as any,
          ],
        }),
      );
    });
    expectOnlyHandlerCalled(initialProps.inputHandler, 'onSwipe');
    expect(initialProps.inputHandler.onSwipe).toHaveBeenCalledWith('Down');

As-is, the test passes. But if I change all of the clientX values to 0 (or the initial clientY value to 0), the test fails because no swipe was detected, with the code bailing out in onMove at that if-check.

Expected behavior
Since the screen is a 0-based grid, it seems like touches starting on the edges should be handled?

@hartzis
Copy link
Collaborator

hartzis commented Jun 26, 2020

@merrywhether nice find. This has existed since this libraries inception.

You're totally right that the actual likelihood that a real user has somehow started a swipe at 0 is extremely low.

I do believe your investigation is correct for where the bug is happening.

if (!state.xy[0] || !state.xy[1] || (event.touches && event.touches.length > 1)) {

We also need to update the initialState.xy here to be [].

const initialState = {
xy: [0, 0],

Please feel free to create a PR if you'd like. If not I may be able to get around to this soonish, but my current priorities are working on v6

@hartzis hartzis added good first issue hopefully straightforward fix help wanted and removed help wanted labels Jun 26, 2020
@upatel32 upatel32 self-assigned this Aug 18, 2020
@hartzis hartzis linked a pull request Aug 19, 2020 that will close this issue
@hartzis
Copy link
Collaborator

hartzis commented Sep 7, 2020

Coming in v6

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug 🐛 good first issue hopefully straightforward fix
Projects
None yet
3 participants