Last Comment Bug 780350 - [AccessFu] Introduce high-level touch gestures
: [AccessFu] Introduce high-level touch gestures
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: Disability Access APIs (show other bugs)
: Trunk
: ARM Android
: -- normal (vote)
: mozilla17
Assigned To: Eitan Isaacson [:eeejay]
:
: alexander :surkov
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2012-08-03 21:08 PDT by Eitan Isaacson [:eeejay]
Modified: 2012-08-18 04:26 PDT (History)
2 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Introduce GestureMangler and add some basic gesture support. (14.26 KB, patch)
2012-08-03 21:10 PDT, Eitan Isaacson [:eeejay]
no flags Details | Diff | Splinter Review
Introduce TouchAdapter and add some gesture support. (23.08 KB, patch)
2012-08-15 13:25 PDT, Eitan Isaacson [:eeejay]
dbolter: review+
Details | Diff | Splinter Review

Description Eitan Isaacson [:eeejay] 2012-08-03 21:08:46 PDT
In B2G we need to handle touch gestures ourselves. So we need to figure out a way to translate individual DOM touch events into gestures that we could use, like double tap, swip, explore, etc.
Comment 1 Eitan Isaacson [:eeejay] 2012-08-03 21:10:18 PDT
Created attachment 648945 [details] [diff] [review]
Introduce GestureMangler and add some basic gesture support.

This is still a WIP, not ready for review. Just putting it here for transparency and safe-keeping so it doesn't get lost over the weekend.
Comment 2 James Teh [:Jamie] 2012-08-03 22:56:02 PDT
If it's of any help, we have Python code in NVDA to handle abstract finger touches and convert them into gestures. Obviously, it won't be of direct use, but the ideas might be useful. It's also GPL which might cause licensing issues, but I'm sure we can figure that out for Mozilla. :)
Comment 3 Eitan Isaacson [:eeejay] 2012-08-08 10:44:36 PDT
(In reply to James Teh [:Jamie] from comment #2)
> If it's of any help, we have Python code in NVDA to handle abstract finger
> touches and convert them into gestures. Obviously, it won't be of direct
> use, but the ideas might be useful. It's also GPL which might cause
> licensing issues, but I'm sure we can figure that out for Mozilla. :)

Thanks Jamie! I think the patch I put here is fully functional. It was fun to implement from scratch. Just not ready to sign off on it.
Comment 4 Eitan Isaacson [:eeejay] 2012-08-15 13:25:59 PDT
Created attachment 652197 [details] [diff] [review]
Introduce TouchAdapter and add some gesture support.

Still a bit raw, but I think it is a good start. Has everything we will mostly need.
Comment 5 David Bolter [:davidb] 2012-08-16 07:19:11 PDT
Comment on attachment 652197 [details] [diff] [review]
Introduce TouchAdapter and add some gesture support.

Review of attachment 652197 [details] [diff] [review]:
-----------------------------------------------------------------

::: accessible/src/jsat/AccessFu.css
@@ +25,5 @@
> +  width: 100%;
> +  height: 100%;
> +  position: fixed;
> +  top: 0px;
> +  left: 0px;

Do you need to set a reasonable z-index to make sure it is always on top?
Comment 6 David Bolter [:davidb] 2012-08-17 09:09:13 PDT
Is Mouse2Touch used?
Comment 7 Eitan Isaacson [:eeejay] 2012-08-17 09:15:24 PDT
(In reply to David Bolter [:davidb] from comment #5)
> Comment on attachment 652197 [details] [diff] [review]
> Introduce TouchAdapter and add some gesture support.
> 
> Review of attachment 652197 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: accessible/src/jsat/AccessFu.css
> @@ +25,5 @@
> > +  width: 100%;
> > +  height: 100%;
> > +  position: fixed;
> > +  top: 0px;
> > +  left: 0px;
> 
> Do you need to set a reasonable z-index to make sure it is always on top?

Not really, since it is the last child appended.
Comment 8 Eitan Isaacson [:eeejay] 2012-08-17 09:15:50 PDT
(In reply to David Bolter [:davidb] from comment #6)
> Is Mouse2Touch used?

For desktop B2G use.
Comment 9 David Bolter [:davidb] 2012-08-17 09:23:37 PDT
(In reply to Eitan Isaacson [:eeejay] from comment #8)
> (In reply to David Bolter [:davidb] from comment #6)
> > Is Mouse2Touch used?
> 
> For desktop B2G use.

So hmmm we need a Utils.MozBuildApp switch for that I guess. Later is fine.
Comment 10 Eitan Isaacson [:eeejay] 2012-08-17 09:28:56 PDT
Comment on attachment 652197 [details] [diff] [review]
Introduce TouchAdapter and add some gesture support.

Review of attachment 652197 [details] [diff] [review]:
-----------------------------------------------------------------

::: accessible/src/jsat/TouchAdapter.jsm
@@ +62,5 @@
> +    this.glass.addEventListener('touchmove', this, true, true);
> +    this.glass.addEventListener('touchstart', this, true, true);
> +
> +    if (Utils.OS != 'Android')
> +      Mouse2Touch.attach(aWindow);

We check OS and not MozBuildApp because it will always be B2G, and we are interested to attach this when we are not on a device (ie. not in "Android".
Comment 11 David Bolter [:davidb] 2012-08-17 09:44:36 PDT
(In reply to Eitan Isaacson [:eeejay] from comment #10)
> Comment on attachment 652197 [details] [diff] [review]
> Introduce TouchAdapter and add some gesture support.
> 
> Review of attachment 652197 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: accessible/src/jsat/TouchAdapter.jsm
> @@ +62,5 @@
> > +    this.glass.addEventListener('touchmove', this, true, true);
> > +    this.glass.addEventListener('touchstart', this, true, true);
> > +
> > +    if (Utils.OS != 'Android')
> > +      Mouse2Touch.attach(aWindow);
> 
> We check OS and not MozBuildApp because it will always be B2G, and we are
> interested to attach this when we are not on a device (ie. not in "Android".

Ah ok thanks! I had some kind of browser find-in-page fail when I went back to this review.
Comment 12 David Bolter [:davidb] 2012-08-17 09:46:27 PDT
In the your naming usage of "Adapter", what is the semantic/ how are you using it? I always think of the Java idiom/pattern.
Comment 13 Eitan Isaacson [:eeejay] 2012-08-17 09:53:42 PDT
(In reply to David Bolter [:davidb] from comment #12)
> In the your naming usage of "Adapter", what is the semantic/ how are you
> using it? I always think of the Java idiom/pattern.

I haven't really given it much thought. Just that we support two platforms and they each use a distinct "adapter" that translates different types of touch events in to higher level gesture events that VirtualCursorController listens for. Originally I called it a GestureMangler. I am open to more accurate names here.
Comment 14 David Bolter [:davidb] 2012-08-17 10:51:44 PDT
OK - sorry for the delay BTW - my review session constantly interrupted.
Comment 15 David Bolter [:davidb] 2012-08-17 12:21:20 PDT
Comment on attachment 652197 [details] [diff] [review]
Introduce TouchAdapter and add some gesture support.

Review of attachment 652197 [details] [diff] [review]:
-----------------------------------------------------------------

OK! r=me. Reviewing is hard. I hoep to revisit some parts of this in case I can think of improvements but don't block on this.

::: accessible/src/jsat/AccessFu.css
@@ +14,1 @@
>  #virtual-cursor-inset { 

Nit: trailing whitespace.

::: accessible/src/jsat/TouchAdapter.jsm
@@ +18,5 @@
> +const EXPLORE_THROTTLE = 100;
> +
> +var TouchAdapter = {
> +  // minimal swipe distance in inches
> +  SWIPE_MIN_DISTANCE: 0.4,

Did you derive these numbers or are they from some source?

@@ +30,5 @@
> +  // maximum consecutive
> +  MAX_CONSECUTIVE_GESTURE_DELAY: 400,
> +
> +  // delay before tap turns into dwell
> +  DWELL_MIN_TIME: 500,

Maybe call this DWELL_THRESHOLD?

@@ +236,5 @@
> +   * Compile a gesture from an individual touch point. This is used by the
> +   * TouchAdapter to compound multiple single gestures in to higher level
> +   * gestures.
> +   */
> +  compile: function TouchPoint_compile(aTime) {

Not sure about the name 'compile' here but will suffice and I'll suggest a different name later if I think of a better one.

@@ +249,5 @@
> +        return {type: 'tap', x: this.startX, y: this.startY};
> +      } else if (!this.done && duration == TouchAdapter.DWELL_MIN_TIME) {
> +        return {type: 'dwell', x: this.startX, y: this.startY};
> +      }
> +

Nit: needless line spacer.

@@ +276,5 @@
> +        else
> +          swipeGesture.type = 'swipeup';
> +      } else {
> +        // A perfect 45 degree swipe?? Not in our book.
> +          return null;

I think I'd make one of the former conditions into a conditionless else and fold a perfect 45 into one of them but your call. Actually maybe we'll want more deadspace in the 45 degree range later - dunno. What you have is fine to start with.
Comment 16 Eitan Isaacson [:eeejay] 2012-08-17 15:33:35 PDT
(In reply to David Bolter [:davidb] from comment #15)
> Comment on attachment 652197 [details] [diff] [review]
> Introduce TouchAdapter and add some gesture support.
> 
> Review of attachment 652197 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> OK! r=me. Reviewing is hard. I hoep to revisit some parts of this in case I
> can think of improvements but don't block on this.
> 

Yeah, this isn't going to be perfect the first time.

> ::: accessible/src/jsat/AccessFu.css
> @@ +14,1 @@
> >  #virtual-cursor-inset { 
> 
> Nit: trailing whitespace.
> 
> ::: accessible/src/jsat/TouchAdapter.jsm
> @@ +18,5 @@
> > +const EXPLORE_THROTTLE = 100;
> > +
> > +var TouchAdapter = {
> > +  // minimal swipe distance in inches
> > +  SWIPE_MIN_DISTANCE: 0.4,
> 
> Did you derive these numbers or are they from some source?
> 

I took them from my early extension prototype from last year. I think I made them up.

> @@ +30,5 @@
> > +  // maximum consecutive
> > +  MAX_CONSECUTIVE_GESTURE_DELAY: 400,
> > +
> > +  // delay before tap turns into dwell
> > +  DWELL_MIN_TIME: 500,
> 
> Maybe call this DWELL_THRESHOLD?

Sure.

> 
> @@ +236,5 @@
> > +   * Compile a gesture from an individual touch point. This is used by the
> > +   * TouchAdapter to compound multiple single gestures in to higher level
> > +   * gestures.
> > +   */
> > +  compile: function TouchPoint_compile(aTime) {
> 
> Not sure about the name 'compile' here but will suffice and I'll suggest a
> different name later if I think of a better one.
> 

Good!

> @@ +249,5 @@
> > +        return {type: 'tap', x: this.startX, y: this.startY};
> > +      } else if (!this.done && duration == TouchAdapter.DWELL_MIN_TIME) {
> > +        return {type: 'dwell', x: this.startX, y: this.startY};
> > +      }
> > +
> 
> Nit: needless line spacer.
> 

Yup.

> @@ +276,5 @@
> > +        else
> > +          swipeGesture.type = 'swipeup';
> > +      } else {
> > +        // A perfect 45 degree swipe?? Not in our book.
> > +          return null;
> 
> I think I'd make one of the former conditions into a conditionless else and
> fold a perfect 45 into one of them but your call. Actually maybe we'll want
> more deadspace in the 45 degree range later - dunno. What you have is fine
> to start with.

OK.

Note You need to log in before you can comment on or make changes to this bug.