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

Drag movement does not include pre-threshold movement #314

Open
1 task done
robatwilliams opened this issue May 31, 2021 · 7 comments
Open
1 task done

Drag movement does not include pre-threshold movement #314

robatwilliams opened this issue May 31, 2021 · 7 comments
Assignees
Labels
enhancement New feature or request

Comments

@robatwilliams
Copy link

Describe the bug
This is more of an issue than a bug; I see this is a popular established library and has had this behaviour for a long time.

The "gesture" starts as soon as the user starts interacting, and the threshold controls when we determine it to be an intentional gesture and call handlers. It doesn't seem intuitive that the threshold also be subtracted from the movement/offset of the gesture, since the gesture had already started before the threshold was reached. The total movement since the start of the gesture is present in _movement.

Use case: multi-pane layout with draggable splitters to resize them.

Background: issue where dragging the panes was a few pixels off. Turned out to be due to filterTaps setting a 3px threshold in the config. Confirmed by seeing the panes were further off when manually setting a 10px threshold.

Sandbox or Video
n/a

Information:

  • React Use Gesture version: v9.1.3 (originally found in v7.0.16)
  • Device: Desktop computer
  • OS: Windows 10
  • Browser Chrome 89

Checklist:

  • I've read the documentation.
  • n/a If this is an issue with drag, I've tried setting touch-action: none to the draggable element.
@dbismut
Copy link
Collaborator

dbismut commented May 31, 2021

Haha I knew this would come at some point, thanks for making this an issue ;)

I'm currently working on v10, and although this would be somewhat a breaking change, I'm open to suggestions on how to handle this.

I agree that this is debatable, the main reason why I though to not include threshold is because when moving the dragged element, you generally want to respond 1:1 with your pointer. If you include the pre-threshold in the movement, you will experience a jump equivalent to the threshold — unless you animate the jump, but you might have to catchup with the pointer, so this will result in using an animation library during the whole gesture if you see what I mean.

However, as you've noted the state does include _movement which isn't meant to be exposed to the user, but can be handy if you want the raw gesture displacement. It also includes _step, which is a vector which components will either be false when no intentional movement is detected or equivalent to the signed threshold amount. I could expose _step (and probably rename it to make it clearer).

Or add a common option to all gestures such as includePreThreshold so that movement and offset include the threshold displacement right from the start.

@robatwilliams
Copy link
Author

No problem, thanks for the library work and quick response. I did think someone must have noticed this already...

I see your point about catching up with the pointer. In a use case where you're dragging to precisely position something (like mine), it's more important that the element accurately reflect the pointer position. So I think information to allow a consumer to achieve either/both should be exposed on the state.

I'm currently stuck on v7 due to pointer events and Safari 12 support (I don't want to add the PEP polyfill while I'm in the middle of untangling Hammer.js from our project, and we should be dropping 12 soon), so _step isn't there. I'll use _movement in anticipation that something equivalent and public will be available in a future version.

I think "displacement" is the more specific and precise word for what's currently called "movement". Introducing that term could be an opportunity to deprecate movement, and introduce a pair of properties containing the word "displacement". This would avoid an inconvenient breaking change, which is nice despite major versions allowing them. But offset is also in the mix here and has the same issue, maybe initial is affected too. The option idea makes it one or the other, which could be limiting, and is a less nice API.

@dbismut
Copy link
Collaborator

dbismut commented May 31, 2021

I'll continue the conversation about the actual issue later, but about Safari 12, try using @use-gesture/react which is currently v10 in beta stage (and which is the future of react-use-gesture). It should revert to mouse events for browsers that don't support PE. Let me know how it goes as I'm interested in this.

@dbismut
Copy link
Collaborator

dbismut commented Jun 1, 2021

Hey @robatwilliams, back to the pre-threshold movement issue: my thinking would be to just add an option in the config, like: includeThreshold. Would that work?

@robatwilliams
Copy link
Author

Yes, but I'm not sure about this for a few reasons.

Are there any other state properties that currently do include the pre-threshold (e.g. initial)? If so, needing to opt-in to make movement consistent with them isn't ideal.

Are there use cases where the library user would like to have both? The option makes it one or the other.

To have the best API possible though, bearing in mind the inconvenience of breaking changes, let's think it through a bit more. I think including the threshold distance would be the most intuitive behaviour, so ideally this wouldn't be opt-in.

We're using the word "movement" to mean "the vector distance this motion has travelled from its origin", for which I believe the correct physics word is "displacement". Movement strictly, although I'm not sure, means a motion having per-axis and absolute velocities, and perhaps a frequency. We have other displacements here too - offset and lastOffset, I haven't experimented with these so don't have a good understanding.

Given the above, a pair of properties, something like displacementTotal and displacementActive would make sense to me. Compromising that to take into account the exising API and established terminology though, how about adding a movementTotal property?

@dbismut
Copy link
Collaborator

dbismut commented Jun 1, 2021

I agree with the terminology, displacement is a better word than movement, but I didn't think of it two years ago unfortunately.

As a clarification, offset represents the cumulative movement across gestures: movement is reset to zero when the gesture starts, while offset isn't.

So regarding which state attributes include threshold, actually only movement and offset are concerned.

initial is the position of the mouse when the drag starts, so it doesn't depend on the threshold being set.

I don't think there's a situation where you need movement / offset with and without threshold in the same gesture tbh. But eventually I'll have to check the internals and see the costs of adding either an option vs two new state attributes.

@dbismut dbismut added the enhancement New feature or request label Sep 6, 2021
@steos
Copy link

steos commented Aug 27, 2022

Hello!

I just ran into this issue while writing an e2e test simulating a drag interaction.
However, when I set delay: true the delta is not just off by the filterTap threshold but it seems to swallow the entire first mousemove, no matter what the delta is.

For example, issuing the following events:

  • mousedown
  • mousemove(10,10)
  • mouseup

and logging the useDrag state in the callback only results in two events being logged

  • {first: true, last: false, delta: [0,0], ... } // but initial is [0,0] and xy is [10,10]
  • {first: false, last: true, delta: [0,0], ... }

when adding another mousemove(10,10) event right after the first I get 3 events

  • {first: true, last: false, delta: [0,0], ... }
  • {first: false, last: false, delta: [10,10], ... }
  • {first: false, last: true, delta: [0,0], ... }

I'm using version 10.2.18.

I'm now calculating the delta myself based on initial and xy which works fine.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

3 participants