Closed Bug 773749 Opened 7 years ago Closed 7 years ago

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

Categories

(Core :: Disability Access APIs, defect, major)

ARM
Android
defect
Not set
major

Tracking

()

RESOLVED FIXED
mozilla17

People

(Reporter: MarcoZ, Assigned: eeejay)

References

Details

Attachments

(1 file, 2 obsolete files)

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.
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.
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.
Just an update. I have a working version locally. It adds support for flick left and right as well.
Depends on: 777560
Complementary to the patch in bug 777560. Not putting this up for review until the other bug is resolved.
OK, I think this is ready for review. It does not break anything if it lands before the Java layer.
Attachment #645972 - Attachment is obsolete: true
Attachment #646253 - Flags: review?(dbolter)
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?
Attachment #646253 - Flags: review?(dbolter) → review+
Rebased.
Attachment #646253 - Attachment is obsolete: true
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.
https://hg.mozilla.org/mozilla-central/rev/a295ff4319fb
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla17
You need to log in before you can comment on or make changes to this bug.