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 inconsistency when component is removed on drop #119

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

gabrielgrant
Copy link

If a drop action causes the draggable component instance to be
destroyed, the dragEnd event isn't fired, resulting in the
dragCoordinator being left in an inconsistent state (it thinks the
object is still being dragged). This change ensures the dragCoordinator
state gets cleaned up before the draggable-object is destroyed.

Note that this fix still won't result in the dragEndHook or dragEndAction
being fired: both of them are still only triggered if a browser dragEnd
event fires. It's not clear if they can be caused to fire in this
case without breaking the API, since both take the dragEnd event
as a parameter, which doesn't exist in this case.

This seems fine for dragEndHook, since it appears to mostly be used for DOM
changes/cleanup and this section of the DOM is going away, but it feels a
bit strange that dragEndAction wouldn't be fired.

Two possible options:

  • send dragEndAction without an event
  • add a new "dragFinally" action that gets called in both cases

Related to #118

If a drop action causes the draggable component instance to be
destroyed, the dragEnd event isn't fired, resulting in the
dragCoordinator being left in an inconsistent state (it thinks the
object is still being dragged). This change ensures the dragCoordinator
state gets cleaned up before the draggable-object is destroyed.

Note that this fix still won't result in the dragEndHook or dragEndAction
being fired: both of them are still only triggered if a browser dragEnd
event fires. It's not clear if they can be caused to fire in this
case without breaking the API, since both take the dragEnd event
as a parameter, which doesn't exist in this case.

This seems fine for dragEndHook, since it appears to mostly be used for DOM
changes/cleanup and this section of the DOM is going away, but it feels a
bit strange that dragEndAction wouldn't be fired.

Two possible options:
- send dragEndAction without an event
- add a new "dragFinally" action that gets called in both cases

Related to mharris717#118
@gabrielgrant
Copy link
Author

AFAICT the CI failure here doesn't seem to actually be related to this PR -- it seems to be a problem starting headless chrome in the test environment: https://travis-ci.org/mharris717/ember-drag-drop/jobs/329727097#L1947

@dgavey
Copy link
Collaborator

dgavey commented Jan 17, 2018

Thanks for the PR, I'll try and take a look tonight

@gabrielgrant
Copy link
Author

gabrielgrant commented Jan 17, 2018

Thanks, though this seems to not be a perfect fix: it appears that (sometimes?) after the willDestroyElement() cleanup has happened, the dragEnd() event will fire, causing dragCoordinator.dragEnded() to be called twice, which (if the template depends on dragCoordinator.currentDragObject) results in a Backtracking render error, since the value is set twice.

So, to summarize, what I'm generally seeing is: if drop destroys the component, this happens before endDrag fires, resulting in cleanup not being performed. If cleanup is performed in the willRemoveElement() hook (as I'm doing in this patch), the endDrag event does fire, meaning cleanup happens twice, resulting in an error. Fairly frustrating -_-

Currently experimenting with a few different workarounds, but still haven't been able to identify why this cleanup in willRemoveElement() causes the event to be handled, when it wasn't before.

Also, at the very least, I think the cleanup in willRemoveElement() should also reset the isDraggingObject flag on the content object.

@panthony
Copy link

panthony commented Mar 7, 2018

Any progress on this issue? I'm having the same problem with the dragEndAction not being called.

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.

3 participants