Opened 8 years ago

Closed 8 years ago

Last modified 8 years ago

#13828 closed defect (fixed)

[CLA][PATCH] Refine dojo/gesture and dojo/gesture/*

Reported by: Evan Owned by: Evan
Priority: high Milestone: 1.7
Component: Events Version: 1.7.0b1
Keywords: Cc: Bryan Forbes, Kris Zyp, bill, Chris Mitchell, Kenneth G. Franqueiro
Blocked By: Blocking:

Description

dojo/gesture and dojo/gesture/* need to be refined based on folks' reviewing comments:

  1. Remove the registry
  2. Resolve outstanding issues regarding swipe gesture
  3. Use on.emit() to fire and bubble gesture events
  4. Write reference guide documentation

Attachments (1)

#13828.patch (37.8 KB) - added by Evan 8 years ago.
Initially finished version, still wrapping up tests/doh

Download all attachments as: .zip

Change History (31)

comment:1 Changed 8 years ago by Evan

Owner: changed from Kris Zyp to Evan

comment:2 Changed 8 years ago by Evan

Summary: Refine dojo/gesture and dojo/gesture/*[CLA][PATCH] Refine dojo/gesture and dojo/gesture/*

Changed 8 years ago by Evan

Attachment: #13828.patch added

Initially finished version, still wrapping up tests/doh

comment:3 Changed 8 years ago by Evan

Cc: Chris Mitchell added

Hi Guys,

Would you take a quick look and let me know your thoughts regarding the changes? Mainly include:

1.Registry is removed, and moved dojo.gesture content to dojo/gesture/Base as a parental class.

2.Changed swipe to keep firing events during touch move, the previous version is actually a flicker gesture(we may added later)

3.Used on.emit() to fire and bubble gesture events

4.Added dojo.touch.cancel handle

BTW, the patch code is also uploaded at https://github.com/evanhw/touch/tree/master/dojo

Thx!

comment:4 Changed 8 years ago by bill

I checked it over a bit. Questions:

  • Not sure what "defaultEvent" or "subEvents" attributes in subclasses of gesture/Base is for? Maybe I'm missing something, but they don't seem necessary. Likewise with init(), _events and _remove(). I'm just not seeing why you need an infrastructure more complicated than dojo/touch uses.
  • You are firing synthetic DOM events as a side effect when on() is called for a node. Is the idea that an app would call on(node, gesture.swipe, function(){/*empty function*/}) and then instead setup handlers on <body> via on(dojo.body(), "swipe.end", ...)? I don't really see the value here.
  • I expected the swipe module to return {move: ..., end: ...} hash rather than being used as swipe (which is the move gesture) and swipe.end. Although either way works.
  • I thought a swipe implied a short, fast gesture but it looks like the swipe code will fire on any touch move, even a long slow one?
  • the lang.getObject("gesture", true, dojo); in gesture/Base seems unnecessary
  • in tap.js there's a clearTimeout() inside a setTimeout() callback that is unnecessary.
  • in the line this.fire(e.target, {type: "tap.doubletap"});, it seems unusual to have an event type containing a dot; it's inconsistent with the browser native event doubleclick (which is called "doubleclick", not "click.doubleclick")
Last edited 8 years ago by bill (previous) (diff)

comment:5 in reply to:  4 Changed 8 years ago by Evan

Thanks, Bill,

Replying to bill:

I checked it over a bit. Questions:

  • Not sure what "defaultEvent" or "subEvents" attributes in subclasses of gesture/Base is for? Maybe I'm missing something, but they don't seem necessary. Likewise with init(), _events and _remove(). I'm just not seeing why you need an infrastructure more complicated than dojo/touch uses.

"defaultEvent" and "subEvents" mainly to accommodate with the Object event, e.g. since we have the following usage:

  • on(node, dojo.gesture.tap, func);
  • on(node, dojo.gesture.tap.hold, func);
  • on(node, dojo.gesture.tap.doubletap, func);

doesn't that already indicate a main-sub relationship e.g tap.hold.

Also we can't provide

  • on(node, dojo.gesture.taphold, func)

since that violates module return way.

Maybe we can have a simper list, but still in the form as e.g. ["tap", "tap.hold", "tap.doubletap"] to be consistent with the Object event usage?

The logic is a bit complex than touch because unlike dojo.touch which is simply listening to native events, besides gesture event, dojo.gesture also needs to listen, process and dispatch "press"|"move"|"release"(including remove them - user won't explicitly do that).

  • You are firing synthetic DOM events as a side effect when on() is called for a node. Is the idea that an app would call on(node, gesture.swipe, function(){/*empty function*/}) and then instead setup handlers on <body> via on(dojo.body(), "swipe.end", ...)? I don't really see the value here.

No, that's a not provided usage. "swipe.end" is simply the event.type user got from callback in

  • on(node, gesture.swipe, function(event){})
  • I expected the swipe module to return {move: ..., end: ...} hash rather than being used as swipe (which is the move gesture) and swipe.end. Although either way works.

Hmm, was just thinng dojo.gesture.swipe is more clean to use.

  • I thought a swipe implied a short, fast gesture but it looks like the swipe code will fire on any touch move, even a long slow one?

Yep, based on my new understanding, swipe is just what you do when swiping the screen of iPad or iPhone, no matter fast or slow, the viewport will be slides.

A faster one but only fired when touchend is a flicker.

  • the lang.getObject("gesture", true, dojo); in gesture/Base seems unnecessary
  • in tap.js there's a clearTimeout() inside a setTimeout() callback that is unnecessary.

Gonna have a check, thx!

  • in the line this.fire(e.target, {type: "tap.doubletap"});, it seems unusual to have an event type containing a dot; it's inconsistent with the browser native event doubleclick (which is called "doubleclick", not "click.doubleclick")

Yep, I have the similar concern, but here just want to be consistent with Object events usage as:

  • on(node, dojo.gesture.tap, func);
  • on(node, dojo.gesture.tap.hold, func);
  • on(node, dojo.gesture.tap.doubletap, func);

Of course we can directly fire "tap", "taphold", "doubletap" or "swipeend" etc. if we don't worry about that.

comment:6 Changed 8 years ago by bill

I talked to Evan about this a long time offline, here's what I learned:

1. DOMNode synthetic events

Although gesture/Base calls on.emit(node, ...), generating synthetic events on the DOMNode with names like "tap.hold", these synthetic events are for internal use only, specifically to deal with bubbling. For example, they are used to make sure that in the second on() callback below, the callback only executes when dojo.stopEvent() is *not* called:

on(inner, dojo.gesture.tap.hold, function(e){
     console.log('inner is tap.hold');
     if(xyz) dojo.stopEvent(e);
});

on(outer, dojo.gesture.tap.hold, function(e){
     console.log('outer is tap.hold');
});

In the case above, the native touchstart, touchmove etc. events from the inner node bubble to the outer node, but with an _locking attribute set. So, the outer handler ignores them. But when dojo.stopEvent() is not called, the "tap.hold" synthetic event also bubbles upwards.

Apps are not meant to listen directly for an event with a string name of "tap.hold".

2. reference counting

Considering app code that does this:

handle1 = on(node, tap, ...)
handle2 = on(node, tap, ...)
handle3 = on(node, tap.hold, ...)
handle4 = on(node, tap.doubletap, ...)

On the first (and only the first) call to on(node, tap or tap.hold or tap.doublehold), dojo/gesture/tap (via dojo/gesture/Base) will setup 4 internal listeners for touchstart, touchmove, touchend, touchcancel. Essentially, it sets up node to publish tap, tap.hold, and tap.doubletap notifications. And then the first on() call and subsequent() on calls are merely setting up subscriptions listening for those published events.

The internal touchstart, touchmove, touchend, and touchcancel listeners will be removed when handle1, handle2, handle3, and handle4 have all been removed.

I would have preferred if each on() call above generated it's own private internal listeners for touchstart, touchmove, touchend, touchcancel. However, it seems like the current design is necessary for the bubbling described above in #1. We need exactly one synthetic "tap" event to be propagated from the inner node, regardless of how many tap listeners have been registered to listen to tap.

The code does seem quite complicated to me though, and I do wonder whether this complication is worth it just to support proper bubbling. OTOH, for something like the dojo/gesture/tap code, I see how it's the same chunk of code that monitors touch events and fires either tap, tap.hold, or tap.doubletap, depending on what touch events occur, so it might be silly to run that code three times just because an app was listening for tap, doubletap, and taphold. Anyway, I'd be interested to hear what others think.


My remaining issue is with swipe vs. flick:

Just looking at (for example) gmail on iphone, there's are two ways to scroll:

  • moving my finger slowly: scrolling starts when I start moving my finger, and stops as soon as I lift up my finger
  • scrolling by a quick flick action: scrolling starts when I start moving my finger, but continues after I remove my finger (slowing down and eventually stopping over time)

I'm unclear how to do that with the current gestures. Is it possible?

Last edited 8 years ago by bill (previous) (diff)

comment:7 Changed 8 years ago by Evan

In [26475]:

Refs #13828 !strict, finished:

  1. Remove the registry
  2. Resolve outstanding issues regarding swipe gesture
  3. Use on.emit() to fire and bubble gesture events

Also move gesture to dojox and marked as experimental - due to the fact that it's still subject to hot discussion and changes, so that we have a safer way to experiment it out and finalize the design and API after 1.7

comment:8 Changed 8 years ago by Evan

In [26476]:

Refs #13828, remove folder of dojo/gesture since all gesture content have been moved to dojox/geture

comment:9 Changed 8 years ago by Evan

In [26477]:

Refs #13828, missed cancel handle for dojo.touch

comment:10 Changed 8 years ago by Kenneth G. Franqueiro

I don't suppose it would've been possible to move gesture from dojo to dojox while preserving all the SVN history (i.e. using svn mv)? Seems a shame to have all history left behind in the old removed location, but I'm not sure if our usual checkout with externals makes such a move difficult.

comment:11 Changed 8 years ago by Evan

In [26480]:

Refs #13828, roll back[26476] - will use a better way for moving to dojox so that all histories can be preserved.

comment:12 Changed 8 years ago by Evan

In [26481]:

Refs #13828, roll back[26475] - will use a better way for moving gesture to dojox so that all histories can be preserved, !strict.

comment:13 Changed 8 years ago by Evan

In [26484]:

Refs #13828, dojo/gesture is still in hot discussion and subject to possible changes, so moved to dojox/gesture and marked as experimental, so that it won't block 1.7 and we'll also have a safer way to experiment it out and finalize the design & API in the future.

All previous change histories are also preserved.

comment:14 Changed 8 years ago by Evan

In [26485]:

Refs #13828, move dojo/gesture.js to dojox/gesture/ folder and renamed to Base.js as an abstract parent class for all gesture impls, !strict.

comment:15 Changed 8 years ago by Evan

In [26486]:

Refs #13828, create test folder/README(dummy content) for dojox/gesture

comment:16 Changed 8 years ago by Evan

In [26487]:

Refs #13828, moving dojo/tests/test_gesture.html to dojox/gesture/tests/test_gesture.html

comment:17 Changed 8 years ago by Evan

In [26488]:

Refs #13828, !strict.

1.Registry is removed, and moved dojo.gesture content to dojox/gesture/Base as an abstract parental class.

2.Changed swipe to keep firing events during touch move, the previous version is actually a flicker gesture(we may added it back later)

3.Used on.emit() to fire and bubble gesture events

4.Test case cleaned up, moved doh func to a coming separate doh/*

comment:18 Changed 8 years ago by Evan

Priority: highestnormal

All done except for more doh test cases and ref doc, so not a 1.7 blocker, changing priority from highest to normal

comment:19 in reply to:  10 Changed 8 years ago by Evan

Replying to kgf:

I don't suppose it would've been possible to move gesture from dojo to dojox while preserving all the SVN history (i.e. using svn mv)? Seems a shame to have all history left behind in the old removed location, but I'm not sure if our usual checkout with externals makes such a move difficult.

Yep, that's feasible, I've done that so all previous changeset history are preserved.

comment:20 Changed 8 years ago by cjolif

In [26507]:

dojo/gesture moved to dojox/gesture, modify charting accordingly. Refs #13828

comment:21 Changed 8 years ago by Evan

In [26512]:

Refs #13828, !strict, trivial fixes and DoH test case added

comment:22 Changed 8 years ago by Evan

In [26540]:

Refs #13828, adding doh test case for gesture bubbling etc.

comment:23 Changed 8 years ago by Evan

In [26664]:

Refs #13828 !strict, comments updates for dojox/gesture so that displayed well in API doc, also added content for README

comment:24 Changed 8 years ago by Evan

In [26691]:

Refs #13828 !strict, trivial comments wording and tests updates

comment:25 Changed 8 years ago by Evan

In [26731]:

Refs #13828, refine comments in dojo/touch so that it works well with API doc generator, remove unneeded dojo/window dependency in test case

comment:26 Changed 8 years ago by Evan

In [26736]:

Refs #13828 !strict, 1 trivial comment wording for dojox/gesture/Base

comment:27 Changed 8 years ago by Evan

In [26843]:

Refs #13828 !strict, trivial comment wording fix

comment:28 Changed 8 years ago by Evan

In [26876]:

Refs #13828 !strict, fix comments in dojox/gesture/* in order to be displayed better in api doc

comment:29 Changed 8 years ago by Evan

Resolution: fixed
Status: newclosed

Docs are added at dojo/touch and dojox/gesture

comment:30 Changed 8 years ago by Evan

In [27023]:

Refs #13828 !strict, use declare(/*===== "dojox.gesture.tap", =====*/, Base, {...}) for anonymous classes so that API doc parser can recognize it. Also applied to 1.7.* branch

Note: See TracTickets for help on using tickets.