bug: native scrolling broken on ios because event.preventDefault() being called on drag gestures by default when inst.options.prevent_default_directions is empty

[jskrzypek]

Type: bug

Platform: ios 8 webview

@perrygovier (and the rest of the team but the commit below is Perry's)

The current nightly build breaks native scrolling in ion-contents when they don't have an ion-list or an ion-scroll wrapping the contents. The view is not scrollable.

To reproduce build a project with the code in this codepen: http://codepen.io/jskrzypek/pen/oXpPmG in this repo: https://github.com/jskrzypek/4008-preventDefault and then follow the instructions on the readme (or below).

I think this commit introduced the bug: 56ab0f2, it's on line https://github.com/driftyco/ionic/blob/master/js/utils/gestures.js#L1174.

It seems this is more likely what was meant (and fixes the bug):

        // Prevent gestures that are not intended for this event handler from firing subsequent times
-        if (inst.options.prevent_default_directions.length === 0
-           || inst.options.prevent_default_directions.indexOf(ev.direction) != -1) {
+       if (inst.options.prevent_default_directions.length > 0
+           && inst.options.prevent_default_directions.indexOf(ev.direction) != -1) {
           ev.srcEvent.preventDefault();

[jskrzypek]

Fix submitted as a PR.

[jskrzypek]

This bug seems to be present on android as well as iOS, which makes sense given that it's coming from the drag gesture code and not from platform-specific code or implementations.

See #4022 for documentation of the issue on Android 4.4 webview

[mhartington]

Just tested this on ios and android. Seems to be working fine for me with 1.0.1. Is there any other information you could provide? I can't seem to replicate it.

[jskrzypek]

@mhartington I'll be in the office tomorrow and push up a repo. I'm happy to help you reproduce on Monday.

[jskrzypek]

I'm having trouble replicating the problem. Fixing the code the way I mentioned fixes the bug we see in our app, but I am having trouble recreating the issue in a demonstration app. I will update once I manage that.

[jskrzypek]

Ok I got it. (The problem with replication was happening because of some very strange behaviour when the html template from my pen is in the state passed to $urlRouterProvider.otherwise(). But I'll document to that later. Also I cannot replicate the error on my android device, but it looks like the same thing that is blocking the bug from the $urlRouterProvider might be (incorrectly) preventing the error from happening on android.)

Here is a repo that demonstrates the bug we see: https://github.com/jskrzypek/4008-preventDefault.

Shameless plug side-note: It's built with generator-mcfly our yeoman generator that gives you a pre-wired gulp & browserify build system for angular apps and options to automatically integrate ionic.

Steps to reproduce:

1. Clone the repo & cd into the directory

   git clone https://github.com/jskrzypek/4008-preventDefault#master && cd 4008-preventDefault
   

2. Install npm & bower dependencies

   npm install && bower install
   

3. Use git to apply a patch to stick some console.log messages on the Drag handler

   git apply ionic.js.patch
   

4. Use our gulp task to build the distributed code (using browserify)

   gulp dist
   

5. cd into the directory where the ionic project sits

   cd dist/app/dev
   

6. Add platforms

   ionic platform add android ios
   

7. Run/build for either platform with ionic run .... Note that I am currently only able to reproduce this bug in ios.

8. Attach your console (safari developer view for ios or chrome://inspect#devices for android)

9. In the tab labeled ion-list you should be able to scroll fine via dragging, and should see messages logged by the drag handler from our patch.

10. Switch to the tab labeled ion-content. On ios you will not be able to scroll via dragging, but messages will be logged to the console showing us that the ev.preventDefault() method is being called on these legitimate drag events.

11. Click the red heart in the upper-right corner. This will run the drag-handler.js script that swaps out the current version of the handler for the one with my suggested fixes.

12. Now you can scroll just fine in the ion-content tab and ev.preventDefault() is not being called when it shouldn't be.

[jskrzypek]

@mhartington Just letting you know that the repo is live with the bug replication. The other issue that was blocking it, I'm not really sure how I should start documenting...

[KillerCodeMonkey]

For me it is working on android devices, but still not scrolling on ios

http://forum.ionicframework.com/t/overflow-scroll-true-ios-scrolling-not-working/28007

[Gunbit]

@jskrzypek i checked your repo out and tested it with IOS and i love your red heart but I have no idea what it does. It would fix my Problem.

[krlwlfrt]

+1

[jskrzypek]

@Gunbit All that button does is run a script that replaces ionic.Gestures.gestures.Drag.handler with a nearly identical function that has this change in it:

        // Prevent gestures that are not intended for this event handler from firing subsequent times
-        if (inst.options.prevent_default_directions.length === 0
-           || inst.options.prevent_default_directions.indexOf(ev.direction) != -1) {
+       if (inst.options.prevent_default_directions.length > 0
+           && inst.options.prevent_default_directions.indexOf(ev.direction) != -1) {
           ev.srcEvent.preventDefault();

Those lines are found here: https://github.com/driftyco/ionic/blob/master/js/utils/gestures.js#L1174-L1175
Thanks should go to @ghaiat for locating the bug 😄

[jskrzypek]

A workaround to implement this fix would be to add the following run block. You might put also want to wrap it in a deviceready or $ionicPlatform.ready() listener to make sure this happens after ionic is attached to the window.

.run(function() {
    ionic.Gestures.gestures.Drag.handler = function dragGesture(ev, inst) {
        if(ev.srcEvent.type == 'touchstart' || ev.srcEvent.type == 'touchend') {
            this.preventedFirstMove = false;

        } else if(!this.preventedFirstMove && ev.srcEvent.type == 'touchmove') {
            // Prevent gestures that are not intended for this event handler from firing subsequent times
            if(inst.options.prevent_default_directions.length > 0 && inst.options.prevent_default_directions.indexOf(ev.direction) != -1) {
                ev.srcEvent.preventDefault();
            }
            this.preventedFirstMove = true;
        }

        // current gesture isnt drag, but dragged is true
        // this means an other gesture is busy. now call dragend
        if(ionic.Gestures.detection.current.name != this.name && this.triggered) {
            inst.trigger(this.name + 'end', ev);
            this.triggered = false;
            return;
        }

        // max touches
        if(inst.options.drag_max_touches > 0 &&
            ev.touches.length > inst.options.drag_max_touches) {
            return;
        }

        switch(ev.eventType) {
            case ionic.Gestures.EVENT_START:
                this.triggered = false;
                break;

            case ionic.Gestures.EVENT_MOVE:
                // when the distance we moved is too small we skip this gesture
                // or we can be already in dragging
                if(ev.distance < inst.options.drag_min_distance &&
                    ionic.Gestures.detection.current.name != this.name) {
                    return;
                }

                // we are dragging!
                if(ionic.Gestures.detection.current.name != this.name) {
                    ionic.Gestures.detection.current.name = this.name;
                    if(inst.options.correct_for_drag_min_distance) {
                        // When a drag is triggered, set the event center to drag_min_distance pixels from the original event center.
                        // Without this correction, the dragged distance would jumpstart at drag_min_distance pixels instead of at 0.
                        // It might be useful to save the original start point somewhere
                        var factor = Math.abs(inst.options.drag_min_distance / ev.distance);
                        ionic.Gestures.detection.current.startEvent.center.pageX += ev.deltaX * factor;
                        ionic.Gestures.detection.current.startEvent.center.pageY += ev.deltaY * factor;

                        // recalculate event data using new start point
                        ev = ionic.Gestures.detection.extendEventData(ev);
                    }
                }

                // lock drag to axis?
                if(ionic.Gestures.detection.current.lastEvent.drag_locked_to_axis || (inst.options.drag_lock_to_axis && inst.options.drag_lock_min_distance <= ev.distance)) {
                    ev.drag_locked_to_axis = true;
                }
                var last_direction = ionic.Gestures.detection.current.lastEvent.direction;
                if(ev.drag_locked_to_axis && last_direction !== ev.direction) {
                    // keep direction on the axis that the drag gesture started on
                    if(ionic.Gestures.utils.isVertical(last_direction)) {
                        ev.direction = (ev.deltaY < 0) ? ionic.Gestures.DIRECTION_UP : ionic.Gestures.DIRECTION_DOWN;
                    } else {
                        ev.direction = (ev.deltaX < 0) ? ionic.Gestures.DIRECTION_LEFT : ionic.Gestures.DIRECTION_RIGHT;
                    }
                }

                // first time, trigger dragstart event
                if(!this.triggered) {
                    inst.trigger(this.name + 'start', ev);
                    this.triggered = true;
                }

                // trigger normal event
                inst.trigger(this.name, ev);

                // direction event, like dragdown
                inst.trigger(this.name + ev.direction, ev);

                // block the browser events
                if((inst.options.drag_block_vertical && ionic.Gestures.utils.isVertical(ev.direction)) ||
                    (inst.options.drag_block_horizontal && !ionic.Gestures.utils.isVertical(ev.direction))) {
                    ev.preventDefault();
                }
                break;

            case ionic.Gestures.EVENT_END:
                // trigger dragend
                if(this.triggered) {
                    inst.trigger(this.name + 'end', ev);
                }

                this.triggered = false;
                break;
        }
    };
})

[jskrzypek]

@mhartington Any progress on checking this out?

[mhartington]

Merged this into our 1.0.2 branch
e10b5d2

[jskrzypek]

@mhartington Awesome, thanks!

[huip]

@jskrzypek Your fixed have a side effects(ion-fresher can only work once)

[jskrzypek]

@huip can you elaborate what you mean

[huip]

@jskrzypek I mean if ion-content contains ion-fresher tag, when pull down to refresh item-list,the pull-down events can not work.

[KillerCodeMonkey]

@JoeyHoutenbos
clone the repo --> switch to branch 1.0.2 and use the gulp "bundle" task to create an own release

or run "gulp build --release" to mini and uglify

[KillerCodeMonkey]

I've built a custom release.. if someone needs:
https://github.com/KillerCodeMonkey/ionic/releases/tag/1.0.2_custom

[mhartington]

Looking into this.

[mhartington]

@JoeyHoutenbos Are you seeing the same issue as @lucasalmeida92 or are you just asking for a 1.0.2 release?

[mhartington]

@lucasalmeida92 and @huip I'm not seeing any issues with pull to refresh

https://youtu.be/pxGvC_copOo

[JoeyHoutenbos]

@KillerCodeMonkey I know I can do this and will use this. But I think the problem is too big to "just" merge it in the next milestone when we're not sure when this will be released. I want to use native scrolling as this improves the performance a lot!

Implementing a workaround by using native scrolling only for Android or implementing a fix as created by jskrzypek doesn't sound like a good solution. Sure I can make my own build of the 1.0.2 branch, but there might be other problems with this build since it's not released yet..
For that reason I'm using version 1.0.0 in some apps instead of the newer 1.0.1. So @mhartington I am asking (or just interested when the release is scheduled) for a 1.0.2 release to fix the scrolling issues on iOS. I don't have the same problem as lucasalmeida92, for me it's just iOS (in every app so far).

[mhartington]

Alright, I thought there was a big issue with the PR in question.

[lucasalmeida92]

@mhartington i dont know whats is happeing yet but when i have more details ill let u guys know!
Thank u for your support ! ;)

For now ill keep using the 1.0.0 for production.

[mhartington]

Sounds good. @JoeyHoutenbos this should be in master now. Feel free to pull from that

[mrtomrichy]

This fix allowed me to scroll on iOS8, however there are issues with the scroll momentum. The next touch after a fling is registered as a click and not a 'stop scrolling' event, and the touch areas have not updated meaning that it clicks whatever was in that position when the momentum kicked in.

[mhartington]

Do you have an example showing this @mrtomrichy

[mrtomrichy]

@mhartington I have made a test project which implemented the fix from above and it suffers from the same issue. You can clone the project from here:

https://github.com/mrtomrichy/Ionic-iOS8-Scroll-Issue

I have also recorded a video of the problem in the simulator, and I have also recreated it on device.

https://vimeo.com/136085384

[mrtomrichy]

@mhartington Don't suppose you've had a chance to look into this? After some more playing around I've noticed that scroll callbacks are not fired on inertial scrolling, which I suspect is part of the issue.

Cheers

[mhartington]

Will look into this today, could you open a new issue for this @mrtomrichy

[MartinMa]

This fix is in conflict with a behavior observed on Android 4.4 devices and caused a regression. The issue related is #3695. The === and || of the code section actually were intentional.

The scrolling issue only occured on iOS right? Maybe there is a need for a platform specific fix.

[jskrzypek]

@MartinMa What's the logic behind that? If the === and || of the code section were intentional as you say then isn't this comment describing the opposite of that behavior?

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

Prior to commit e10b5d2 this would seem to indicate that either the comment was incorrect or the code was.

Also the bug was documented on android 4.4 as well in issue #4022

[mlynch]

Did 1.2.1 resolve any of these issues? PTR with native scrolling received a number of related fixes

[mlynch]

I cannot reproduce this. Can someone provide a simple example case that shows the issue in Ionic today? Thanks.

[mlynch]

Closing as this should be fixed.

[gsanjeevkumar]

I have a situation with scrolling of list

in the controller i do ( also tried with or without Platform.isIOS Check... did not work

set the code for controller and the template... Please let me know where am I doing it wrong

`$scope.contentScrolling = function(){
if (ionic.Platform.isIOS()) {
$ionicScrollDelegate.$getByHandle('listscroll').resize();
};

};

        <ion-list class="item-previews">

            <ion-item href="#/item-details-phones/{{item.ID}}" class="item-icon-right item-preview" collection-repeat="item in itemsArray | orderBy: 'Name'">

                <div class="item-thumbnail" style="background-image: url( '{{item.ThumbnailScr}}' );"></div>
                <ion-option-button class="button-positive" ng-click="showPoints(item)">points</ion-option-button>
                <ion-option-button class="button-balanced" ng-click="showMap(item)">map</ion-option-button>
                <div class="item-data">
                <span class="item-name" ng-bind-html="item.Name"></span>
                <div class="sub-title" class="item-location">
                <span ng-bind-html="item.Location"></span>
                <span class="item-designation">{{item.itemType}} item</span>
                <span class="item-presidential" ng-class="{'show-label' : item.Presidential}">Presidential Suite</span>

                </div>
                <i class="icon ion-chevron-right icon-accessory"></i>
                </div>

            </ion-item>

        </ion-list>

    </ion-scroll>`

[srameshr]

@mlynch I am on 1.2.4 This still exists. Not able to scroll unless I set jsScrolling to true