bug: ng-click handlers called twice on IE #2885

Closed
vjrantal opened this Issue Jan 7, 2015 · 23 comments

Comments

Projects
None yet
@vjrantal

vjrantal commented Jan 7, 2015

Type: bug

Platform: windows

To reproduce the duplicate handlers on Internet Explorer, open http://codepen.io/vjrantal/pen/GgNdVj with IE11 and click the show/alert/confirm buttons. The actual (problematic) behavior is that the popups gets triggered twice, while the expected behavior is that they get triggered once.

The same actual behavior can be reproduced on desktop IE11 and the IE mobile on Windows Phone 8.1. Tip: If you don't have Windows and IE installed, you can use https://remote.modern.ie to test on OS X, iOS or Android.

The code in the the codepen logs the events the handlers get an you see something like this in the IE F12 developer tools console:

[object MouseEvent]
[object PointerEvent]

@vjrantal

This comment has been minimized.

Show comment
Hide comment
@vjrantal

vjrantal Jan 7, 2015

For debugging purposes - the issue seems to go away if you add this piece of JavaScript to the codepen at http://codepen.io/vjrantal/pen/GgNdVj :

window.addEventListener('click', function(event) {
  if (Object.prototype.toString.call(event) == '[object PointerEvent]') {
    event.stopPropagation();
  }
}
, true);

vjrantal commented Jan 7, 2015

For debugging purposes - the issue seems to go away if you add this piece of JavaScript to the codepen at http://codepen.io/vjrantal/pen/GgNdVj :

window.addEventListener('click', function(event) {
  if (Object.prototype.toString.call(event) == '[object PointerEvent]') {
    event.stopPropagation();
  }
}
, true);
@gfort

This comment has been minimized.

Show comment
Hide comment
@gfort

gfort Feb 2, 2015

As I think stopping the propagation of the PointerEvent might create unexpected behaviors, one alternative solution is to replace the tag "button" by the tag "a" with the class 'button'.

gfort commented Feb 2, 2015

As I think stopping the propagation of the PointerEvent might create unexpected behaviors, one alternative solution is to replace the tag "button" by the tag "a" with the class 'button'.

@corebonts

This comment has been minimized.

Show comment
Hide comment
@corebonts

corebonts Mar 2, 2015

My solution is to overwrite the ng-click handling and simply drop the second event:

ionic.Platform.isIE = function() {
  return ionic.Platform.ua.toLowerCase().indexOf('trident') > -1;
}

if (ionic.Platform.isIE()) {
  angular.module('ionic')
    .factory('$ionicNgClick', ['$parse', '$timeout', function($parse, $timeout) {
      return function(scope, element, clickExpr) {
        var clickHandler = angular.isFunction(clickExpr) ? clickExpr : $parse(clickExpr);

        element.on('click', function(event) {
          scope.$apply(function() {
            if (scope.clicktimer) return; // Second call
            clickHandler(scope, {$event: (event) });
            scope.clicktimer = $timeout(function() { delete scope.clicktimer; }, 1, false);
          });
        });

        // Hack for iOS Safari's benefit. It goes searching for onclick handlers and is liable to click
        // something else nearby.
        element.onclick = function(event) {};
      };
    }]);
}

The magic is around the clicktimer part.

My solution is to overwrite the ng-click handling and simply drop the second event:

ionic.Platform.isIE = function() {
  return ionic.Platform.ua.toLowerCase().indexOf('trident') > -1;
}

if (ionic.Platform.isIE()) {
  angular.module('ionic')
    .factory('$ionicNgClick', ['$parse', '$timeout', function($parse, $timeout) {
      return function(scope, element, clickExpr) {
        var clickHandler = angular.isFunction(clickExpr) ? clickExpr : $parse(clickExpr);

        element.on('click', function(event) {
          scope.$apply(function() {
            if (scope.clicktimer) return; // Second call
            clickHandler(scope, {$event: (event) });
            scope.clicktimer = $timeout(function() { delete scope.clicktimer; }, 1, false);
          });
        });

        // Hack for iOS Safari's benefit. It goes searching for onclick handlers and is liable to click
        // something else nearby.
        element.onclick = function(event) {};
      };
    }]);
}

The magic is around the clicktimer part.

@vjrantal

This comment has been minimized.

Show comment
Hide comment
@vjrantal

vjrantal Mar 25, 2015

Tested that this bug is not reproducible on the very latest browser included in the Windows 10 technical preview when experimental features are enabled. More about the release and how to enable those features can be found at http://blogs.msdn.com/b/ie/archive/2015/03/18/rendering-engine-updates-in-march-for-the-windows-10-technical-preview.aspx.

Tested that this bug is not reproducible on the very latest browser included in the Windows 10 technical preview when experimental features are enabled. More about the release and how to enable those features can be found at http://blogs.msdn.com/b/ie/archive/2015/03/18/rendering-engine-updates-in-march-for-the-windows-10-technical-preview.aspx.

@mhartington mhartington added this to the 1.1 milestone Apr 16, 2015

@CarlSteven CarlSteven referenced this issue in DataAnalyticsinStudentHands/CommunityHealthWorkersApp Apr 27, 2015

Open

[IE 11] Double modals #52

@perrygovier

This comment has been minimized.

Show comment
Hide comment
@perrygovier

perrygovier May 5, 2015

Member

Thanks for the leg work on this @vjrantal. I'd like to fix this immediately after the 1.0, but since tap.js can be such a can of obscure-device-specific-worms, I'd like to wait until then before rocking the boat.

Member

perrygovier commented May 5, 2015

Thanks for the leg work on this @vjrantal. I'd like to fix this immediately after the 1.0, but since tap.js can be such a can of obscure-device-specific-worms, I'd like to wait until then before rocking the boat.

@vjrantal

This comment has been minimized.

Show comment
Hide comment
@vjrantal

vjrantal May 5, 2015

Thanks for the update.

I was actually recently looking into http://patrickhlauke.github.io/touch/tests/results/, which shows that there is some amount of variation in the exact behavior even between different versions of the same browser.

One challenge is that regressions might not be easy to catch with unit tests (because a true finger tap might result into different events than a JavaScript-triggered click). However, Ionic seems to have a fairly comprehensive manual test list at https://github.com/driftyco/ionic/blob/master/test/unit/utils/tap.unit.js which should be helpful when testing.

Using an external library to achieve a reasonable tap behavior might be one option.

vjrantal commented May 5, 2015

Thanks for the update.

I was actually recently looking into http://patrickhlauke.github.io/touch/tests/results/, which shows that there is some amount of variation in the exact behavior even between different versions of the same browser.

One challenge is that regressions might not be easy to catch with unit tests (because a true finger tap might result into different events than a JavaScript-triggered click). However, Ionic seems to have a fairly comprehensive manual test list at https://github.com/driftyco/ionic/blob/master/test/unit/utils/tap.unit.js which should be helpful when testing.

Using an external library to achieve a reasonable tap behavior might be one option.

@perrygovier

This comment has been minimized.

Show comment
Hide comment
@perrygovier

perrygovier May 7, 2015

Member

It's a tricky area. We started with FastClick and implemented most of Hammer.js which are great, but both had issues we had to fix to the point they don't really resemble their original form. We'll reevaluate for v2, but for v1, there just isn't/wasn't anything as comprehensive. A particular problem is form elements and preventing ghost clicks when an element animates to a new position when clicked. That combined with a myriad of different behavior, between devices, manufacturers, and OSes... it gets ugly. We're not aware of anyone who's solved it better than tap.js, but we'd gladly adopt any open source project that has.

Member

perrygovier commented May 7, 2015

It's a tricky area. We started with FastClick and implemented most of Hammer.js which are great, but both had issues we had to fix to the point they don't really resemble their original form. We'll reevaluate for v2, but for v1, there just isn't/wasn't anything as comprehensive. A particular problem is form elements and preventing ghost clicks when an element animates to a new position when clicked. That combined with a myriad of different behavior, between devices, manufacturers, and OSes... it gets ugly. We're not aware of anyone who's solved it better than tap.js, but we'd gladly adopt any open source project that has.

@Z-Clive

This comment has been minimized.

Show comment
Hide comment
@Z-Clive

Z-Clive May 23, 2015

Same problem here, no matter if you're running IE11 on your desktop or on your phone.
An 'a' tag instead of 'button' works just fine.
They have the almost same call tree, so that's very tricky.

Z-Clive commented May 23, 2015

Same problem here, no matter if you're running IE11 on your desktop or on your phone.
An 'a' tag instead of 'button' works just fine.
They have the almost same call tree, so that's very tricky.

@nwcnwc

This comment has been minimized.

Show comment
Hide comment
@nwcnwc

nwcnwc May 23, 2015

@corebonts thanks, that fix works for now--for ng-click. ng-submit is still firing twice--though I'm sure if I understood what your workaround was doing I could write a similar one for ng-submit.

nwcnwc commented May 23, 2015

@corebonts thanks, that fix works for now--for ng-click. ng-submit is still firing twice--though I'm sure if I understood what your workaround was doing I could write a similar one for ng-submit.

@hallbox

This comment has been minimized.

Show comment
Hide comment
@hallbox

hallbox Jun 8, 2015

Thanks @corebonts! Hopefully the Ionic Team can learn from your fix in future versions.

hallbox commented Jun 8, 2015

Thanks @corebonts! Hopefully the Ionic Team can learn from your fix in future versions.

@mhartington mhartington modified the milestones: 1.1, 1.2 Aug 15, 2015

jtbdevelopment added a commit to jtbdevelopment/TwistedBattleship that referenced this issue Sep 12, 2015

@tomchiverton

This comment has been minimized.

Show comment
Hide comment
@tomchiverton

tomchiverton Sep 14, 2015

Also effects elements which need two taps on the pop'd up list to remove the options : <select ng-model="a" name="c"> <option value=""></option> <option ng-repeat="a in b">{{a}}</option> </select> Can any one confirm ?

Also effects elements which need two taps on the pop'd up list to remove the options : <select ng-model="a" name="c"> <option value=""></option> <option ng-repeat="a in b">{{a}}</option> </select> Can any one confirm ?

@iver56

This comment has been minimized.

Show comment
Hide comment
@iver56

iver56 Sep 14, 2015

@tomchiverton Yes, I have noticed that too. On Windows Phone 8.1.

iver56 commented Sep 14, 2015

@tomchiverton Yes, I have noticed that too. On Windows Phone 8.1.

@tomchiverton

This comment has been minimized.

Show comment
Hide comment
@gregor-srdic

This comment has been minimized.

Show comment
Hide comment
@gregor-srdic

gregor-srdic Sep 30, 2015

thank you @corebonts, your solution seems to work for me!

thank you @corebonts, your solution seems to work for me!

@mhartington

This comment has been minimized.

Show comment
Hide comment
@mhartington

mhartington Oct 26, 2015

Member

We will not be supporting WP8 with ionic v1.

Member

mhartington commented Oct 26, 2015

We will not be supporting WP8 with ionic v1.

@TomMettam

This comment has been minimized.

Show comment
Hide comment
@TomMettam

TomMettam Nov 17, 2015

I encountered this issue and implemented a fix here: #4638

I encountered this issue and implemented a fix here: #4638

@gpascual2

This comment has been minimized.

Show comment
Hide comment
@gpascual2

gpascual2 Nov 18, 2015

Thanks Tom! that did it on my app for 'windowsphone' platform. However the issue is still there for windows 10 universal apps. It worked well if the fix is applied not only to 'windowsphone' platform but to 'win64' too.

This is my dirty test code:

requiresNativeClick: function(ele) {
    var platform = ionic.Platform.platform();
    if ((platform == 'windowsphone' || platform == 'win64') && (ele.tagName == 'A' || ele.tagName == 'BUTTON' || ele.hasAttribute('ng-click') || (ele.tagName == 'INPUT' && (ele.type == 'button' || ele.type == 'submit')))) {
      return true; //Windows Phone edge case, prevent ng-click (and similar) events from firing twice on this platform
    }
    if (!ele || ele.disabled || (/^(file|range)$/i).test(ele.type) || (/^(object|video)$/i).test(ele.tagName) || ionic.tap.isLabelContainingFileInput(ele)) {
      return true;
    }
    return ionic.tap.isElementTapDisabled(ele);
  },

Thanks Tom! that did it on my app for 'windowsphone' platform. However the issue is still there for windows 10 universal apps. It worked well if the fix is applied not only to 'windowsphone' platform but to 'win64' too.

This is my dirty test code:

requiresNativeClick: function(ele) {
    var platform = ionic.Platform.platform();
    if ((platform == 'windowsphone' || platform == 'win64') && (ele.tagName == 'A' || ele.tagName == 'BUTTON' || ele.hasAttribute('ng-click') || (ele.tagName == 'INPUT' && (ele.type == 'button' || ele.type == 'submit')))) {
      return true; //Windows Phone edge case, prevent ng-click (and similar) events from firing twice on this platform
    }
    if (!ele || ele.disabled || (/^(file|range)$/i).test(ele.type) || (/^(object|video)$/i).test(ele.tagName) || ionic.tap.isLabelContainingFileInput(ele)) {
      return true;
    }
    return ionic.tap.isElementTapDisabled(ele);
  },

@ryaa ryaa referenced this issue in rajeshwarpatlolla/ionic-datepicker Jan 3, 2016

Closed

Datepicker or timepicker is opened twice under IE 10 #138

@jdnichollsc

This comment has been minimized.

Show comment
Hide comment
@jdnichollsc

jdnichollsc Jan 4, 2016

The issue exist in Ionic 1.2. I hate you IE11!

The issue exist in Ionic 1.2. I hate you IE11!

@mistic100

This comment has been minimized.

Show comment
Hide comment
@mistic100

mistic100 Jan 7, 2016

I confirm that the bug append in IE11.
Replacing <button> by <a class="button"> works but would be better to handle it in the lib.

I confirm that the bug append in IE11.
Replacing <button> by <a class="button"> works but would be better to handle it in the lib.

@tobbbe

This comment has been minimized.

Show comment
Hide comment
@tobbbe

tobbbe Jan 8, 2016

Yep, issue still exist in 1.2.1 (#4638 doesnt seem to fix it)

thank you @mistic100 that worked!

tobbbe commented Jan 8, 2016

Yep, issue still exist in 1.2.1 (#4638 doesnt seem to fix it)

thank you @mistic100 that worked!

@healyje

This comment has been minimized.

Show comment
Hide comment
@healyje

healyje Jan 21, 2016

Glad to see the button / class=button workaround. In desperation I had finally written a locking service around a $rootScope.lock with a .getLock() and .releaseLock() - overkill I know, but it worked.

healyje commented Jan 21, 2016

Glad to see the button / class=button workaround. In desperation I had finally written a locking service around a $rootScope.lock with a .getLock() and .releaseLock() - overkill I know, but it worked.

@maksad

This comment has been minimized.

Show comment
Hide comment
@maksad

maksad Jan 27, 2016

@jdnichollsc try to add type="button" to the button element. As @cverb pointed in this SO issue the default behaviour is submit and apparently that messes with the code.
At least my issue was fixed.

maksad commented Jan 27, 2016

@jdnichollsc try to add type="button" to the button element. As @cverb pointed in this SO issue the default behaviour is submit and apparently that messes with the code.
At least my issue was fixed.

@jdnichollsc

This comment has been minimized.

Show comment
Hide comment
@jdnichollsc

jdnichollsc Jan 27, 2016

I'm using the solution of @corebonts, thanks man! 👯

I'm using the solution of @corebonts, thanks man! 👯

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment