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

Clash storing event listeners for 'dragenter' #515

Open
lachdoug opened this issue Oct 1, 2019 · 1 comment
Open

Clash storing event listeners for 'dragenter' #515

lachdoug opened this issue Oct 1, 2019 · 1 comment
Labels
bug help wanted info needed More info needs to be provided to discuss/solve the issue PR needed This issue requires a PR to be fixed. If you have time to help, please send a PR.

Comments

@lachdoug
Copy link
Contributor

lachdoug commented Oct 1, 2019

Two listeners are added for 'dragenter'.

One 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'?

See issue #512 for background

@lukasoppermann
Copy link
Owner

lukasoppermann commented Jul 5, 2020

The first occurence is lines 344-L370

    _on(sortableElement, 'dragenter', (e) => {
// ...

The second dragenter is lines 580-598

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

To resolve this we need to do the following:

  1. figure out what the two events do
  2. figure out if we need the dragenter event as well as the dragover for the seccond one
  3. see if we can combine the two dragenter events or remove them

If I read it correctly, lines 344-L370 are only needed to fire the sortenter event. So this could be moved into a function and be called within lines 580-598

The difference between dragenter and dragover is:

  • dragenter fires only once, when the mouse enters the element with the eventlistener
  • dragover fires constantly while the mouse is within the element with the eventlistener

@kaffarell do you see it like as well? This would mean we would need to separate the event in lines 580-598 as the sortenter event should only fire once. Best would be to create a function for the codes in lines 344-L370 as well and than call both functions for dragenter but only the last one for dragover

@lukasoppermann lukasoppermann added info needed More info needs to be provided to discuss/solve the issue PR needed This issue requires a PR to be fixed. If you have time to help, please send a PR. labels Jul 5, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug help wanted info needed More info needs to be provided to discuss/solve the issue PR needed This issue requires a PR to be fixed. If you have time to help, please send a PR.
Projects
None yet
Development

No branches or pull requests

2 participants