Move cancel to the end of event queue #80

v0lkan · 2016-11-04T02:09:41Z

Bottom line up front: Awesome plugin! Keep up the good work. — I’ve looked into a lot of similar components, and this is, by far the most stable, and the most easy to integrate component that I’ve seen so far. — Kudos for that.

Summary

This is for people with really sensitive touch devices (like trackpads configured to be really fast; or especially for designers who have gotten used to work fast and with precision.

What was Happening

(hard to describe verbally but I’ll try)

The user starts moving her finger from outside the sortable list towards a sortable container.
(while not stopping the finger movement on the trackpad) the user lightly touches the trackpad (in trackpad terms: this is a regular touch, not a force touch)
(still not stopping the finger movement) the user continues to move her finger while keeping the finger lightly touched; hence triggering a touchmove event.

User’s Experience

The user has _touched the sortable container, but without having time to register a delta a cancel event is fired in the move handler because pressTimer runs the handlePress in a timer. This will cancel the drag but since the user assumes that she’s still dragging, she’ll continue to drag hopelessly expecting the container to move, with no luck.

She will trying to drag the container; but the container will not moving at all.
(because the movement is cancelled before even being able to start; though the finger is still moving on the tracpad and the finger is still pressing against the trackpad)

Since the expected behavior is to see the container drag; container not moving cause a mild annoyance.

How does this commit solve the issue?

By setting an immediate timer, we move the cancel event to the tail of the timer queue, and ensure that it is fired after the pressTimer.

That should not impact a typical use case, and enhance the edge case described in this commit.

Other Options

If this timer fix does not work for some reason; a second option might be to track the state of the pressTimer; and not fire cancel if the press timer has been set but no delta registered yet — though I feel like that will make things even more complicated.

This is for people with really sensitive touch devices (like trackpads configured to be really fast; or especially for designers who have gotten used to work fast and with precision. **What was happening** (hard to describe verbally but I’ll try) * The user starts moving her finger from outside the sortable list towards a sortable container. * (while not stopping the finger movement on the trackpad) the user lightly touches the trackpad (in trackpad terms: this is a regular touch, not a force touch) * (still not stopping the finger movement) the user continues to move her finger while keeping the finger lightly touched; hence triggering a touchmove event. **what happens?** The user has `_touched` the sortable container, but without having time to register a delta a cancel event is fired in the move handler because pressTimer runs the handlePress in a timer. **How does this commit solve the issue?** By setting an immediate timer, we move the cancel event to the tail of the timer queue, and ensure that it is fired after the pressTimer. That should not impact a typical use case, and enhance the edge case described in this commit. **Other options** If this timer fix does not work for some reason; a second option might be to track the state of the pressTimer; and not fire `cancel` if the press timer has been set but no delta registered yet — though I feel like that will make things even more complicated.

# Conflicts: # src/SortableContainer/index.js

clauderic · 2016-11-11T00:51:06Z

Hey @v0lkan, can you give me more insight into how to replicate this / on what platform? What kind of touchpad showcases this behaviour? I'm personally unable to replicate this.

I'll also need to test this to make sure there are no side-effects

v0lkan · 2016-11-11T04:49:37Z

Hi @clauderic the fix will cause regression because I mistakenly used this.cancel instead of something like () => { this.cancel() }.

I will amend a change.

I am also recording a video (with and without the patch) to show you the experience.

I am using the recent Apple magic trackpad, on a pretty recent version of Macbook Pro (not the latest one on the apple event — without the escape key; the one before that :) )
I tested both recent chrome and safari and outcomes were the same.

v0lkan · 2016-11-11T05:38:39Z

Hi @clauderic here’s my recording: https://www.youtube.com/watch?v=g-MkFj10C-U

I also annotated it at important checkpoints;

If it does not cause any regressions, the patch will definitely improve the user experience.

I’ll amend a change momentarily to fix the stuff that mentioned above.

v0lkan · 2016-11-16T22:29:21Z

Hi @clauderic; did you have time to watch the video?

Do you have any other input from me to replicate the issue.

Best,

Volkan.

levithomason · 2016-12-06T17:18:42Z

Confirming the exact issue here:

MacOS Sierra 10.12.1 (16B2555)
Chrome 54.0.2840.98 (64-bit)
react 15.4 / react-sortable-hoc 0.3.0

Most the time, the drag does not work on the first try. My naive intuition was that something in the demo page was just not done loading and I had to wait. Then locally, I thought maybe there was a default pressDelay of ~200ms applied. I came looking for a prop that I could set to make items immediately draggable.

I applied the patch manually to the dist files and the issue is mitigated.

v0lkan · 2016-12-06T21:40:10Z

Glad that I was not being crazy and I’m happy to see someone else also got impacted (because it verifies and replicates the issue) :)

And its good that the patch fixes the issue.

Let’s hope there are no regressions.

cc: @clauderic

clauderic · 2016-12-08T02:19:18Z

Hey @v0lkan,

This looks good to be merged at this point. The last thing I want to check before merging though is that this does not conflict with the changes that were introduced with #71.

levithomason · 2016-12-08T02:21:49Z

I can test this real quick, hang tight..

levithomason · 2016-12-08T02:31:12Z

@clauderic Tested and all is well:

MacBook Pro (Retina, 15-inch, Mid 2015) version 10.12.1 (16B2555)
Google Chrome Version 54.0.2840.98 (64-bit)

react-sortable-hoc v0.4.0
react v15.4.0

pressDelay={0}

Instant dragging, no issues.

pressDelay={1000}

Dragging starts after 1s of holding a click.

no pressDelay

Instant dragging, no issues.

Seems both patches work perfectly together 👍

levithomason · 2016-12-08T02:31:53Z

Oh, should've noted, without the patch here in this PR there is still the issue of having to rest the mouse before dragging else nothing happens.

clauderic · 2016-12-08T03:01:19Z

Just tested myself, looks good!

LGTM 🚢

v0lkan · 2017-03-11T00:01:49Z

A bit late to the party :) — Glad to see this merged.

Cheers,

V.

b2whats · 2017-04-24T21:26:39Z

Up!
#171 - questions and this decision have problem

fix: Move cancel to the end of event queue

v0lkan added 3 commits November 3, 2016 19:08

Merge branch 'master' of github.com:v0lkan/react-sortable-hoc

631460e

# Conflicts: # src/SortableContainer/index.js

preserved context binding on deferred call to cancel

f579e24

clauderic mentioned this pull request Dec 1, 2016

Not working on OS X, with 3 finger drag gesture. Have tested in Safari and Chrome. #88

Closed

tweak: cancel is already bound to current execution context

998a249

clauderic merged commit dd4a7f5 into clauderic:master Dec 8, 2016

DimitarNestorov pushed a commit to codemotionapps/react-sortable-hoc that referenced this pull request Feb 4, 2019

Merge pull request clauderic#80 from v0lkan/master

2befc1f

fix: Move cancel to the end of event queue

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Move cancel to the end of event queue #80

Move cancel to the end of event queue #80

v0lkan commented Nov 4, 2016 •

edited

Loading

clauderic commented Nov 11, 2016 •

edited

Loading

v0lkan commented Nov 11, 2016 •

edited

Loading

v0lkan commented Nov 11, 2016

v0lkan commented Nov 16, 2016

levithomason commented Dec 6, 2016

v0lkan commented Dec 6, 2016 •

edited

Loading

clauderic commented Dec 8, 2016

levithomason commented Dec 8, 2016

levithomason commented Dec 8, 2016

levithomason commented Dec 8, 2016

clauderic commented Dec 8, 2016

v0lkan commented Mar 11, 2017

b2whats commented Apr 24, 2017 •

edited

Loading

Move cancel to the end of event queue #80

Move cancel to the end of event queue #80

Conversation

v0lkan commented Nov 4, 2016 • edited Loading

clauderic commented Nov 11, 2016 • edited Loading

v0lkan commented Nov 11, 2016 • edited Loading

v0lkan commented Nov 11, 2016

v0lkan commented Nov 16, 2016

levithomason commented Dec 6, 2016

v0lkan commented Dec 6, 2016 • edited Loading

clauderic commented Dec 8, 2016

levithomason commented Dec 8, 2016

levithomason commented Dec 8, 2016

pressDelay={0}

pressDelay={1000}

no pressDelay

levithomason commented Dec 8, 2016

clauderic commented Dec 8, 2016

v0lkan commented Mar 11, 2017

b2whats commented Apr 24, 2017 • edited Loading

v0lkan commented Nov 4, 2016 •

edited

Loading

clauderic commented Nov 11, 2016 •

edited

Loading

v0lkan commented Nov 11, 2016 •

edited

Loading

v0lkan commented Dec 6, 2016 •

edited

Loading

b2whats commented Apr 24, 2017 •

edited

Loading