Last Comment Bug 773749 - [AccessFu] In Jelly Bean, double-tapping to activate an item does not work in web content
: [AccessFu] In Jelly Bean, double-tapping to activate an item does not work in...
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: Disability Access APIs (show other bugs)
: Trunk
: ARM Android
: -- major (vote)
: mozilla17
Assigned To: Eitan Isaacson [:eeejay]
:
Mentors:
Depends on: 777560
Blocks:
  Show dependency treegraph
 
Reported: 2012-07-13 11:45 PDT by Marco Zehe (:MarcoZ) on PTO until August 15
Modified: 2012-08-21 19:10 PDT (History)
2 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Add Jelly Bean support to AccessFu. (9.97 KB, patch)
2012-07-25 17:52 PDT, Eitan Isaacson [:eeejay]
no flags Details | Diff | Splinter Review
Bug 773749 - Add Jelly Bean support to AccessFu. (11.57 KB, patch)
2012-07-26 12:13 PDT, Eitan Isaacson [:eeejay]
dbolter: review+
Details | Diff | Splinter Review
Bug 773749 - Add Jelly Bean support to AccessFu. (11.74 KB, patch)
2012-08-17 16:03 PDT, Eitan Isaacson [:eeejay]
no flags Details | Diff | Splinter Review

Description Marco Zehe (:MarcoZ) on PTO until August 15 2012-07-13 11:45:09 PDT
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] 2012-07-13 11:50:00 PDT
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) on PTO until August 15 2012-07-13 11:53:22 PDT
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] 2012-07-24 15:09:28 PDT
Just an update. I have a working version locally. It adds support for flick left and right as well.
Comment 4 Eitan Isaacson [:eeejay] 2012-07-25 17:52:02 PDT
Created attachment 645972 [details] [diff] [review]
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] 2012-07-26 12:13:01 PDT
Created attachment 646253 [details] [diff] [review]
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] 2012-07-27 17:22:10 PDT
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] 2012-08-17 16:03:46 PDT
Created attachment 652965 [details] [diff] [review]
Bug 773749 - Add Jelly Bean support to AccessFu.

Rebased.
Comment 8 Eitan Isaacson [:eeejay] 2012-08-20 14:57:16 PDT
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 10 Ryan VanderMeulen [:RyanVM] 2012-08-21 19:10:18 PDT
https://hg.mozilla.org/mozilla-central/rev/a295ff4319fb

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