Last Comment Bug 766779 - [AccessFu] Introduce Android explore by touch
: [AccessFu] Introduce Android explore by touch
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: Disability Access APIs (show other bugs)
: Trunk
: ARM Android
: -- normal (vote)
: mozilla16
Assigned To: Eitan Isaacson [:eeejay]
:
Mentors:
Depends on: 769217 768863
Blocks: 757721
  Show dependency treegraph
 
Reported: 2012-06-20 16:44 PDT by Eitan Isaacson [:eeejay]
Modified: 2012-10-02 09:27 PDT (History)
1 user (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Introduce Explore by touch (11.01 KB, patch)
2012-06-20 16:45 PDT, Eitan Isaacson [:eeejay]
no flags Details | Diff | Splinter Review
Bug 766779 - Introduce Explore by touch (11.48 KB, patch)
2012-06-21 17:28 PDT, Eitan Isaacson [:eeejay]
no flags Details | Diff | Splinter Review
Introduce Explore by touch (11.09 KB, patch)
2012-06-22 16:01 PDT, Eitan Isaacson [:eeejay]
dbolter: review+
Details | Diff | Splinter Review

Description Eitan Isaacson [:eeejay] 2012-06-20 16:44:16 PDT

    
Comment 1 Eitan Isaacson [:eeejay] 2012-06-20 16:45:39 PDT
Created attachment 635121 [details] [diff] [review]
Introduce Explore by touch
Comment 2 Eitan Isaacson [:eeejay] 2012-06-20 17:23:44 PDT
Comment on attachment 635121 [details] [diff] [review]
Introduce Explore by touch

Not entirely happy with this yet, so I am going to sleep on it. Sorry for the spam.
Comment 3 Marco Zehe (:MarcoZ) 2012-06-21 00:30:48 PDT
Comment on attachment 635121 [details] [diff] [review]
Introduce Explore by touch

>+  moveToPoint: function moveToPoint(document, aX, aY) {

nit: aDocument.

Also, you might want to put this ina small utility method since you have the same code in two places now:

>+    let document = Utils.getBrowserApp(this.chromeWin).
>+      selectedBrowser.contentDocument;

And:

>+    if (aContext.oldAccessible) {
>+      // This isn't really used by TalkBack so this is a half-hearted attempt
>+      // for now.

Is this meant to do something like announcing the exits from landmarks etc., when one touches a new area? Am not certain if this adds too much verbosity...
Comment 4 Eitan Isaacson [:eeejay] 2012-06-21 16:48:21 PDT
Thanks for the feedback.

(In reply to Marco Zehe (:MarcoZ) from comment #3)
> Comment on attachment 635121 [details] [diff] [review]
> Introduce Explore by touch
> 
> >+  moveToPoint: function moveToPoint(document, aX, aY) {
> 
> nit: aDocument.
> 

Yup.

> Also, you might want to put this ina small utility method since you have the
> same code in two places now:
> 
> >+    let document = Utils.getBrowserApp(this.chromeWin).
> >+      selectedBrowser.contentDocument;
> 
> And:
> 

I made a function for it in Utils.

> >+    if (aContext.oldAccessible) {
> >+      // This isn't really used by TalkBack so this is a half-hearted attempt
> >+      // for now.
> 
> Is this meant to do something like announcing the exits from landmarks etc.,
> when one touches a new area? Am not certain if this adds too much
> verbosity...

Yeah, I don't know who and what expects the exit event, but I will keep it there in case something out there is asserting for two enter events without an exit event. Dunno. I'll keep the text field empty though..
Comment 5 Eitan Isaacson [:eeejay] 2012-06-21 17:28:48 PDT
Created attachment 635537 [details] [diff] [review]
Bug 766779 - Introduce Explore by touch

OK! I am much happier with this one. It throttles the actual moveToPoint calls to no more than 10 a second. This makes everything feel more responsive. There is a bit of function nesting here, but nothing too much for David - a Dojo veteran :)
Comment 6 Marco Zehe (:MarcoZ) 2012-06-22 00:07:02 PDT
Comment on attachment 635537 [details] [diff] [review]
Bug 766779 - Introduce Explore by touch

Just one question: When are you going to use the VirtualCursor_ prefix in function names and when not? You actually add one in this patch, but other new functions don't have it.
Comment 7 David Bolter [:davidb] 2012-06-22 09:10:19 PDT
Comment on attachment 635537 [details] [diff] [review]
Bug 766779 - Introduce Explore by touch

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

::: accessible/src/jsat/Presenters.jsm
@@ +238,5 @@
>      if (!aContext.accessible)
>        return;
>  
> +    let isExploreByTouch = aReason == Ci.nsIAccessiblePivot.REASON_POINT &&
> +      Utils.AndroidSdkVersion >= 14;

Is this the right indenting? I'd find it more readable if 'aReason' and 'Utils' lined up.

@@ +248,5 @@
> +         gecko: {
> +           type: 'Accessibility:Event',
> +           eventType: this.ANDROID_VIEW_HOVER_EXIT,
> +           text: []
> +         }

Presumably the most the user would want here is a subtle sonification? Would we file a bug somewhere on Android or Talkback for that?

@@ +258,5 @@
> +    if (isExploreByTouch) {
> +      // Just provide the parent for some context, no need to utter the entire
> +      // ancestry change since it doesn't make sense in spatial navigation.
> +      for (var i = aContext.newAncestry.length - 1; i >= 0; i--) {
> +        let utter = UtteranceGenerator.genForObject(aContext.newAncestry[i]);

Does this loop really only provide the parent context?

::: accessible/src/jsat/Utils.jsm
@@ +49,5 @@
>    },
>  
> +  getCurrentContentDoc: function getCurrentContentDoc(aWindow) {
> +    return this.getBrowserApp(aWindow).selectedBrowser.contentDocument;
> +  },

Why did you choose to add "Current" to the name? Is this to make it obvious that a window's content document can change?

::: accessible/src/jsat/VirtualCursorController.jsm
@@ +422,5 @@
>    },
>  
> +  handleEvent: function VirtualCursorController_handleEvent(aEvent) {
> +    switch (aEvent.type) {
> +      case 'keypress':

Are you expecting more cases for this switch?

@@ +439,5 @@
> +    if (this._handleMousemove._timeoutHandler) {
> +      this._handleMousemove._lastEvent = aEvent;
> +    } else {
> +      // Just started moving. Immediately call moveToPoint, and ignore next
> +      // move events for given interval.

Instead of "next", I'd say "intermediate".

@@ +456,5 @@
> +          this.chromeWin.setTimeout(
> +            function() {
> +              this._handleMousemove._timeoutHandler = 0;
> +            }.bind(this), 100);
> +        }.bind(this), 100);

I get it, and I just want to decide if my spidey sense is okay with it...

@@ +629,5 @@
>        } catch (x) {
>          doc = doc.parentDocument;
>          continue;
>        }
> +      vc.moveNext(aRule || TraversalRules.Simple, aAccessible, true);

I take it there is always a parent document with a vc?
Comment 8 Eitan Isaacson [:eeejay] 2012-06-22 09:22:51 PDT
(In reply to David Bolter [:davidb] from comment #7)
> Comment on attachment 635537 [details] [diff] [review]
> Bug 766779 - Introduce Explore by touch
> 
> Review of attachment 635537 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: accessible/src/jsat/Presenters.jsm
> @@ +238,5 @@
> >      if (!aContext.accessible)
> >        return;
> >  
> > +    let isExploreByTouch = aReason == Ci.nsIAccessiblePivot.REASON_POINT &&
> > +      Utils.AndroidSdkVersion >= 14;
> 
> Is this the right indenting? I'd find it more readable if 'aReason' and
> 'Utils' lined up.
> 

It is right. I could put it in parenthesis, and then it would line up...

> @@ +248,5 @@
> > +         gecko: {
> > +           type: 'Accessibility:Event',
> > +           eventType: this.ANDROID_VIEW_HOVER_EXIT,
> > +           text: []
> > +         }
> 
> Presumably the most the user would want here is a subtle sonification? Would
> we file a bug somewhere on Android or Talkback for that?
> 

It is not really a bug as much as there is no reason to populate the text field, so I didn't bother.

> @@ +258,5 @@
> > +    if (isExploreByTouch) {
> > +      // Just provide the parent for some context, no need to utter the entire
> > +      // ancestry change since it doesn't make sense in spatial navigation.
> > +      for (var i = aContext.newAncestry.length - 1; i >= 0; i--) {
> > +        let utter = UtteranceGenerator.genForObject(aContext.newAncestry[i]);
> 
> Does this loop really only provide the parent context?
> 

Yes, we break out of it after we get to the first utterable ancestor.

> ::: accessible/src/jsat/Utils.jsm
> @@ +49,5 @@
> >    },
> >  
> > +  getCurrentContentDoc: function getCurrentContentDoc(aWindow) {
> > +    return this.getBrowserApp(aWindow).selectedBrowser.contentDocument;
> > +  },
> 
> Why did you choose to add "Current" to the name? Is this to make it obvious
> that a window's content document can change?
> 

Because it is the content document of the selected browser/tab.

> ::: accessible/src/jsat/VirtualCursorController.jsm
> @@ +422,5 @@
> >    },
> >  
> > +  handleEvent: function VirtualCursorController_handleEvent(aEvent) {
> > +    switch (aEvent.type) {
> > +      case 'keypress':
> 
> Are you expecting more cases for this switch?
> 

Not that I could think of now. Do you really hate switch statements?

> @@ +439,5 @@
> > +    if (this._handleMousemove._timeoutHandler) {
> > +      this._handleMousemove._lastEvent = aEvent;
> > +    } else {
> > +      // Just started moving. Immediately call moveToPoint, and ignore next
> > +      // move events for given interval.
> 
> Instead of "next", I'd say "intermediate".
> 

Sounds good.

> @@ +456,5 @@
> > +          this.chromeWin.setTimeout(
> > +            function() {
> > +              this._handleMousemove._timeoutHandler = 0;
> > +            }.bind(this), 100);
> > +        }.bind(this), 100);
> 
> I get it, and I just want to decide if my spidey sense is okay with it...
> 

Please do!

> @@ +629,5 @@
> >        } catch (x) {
> >          doc = doc.parentDocument;
> >          continue;
> >        }
> > +      vc.moveNext(aRule || TraversalRules.Simple, aAccessible, true);
> 
> I take it there is always a parent document with a vc?

Yeah, that is a leftover from a previous patch you r+ed and said I could remove the null check. I could remove that change...
Comment 9 David Bolter [:davidb] 2012-06-22 10:19:39 PDT
(In reply to Eitan Isaacson [:eeejay] from comment #8)
> (In reply to David Bolter [:davidb] from comment #7)
> > Comment on attachment 635537 [details] [diff] [review]
> > Bug 766779 - Introduce Explore by touch
> > 
> > Review of attachment 635537 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > ::: accessible/src/jsat/Presenters.jsm
> > @@ +238,5 @@
> > >      if (!aContext.accessible)
> > >        return;
> > >  
> > > +    let isExploreByTouch = aReason == Ci.nsIAccessiblePivot.REASON_POINT &&
> > > +      Utils.AndroidSdkVersion >= 14;
> > 
> > Is this the right indenting? I'd find it more readable if 'aReason' and
> > 'Utils' lined up.
> > 
> 
> It is right. I could put it in parenthesis, and then it would line up...
> 
> > @@ +248,5 @@
> > > +         gecko: {
> > > +           type: 'Accessibility:Event',
> > > +           eventType: this.ANDROID_VIEW_HOVER_EXIT,
> > > +           text: []
> > > +         }
> > 
> > Presumably the most the user would want here is a subtle sonification? Would
> > we file a bug somewhere on Android or Talkback for that?
> > 
> 
> It is not really a bug as much as there is no reason to populate the text
> field, so I didn't bother.
> 
> > @@ +258,5 @@
> > > +    if (isExploreByTouch) {
> > > +      // Just provide the parent for some context, no need to utter the entire
> > > +      // ancestry change since it doesn't make sense in spatial navigation.
> > > +      for (var i = aContext.newAncestry.length - 1; i >= 0; i--) {
> > > +        let utter = UtteranceGenerator.genForObject(aContext.newAncestry[i]);
> > 
> > Does this loop really only provide the parent context?
> > 
> 
> Yes, we break out of it after we get to the first utterable ancestor.
> 
> > ::: accessible/src/jsat/Utils.jsm
> > @@ +49,5 @@
> > >    },
> > >  
> > > +  getCurrentContentDoc: function getCurrentContentDoc(aWindow) {
> > > +    return this.getBrowserApp(aWindow).selectedBrowser.contentDocument;
> > > +  },
> > 
> > Why did you choose to add "Current" to the name? Is this to make it obvious
> > that a window's content document can change?
> > 
> 
> Because it is the content document of the selected browser/tab.
> 
> > ::: accessible/src/jsat/VirtualCursorController.jsm
> > @@ +422,5 @@
> > >    },
> > >  
> > > +  handleEvent: function VirtualCursorController_handleEvent(aEvent) {
> > > +    switch (aEvent.type) {
> > > +      case 'keypress':
> > 
> > Are you expecting more cases for this switch?
> > 
> 
> Not that I could think of now. Do you really hate switch statements?

It is just something I inherited from my dojo reviewers. I don't mind personally.

> > @@ +439,5 @@
> > > +    if (this._handleMousemove._timeoutHandler) {
> > > +      this._handleMousemove._lastEvent = aEvent;
> > > +    } else {
> > > +      // Just started moving. Immediately call moveToPoint, and ignore next
> > > +      // move events for given interval.
> > 
> > Instead of "next", I'd say "intermediate".
> > 
> 
> Sounds good.
> 
> > @@ +456,5 @@
> > > +          this.chromeWin.setTimeout(
> > > +            function() {
> > > +              this._handleMousemove._timeoutHandler = 0;
> > > +            }.bind(this), 100);
> > > +        }.bind(this), 100);
> > 
> > I get it, and I just want to decide if my spidey sense is okay with it...
> > 
> 
> Please do!
> 
> > @@ +629,5 @@
> > >        } catch (x) {
> > >          doc = doc.parentDocument;
> > >          continue;
> > >        }
> > > +      vc.moveNext(aRule || TraversalRules.Simple, aAccessible, true);
> > 
> > I take it there is always a parent document with a vc?
> 
> Yeah, that is a leftover from a previous patch you r+ed and said I could
> remove the null check. I could remove that change...

Nah.
Comment 10 David Bolter [:davidb] 2012-06-22 10:20:11 PDT
That was an accidental submit... sorry for all the paste.
Comment 11 David Bolter [:davidb] 2012-06-22 10:22:36 PDT
> > > > +    let isExploreByTouch = aReason == Ci.nsIAccessiblePivot.REASON_POINT &&
> > > > +      Utils.AndroidSdkVersion >= 14;
> > > 
> > > Is this the right indenting? I'd find it more readable if 'aReason' and
> > > 'Utils' lined up.
> > > 
> > 
> > It is right. I could put it in parenthesis, and then it would line up...

That'd be my preference.

> > > Does this loop really only provide the parent context?
> > > 
> > 
> > Yes, we break out of it after we get to the first utterable ancestor.

OK.

> > > Why did you choose to add "Current" to the name? Is this to make it obvious
> > > that a window's content document can change?
> > > 
> > 
> > Because it is the content document of the selected browser/tab.

OK.

(Thinking about the timeouts now)
Comment 12 David Bolter [:davidb] 2012-06-22 11:05:53 PDT
Comment on attachment 635537 [details] [diff] [review]
Bug 766779 - Introduce Explore by touch

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

::: accessible/src/jsat/VirtualCursorController.jsm
@@ +456,5 @@
> +          this.chromeWin.setTimeout(
> +            function() {
> +              this._handleMousemove._timeoutHandler = 0;
> +            }.bind(this), 100);
> +        }.bind(this), 100);

Instead of the first timeout have you thought of comparing event timestamps? Something like:
if (!lastMoveTime || lastMoveTime - curMoveTime > 100)

That might get rid of one timeout and make the code simpler.
Comment 13 Eitan Isaacson [:eeejay] 2012-06-22 11:08:58 PDT
(In reply to David Bolter [:davidb] from comment #12)
> Comment on attachment 635537 [details] [diff] [review]
> Bug 766779 - Introduce Explore by touch
> 
> Review of attachment 635537 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: accessible/src/jsat/VirtualCursorController.jsm
> @@ +456,5 @@
> > +          this.chromeWin.setTimeout(
> > +            function() {
> > +              this._handleMousemove._timeoutHandler = 0;
> > +            }.bind(this), 100);
> > +        }.bind(this), 100);
> 
> Instead of the first timeout have you thought of comparing event timestamps?
> Something like:
> if (!lastMoveTime || lastMoveTime - curMoveTime > 100)
> 
> That might get rid of one timeout and make the code simpler.

No! But that is a good idea. I forgot about event timestamps. I'll upload another patch soon. Or come back and say that it got even more complex.
Comment 14 Eitan Isaacson [:eeejay] 2012-06-22 16:01:01 PDT
Created attachment 635960 [details] [diff] [review]
Introduce Explore by touch

Golly, good catch. I don't know how I waisted a sunny afternoon on that silly timeout stuff.
Comment 16 Ed Morley [:emorley] 2012-06-26 01:58:45 PDT
https://hg.mozilla.org/mozilla-central/rev/b4fff5e5afb3

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