766779 - [AccessFu] Introduce Android explore by touch

Status: RESOLVED FIXED

(Core :: Disability Access APIs, defect)

Comment 1

[Eitan Isaacson [:eeejay]]

Attachment: Introduce Explore by touch

Comment 2

[Eitan Isaacson [:eeejay]]

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)]

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]]

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]]

Attachment: 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)]

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] (NeedInfo me for attention)]

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]]

(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] (NeedInfo me for attention)]

(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] (NeedInfo me for attention)]

That was an accidental submit... sorry for all the paste.

Comment 11

[David Bolter [:davidb] (NeedInfo me for attention)]

> > > > +    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] (NeedInfo me for attention)]

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]]

(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]]

Attachment: Introduce Explore by touch

Golly, good catch. I don't know how I waisted a sunny afternoon on that silly timeout stuff.

Comment 15

[Eitan Isaacson [:eeejay]]

http://hg.mozilla.org/integration/mozilla-inbound/rev/b4fff5e5afb3

Comment 16

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/b4fff5e5afb3