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.

Sign up for GitHub

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

Jump to bottom

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

Merged
merged 3 commits into from
Jul 11, 2017
Merged

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

Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
2db7921
Simplify the even flow, end drag whenever receiving mousemove after d…
hunterbmt Jun 19, 2017
95fb05c
Merge branch 'master' into master
hunterbmt Jun 19, 2017
41eb8ca
Merge branch 'master' into master
darthtrevino Jul 11, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
30 changes: 6 additions & 24 deletions packages/react-dnd-html5-backend/src/HTML5Backend.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,7 +32,6 @@ export default class HTML5Backend {
this.getSourceClientOffset = this.getSourceClientOffset.bind(this);
this.handleTopDragStart = this.handleTopDragStart.bind(this);
this.handleTopDragStartCapture = this.handleTopDragStartCapture.bind(this);
this.handleTopDragEndCapture = this.handleTopDragEndCapture.bind(this);
this.handleTopDragEnter = this.handleTopDragEnter.bind(this);
this.handleTopDragEnterCapture = this.handleTopDragEnterCapture.bind(this);
this.handleTopDragLeaveCapture = this.handleTopDragLeaveCapture.bind(this);
Expand All @@ -41,7 +40,7 @@ export default class HTML5Backend {
this.handleTopDrop = this.handleTopDrop.bind(this);
this.handleTopDropCapture = this.handleTopDropCapture.bind(this);
this.handleSelectStart = this.handleSelectStart.bind(this);
this.endDragIfSourceWasRemovedFromDOM = this.endDragIfSourceWasRemovedFromDOM.bind(this);
this.endDrag = this.endDrag.bind(this);
this.endDragNativeItem = this.endDragNativeItem.bind(this);
this.asyncEndDragNativeItem = this.asyncEndDragNativeItem.bind(this);
}
Expand Down Expand Up @@ -83,7 +82,6 @@ export default class HTML5Backend {
addEventListeners(target) {
target.addEventListener('dragstart', this.handleTopDragStart);
target.addEventListener('dragstart', this.handleTopDragStartCapture, true);
target.addEventListener('dragend', this.handleTopDragEndCapture, true);
target.addEventListener('dragenter', this.handleTopDragEnter);
target.addEventListener('dragenter', this.handleTopDragEnterCapture, true);
target.addEventListener('dragleave', this.handleTopDragLeaveCapture, true);
Expand All @@ -96,7 +94,6 @@ export default class HTML5Backend {
removeEventListeners(target) {
target.removeEventListener('dragstart', this.handleTopDragStart);
target.removeEventListener('dragstart', this.handleTopDragStartCapture, true);
target.removeEventListener('dragend', this.handleTopDragEndCapture, true);
target.removeEventListener('dragenter', this.handleTopDragEnter);
target.removeEventListener('dragenter', this.handleTopDragEnterCapture, true);
target.removeEventListener('dragleave', this.handleTopDragLeaveCapture, true);
Expand Down Expand Up @@ -228,12 +225,7 @@ export default class HTML5Backend {
this.currentNativeSource = null;
}

endDragIfSourceWasRemovedFromDOM() {
const node = this.currentDragSourceNode;
if (document.body.contains(node)) {
Copy link

Choose a reason for hiding this comment

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

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

Copy link

@bjrmatos bjrmatos Jul 24, 2017 •
edited

Choose a reason for hiding this comment

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

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):

dragging before

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

dragging now

  • 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.

return;
}

endDrag() {
if (this.clearCurrentDragSourceNode()) {
this.actions.endDrag();
}
Expand All @@ -247,16 +239,17 @@ export default class HTML5Backend {

// Receiving a mouse event in the middle of a dragging operation
// means it has ended and the drag source node disappeared from DOM,
// so the browser didn't dispatch the dragend event.
this.window.addEventListener('mousemove', this.endDragIfSourceWasRemovedFromDOM, true);
// so the browser didn't dispatch the dragend event or browser fail
// to trigger dragend (i.e: Chrome 59)
this.window.addEventListener('mousemove', this.endDrag, true);
}

clearCurrentDragSourceNode() {
if (this.currentDragSourceNode) {
this.currentDragSourceNode = null;
this.currentDragSourceNodeOffset = null;
this.currentDragSourceNodeOffsetChanged = false;
this.window.removeEventListener('mousemove', this.endDragIfSourceWasRemovedFromDOM, true);
this.window.removeEventListener('mousemove', this.endDrag, true);
return true;
}

Expand Down Expand Up @@ -389,15 +382,6 @@ export default class HTML5Backend {
}
}

handleTopDragEndCapture() {
if (this.clearCurrentDragSourceNode()) {
// Firefox can dispatch this event in an infinite loop
// if dragend handler does something like showing an alert.
// Only proceed if we have not handled it already.
this.actions.endDrag();
}
}

handleTopDragEnterCapture(e) {
this.dragEnterTargetIds = [];

Expand Down Expand Up @@ -539,8 +523,6 @@ export default class HTML5Backend {

if (this.isDraggingNativeItem()) {
this.endDragNativeItem();
} else {
this.endDragIfSourceWasRemovedFromDOM();
}
}

Expand Down