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

RESOLVED FIXED in mozilla17

Status

()

Core
Disability Access APIs
--
major
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: MarcoZ, Assigned: eeejay)

Tracking

Trunk
mozilla17
ARM
Android
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 2 obsolete attachments)

(Reporter)

Description

5 years ago
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.
(Assignee)

Comment 1

5 years ago
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.
(Reporter)

Comment 2

5 years ago
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.
(Assignee)

Comment 3

5 years ago
Just an update. I have a working version locally. It adds support for flick left and right as well.
(Assignee)

Updated

5 years ago
Depends on: 777560
(Assignee)

Comment 4

5 years ago
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.
(Assignee)

Comment 5

5 years ago
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.
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+
(Assignee)

Comment 7

5 years ago
Created attachment 652965 [details] [diff] [review]
Bug 773749 - Add Jelly Bean support to AccessFu.

Rebased.
Attachment #646253 - Attachment is obsolete: true
(Assignee)

Comment 8

5 years ago
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.
(Assignee)

Comment 9

5 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/a295ff4319fb
Assignee: nobody → eitan
https://hg.mozilla.org/mozilla-central/rev/a295ff4319fb
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla17
You need to log in before you can comment on or make changes to this bug.