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

Dragging in mobile is buggy - scrolls page and triggers error #1022

Closed
itaibh opened this issue Jan 15, 2017 · 30 comments
Closed

Dragging in mobile is buggy - scrolls page and triggers error #1022

itaibh opened this issue Jan 15, 2017 · 30 comments
Labels

Comments

@itaibh
Copy link

itaibh commented Jan 15, 2017

When I switch to mobile mode in Chrome 55 and I try to drag some sortable element, I get this error on every "finger" move:

Sortable.js?1:565 Unable to preventDefault inside passive event listener due to target being treated as passive. See https://www.chromestatus.com/features/5093566007214080

In addition, if the page has where to scroll, it scrolls.

If you want to see for yourself you can go to the official Sortable.js demo (http://rubaxa.github.io/Sortable/), open up dev tools, enable device emulation mode and start moving elements around.

@itaibh
Copy link
Author

itaibh commented Jan 15, 2017

The easy fix (not sure if correct fix, though) is to update the implementation of the function _on in line 1017 (at least in my version) to:

    function _on(el, event, fn) {
        el.addEventListener(event, fn, {capture: false, passive: false});
    }

meaning - to change the last parameter from false to {capture:false, passive: false}, since since chromium version 54 the default for passive mode in mobile devices changed to true.

@jeffreznik
Copy link

Can this please be fixed? The latest Chrome update broke Sortable.js on mobile.

@mackerson
Copy link

+1, working on Chrome 55 on android 7.1.1, broken on Chrome 56 on android 6.0.1. Also seeing this issue in mobile Safari. Reproducible on desktop Chrome 56 with device emulation enabled.

@mumblyOMOD
Copy link

+1

@RubaXa
Copy link
Collaborator

RubaXa commented Feb 16, 2017

Fixed: 42f57f1

@RubaXa RubaXa added the bug label Feb 16, 2017
@mackerson
Copy link

Confirmed working on latest in chrome 56! Thank you! I just tossed some cash into the donation link on the site. I'm still seeing the behavior on mobile Safari, although the behavior is consistent with the way it was behaving on chrome - is it possible that Safari is also ignoring the preventDefault?

@RubaXa
Copy link
Collaborator

RubaXa commented Feb 16, 2017

Quite possible.

@RByers
Copy link

RByers commented Feb 17, 2017

Sorry for the trouble, this is a breaking change in Chrome 56 to improve scroll performance. Note that your fix isn't compatible with browsers that don't support EventListenerOptions - you may want to use feature detection.

However, it looks like your code also supports pointer events - which don't disable scrolling via preventDefault either (regardless of the passive setting). I checked in Microsoft Edge with a touchscreen and it's indeed broken there as well. I believe the right fix (for both pointer events and Chrome 56+ touch events) is to add a touch-action:none CSS rule to explicitly disable touch scrolling over any draggable item. I verified such a change fixes the demo so that it works properly on Edge.

RubaXa added a commit that referenced this issue Feb 17, 2017
@RubaXa
Copy link
Collaborator

RubaXa commented Feb 17, 2017

@RByers try touch-action-branch (314db5e)

@SaeedPrez
Copy link

Thanks @RubaXa, the touch-action:none seems to have temporarily fixed the issue until you release next version.

@RubaXa
Copy link
Collaborator

RubaXa commented Feb 17, 2017

So? Add or not it is in the master?

@elonmallin
Copy link
Contributor

Hi, I think the way to fix it must be with feature detection as @RByers mentioned.
touch-action:none might solve it for some cases but what if you have a sortable list that takes up the whole screen? Then you can't scroll. The delay property is supposed to solve this I guess so that you can both scroll when touch dragging over your sortable list and start a drag if you press and hold for the delay.

So I guess something like the below?

var supportsPassive = false;
try {
  var opts = Object.defineProperty({}, 'passive', {
    get: function() {
      supportsPassive = true;
    }
  });
  window.addEventListener("test", null, opts);
} catch (e) {}
captureMode = supportsPassive ? {capture: false, passive: false} : false,

// remove this:
// el.style['touch-action'] = 'none'; // #1022

@RByers
Copy link

RByers commented Feb 17, 2017

If you really want to support both touch scrolling and dragging based on how long the user holds their finger down after touchstart, then yeah using passive:false (and having potentially janky scrolling as a result) is unfortunately the only option today.

Can someone point to a demo where that works? I'm curious to see what the UX feels like - it sounds pretty strange to support both interactions with roughly the same gesture. Is this is a "long press to pick up and then drag" sort of UX?

@elonmallin
Copy link
Contributor

Don't think there is a demo for this. But delay is something you set in the options object when you create your sortable. So you decide the milliseconds you want. It is not a totally uncommon use-case I guess. Every phone has their apps on the home screens, you can reorder if you first do a long press but also swipe left and right to get to another screen.

@RByers
Copy link

RByers commented Feb 17, 2017

Thanks, yes I agree that with the right affordances that's a reasonable UX. Spec issue filed, thanks!

@RubaXa
Copy link
Collaborator

RubaXa commented Feb 17, 2017

@elonmallin PR?

@elonmallin
Copy link
Contributor

@RubaXa Sure, see if this works #1042

@RubaXa
Copy link
Collaborator

RubaXa commented Feb 17, 2017

I specifically left the possibility: https://github.com/RubaXa/Sortable/blob/touch-action/Sortable.js#L70

@elonmallin
Copy link
Contributor

#1043 Guess you meant that the PR should be in the touch-action branch? Also removed the touch-action:none.

@RubaXa
Copy link
Collaborator

RubaXa commented Feb 18, 2017

Done.

@SaeedPrez
Copy link

@RubaXa when are you planning to tag a new release so we can update via NPM? Or is it possible to update without you first tagging?

@RubaXa
Copy link
Collaborator

RubaXa commented Feb 18, 2017

Tomorrow.

@RubaXa
Copy link
Collaborator

RubaXa commented Feb 19, 2017

sortablejs@1.5.0

@ohute-couture
Copy link
Contributor

I installed sortablejs@1.5.1 and this bug was not resolved
(http://rubaxa.github.io/Sortable/)

1d1e781
The calling sequence seems to be a problem.
I ask for confirmation.

@nomanr
Copy link

nomanr commented May 10, 2017

1.5.1 not working on chrome for mobile or WebView chrome client. Any update on this issue? Thanks

@davidcp90
Copy link

1.6.0 is throwing the same error. Any ideas on how to fix it?

@yu1222
Copy link
Contributor

yu1222 commented Sep 20, 2017

Any update?

@wattrobert
Copy link

Just checking if this is still on the table for being fixed?

@owen-m1
Copy link
Member

owen-m1 commented Dec 2, 2018

@wattrobert This error will not exist in the next version

@owen-m1
Copy link
Member

owen-m1 commented Jan 17, 2019

In 1.8.1 it should now work

@owen-m1 owen-m1 closed this as completed Jan 17, 2019
This issue was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests