RyanMullins / angular-hammer

Hammer.js v2 support for AngularJS
http://ryanmullins.github.io/angular-hammer/
MIT License
188 stars 55 forks source link

Unable to trigger .click() and .stopPropagation() events #26

Closed equilerex closed 9 years ago

equilerex commented 9 years ago

Hi, i am looking into implementing hammer on my project... the code looks nice and solid so thanks for the hard work.

I have however noticed that .click(), dblclick(), and $event.stopPropagation() dont work with hm-tap /hm-doubletap. Isnt hm-tap supposed to mimic ng-click since it replaces the ng-click in browsers as well? while .click()/.dblclick() arent really the angular way of doing things, i would imagine they should be there for the sake of consistency, while stopPropagation is a pretty vital feature.

example of the issue: http://plnkr.co/edit/ybwPz4a7ow8QeEB7FFlZ?p=preview

any plans of including them somewhere along the line or are they left out for a specific purpose?

RyanMullins commented 9 years ago

Let me think about this for a bit. I'm not 100% sure that I can even pass an angular generated event through the hm-tap handler because of the scope in which they're defined. In the meantime, try adding recognizer options that enable preventDefault.

<button hm-recognizer-options='{"preventDefault":true}' hm-tap="stop($event)" >$event.stopPropagation()</button>
equilerex commented 9 years ago

I see, thanks for the reply. luckily for me, the feature i was working on could be rearranged so i could avoid having one hm-tap element inside another one... i have however used that kind of behaviour in the past so it can definitely happen... As for preventDefault, it does not have any effect since im not trying to stop the element i clicked doing its thing rather than prevent the click bubbling through and triggering tap events assigned to its parent elements.

the $(".elm").click() event should not really be that tricky though i would imagine

RyanMullins commented 9 years ago

I see, I'll add a stopPropagation option to the recognizer options in v2.1.10. Might not happen today though, swamped with other work.

RyanMullins commented 9 years ago

Here's a bit of documentation that might be useful to you until I implement this feature: https://github.com/hammerjs/hammer.js/wiki/Event-delegation-and-how-to-stopPropagation---preventDefaults

equilerex commented 9 years ago

very cool and no rush.. this has been the fastest correspondence ive seen on github so props to you ;)

RyanMullins commented 9 years ago

I was able to very quickly implement the stopPropagation component of this issue (honestly that part was easy). BUt the click()/dblClick() bit is proving to be more difficult. Looking into it on your plunk now.

RyanMullins commented 9 years ago

Okay, here's what I've got...

The jQuery.click() function that you're using in that plunker is triggering the same behavior as a standard DOM Mouse Click Event. Hammer does listen to DOM click events to generate Hammer Events. But, from what I can tell digging through the source code for jQuery.click() (line 12), jQuery.trigger() (line 224), Hammer.Manager.recognize() (line 71), and Hammer.Recognizer.recognize() (line 224), it seems like Hammer Managers will never be triggered (i.e. go through a recognizer session).

jQuery is triggering the registered event handlers directly (starts on line 299) instead of triggering a DOM Event. Without the DOM Event, the Hammer Input's domHandler function (line 48) is never fired and therefore never triggers the recognize sequence. When you add an ngClick or ngDblclick you are adding a click handler for that element, which can be triggered directly by jQuery once it computes the event's bubble path through the DOM.

So I guess the short answer is that no, I can't respond to jQuery.click() or jQuery.dblclick() currently, and won't be able to until jQuery starts firing off DOM Events (instead of firing handlers directly). Sorry that has to be the answer, but thank you for making me dig around in all of this source code, I learned a lot.

RyanMullins commented 9 years ago

Closing because I believe the commit referenced above (b66e314) fixes this issue to the best of my ability. Please reopen if you are unsatisfied with the solution I have implemented, or if you find a bug.

equilerex commented 9 years ago

wow, that was some fast work :) unfortunately, it seems like the new updated plugin is broken(not sure if its due to this update or one of the previous ones) the example i made has issue with: "Failed to instantiate module hmTime due to: Error: [$injector:modulerr]" http://plnkr.co/edit/7Uqn6CFYAXuUcla7Xcfv?p=preview

while your own live examples are failing with: Error: [$rootScope:inprog] http://errors.angularjs.org/1.2.25/$rootScope/inprog?p0=%24apply http://ryanmullins.github.io/angular-hammer/examples/dragging http://ryanmullins.github.io/angular-hammer/examples/default and custom just ignores everything: http://ryanmullins.github.io/angular-hammer/examples/custom

RyanMullins commented 9 years ago

You're calling $scope.$apply() from inside the event handler, which is already executing inside of an $apply. I've fixed that issue here: http://plnkr.co/edit/KwuQrxV2cFhnwRnBfZj3?p=preview

But, I have found that there is still a problem with the requireFailure and recognizeWith functions (see #23) and will be using that as a means on debugging that problem.

RyanMullins commented 9 years ago

To avoid that problem in the future, you could implement a safeApply function on your root scope. This function looks something like:

$rootScope.safeApply = function (fn) {
  var phase = this.$$phase;

  if (phase == '$apply' || phase == '$digest') {
    if (fn != null && typeof fn === 'function') {
      fn();
    }
  } else {
    this.$apply(fn);
  }
}
RyanMullins commented 9 years ago

Also... it would appear I have forgotten to update all my demos on the github.io site... Like I said on Twitter, I'm starting to remember all this stuff, but I need a better distribution system.

equilerex commented 9 years ago

Excactly, the apply was a code from your demo(which means that your github demos on http://ryanmullins.github.io/angular-hammer/examples/ are broken for the moment). from what i remember, it was there since without it, the "eventType" wouldnt update.

my plnkr however had broken since i had mixed up the src and type attributes when updating the hammer.js so that was my own screw up ;)