kornelski / slip

Slip.js — UI library for manipulating lists via swipe and drag gestures
BSD 2-Clause "Simplified" License
2.44k stars 213 forks source link

'click' gets triggered when element is dropped (mouse only) #103

Open Samlibardi opened 6 years ago

Samlibardi commented 6 years ago

How to reproduce:

This happens probably because the mousedown and mouseup events are triggered in the same position in the element.

So far, I've been able to work around this by adding the following:

this.target.node.addEventListener('click',function h(e) {
                            e.stopPropagation(); 
                            e.preventDefault(); 
                            this.removeEventListener(e.type,h,true);
                        },true);

in the onEnd method of the reorder state. This captures the click event and removes itself allowing it to be clicked (see #104 though). It feels very hacky and I don't know how well it behaves. It's been working fine for the time.

kornelski commented 6 years ago

click should be stopped by preventDefault on either mousedown or mouseup events. Maybe check handlers for these? I can't check now, but I vaguely remember it was tough to allow both text selection and prevent mouse events.

Samlibardi commented 6 years ago

I've searched around and apparently mouseup doesn't prevent click events. However there seems to be an easier workaround:

https://stackoverflow.com/questions/18342747/why-cant-a-mouseup-event-prevent-a-click-event

Returning false on the mousedown event should prevent click from being raised. Goes without saying that it should be reraised if the dragging is not executed though.


From: Kornel notifications@github.com Sent: Thursday, July 12, 2018 12:36:51 PM To: kornelski/slip Cc: Samuel Libardi Godoy; Author Subject: Re: [kornelski/slip] 'click' gets triggered when element is dropped (mouse only) (#103)

click should be stopped by preventDefault on either mousedown or mouseup events. Maybe check handlers for these? I can't check now, but I vaguely remember it was tough to allow both text selection and prevent mouse events.

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHubhttps://nam02.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com%2Fkornelski%2Fslip%2Fissues%2F103%23issuecomment-404555197&data=02%7C01%7C%7C572864167951478708ae08d5e80d4a07%7C84df9e7fe9f640afb435aaaaaaaaaaaa%7C1%7C0%7C636670066123344588&sdata=M5sZin8PCd6lKQmorENAjDRu%2BHEjf67PmJ0LkTLU9sQ%3D&reserved=0, or mute the threadhttps://nam02.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com%2Fnotifications%2Funsubscribe-auth%2FAcMGs36wyOg3rUeCxcHCXzft6pmjBXsYks5uF20TgaJpZM4VM80w&data=02%7C01%7C%7C572864167951478708ae08d5e80d4a07%7C84df9e7fe9f640afb435aaaaaaaaaaaa%7C1%7C0%7C636670066123344588&sdata=CMnShEyXs%2Bctji4%2FLRa0EtJ7PXlPcYWowuRQslLqtr4%3D&reserved=0.

kornelski commented 6 years ago

Bummer. Preventing in mousedown is not a good option, as it'll prevent text selection. Re-raising may have problems with trusted events too.

So I suppose stopping the click is the way to go.

Samlibardi commented 6 years ago

Then I suppose adding a dedicated click handler in the Slip constructor is a cleaner solution than what I initially suggested. Returning true allows anchors and buttons to work properly as well. I'll try to apply this in my project and if it works fine I'll post a PR with the changes.