Destroy sortable not removing event listeners.

[lachdoug]

I'm opening this issue because:

sortable( '.my-list', 'destroy' ) does not remove event listeners for

• dragend
• dragenter
• dragstart
• sortupdate

This causes errors to be logged when using HTML5Sortable with with nested lists.

supporting information:

• I use sortable version: 0.9.16

How can the issue be reproduce the problem?

Codepen example here

On a page with a nested list...

1. Create sortable on child list.
2. Remove sortable using sortable( ?, 'destroy' )
3. Create sortable on the child's parent list.
4. Drag an item from the parent list across the child list.
5. See errors in console.

The error message is TypeError: options is undefined from var items = _filter(sortableElement.children, options.items);

[lukasoppermann]

I agree that this is an issue. Could you please send a PR to fix this?

[lachdoug]

A fix is to turn off the isSortable flag upon sortable destroy. This seems like the right thing to do regardless.

I'll send a pull request with sortableElement.isSortable = false added to _destroySortable.

---

This does not solve the issue that event listeners are not removed from the sortable element.

The 'dragstart' and 'dragend' events are easy to deal with. We can simply add removeEventListener(sortableElement, 'dragstart'); and removeEventListener(sortableElement, 'dragend'); to _destroySortable.

The 'dragenter' listener is problematic.

Two listeners are added for 'dragenter'.

One in here:

addEventListener(sortableElement, 'dragenter', function (e) {
              var target = getEventTarget(e);
              var sortableContainer = findSortable(target, e);
...

and the second here:

addEventListener(listItems.concat(sortableElement), 'dragenter', onDragOverEnter);

The callback is stored using the event name

element.addEventListener(eventName, callback);
store(element).setData("event" + eventName, callback);

Which causes one to overwrite the other.

Now only one callback can be retrieved for removal

element.removeEventListener(eventName, store(element).getData("event" + eventName));
store(element).deleteData("event" + eventName);

A fix could be to remove this line altogether.

addEventListener(listItems.concat(sortableElement), 'dragenter', onDragOverEnter);

Dragging would still be caught by the 'dragover' event listener in the adjacent line. Is there a reason why we need both 'dragover' and 'dragenter'?

[lukasoppermann]

Hey @lachdoug,

I would have to look into why this is done this way. It may be a bug but I think there is a reason.

However I would appreciate if you could send multiple small PRs, because this makes merging easier and faster.

E.g. removing isSortable or setting it to false as suggested (and nothing else), could be one PR that is easy to merge.

'dragstart' and 'dragend' could be a second PR

And the last has to be figured out first and could be a third one in the end. Maybe with a bugfix first.

[lachdoug]

Will do.