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

Fix mouse drag suppression #26

Merged

Conversation

spencercornish-wk
Copy link
Contributor

@spencercornish-wk spencercornish-wk commented Oct 11, 2018

We noticed that drag suppression hasn't really been working. This fixes that, by effectively not starting a drag event until the suppression distance has been met. Once the distance has been exceeded, the drag started event is dispatched.

@marcojakob

@marcojakob
Copy link
Owner

You are right. It does not prevent the drag events to be fired. It only suppresses that a click is fired when the drag distance is smaller than clickSuppression. See #13 why this was implemented.

Can you describe, what usecase you have where this is important?

BTW: Your fix doesn't seem to work. With moveEvent is MouseEvent in the if statement it cancels every attempt to drag something.

@spencercornish-wk
Copy link
Contributor Author

Yeah, I'm fixing that now, sorry. It randomly worked in Dartium, just not chrome :( The issue we are trying to solve is not so much that the click doesn't bubble through (that's working fine) but rather that there's still a visual artifact. Here are some GIFs showing what I'm talking about.

Without fix

w_o ds 2

With fix

ds

@marcojakob
Copy link
Owner

Thanks for the helpful gifs @spencercornish-wk. I've moved parts of your code to draggable_manager and simplifyed it a bit.

  • I've changed the name of the property from clickSuppression to minDragStartDistance.
  • Measuring the distance again to allow for clicks in dragEnd is not necessary any more (former clickSuppression). The reason is that the drag does not start at all now if lower than minDragStartDistance.
  • I've decided to not reset the start position of the avatar. This is to keep the dragging element underneath the mouse.
  • I've chosen a default of 4 pixels. This seems to be the standard (see dragging in Google Drive/Gmail, Draggabilly, etc.)

Can you test if this works as expected?

@marcojakob
Copy link
Owner

marcojakob commented Oct 12, 2018

@dustyholmes-wf You originally added the pull request for #13 to allow configurable clickSuppression. Could you take a look at this and let me know if the minDragStartDistance as implemented in this PR also works for you? It would make using the options more understandable and with the 4 pixel min distance we'd have a sensible default.

CHANGELOG.md Outdated
@@ -1,10 +1,15 @@
# Changelog

## Version 1.4.0 (2018-10-12)

- BREAKING CHANGE: Add an option `minDragStartDistance` on `Draggable` to define the minimum distance in
Copy link
Contributor

Choose a reason for hiding this comment

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

Could this be done without a breaking change by making clickSuppression set minDragStartDistance?

I know within our organization a lot of libraries use this library and it would cause a decent amount of pain if this was released as a breaking change.

@dustyholmes-wf
Copy link
Contributor

I believe this achieves the same concept that I was trying to address in my previous changes, I've always been sad about my variable choice too. I think the change generally looks like a step in the right direction.

@spencercornish-wk
Copy link
Contributor Author

@marcojakob That works great, and is much more elegant than what I came up with. I also agree with @dustyholmes-wf about the breaking change stuff. I do think it looks cleaner as an optional named prop, but if clickSuppression could be left around for backwards compatibility I would also appreciate it

Mark `clickSuppression` as deprecated
@marcojakob
Copy link
Owner

marcojakob commented Oct 12, 2018

What do you think about this? I've marked it as @deprecated.

 /// The minimum distance for a drag to prevent click events.
  /// DEPRECATED: Please use [minDragStartDistance] instead.
  @deprecated
  set clickSuppression(int distance) {
    minDragStartDistance = distance;
  }

Would this work or will it still break your code?

Note: The behavior is still a bit different. The drag will not start at all with minDragStartDistance while before with clickSuppression the drag started but the click was still fired if the distance wasn't enough.

So, as long as you are not depending on events being fired during a drag in the tiny first movement, it is only a very small visual change. For the first few pixels, the avatar will not show yet. But I think this is mostly how it's intendend and how other (big) libraries and web apps do it.

@dustyholmes-wf
Copy link
Contributor

That looks good. There is still technically a breaking change since clickSuppression could be read before. I'd suggest a getter too (also deprecated) to protect against the unlikely case that someone was reading that value. Maybe in a test.

@spencercornish-wk
Copy link
Contributor Author

This looks awesome. Thanks so much @marcojakob !!

@marcojakob marcojakob merged commit a2bfd93 into marcojakob:master Oct 12, 2018
@marcojakob
Copy link
Owner

:shipit:

Thanks for all the feedback @spencercornish-wk @dustyholmes-wf .

Released as v1.4.0 on pub

@spencercornish-wk spencercornish-wk deleted the OUT-5-avatar-drag-supression branch July 19, 2019 21:51
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.

None yet

3 participants