Touch events and DragListGroup
298 views
Skip to first unread message
AndyDuncan
Sep 27, 2011, 2:21:31 PM9/27/11
to Closure Library Discuss
I'm trying to get DragListGroup working on touch devices but I'm
having issues where the device wants to scroll after dragging for a
few pixels.
I was able to get around this by attaching a listener to my drag
handle and calling preventDefault on goog.events.EventType.TOUCHMOVE
events, but it seems like this is something that should be handled in
the dragger class.
Is there a reason that the default implementation doesn't call
preventDefault to keep the window from scrolling and that is left to
the user? (I'm posting this half as a question and half so that it's
in the mailing list the next time someone runs into this problem).
Cheers,
-Andy
having issues where the device wants to scroll after dragging for a
few pixels.
I was able to get around this by attaching a listener to my drag
handle and calling preventDefault on goog.events.EventType.TOUCHMOVE
events, but it seems like this is something that should be handled in
the dragger class.
Is there a reason that the default implementation doesn't call
preventDefault to keep the window from scrolling and that is left to
the user? (I'm posting this half as a question and half so that it's
in the mailing list the next time someone runs into this problem).
Cheers,
-Andy
Hunter Blanks
Sep 28, 2011, 11:20:21 PM9/28/11
to Closure Library Discuss
Andy,
One of our engineers did some good work on this problem and
subclassed out a pretty good, iPad compatible version. I'll ask
him if he has any recollection, or see if we can't release that
code upstream.
-HJB
One of our engineers did some good work on this problem and
subclassed out a pretty good, iPad compatible version. I'll ask
him if he has any recollection, or see if we can't release that
code upstream.
-HJB
Andrew Mattie
Sep 29, 2011, 8:12:55 PM9/29/11
to closure-lib...@googlegroups.com
It's a bug. preventDefault is being called here:
I see the problem and it's sadly my fault. The touch event is being re-initialized here with the actual Touch:
Prior to that reinit, none of the drag stuff worked at all w/ touch as clientX/Y and screenX/Y weren't available on the TouchEvent object. As you noticed though, reinit'ing that event will cause preventDefault and stopPropagation to not work at all and getBrowserEvent to return the Touch, which looks like an Event but isn't, instead of the TouchEvent. That's a problem now and will likely become even more of a problem as the TouchEvent spec is enhanced.
The most obvious solution then would be to kill the reinit and, in it's place, add in functionality which returns goog.math.Coordinate's when clientX/Y and screenX/Y are needed. The caveat to doing that is that events are dispatched here[1], here[2], and here[3] with the reinit'd event that we now know is broken. Who knows what type of code is consuming that event and thus risk breaking unknown code to make that right. One example consequence of that switch would arise if someone was trying to look for clientX on that reinit'd event, and it suddenly switched to a TouchEvent; clientX would then be undefined.
[1] http://code.google.com/p/closure-library/source/browse/trunk/closure/goog/fx/dragger.js?r=1302#443
[2] http://code.google.com/p/closure-library/source/browse/trunk/closure/goog/fx/dragger.js?r=1302#473
[3] http://code.google.com/p/closure-library/source/browse/trunk/closure/goog/fx/dragger.js?r=1302#552
An easy alternative would be to save the browser event ala e.getBrowserEvent() before the event is reinit'd and then call preventDefault() on that. I confirmed that that fixes the problem. The downside to that is that the broken event exposed in the event dispatches will continue to be broken.
After thinking about this for a while, my vote would be to make the potentially breaking change rather than to keep this broken as it is going forward. My rationale is as follows:
1) Everywhere a DragEvent is dispatched, clientX and clientY are exposed directly as properties of the DragEvent. If one needed those properties, I theorize that they'll be accessed as those properties instead of properties on the event.
2) While that code has been around for ~9mo now (http://code.google.com/p/closure-library/source/detail?r=666&path=/trunk/closure/goog/fx/dragger.js), it's of course going to be around for the indefinite future and so I think it'll be easier to deal with the potential pain now vs have it remain broken.
That said, perhaps I'm being too callous about such a change. There are good counterpoints. Is it reasonable in this case to fix one thing that could potentially break another? This isn't the best situation, but I think something has to be done. Either way, I'll work on the fix.
As a side note, it feels to me that there's room here to have goog.events.BrowserEvent handle touch events more directly. It'd be nice if the clientX/Y, pageX/Y, screenX/Y and target properties were init'd in BrowserEvent for touch events to the properties of the same name for the first target tough or changed touch (depending on the event type). I tend to repeat that functionality often, and it would have prevented this very problem.
Andrew
Andrew Mattie
Sep 29, 2011, 8:50:06 PM9/29/11
to closure-lib...@googlegroups.com
You know how sometimes you can get to the end of a problem writeup, thinking there's no good solution, and then have it hit you at the last minute? I think I just did that but sent out the email a minute too late.
In the last paragraph of my email, I mentioned changing BrowserEvent so it better handles the useful properties of the first target or changed touch. I think doing that and then completely removing the event reinit in dragger would completely solve this problem. That way, the event that's sent out in the dispatch has all the useful properties already available on it for anyone who's consuming it, preventDefault and stopPropagation would work again, and the TouchEvent is available in getBrowserEvent for the future. Changing BrowserEvent would also help with the annoyances of handling the most common case of touch device development where you just want to know info about the first touch and that's it.
That does raise the question of what the first touch would be though. I standardized on the first target touch in touchstart / touchmove events and the first changed touch in touchend / touchcancel events (since target touches are null then).
Thoughts anyone?
Andrew
Andrew Mattie
Sep 30, 2011, 12:52:41 PM9/30/11
to closure-lib...@googlegroups.com
Kyle Chronis
Sep 30, 2011, 4:57:07 PM9/30/11
to Closure Library Discuss
Andrew,
Thanks for the detailed explanation and the quick fix, that patch
works great and also fixes an issue we were seeing on honeycomb
tablets that my hack didn't solve (I was only killing move events, the
others were getting through and causing issues). For what it's worth
the target/first changed choice seems reasonable to me.
Thanks again,
-Andy
On Sep 30, 9:52 am, Andrew Mattie <amat...@gmail.com> wrote:
> Here's my proposed fix:
>
> http://codereview.appspot.com/5150047/
>
>
>
>
>
>
>
> On Thu, Sep 29, 2011 at 5:50 PM, Andrew Mattie <amat...@gmail.com> wrote:
> > You know how sometimes you can get to the end of a problem writeup,
> > thinking there's no good solution, and then have it hit you at the last
> > minute? I think I just did that but sent out the email a minute too late.
>
> > In the last paragraph of my email, I mentioned changing BrowserEvent so it
> > better handles the useful properties of the first target or changed touch. I
> > think doing that and then completely removing the event reinit in dragger
> > would completely solve this problem. That way, the event that's sent out in
> > the dispatch has all the useful properties already available on it for
> > anyone who's consuming it, preventDefault and stopPropagation would work
> > again, and the TouchEvent is available in getBrowserEvent for the future.
> > Changing BrowserEvent would also help with the annoyances of handling the
> > most common case of touch device development where you just want to know
> > info about the first touch and that's it.
>
> > That does raise the question of what the first touch would be though. I
> > standardized on the first target touch in touchstart / touchmove events and
> > the first changed touch in touchend / touchcancel events (since target
> > touches are null then).
>
> > Thoughts anyone?
>
> > Andrew
>
> >> [2]
> >>http://code.google.com/p/closure-library/source/browse/trunk/closure/...
> >> [3]
> >>http://code.google.com/p/closure-library/source/browse/trunk/closure/...
Thanks for the detailed explanation and the quick fix, that patch
works great and also fixes an issue we were seeing on honeycomb
tablets that my hack didn't solve (I was only killing move events, the
others were getting through and causing issues). For what it's worth
the target/first changed choice seems reasonable to me.
Thanks again,
-Andy
On Sep 30, 9:52 am, Andrew Mattie <amat...@gmail.com> wrote:
> Here's my proposed fix:
>
> http://codereview.appspot.com/5150047/
>
>
>
>
>
>
>
> On Thu, Sep 29, 2011 at 5:50 PM, Andrew Mattie <amat...@gmail.com> wrote:
> > You know how sometimes you can get to the end of a problem writeup,
> > thinking there's no good solution, and then have it hit you at the last
> > minute? I think I just did that but sent out the email a minute too late.
>
> > In the last paragraph of my email, I mentioned changing BrowserEvent so it
> > better handles the useful properties of the first target or changed touch. I
> > think doing that and then completely removing the event reinit in dragger
> > would completely solve this problem. That way, the event that's sent out in
> > the dispatch has all the useful properties already available on it for
> > anyone who's consuming it, preventDefault and stopPropagation would work
> > again, and the TouchEvent is available in getBrowserEvent for the future.
> > Changing BrowserEvent would also help with the annoyances of handling the
> > most common case of touch device development where you just want to know
> > info about the first touch and that's it.
>
> > That does raise the question of what the first touch would be though. I
> > standardized on the first target touch in touchstart / touchmove events and
> > the first changed touch in touchend / touchcancel events (since target
> > touches are null then).
>
> > Thoughts anyone?
>
> > Andrew
>
> > On Thu, Sep 29, 2011 at 5:12 PM, Andrew Mattie <amat...@gmail.com> wrote:
>
> >> It's a bug. preventDefault is being called here:
>
> >>http://code.google.com/p/closure-library/source/browse/trunk/closure/...
>
> >> It's a bug. preventDefault is being called here:
>
>
> >> I see the problem and it's sadly my fault. The touch event is being
> >> re-initialized here with the actual Touch:
>
> >>http://code.google.com/p/closure-library/source/browse/trunk/closure/...
> >> I see the problem and it's sadly my fault. The touch event is being
> >> re-initialized here with the actual Touch:
>
>
> >> Prior to that reinit, none of the drag stuff worked at all w/ touch as
> >> clientX/Y and screenX/Y weren't available on the TouchEvent object. As you
> >> noticed though, reinit'ing that event will cause preventDefault and
> >> stopPropagation to not work at all and getBrowserEvent to return the Touch,
> >> which looks like an Event but isn't, instead of the TouchEvent. That's a
> >> problem now and will likely become even more of a problem as the TouchEvent
> >> spec is enhanced.
>
> >> The most obvious solution then would be to kill the reinit and, in it's
> >> place, add in functionality which returns goog.math.Coordinate's when
> >> clientX/Y and screenX/Y are needed. The caveat to doing that is that events
> >> are dispatched here[1], here[2], and here[3] with the reinit'd event that we
> >> now know is broken. Who knows what type of code is consuming that event and
> >> thus risk breaking unknown code to make that right. One example consequence
> >> of that switch would arise if someone was trying to look for clientX on that
> >> reinit'd event, and it suddenly switched to a TouchEvent; clientX would then
> >> be undefined.
>
> >> [1]
> >>http://code.google.com/p/closure-library/source/browse/trunk/closure/...
> >> Prior to that reinit, none of the drag stuff worked at all w/ touch as
> >> clientX/Y and screenX/Y weren't available on the TouchEvent object. As you
> >> noticed though, reinit'ing that event will cause preventDefault and
> >> stopPropagation to not work at all and getBrowserEvent to return the Touch,
> >> which looks like an Event but isn't, instead of the TouchEvent. That's a
> >> problem now and will likely become even more of a problem as the TouchEvent
> >> spec is enhanced.
>
> >> The most obvious solution then would be to kill the reinit and, in it's
> >> place, add in functionality which returns goog.math.Coordinate's when
> >> clientX/Y and screenX/Y are needed. The caveat to doing that is that events
> >> are dispatched here[1], here[2], and here[3] with the reinit'd event that we
> >> now know is broken. Who knows what type of code is consuming that event and
> >> thus risk breaking unknown code to make that right. One example consequence
> >> of that switch would arise if someone was trying to look for clientX on that
> >> reinit'd event, and it suddenly switched to a TouchEvent; clientX would then
> >> be undefined.
>
> >> [1]
> >> [2]
> >>http://code.google.com/p/closure-library/source/browse/trunk/closure/...
> >> [3]
> >>http://code.google.com/p/closure-library/source/browse/trunk/closure/...
>
> >> An easy alternative would be to save the browser event ala
> >> e.getBrowserEvent() before the event is reinit'd and then call
> >> preventDefault() on that. I confirmed that that fixes the problem. The
> >> downside to that is that the broken event exposed in the event dispatches
> >> will continue to be broken.
>
> >> After thinking about this for a while, my vote would be to make the
> >> potentially breaking change rather than to keep this broken as it is going
> >> forward. My rationale is as follows:
>
> >> 1) Everywhere a DragEvent is dispatched, clientX and clientY are
> >> exposed directly as properties of the DragEvent. If one needed those
> >> properties, I theorize that they'll be accessed as those properties instead
> >> of properties on the event.
> >> 2) While that code has been around for ~9mo now (
> >>http://code.google.com/p/closure-library/source/detail?r=666&path=/tr...),
> >> An easy alternative would be to save the browser event ala
> >> e.getBrowserEvent() before the event is reinit'd and then call
> >> preventDefault() on that. I confirmed that that fixes the problem. The
> >> downside to that is that the broken event exposed in the event dispatches
> >> will continue to be broken.
>
> >> After thinking about this for a while, my vote would be to make the
> >> potentially breaking change rather than to keep this broken as it is going
> >> forward. My rationale is as follows:
>
> >> 1) Everywhere a DragEvent is dispatched, clientX and clientY are
> >> exposed directly as properties of the DragEvent. If one needed those
> >> properties, I theorize that they'll be accessed as those properties instead
> >> of properties on the event.
> >> 2) While that code has been around for ~9mo now (
> >> it's of course going to be around for the indefinite future and so I think
> >> it'll be easier to deal with the potential pain now vs have it remain
> >> broken.
>
> >> That said, perhaps I'm being too callous about such a change. There are
> >> good counterpoints. Is it reasonable in this case to fix one thing that
> >> could potentially break another? This isn't the best situation, but I think
> >> something has to be done. Either way, I'll work on the fix.
>
> >> As a side note, it feels to me that there's room here to have
> >> goog.events.BrowserEvent handle touch events more directly. It'd be nice if
> >> the clientX/Y, pageX/Y, screenX/Y and target properties were init'd in
> >> BrowserEvent for touch events to the properties of the same name for the
> >> first target tough or changed touch (depending on the event type). I tend to
> >> repeat that functionality often, and it would have prevented this very
> >> problem.
>
> >> Andrew
>
> >> it'll be easier to deal with the potential pain now vs have it remain
> >> broken.
>
> >> That said, perhaps I'm being too callous about such a change. There are
> >> good counterpoints. Is it reasonable in this case to fix one thing that
> >> could potentially break another? This isn't the best situation, but I think
> >> something has to be done. Either way, I'll work on the fix.
>
> >> As a side note, it feels to me that there's room here to have
> >> goog.events.BrowserEvent handle touch events more directly. It'd be nice if
> >> the clientX/Y, pageX/Y, screenX/Y and target properties were init'd in
> >> BrowserEvent for touch events to the properties of the same name for the
> >> first target tough or changed touch (depending on the event type). I tend to
> >> repeat that functionality often, and it would have prevented this very
> >> problem.
>
> >> Andrew
>
AndyDuncan
Sep 30, 2011, 5:05:02 PM9/30/11
to closure-lib...@googlegroups.com
(this was from me, sorry, Kyle was still logged in on my machine).
josh on
Dec 12, 2012, 8:49:47 PM12/12/12
to closure-lib...@googlegroups.com, andyd...@gmail.com
I just manually applied this patch, and it fixed my problem. Thank you. What is the status of this? Is this likely to be included in future revisions? The is the only change I will have to maintain.
It seems like a pretty big flaw in the library that the touch co-ordinates are not available in touch events. Is there another work around that does not involve a patch?
Andrew Mattie
Dec 13, 2012, 12:15:46 AM12/13/12
to closure-lib...@googlegroups.com
I continue to maintain the compatibility of the patch against new revs of the library as I have time, but its actual ingestion status is unknown to me. I don't see any reason why it can't / shouldn't be ingested though as I really don't think the change I made will break anything down the line.
I remain happy to work with any committer who wants to take this under their wing and finally bring it in.
Andrew
josh on
Dec 13, 2012, 12:42:43 PM12/13/12
to closure-lib...@googlegroups.com
Thanks again.
Reply all
Reply to author
Forward
0 new messages