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

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

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

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

vjrantal opened this issue Jan 7, 2015 · 23 comments
Assignees
Milestone

Comments

@vjrantal
Copy link

@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
Copy link
Author

@vjrantal 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
Copy link

@gfort 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
Copy link

@corebonts corebonts commented 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.

@vjrantal
Copy link
Author

@vjrantal vjrantal commented 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.

@perrygovier
Copy link
Member

@perrygovier 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
Copy link
Author

@vjrantal 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
Copy link
Member

@perrygovier 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
Copy link

@Z-Clive 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
Copy link

@nwcnwc 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
Copy link

@hallbox 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
Copy link

@tomchiverton tomchiverton commented 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 ?

@iver56
Copy link

@iver56 iver56 commented Sep 14, 2015

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

@tomchiverton
Copy link

@tomchiverton tomchiverton commented Sep 14, 2015

@gregor-srdic
Copy link

@gregor-srdic gregor-srdic commented Sep 30, 2015

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

@mhartington
Copy link
Member

@mhartington mhartington commented Oct 26, 2015

We will not be supporting WP8 with ionic v1.

@TomMettam
Copy link

@TomMettam TomMettam commented Nov 17, 2015

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

@gpascual2
Copy link

@gpascual2 gpascual2 commented 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);
  },
@jdnichollsc
Copy link

@jdnichollsc jdnichollsc commented Jan 4, 2016

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

@mistic100
Copy link

@mistic100 mistic100 commented 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.

@tobbbe
Copy link

@tobbbe tobbbe commented Jan 8, 2016

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

thank you @mistic100 that worked!

@availsys
Copy link

@availsys availsys 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
Copy link

@maksad 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
Copy link

@jdnichollsc jdnichollsc commented Jan 27, 2016

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

@ionitron-bot ionitron-bot bot locked and limited conversation to collaborators Sep 7, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet