Simplifying the flow. Observing 'mousemove' after 'dragstart' to trigger endDrag instead of depending on browser to trigger 'dragend'

[hunterbmt]

Simplifying the flow, so can prevent the error to occurs when the browser fails to trigger 'dragend' or 'drop'. I.e #789
Now we depend only on 'mousemove' triggering after 'dragstart' to determine that the dragging is ended.

[darthtrevino]

Hey @hunterbmt, thanks for the PR, sorry for not getting to it for a while. I'll give this a deeper look later tonight

[hunterbmt]

@DanielKoehler thanks for taking care this one.

[darthtrevino]

[bjrmatos]

why this check is gone now? after seeing #789 i don't see any example that shows the problem.. the issue even says that the react-dnd example works normally in chrome 59

[bjrmatos]

please @darthtrevino consider revert this change, it is changing the behaviour of react-dnd..

• for example if i drag a component and drop it outside of a valid drop area and i don't move the mouse, the preview is hanging until i move the mouse (because this PR changed from using the native dragend event to mousemove event).

before this PR (endDrag received inmediatly):

with this PR (endDrag not received until i move the mouse):

• before this PR if i start dragging something and press the "ESC" key the dragging is cancelled and then the endDrag method is called normally, with this PR that behaviour is gone

• before this PR the endDrag method was always called when the preview image (or ghost preview) actually returns to the original location where the dragging started, but now this is gone completly and the timing of the event is different. (with the additionally thing that we now need to move the mouse to get the endDrag method called)

since this is a HTML5 drag and drop implementation i think it is completly reasonable that we need to use the html5 drag and drop events, the browser handles a lot of cases automatically.. we are just getting in troubles by eliminating listening the dragend event.

if there is a problem with chrome 59 (like this issue describes) we need to focus on reproduce the real problem instead of adding a workaround that changes the behaviour of everything. @hunterbmt can you please provide a minimal example where react-dnd fails with the dragend event in Chrome 59? i don't have any problem with Chrome 59, but i'm curious and willing to help to identify your real issue, if there is a problem it should be reproducible with a minimal example.

[bjrmatos]

@darthtrevino @theTechie i made some comments about this change, please let me know your feedback. thnks

[bjrmatos]

and cc @hunterbmt just in case you don't get the notification for the mention in the review comment.

[hunterbmt]

@bjrmatos I agreed with you about the behavior changes in the PR. I didn't think about all the cases when submitting the PR.
Since we alway check if we have currentSourceNode before trigger 'endDrag', I think having 'dragend' and 'drop' (for sourceNode removed from DOM cases) to trigger 'endDrag' beside 'mousemove' after 'dragstart' to trigger 'endDrag' won't hurt at all while covering all the case you mention.
I cannot give you the example for Chrome 59 issues since it's a production code and it's quite a big application even though the logic at DnD component is quite straightforward. The code works normal on FF, IE, Safari and Chrome 58 but not Chrome 59 since it won't trigger 'dragend' event after 'drop' at all. In some cases, it won't even trigger 'drop' when I drag item into un-dropable area. But according to couple comments on the issues, I don't think that I face that issue alone.
I will create another PR to fix the issue without impact current behavior of react dnd. Sorry for a bad PR.
In the meantime, please help me to revert the PR first to avoid any accidentally release.

Regards,

[darthtrevino]

It's not published yet, I wanted to test it some more before doing that. I'll revert it

…

On Tue, Jul 25, 2017 at 7:58 AM hunterbmt ***@***.***> wrote: @bjrmatos <https://github.com/bjrmatos> I agreed with you about the behavior changes in the PR. I didn't think about all the cases when submitting the PR. Since we alway check if we have currentSourceNode before trigger 'endDrag', I think having 'dragend' and 'drop' (for sourceNode removed from DOM cases) to trigger 'endDrag' beside 'mousemove' after 'dragstart' to trigger 'endDrag' won't hurt at all while covering all the case you mention. I cannot give you the example for Chrome 59 issues since it's a production code and it's quite a big application even though the logic at DnD component is quite straightforward. The code works normal on FF, IE, Safari and Chrome 58 but not Chrome 59 since it won't trigger 'dragend' event after 'drop' at all. In some cases, it won't even trigger 'drop' when I drag item into un-dropable area. But according to couple comments on the issues, I don't think that I face that issue alone. I will create another PR to fix the issue without impact current behavior of react dnd. Sorry for a bad PR. In the meantime, please help me to revert the PR first to avoid any accidentally release. Regards, — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#801 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAG7iLKQaKDzGi94qW6HD83uC4c4zGwsks5sRgKRgaJpZM4N9-Qt> .

[bjrmatos]

thnks @darthtrevino

I think having 'dragend' and 'drop' (for sourceNode removed from DOM cases) to trigger 'endDrag' beside 'mousemove' after 'dragstart' to trigger 'endDrag' won't hurt at all while covering all the case you mention.

let's see.. firing endDrag by listening dragend allows to handle a lot of functionality for free, so if you can make the change that you have in mind i'm willing to help to test that it does not change the current behaviour.

I cannot give you the example for Chrome 59 issues since it's a production code and it's quite a big application even though the logic at DnD component is quite straightforward. The code works normal on FF, IE, Safari and Chrome 58 but not Chrome 59 since it won't trigger 'dragend' event after 'drop' at all. In some cases, it won't even trigger 'drop' when I drag item into un-dropable area. But according to couple comments on the issues, I don't think that I face that issue alone.

i understand that point but every issue needs at least a reproducible case, so maintainers or other people can help to verify the problem and think in the better solution, if there is no reproducible case the issue turns into something that maintainers or users of the library can not trust at 100% because there is no proof of the problem described. i understand that your codebase is big but trying to isolate the issue and removing some not relevant parts of your code to create a reproducible case should be not very hard and i think it is the only way to identify or verify the real issue so everyone can be happy.