6pac / SlickGrid

A lightning fast JavaScript grid/spreadsheet
https://github.com/6pac/SlickGrid/wiki
MIT License
1.84k stars 423 forks source link

Drag is not working in Mobile mode #488

Closed datcv closed 2 years ago

datcv commented 4 years ago

I am using the example from example-spreadsheet.html, it is using cellrangeselector. When I switch to mobile mode, then I cannot drag the grid, lots of exceptions occur.

Uncaught TypeError: Cannot read property 'top' of null at CellRangeDecorator.show (slick.cellrangedecorator.js:51) at SlickGrid.handleDragStart (slick.cellrangeselector.js:115) at Event.notify (slick.core.js:183) at trigger (slick.grid.js:2557) at HTMLDivElement.handleDragStart (slick.grid.js:4372) at HTMLDivElement.dispatch (smartadmin?v=sWsW48cfF7dhZ0bfrGhYAu735pMkaKh_DjqCPyjcA0A1:1) at HTMLDivElement.$event.dispatch (jquery.event.drag-2.3.0.js:385) at HTMLDivElement. (jquery.event.drag-2.3.0.js:270) at Function.each (smartadmin?v=sWsW48cfF7dhZ0bfrGhYAu735pMkaKh_DjqCPyjcA0A1:1) at i.fn.init.each (smartadmin?v=sWsW48cfF7dhZ0bfrGhYAu735pMkaKh_DjqCPyjcA0A1:1)

6pac commented 4 years ago

I can confirm that you are correct, but I don't know why this is the case. It appears that mobile mode handles the drag event quite differently to non-mobile mode. I'd have to look into it more to offer some opinion as to why.

ghiscoding commented 4 years ago

Not surprising since this lib was built when the first iPhone was not even out yet lol Touch is not the same as drag though, I don't even know if it's possible to drag with touch!?

datcv commented 4 years ago

I can confirm that you are correct, but I don't know why this is the case. It appears that mobile mode handles the drag event quite differently to non-mobile mode. I'd have to look into it more to offer some opinion as to why.

I am not sure if this can fix it or not, but while debugging it, I came with a temporary fix for that. I updated the handleDragStart in slick.grid.js as below. It just works for my purpose but not sure there are any side effects.

function handleDragStart(e, dd) {
+            if (typeof dd.startX === "undefined") {
+               if (e.cancelable)
+                    e.preventDefault();
+                return false;
+           }

            var cell = getCellFromEvent(e);
            if (!cell || !cellExists(cell.row, cell.cell)) {
                return false;
            }

            var retval = trigger(self.onDragStart, dd, e);
            if (e.isImmediatePropagationStopped()) {
                return retval;
            }

            return false;
}
ghiscoding commented 4 years ago

can you highlight the differences? you could use ```diff and add + or - in front of each line you changed, that would help us a lot identify what you've changed. Thanks

datcv commented 4 years ago

Basically, I have added a condition check for startX:

+            if (typeof dd.startX === "undefined") {
+               if (e.cancelable)
+                    e.preventDefault();
+                return false;
+           }
6pac commented 4 years ago

I read an article to jog my memory, and I remember now, apart from hover, the touch and mouse interfaces should be compatible. It does appear that by default they are being interpreted differently. Will check out your changes.

ghiscoding commented 4 years ago

Have you tried a few examples from the SlickGrid project? Perhaps you can do a PR and see if any of the Cypress E2E tests that I've created here all pass. I think I had couple of test to drag a column to another to another position and/or make a column wider.

ghiscoding commented 4 years ago

@datcv Why not fix the slick.cellrangedecorator.js file instead? By changing this line

We could just wrap the _elem.css inside an if condition to make sure the object exists before trying to read it.

+ if (from && to && options && options.offset) {
     _elem.css({
       top: from.top + options.offset.top,
       left: from.left + options.offset.left,
       height: to.bottom - from.top + options.offset.height,
       width: to.right - from.left + options.offset.width
     });
+  }

That wouldn't fix the touch problem, but it would certainly fix the error thrown. We want to avoid having to change any code in the slick.core.js as much as possible.

EDIT

I created a PR #496 it might not fix your issue but it should get rid of the errors thrown in the console.

ghiscoding commented 4 years ago

@datcv Now that the PR #496 is merged and a new version was released including this code change, does that help with your issue? Some feedback would be useful, thanks.

BCPaulPaul commented 4 years ago

Is this fixed in the demo? I cannot drag (move rows) in Chrome on Android using the demo.

This demo doesn't do anything on Chrome Android: https://6pac.github.io/SlickGrid/examples/example-row-detail-selection-and-move.html

This demo always puts the dragged row on the top, not matter where it is dragged, on Chrome Android: http://6pac.github.io/SlickGrid/examples/example9-row-reordering-simple.html

Thanks

Edit: Note that if I connect a Bluetooth mouse to my Android phone, then the drag and drop works in Chrome Android. So it is likely just not handling touch events properly.

ghiscoding commented 4 years ago

Is this fixed in the demo? I cannot drag (move rows) in Chrome on Android using the demo.

Technically speaking a drag event is not a touch event (move) and that is normal that nothing happens, you just can't drag on a mobile phone with SlickGrid. I'll mention it again, SlickGrid came out before the first iPhone (first touch phone) was created and there's no touch friendly lib used by SlickGrid. The lib that SlickGrid uses jquery.event.drag is a jQuery customized lib and I'm 100% sure that it doesn't support any touch event. The PR that I mentioned earlier is simply to remove the errors thrown in the console, nothing more.

... so yes that is normal that nothing happens.

BCPaulPaul commented 4 years ago

Any plans to make it work on mobile devices? If not, I will be forced to use another library, which I don't really want to do because SlickGrid is ... really slick!

ghiscoding commented 4 years ago

Well this is an Open Source project so if anyone has time to replace jquery.event.drag by a more touch friendly lib, then please go for it and submit a PR, a small hint though, it's not an easy task... just saying

I know that the person who created the slickgrid-es6 fork replaced it with Interact.js but I don't know if that works with mobile phone or not (his fork has a demo, perhaps someone can try on a mobile phone and confirm). I personally don't care about not having drag on a mobile phone, so I won't work on this.

BCPaulPaul commented 4 years ago

I understand. Not sure if my skills are sufficiently advanced to craft a solid solution. I might try. Thanks

ghiscoding commented 2 years ago

@BCPaulPaul Hi there, I know you've opened this issue a while ago but good news, this is currently in active development and will be fixed in the next major version which will remove jQueryUI and replace jquery.event.drag-2.3.js with a custom implementation that works with touch. The next major version is expected within a month from now, you can see below a demo of dragging with touch on a laptop

brave_SuQeVsuyYj

ghiscoding commented 2 years ago

closed by recent PR #695, take a look at the new 3.0.0 release. We now have a new slick.interactions.js that added support for touch, which fixes this issue, and replaces jquery.event.drag-2.3.0.js which didn't support touch