onDestroy should stop auto scroll immediately.

[sangcu]

There is the case when a user drags an item, then, press ESC to cancel drag event. In this case, we should stop auto-scroll immediately in destroy method instead of waiting for mouseup event to be dispatched.
@hollowdoor: what do you think about this?

[hollowdoor]

Sorry I'm confused. The destroy method is meant for a total clean up when you want to throw away the auto scroll functionality, or the main element is removed/deleted. Are you thinking of a simple "stopping" of the scrolling, or something else?

To temporarilly stop autoscrolling you can save a boolean ESC state variable, and return from the autoScroll options method that boolean state. Unless you would like something more externally accessible like an instance pause method, or pauseOn('ESC'). I wouldn't be opposed to that unless you're talking about something else.

[sangcu]

Hi @hollowdoor
It would be better to get the demo: http://jsfiddle.net/Lkyrf91z/
In this example, I attached a keydown event to cancel dragula when user press ECS.
The problem is I'm dragging item and autoscroll is scrolling, meanwhile, I press ECS, dragula will be canceled but autoscroller is scrolling.

we can call destroy to total clean up Autoscroller, but it's still scrolling until I raise mouseup event.

The reason is because the destroy method didn't cancelFrame

cancelFrame(animationFrame);
cancelFrame(windowAnimationFrame)

so I think it should call this function to clean up.

[hollowdoor]

Ok. I see now. You want the next loop canceled. Though the destroy method will do more than just cancel even if that functionality is added. In the fiddle you provide dragula dragging effect is still available after cancel. Are you sure you want this scroll stopping effect in destroy.

Though I think this issue has brought to my attention a bug in destroy. So I thank you for that. I think I will create a cancel, and/or pause method. This method will be public for potential temporary scroll stop, and used by destroy to enhance the clean up steps when destroying like you're suggesting. Either way this would fix the destroy functionality, and add a temporary pause/cancel for when that effect is needed.

Also I just realized dragula is still dragging in the fiddle after cancel. Only the drop is canceled. Is this the effect you wanted to show me?

[sangcu]

How do you know Dragula still dragging? I have checked drake.isDragging after the call drake.cancel and it returns false for that.

That's would be great that we can fix the destroy to make scroll stop. I can create a pull request for this.

[hollowdoor]

I don't know if Dragula is dragging. I should say the behavior in that fiddle is that visually it's still dragging. Sorry.

If you would like to make a pull request that would be great.

[hollowdoor]

I just realized this could be a breaking change, or at least a surprising behavior for some.

@sangcu If you go through with the PR could you add a boolean switch to destroy to turn off/on the animation cancel so at least people can make that quick change if they don't won't this new behavior? This would let people do scroller.destroy(false); to make it easy to continue scrolling temporarily if they want that.

[sangcu]

Hi @hollowdoor
Sorry for late response.

I understand the changes would make breaking changes in the code. However, the destroy meaning for clean up everything, so I don't think add parameter to this method to keep something still work is make sense.

so if you agreed, I would add a parameter to this function as your suggested for the time being. Based on that, we can increase the version number as semver to add one more function to cleanAnimation, and then remove parameter from destroy method.

[hollowdoor]

I also think that a boolean switch isn't appropriate in the long term. I'm just troubled by making the change in a minor version while maybe people might be using the bug as a feature. I also see that you, and possibly others might want this corrected in the present. As I would.

A temporary boolean switch will have to do for now.

When enough new breaking ideas are collected for a major version we can remove the boolean switch when those other ideas are implemented. The syncMove method for instance has not been properly vetted yet.

Sorry for late response.

Take your time. We can start worrying if a bunch of other people start adding plus ones to this issue. :)

[hollowdoor]

I'm closing this for now. I'll create a new issue for version 3 that will include removal of the boolean switch added to fix this.