773749 - [AccessFu] In Jelly Bean, double-tapping to activate an item does not work in web content

Status: RESOLVED FIXED

(Core :: Disability Access APIs, defect)

Description

[Marco Zehe (:MarcoZ)]

My Nexus just got updated to Jelly Bean. While the native UI works fine in terms of selecting and double-tapping, web content does not. Double-tapping to activate the selected item has one of two possible effects:

1. It activates an item from the native UI, presumably the last one touched there or the first found.

2. It simply sets our virtual cursor focus to the item that happens to be under the finger that double-taps.

Comment 1

[Eitan Isaacson [:eeejay]]

I suspected our EBT would break in JB... Once I get my hands on a JB device, and Google dumps their code in AOSP we will figure this out.

Same with swipe gestures, they probably don't work either.

Comment 2

[Marco Zehe (:MarcoZ)]

They only walk through the native UI pieces. But there, they work. Although it's sometimes a bit funny, I feel they walk things that aren't visible.

Comment 3

[Eitan Isaacson [:eeejay]]

Just an update. I have a working version locally. It adds support for flick left and right as well.

Comment 4

[Eitan Isaacson [:eeejay]]

Attachment: Add Jelly Bean support to AccessFu.

Complementary to the patch in bug 777560. Not putting this up for review until the other bug is resolved.

Comment 5

[Eitan Isaacson [:eeejay]]

Attachment: Bug 773749 - Add Jelly Bean support to AccessFu.

OK, I think this is ready for review. It does not break anything if it lands before the Java layer.

Comment 6

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

Comment on attachment 646253 [details] [diff] [review]
Bug 773749 - Add Jelly Bean support to AccessFu.

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

Provisional r=me since I'm not sure I can iterate soon. Treat the nits and questions as "please be sure". I don't see anything scary.

::: accessible/src/jsat/AccessFu.jsm
@@ +54,1 @@
>  

nit: remove extra line space.

@@ +69,5 @@
>  
>      // Implicitly add the Android presenter on Android.
> +    if (Utils.MozBuildApp == 'mobile/android') {
> +      this._androidPresenter = new AndroidPresenter();
> +      this.addPresenter(this._androidPresenter);

This smells funny (assigning to |this| and then calling |this|.add) but I can see why you do it this way.

@@ +216,5 @@
> +        VirtualCursorController.
> +          moveBackward(Utils.getCurrentContentDoc(this.chromeWin));
> +        break;
> +      case 'Accessibility:CurrentObject':
> +        this._androidPresenter.accessibilityFocus();

This smells funny too but okay for now. I don't have a great alternative in mind but maybe we can have a fresh look when I'm back.

::: accessible/src/jsat/Presenters.jsm
@@ +154,2 @@
>  
>      if (!aContext.accessible) {

Should this check aContext.accessible or just aContext? I guess it is probably correct the way you have it.

@@ +187,2 @@
>      let vp = Utils.getViewport(this.chromeWin) || { zoom: 1.0, offsetY: 0 };
> +    let r = aContext.bounds.scale(vp.zoom, vp.zoom).expandToIntegers();

nice

@@ +215,5 @@
>    ANDROID_WINDOW_STATE_CHANGED: 0x20,
>    ANDROID_VIEW_HOVER_ENTER: 0x80,
>    ANDROID_VIEW_HOVER_EXIT: 0x100,
>    ANDROID_VIEW_SCROLLED: 0x1000,
> +  ANDROID_ANNOUNCEMENT: 0x4000,

Interesting.

@@ +232,5 @@
>      let isExploreByTouch = (aReason == Ci.nsIAccessiblePivot.REASON_POINT &&
>                              Utils.AndroidSdkVersion >= 14);
> +    let focusEventType = (Utils.AndroidSdkVersion >= 16) ?
> +      this.ANDROID_VIEW_ACCESSIBILITY_FOCUSED :
> +      this.ANDROID_VIEW_FOCUSED;

Why is this assignment so far away from where it is later used (in sendMessageToJava)?

@@ +517,5 @@
> +        QueryInterface(Ci.nsIAccessible);
> +      docRoot.getBounds(docX, docY, {}, {});
> +
> +      this._bounds = new Rect(objX.value, objY.value, objW.value, objH.value).
> +        translate(-docX.value, -docY.value);

Snazzy.

@@ +520,5 @@
> +      this._bounds = new Rect(objX.value, objY.value, objW.value, objH.value).
> +        translate(-docX.value, -docY.value);
> +    }
> +
> +    return this._bounds.clone();

What is the clone for?

Comment 7

[Eitan Isaacson [:eeejay]]

Attachment: Bug 773749 - Add Jelly Bean support to AccessFu.

Rebased.

Comment 8

[Eitan Isaacson [:eeejay]]

Sorry I didn't reply to this earlier.

In reply to David Bolter [:davidb] from comment #6)
> Comment on attachment 646253 [details] [diff] [review]
> Bug 773749 - Add Jelly Bean support to AccessFu.
> 
> Review of attachment 646253 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Provisional r=me since I'm not sure I can iterate soon. Treat the nits and
> questions as "please be sure". I don't see anything scary.
> 
> ::: accessible/src/jsat/AccessFu.jsm
> @@ +54,1 @@
> >  
> 
> nit: remove extra line space.
> 

Sure.

> @@ +69,5 @@
> >  
> >      // Implicitly add the Android presenter on Android.
> > +    if (Utils.MozBuildApp == 'mobile/android') {
> > +      this._androidPresenter = new AndroidPresenter();
> > +      this.addPresenter(this._androidPresenter);
> 
> This smells funny (assigning to |this| and then calling |this|.add) but I
> can see why you do it this way.
> 

I know!

> @@ +216,5 @@
> > +        VirtualCursorController.
> > +          moveBackward(Utils.getCurrentContentDoc(this.chromeWin));
> > +        break;
> > +      case 'Accessibility:CurrentObject':
> > +        this._androidPresenter.accessibilityFocus();
> 
> This smells funny too but okay for now. I don't have a great alternative in
> mind but maybe we can have a fresh look when I'm back.
> 

Yeah, we could think about that for a bit. Maybe "redisplay()".

> ::: accessible/src/jsat/Presenters.jsm
> @@ +154,2 @@
> >  
> >      if (!aContext.accessible) {
> 
> Should this check aContext.accessible or just aContext? I guess it is
> probably correct the way you have it.
> 

'tis :)

> @@ +187,2 @@
> >      let vp = Utils.getViewport(this.chromeWin) || { zoom: 1.0, offsetY: 0 };
> > +    let r = aContext.bounds.scale(vp.zoom, vp.zoom).expandToIntegers();
> 
> nice
> 

thanks.

> @@ +215,5 @@
> >    ANDROID_WINDOW_STATE_CHANGED: 0x20,
> >    ANDROID_VIEW_HOVER_ENTER: 0x80,
> >    ANDROID_VIEW_HOVER_EXIT: 0x100,
> >    ANDROID_VIEW_SCROLLED: 0x1000,
> > +  ANDROID_ANNOUNCEMENT: 0x4000,
> 
> Interesting.

Yep.

> 
> @@ +232,5 @@
> >      let isExploreByTouch = (aReason == Ci.nsIAccessiblePivot.REASON_POINT &&
> >                              Utils.AndroidSdkVersion >= 14);
> > +    let focusEventType = (Utils.AndroidSdkVersion >= 16) ?
> > +      this.ANDROID_VIEW_ACCESSIBILITY_FOCUSED :
> > +      this.ANDROID_VIEW_FOCUSED;
> 
> Why is this assignment so far away from where it is later used (in
> sendMessageToJava)?
> 

Hm, I'll move that down.

> @@ +517,5 @@
> > +        QueryInterface(Ci.nsIAccessible);
> > +      docRoot.getBounds(docX, docY, {}, {});
> > +
> > +      this._bounds = new Rect(objX.value, objY.value, objW.value, objH.value).
> > +        translate(-docX.value, -docY.value);
> 
> Snazzy.
> 

That's how we do things here.

> @@ +520,5 @@
> > +      this._bounds = new Rect(objX.value, objY.value, objW.value, objH.value).
> > +        translate(-docX.value, -docY.value);
> > +    }
> > +
> > +    return this._bounds.clone();
> 
> What is the clone for?

The cached bounds are mutable, you don't want the translations that are done on it later to affect it, so we give a copy. I guess you could clone in from the "caller" too, but it seems to be harder to screw stuff up this way.

Comment 9

[Eitan Isaacson [:eeejay]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/a295ff4319fb

Comment 10

[Ryan VanderMeulen [:RyanVM]]

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