HotStudio / touchy

jQuery plugin for touch events
touchyjs.org/
Other
357 stars 108 forks source link

handleGestureChange touches = event.touches, #8

Closed noelhunter closed 11 years ago

noelhunter commented 11 years ago

In this section:

handleGestureChange: function (e) {
            var eventType = this.context,
                $target = getTarget(e, eventType);

            if ($target) {
                var $target = $(e.target),
                event = e.originalEvent,
                touches = event.touches,

Shouldn't event.touches be e.touches?

noelhunter commented 11 years ago

Maybe not-- but when I try to reference touches later in handlegesturechange, it's undefined

fisherwebdev commented 11 years ago

I think this is cruft. The variable touches is not utilized anywhere in the rest of the function. event.touches is definitely wrong, but it's currently failing silently. I'm going to pull it out today.

I was actually intending to pull out all handleGestureChange code from Touchy. I'm not convinced there is any performance benefit from using the handleGestureChange code (which is only on Mobile Safari) instead of the full extent of the handleTouchMove code that is used in every other browser.

Your thoughts?

noelhunter commented 11 years ago

It would certainly simplify things, since at present there is not a reliable way to guess which section of code will be executed (depending on the browser). The problem I am trying to fix it that somehow requiredtouches is not being respected (I have events firing with fewer touches than I specify). That's why I was looking at the touches variable there-- I was going to manually add touches.length checks.

fisherwebdev commented 11 years ago

Please say more about what you are trying to do. requiredTouches on which Touchy gesture: longpress, drag, swipe, rotate, or pinch? I just revised the requiredTouches code yesterday. It's possible there is an edge case that I missed.

noelhunter commented 11 years ago

I am trying to stop longpress from firing at the end of drag, pinch and rotate. I set the requiredtouches for drag and long press to 1, and rotate to 3. Then I set a flag on drag, and check for it when I handle longpress. That works, but pinch and rotate cause longpress to fire in some cases. I think it may be when the user pauses. At any rate, I'd expect that if i set requiredtouches for longpress to 1, it would never fire at the end of a two touch gesture, but it does. SInce pinch does not have start and end phases, there is no way to set a flag when it starts, to make longpress ignore the call. I hope that makes sense.

fisherwebdev commented 11 years ago

That's a tricky one. The requiredTouches are designed to be a gatekeeper on the beginning of an gesture, not the end. You can probably imagine the difficulty of preventing the behavior with requiredTouches at the end, because eventually the number of touches will almost always count down from two (or however many fingers are touching the screen), to one, and then finally touchend will get triggered.

I think the proper solution here is to give all the gestures start, move and end phases, as you mentioned above, so that state can get maintained within the event handlers. This will also have the added benefit of getting the Touchy API to be more uniform, which is something I'd like to see anyway.

Do you have any ideas on a better way to approach this?

I can get to this tonight (California time). I'm going to put it up on a development branch at first, and then I'll merge it into master with some other changes later this week. I'll leave a note in the issue tracker (here) when it's done.

fisherwebdev commented 11 years ago

To clarify, requiredTouches is the minimum number of touches to get things going, not the maximum or exact requirement.

I could see some kind of exact requirement on longpress, and this would be used in the anonymous function found at lines 180-182. If you do something like that, would you mind showing me what you did? Otherwise, I will give this some thought and maybe implement this soon.

noelhunter commented 11 years ago

I see, so I was confused about requiredTouches-- I was expecting it to be an exact count, and it's a minimum, due to the way the touches happen. The core problem is multiple events firing, which people would want sometimes (say rotate and drag), but not other times. If there is a start, move and end phase for everything, then people easily can handle that with a flag, so that if any handler is running, then they can ignore others. It would be nice to have a property assigned in the data for the object, like "touch-action-running", so that handlers could check, when called for an object, to see if another action had already been called, and to have that property set automatically on the start and unset on the end. So then the longpress handler could check, and if the data("touchpressaction") was empty, if could process the event, but if another action was in progress, it could ignore it.

I guess this could be done at present by checking for values in $target.data('touchyDrag'), $target.data('touchySwipe'), etc, but it might be better to have a standardized way. Maybe there is a way I am missing.

fisherwebdev commented 11 years ago

I did some work on this and put it up on the development branch. The data object coming into your event handlers now contains an "active" property, which is a hash of booleans. These booleans will show you which events are currently active, so you should be able to pivot your code on those flags.

So within the data object you have something that looks like this:

active: {
  longpress: false,
  drag:      false,
  swipe:     false,
  pinch:     false,
  rotate:    false
}

Please let me know if this works for you.

noelhunter commented 11 years ago

I think that will work-- right now i am getting an error on line 372:

        if (data.preventDefault.move) {

undefined' is not an object (evaluating 'data.preventDefault.move')

I comment that out, and I get further. However, I am not able to check the active state the way I assume. I am trying:

    .bind( "touchy-longpress", function(event, $target, data) {         
            if ( ! data.active.pinch && ! data.active.drag && ! data.active.rotate) {

But on longpress, I get an error undefined' is not an object (evaluating 'data.active.pinch')

fisherwebdev commented 11 years ago

Okay, that should have been data.settings.preventDefault, and I had missed it in the handleGestureChange function.

Likewise, data.active was not getting populated in the handleGestureChange function.

That's indicative of the extra maintenance overhead of dealing with ongesturechange. I've decided to pull it out of the plugin entirely. This will make the overall size of Touchy smaller, and reduce the lines of code I have to maintain. All around it seems like a good thing. I had been told (verbally, by a fellow developer on a project) that ongesturechange was somehow "closer to the metal" and thus more performant, but I have not found this too be true in practice. So I'm killing that part of the code off entirely.

This is on the development branch, and will be a significant part of the 1.2 release I'm planning for the end of this week.

Thanks for helping to push this forward. Let me know if you are having any other problems.

noelhunter commented 11 years ago

Thanks. I can verify the preventdefault error is gone, and I see no change in performance on the iPad in Safari, so that's good. I still get:

TypeError: 'undefined' is not an object (evaluating 'data.active.pinch')

on the code:

.bind( "touchy-longpress", function(event, $target, data) {         
        if ( ! data.active.pinch && ! data.active.drag && ! data.active.rotate) {

when I handle a long press. Am I referencing it right? Thanks for all the help. If it would be useful, I can send the handlers for use as an example or documentation when done.

fisherwebdev commented 11 years ago

The longpress gesture has a phase in the callback signature like this, so your function should look like:

.bind( "touchy-longpress", function(event, phase, $target, data) {

I'm also noticing a couple of other things, as I dig through this code more deeply.

  1. There is no real reason why there should be a phase on the longpress. This will probably get pulled out in a 2.0 release, along with moving all data into the event object itself.
  2. I told you the wrong thing earlier. The requiredTouches are in fact the exact number of touches required to trigger the event. Sorry for the confusion.
  3. If two of the fingers hit the screen at exactly the same time, then events with a requiredTouches value of 1 will not fire. This is easily done in the iOS Simulator in XCode, but difficult on an actual device. But not impossible. Should the requiredTouches be able to be overridden by a minimumTouches for this reason?

And yes, I would very much like to get a copy of your code! If you would like to simply send a pull request with your example added to either the examples or the test pages (depending on what is more appropriate -- examples are more "shiny demos", tests are bare bones), that would be totally great. Please do that on the development branch. Alternatively, you can just email me the code: fisherwebdev@gmail.com

noelhunter commented 11 years ago

It's working pretty well now. I can get pinch and rotate working fine. Longpress and drag still conflict-- I think because drag always fires on a longpress. Could you add a px threshold to drag, so that drag does not start unless the touch moved more than a certain number of pixels (similar to pinch)?

So right now, I have longpress and drag set on requiredtouches=1. By setting the msthresh to 5000, I am able to drag without long pressfiring. Rotate and pinch are set to required touches=2, so they fire together, but this behavior seems pretty natural-- resize and rotate.

I will send the sample code when I get it working well, in simplified version.

fisherwebdev commented 11 years ago

If you don't mind, could you add that pxThresh feature request as a new issue? Easier to track that way. Thanks!

noelhunter commented 11 years ago

I added a new issue, and will also send the example code soon.

On Mon, Apr 15, 2013 at 5:54 PM, Bill Fisher notifications@github.comwrote:

If you don't mind, could you add that pxThresh feature request as a new issue? Easier to track that way. Thanks!

— Reply to this email directly or view it on GitHubhttps://github.com/HotStudio/touchy/issues/8#issuecomment-16413950 .

Noel Hunter PO Box 17383, Winston-Salem, NC 27116