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 jquery.event.drag suppressing native drag and drop #239

Closed
yamass opened this issue May 8, 2018 · 12 comments · Fixed by #454
Closed

Fix jquery.event.drag suppressing native drag and drop #239

yamass opened this issue May 8, 2018 · 12 comments · Fixed by #454

Comments

@yamass
Copy link
Contributor

yamass commented May 8, 2018

I am using slickgrid in combination with fullcalendar and some custom component using native drag and drop.

Problem:

Native drag and drop does not work in our whole application, since jquery.event.drag cancels them globally!

Details / Cause analysis:

  • jquery.event.drag registers special drag events ("drag", "draginit", "dragstart", "dragend")
  • These are implemented by listening to mouse and touch events: $event.add( this, "touchstart mousedown", drag.init, data ); (v2.3.0, line 82)
  • So when a jquery listener for mousedown events is registered to a DOM node (something like $myDiv.on("mousedown", handleDragStart)), the jquery.event.drag code gets executed.
  • jquery.event.drag will however event.preventDefault() by returning false: if ( !drag.touched || dd.live ) return false; (v2.3.0, line 158; will return false, since mousedown is not a touch event)
  • Now, since fullcalendar registers a mousedown event handler via jQuery on document... No native drag and drop on the whole page.

I see two possible solutions:

  1. fix jquery.event.drag: make it never return false by removing the return false statement.
				// // helps prevent text selection or scrolling
				// if ( !drag.touched || dd.live )
				//     return false;

(Developers should care for text selection themselves via CSS: user-select: none;)
I can provide a pull request for this, if you want.

  1. Remove the dependency to jquery.event.drag completely (or at least make it optional). It is only needed for three things as far as I can see:
  • column resizing (can easily be replaced by naive implementation with mouse events)
  • slick.cellrangeselector.js plugin
  • slick.rowmovemanager.js plugin
  • public interface: onDragInit, onDragStart, onDrag, onDragEnd
    I can provide a pull request for the column resizing implementation, which would make the dependency to jquery.event.drag optional for many developers.
@ghiscoding
Copy link
Collaborator

Thanks for filling this issue, I am also interested in this since it is causing a similar issue with a user that also included Froala Editor library.

I tried your 1st patch but only commenting out the 2 lines 157-158 with return false was not enough, I also had to comment out line 82 $event.add( this, "touchstart mousedown", drag.init, data ); but if I do that then the resizing column header stops working.

Option 2 is probably better but might require more changes and touching a few files.

If you're willing to create a PR, I would vote for option 2

@6pac
Copy link
Owner

6pac commented May 9, 2018

This has been attempted, but got very quickly complicated. Check out on the new Slickgrid website - there's a section discussing this.

It would be great to get rid of the dependencies, if it is indeed possible without major headaches.
I suspect option 1 is going to be more viable, and it would be good to have an option to control 'new' or 'old' behaviour.

@ghiscoding
Copy link
Collaborator

@6pac
Congratulation on the new site, it has most of everything in there :)

Option 1 wasn't working in my case though (with this other lib that requires touch/mouse), I tried modifying a few other lines in the jquery.event.drag file but until I remove the event completely (which removes the resizing feature). So returning false might work for some but doesn't seem to work for all.

@6pac
Copy link
Owner

6pac commented May 9, 2018

The site is basic, but hopefully it will take care of the most important issues so we can just link to it rather than having to chase down the appropriate issues. Any suggestions or enhancements welcome! I'll get around to putting it on the wiki or its own github account soon.

Would it be possible for you to put together a PR for a really simple example with the touch lib in the tests folder? Would be good to have a shared test case.

@ghiscoding
Copy link
Collaborator

ghiscoding commented May 9, 2018

@6pac
I was provided the following test site for the issue that I described with the other lib Froala Editor. It is using my Aurelia-Slickgrid lib (which is just a wrapper of SlickGrid) and after installing it, I went in the node_modules and modified, as @yamass described, the file jquery.event.drag-2.3.0.js (which is the one used by my lib)

git clone https://github.com/haimavni/test-grid.git
cd test-grid
npm install
au run --watch
In your browser, enter the address localhost:9000

@ghiscoding
Copy link
Collaborator

ghiscoding commented Jun 4, 2018

@6pac
I will reply to your comment you've put in the other issue #248

I think reworking SlickGrid to remove the jQuery drag and drop dependencies is a great idea. HOWEVER, articles like this one scare the hell out of me. Seems no-one has posted much about native browser HTML5 Drag and Drop since 2010 - there's a lot of talk from that year (eg. here, here and more recent but less complete here ), but I can't find anywhere where someone has said 'this is all resolved now'. So I'm forced to conclude that we're opening a big time can of worms trying to do that - and that's what the contributor who originally tried and failed to do it said: see #14 #15

Here's my reply:
If you look at the es6-slickgrid (mentioned in #41), that person took your repo and converted it to ES6. He dropped the jquery drag and drop and replaced it by interact.js in the his commit. This interact lib is very active, so I'm quite sure they have covered the issue that jquery drag still has, mainly global events. From their feature list, they do mention it's standalone and customizable

Here's the features shown on their GitHub

Features include:

  • inertia and snapping
  • multi-touch, simultaneous interactions
  • cross browser and device, supporting the desktop and mobile versions of Chrome, Firefox and Opera as - well as Internet Explorer 9+
  • interaction with SVG elements
  • being standalone and customizable
  • not modifying the DOM except to change the cursor (but you can disable that)

@loonix
Copy link
Contributor

loonix commented May 10, 2019

Hey guys, was looking for the solution for this issue and made a workaround that works with froala and slickgrid (on my current project), if it suits to someone:

loonix@ff97884#diff-28bc52c53e20a72616236359a43571a2

@ghiscoding
Copy link
Collaborator

@loonix
Can you explain what the hack does?
Would that be generic enough to avoid having conflicts with any other 3rd party lib (not just Froala)?
If so, we could do a PR if it helps a lot of people

@loonix
Copy link
Contributor

loonix commented May 10, 2019

When it initiates it does not detect if you have just selected text (if I am not wrong) so I have added a conditional that checks if the user is dragging, if it is not dragging it is selecting, so I set the textselect to true.

I haven't tested with other plugins, for me at the moment is working:
Froala text editor
Slickgrid - select cells and reorder

Happy to do a PR if everyone agrees

@ghiscoding
Copy link
Collaborator

@loonix
Thanks for the explanation, it helps in understanding it, make sense to me :)

@6pac
Would you be ok for a PR the changes mentioned in previous post?

@loonix
Copy link
Contributor

loonix commented May 13, 2019

@6pac @ghiscoding made the PR.
Any chance you know when this will go in?

6pac added a commit that referenced this issue May 13, 2019
Fix to Fix jquery.event.drag suppressing native drag and drop #239
@ghiscoding
Copy link
Collaborator

ghiscoding commented Jan 9, 2020

@yamass @loonix
Took a look at this and I think I found a solution, by doing a Google search, I found this on Froala GitHub:

The jquery.event.drag has a not option which defines what elements on the page drag doesn't apply to. If you set that option to .froala-box *, .froala-editor * it works very well. From my point of view, jquery.event.drag is not implemented right because by default it prevents clicking on all elements from the page except input tags.

With this in mind, I came up with 2 lines of code to change in the slick.event.drag.js file. It will discard any DOM elements that doesn't start with the word slick, by doing so we are making sure that we are in a Slick Grid. This should fix all issues (Froala and Full Date Calendar).

change from

// check for suppressed selector
if ( $( event.target ).is( dd.not ) )

to this new change

// check for suppressed selector 
// make sure the target css class starts with "slick-" so that we know it's in a Slick Grid and not outside of it
var targetClass = $( event.target ).attr('class') || "";                
if ( $( event.target ).is( dd.not ) || (!targetClass || targetClass.toString().indexOf('slick-') === -1))
  return;

I'll make a PR soon, though I would be happy to get some feedback

EDIT
After some tweaking, I tested the fix with the Froala version (an older version, 2.3.5) that was causing the issue and it works. So I'm quite sure that this patch will close this issue. I also tested a few of the SlickGrid examples (column resize, column drag, cell selection to copy) and everything works. I will prepare a PR then.

@6pac 6pac closed this as completed in #454 Jan 10, 2020
6pac added a commit that referenced this issue Jan 10, 2020
fix(drag): make sure drag event are only for slickgrid, fixes #239
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 a pull request may close this issue.

4 participants