bug: Drag events not working in Android 4.4.2

[jgw96]

From @MartinMa on May 11, 2015 6:25

Type: bug

Platform: android 4.4 webview

This issue refers to a regression introduced with this commit from 19 Mar.

This is a new issue as desired by @perrygovier. Again (concerning above commit), what exactly do you mean by "Prevent gestures from breaking native scrolling"? Is there a use/test case? I could't find a related issue.

Here is a codepen that demonstrates the issue (try to drag the red box): http://codepen.io/MartinMa/pen/vOEmVR

Please confirm.

To get it up and running on your Android device, run the following commands using Ionic CLI.

ionic start dragtest http://codepen.io/MartinMa/pen/vOEmVR
cd dragtest
ionic platform add android
ionic run android

As already mentioned by @gregor-srdic, this issue also affects built-in events like on-hold. (see issue 1729)

There is a work around. You have to pass {prevent_default_directions = ['left', 'up', 'right', 'down']} as fourth argument to $ionicGesture.on.

I think preventDefault(); should be the default, if 'overflow-scroll' isn't present? With the current behavior it breaks drag gestures when using Ionic scroll - which is the default. So it's broken by default ;)

Copied from original issue: driftyco/ionic#3695

[jgw96]

From @perrygovier on May 11, 2015 18:24

Thanks for creating a new issue for this. I'd like to make it as a fast follower for the 1.0.

[jgw96]

From @perrygovier on May 20, 2015 16:16

Closed with #3769 on the 1.0.1 branch.

Thanks!

[jgw96]

From @MartinMa on May 21, 2015 7:49

Perfect, a one-liner. :+1: Thanks! :beers:

[jgw96]

From @benlozano on December 5, 2015 20:12

@perrygovier I confirmed that this fix exists in my current ionic (1.1.0) and drag events are still broken on 4.4.4 (and have always been for me). Any insight into whether this is the same as your issue? I have seen this get tossed around and really haven't had any luck getting the ionic gestures to work cross-platform.

[jgw96]

From @MartinMa on December 22, 2015 9:21

I can confirm that this issue is present (again) on Android 4.4.2 running Ionic 1.2.1. This seems to be a regression (again) introduced with commit e10b5d23c9cb3e04c6374574303e55aef6aefee4

This part of the code seems to change back and forth because of an iOS scrolling issue (see #4008).

If you have an Android device with a 4.4 webview you can reproduce the issue as mentioned above:

ionic start dragtest http://codepen.io/MartinMa/pen/vOEmVR
cd dragtest
ionic platform add android
ionic run android

[jgw96]

From @mlynch on December 24, 2015 3:20

Okay, I just reverted that old fix to enable dragging to work correctly. Looking more closely, I think the fix for the side menu issue in #4022 was not correct.

I'm still looking into how to get these both playing well together for 1.2.1.

[jgw96]

From @jskrzypek on December 24, 2015 9:14

@mlynch I want to be sure that this doesn't break native scrolling again...

[jgw96]

From @mlynch on December 24, 2015 15:38

I'm not really sure what the original issue was, and scrolling works fine with this change on iOS and Android. Any more info or thoughts on the impact?

[jgw96]

From @jskrzypek on December 24, 2015 18:21

You can see the original issue by following the steps to reproduce i listed in #4008 in the ios emulator or on a device.

It might no longer be an issue I'll try check it tomorrow.

[jgw96]

From @MartinMa on December 28, 2015 15:15

I created a new build of Ionic based on the current master branch (with the reverted old fix). The good news is, that the red-box-dragging-demo is working again.

Unfortunately native scrolling is broken for me now. It doesn't work at all, even though the webview is receiving all touchmove events. I used an example with a simple list as described in #4022 (first post). JS-scrolling on the other hand works fine.

All testing done on a Samsung Galaxy Tab 3 with Android 4.4.2.

This webview bug is also documented on the Android issue tracker: WebView touch events are not fired properly. See comment 41 and later.

This seems to be "normal behavior" on Android 4.4 "KitKat". The first touchmove event is fireing reliably, but immediately followed by touchcancel, which results in a release event by the ionic-ified hammer.js implementation.

If the browser is detecting a scroll gesture it cancels the touch event ("touchcancel") and native browser scrolling takes over.

Then, all subsequent touchmove events never reach the webview. But if you prevent default, you get all touch events, and native scrolling doesn't work. It's kind of a vicious cycle.

What do you think is the best solution? Fallback to js-scrolling on Android 4.4?

Which devices are you testing on?

[jgw96]

From @mlynch on December 28, 2015 15:39

I tested on 4.4.2 and it worked fine, so let's dig in: are you adding any gesture listeners to your app? Or is it breaking on a vanilla example?

[jgw96]

From @mlynch on December 29, 2015 2:10

Okay, can confirm now. Looking into this and will fix for 1.2.2

[jgw96]

From @mlynch on December 29, 2015 17:42

Fix for this will be out in 1.2.2

[jgw96]

From @mlynch on December 31, 2015 4:8

Grrr, that had bad side effects. Need a different solution.

[jgw96]

From @MartinMa on January 1, 2016 10:34

Hey @mlynch thank you very much for your effort! What kind of side effects? Have you really been able to reproduce? For me it is breaking in a vanilla example. I'll try to provide a new example utilizing native scrolling and custom drag gestures. From what you are experiencing it doesn't seem to affect "KitKat" in general.

[jgw96]

From @MartinMa on January 5, 2016 10:19

I made a quick minimal vanilla example demonstrating the general issue at MartinMa/drag_vanilla with a possible solution. It is a simple Cordova project (build instructions included).

On Android 4.4.2 (at least on my device) the situation is as follows. I register event handlers for touchstart, touchmove, touchend and touchcancel. When doing a common swipe or drag gesture, the event handlers are fired three times. 1. touchstart, 2. touchmove and 3. touchcancel.

What's wrong here? Actually touchmove should be triggered multiple times and the gesture should end with touchend. What happens is, that after touchcancel "the browser takes over" and performs native scrolling. Also, all subsequent touchmove events don't make to to the webview.

But if you preventDefault on the first touchmove (or already at touchstart, that works as well), the touchmove events keep coming. This is demonstrated in the example (see line 36 in index.js). At the same time, native scrolling of the unordered list works fine, because preventDefault isn't called here. The touch events are only triggered in the "drag element" (red box).

For Ionic, I think, we need to figure out whether js-scrolling or native scrolling is active in a specific content view. If native scrolling is active: don't preventDefault, otherwise: always preventDefault on the first move to make the touch events keep coming in.

[jgw96]

From @MartinMa on January 5, 2016 16:13

@mlynch I added a pull request with a fix for this issue. I also removed the length check because it is not needed.

The fix is inspired by @perrygovier (see commit 71e89715eec85a03ceb04d53f8406dab75ac71dc fix(refresher): fix pull to refresh with native scrolling on kitkat).

(Sorry for the mess with the GitHub references above. I had to update my pull request because of a linter error.)

[jgw96]

From @jskrzypek on January 5, 2016 16:19

Interesting @MartinMa. Now I understand the flow of the system a bit better, that makes sense and I agree, we probably need two separate behaviors for the native- and js-scrolling. This also seems like it should be a separate check from killing the drag event if its going in the wrong direction.

Your fix is a step in the right direction but I think a slightly more general rewrite might be called for.

tl;dr: In js-scrolling calling preventDefault on the first touchmove bypasses the browser and allows javascript handling of the event, so perhaps the directions in the prevent_default_directions array should be the ones for which we want to enable scrolling. This causes problems with native scrolling, so, more usefully, we might make it so that when not doing native scrolling, prevent_default_directions are the directions for which we want to preventDefault every touchmove, not just the first.

I'm also wondering about the comment Perry added for explanation, is it actually telling us what's going on? It seems to me now that this describes the opposite of the actual behavior at that point.

// Prevent gestures that are not intended for this event handler from firing subsequent times

It comes from commit https://github.com/driftyco/ionic/commit/56ab0f2b5d7f5bd35ed6252749cfa1867c3b0f6a, but if we look at the code we see that if the current direction is in the prevent_default_directions array, then preventDefault is not called. This was the seemingly broken logic that I was trying to fix in https://github.com/driftyco/ionic/commit/e10b5d23c9cb3e04c6374574303e55aef6aefee4.

I think possibly what's happened is that the logic needs to get inverted between the js-scrolling and native scrolling scenarios, as you suggest, but this wasn't always clear during development. Additionally I think the logic around the implementation of prevent_default_directions is buggy and should be moved.

It seems to me that the code Perry wrote in https://github.com/driftyco/ionic/commit/56ab0f2b5d7f5bd35ed6252749cfa1867c3b0f6a only worked to cancel wrong-direction drags in js-scrolling by abusing the fact that no overflow-scroll was enabled for those directions. When the drag handler received a drag event that started off in the wrong direction (i.e. a direction named in prevent_default_directions), preventDefault would not get called and the browser captured the event. When this happened, because there was no overflow for the browser to natively scroll over, the wrong-direction drag events were effectively canceled, because the native scrolling would swallow the subsequent touchmove events. I think because the logic was, the separate ideas of "enable js-scrolling by calling preventDefault so we receive touchmove" and "don't obey wrong-direction drags by killing touchmove events with bad directions" got coupled together. This coupling makes it hard to allow for both js-crolling and native scrolling scenarios. we need to decouple this code before moving forward. Additionally we should figure out if this way of cancelling wrong-direction drag gestures is actually the best way. We have separate overflow-x and overflow-y that should work on all browsers, so we shouldn't need to bother with disabling these in native, but what if we're doing js-scrolling and the element has inherited overflow-scroll for the wrong directions? I think the current regime will just hand off to the browser and the native scroll will happen even in the wrong directions. What we might want to do is call stopPropogating after preventDefault to prevent the browser from taking over, but that is possibly dangerous.

Here's a verbal description of what I think we should be doing in the drag handler, taken form the top:

• First check to see if we are in a js-scrolling or native scrolling scenario. I'm not sure what the best way to do this is, but one way I've thought of is to test the computed styles of the element being dragged and see if it has overflow scrolling enabled. We can do that with a test like this one:

  var targetStyle = window.getComputedStyle(ev.target);
  if (targetStyle.overflow === 'auto' || targetStyle.overflow === 'scroll' || targetStyle.overflowX === 'auto' ||   targetStyle.overflowY === 'scroll') {
    // overflow scrolls in at least one direction, so we are in a native scrolling scenario.
  }

• In the native scrolling scenario we should not call preventDefault in any direction. If people are using native scrolling and they want to limit scrolling in some direction, differentiating between overflow-x and overflow-y properties already handles that. Maybe there are other complexities, but as far as I can tell this is what overflow-scroll is for, and anything more complex should just be handled by its own logic passed to $ionicGesture.on.
• In the js-scrolling scenario preventDefault should be called on either touchstart or the first touchmove, regardless of the direction. On subsequent touchmoves, if the direction of the move is good, we let things happen as normal, but if the direction is bad we should call preventDefault on it to stop these events from having further consequences. We could also return false to ensure that they are completely quashed. A benefit of this scenario is that even if the first touchmove goes in the wrong direction we still allow subsequent correct-direction touchmoves to be processed by the handler.

[jgw96]

From @MartinMa on January 5, 2016 16:51

Hi @jskrzypek I agree that the coupling you mentioned is unfortunate. That's why I removed the length check altogether in the pull request. The code is equivalent without it. indexOf will return -1 when the array is empty. But I'm not sure if "cancelling wrong-direction drag gestures" works in all scenarios. I need to give this more thought.

I think the fix proposal doesn't break native scrolling. I tested it on my device, it works. This is because the touchstart covered in the new check stems from a custom attached gesture. It is only triggered on elements that get attached user defined gestures (i.e. $ionicGesture.on). Also, this codepath is only relevant on a specific version Android. So it doesn't affect the other platforms.

I have to think about your description...

[jgw96]

From @MartinMa on January 6, 2016 12:57

I added an example to prove that this fix is functioning correctly: https://github.com/MartinMa/dragtest

I also added an explanation of how and why it works (see README.md). I'd be very happy see this fix land and be included in the next release 1.2.4. :smile:

@jskrzypek I think we should move the prevent_default_directions topic to a new issue. From the built-in components only listView and sideMenuContent use this feature.

[jgw96]

From @MartinMa on January 11, 2016 16:19

Hold up. The fix provided, doesn't play well with sideMenuContent (and probably listView, too) — it prevents native scrolling there.

Update I updated the pull request and moved the check (slightly altered) to detect. Should be good now. I can add more examples if you like.

[jgw96]

From @harpritt on April 21, 2016 22:25

Hi all, will @MartinMa 's fix make it to the next release?

Cheers in advance

[jgw96]

From @wbhob on January 6, 2017 2:40

It probably made it into 1.3.2. I don't use v1 anymore, can anyone verify?