785852 - [AccessFu] Support editing mode navigation in Jelly Bean

Status: RESOLVED FIXED

(Core :: Disability Access APIs, defect)

Description

[Marco Zehe (:MarcoZ)]

STR:
1. Set focus on an entry anywhere, and type a few characters.
2. Quickly swipe down and up (in one motion). This will set Talkback to navigation by character.
3. Swipe right and left.

Expected: We should navigate through characters that we just typed.
Actual: Only a click is heard as if nothing is in the text field.

Comment 1

[Marco Zehe (:MarcoZ)]

Sorry for the spam, this happens if you dogfood a not quite fully accessible nightly build. ;)

Comment 2

[Marco Zehe (:MarcoZ)]

This is still an issue, and not even the implementation of editor key bindings in bug 831144 fixed it.
Since edits are native Android widgets even when they come from web pages, what we need to do, I think, is make sure all the AccessibilityNodeInfo properties pertaining to editing gestures are exposed properly:
http://developer.android.com/reference/android/view/accessibility/AccessibilityNodeInfo.html

Specifically, I believe we have to implement the movement granularities for character, word, etc.

Comment 3

[Max Li [:maxli]]

Attachment: Patch

Comment 4

[Marco Zehe (:MarcoZ)]

Comment on attachment 763262 [details] [diff] [review]
Patch

I built locally with this patch and noticed that the granularity modes "Words" and "Paragraphs" are still available, but when swiping left and right while either of these is selected, movement by character is still being executed. Can we prevent these two modes from showing up at all? And if not, could we tap into the nsIAccessibleText or other methods to actually move by words?

Comment 5

[Eitan Isaacson [:eeejay]]

Comment on attachment 763262 [details] [diff] [review]
Patch

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

I'll defer to Marco's feedback here. If we do this, we either need to remove the paragraph and word option, or support it.

Comment 6

[Max Li [:maxli]]

It appears that versions of Talkback prior to the current 3.4 beta necessarily display word and paragraph granularities (though the 3.4 beta actually checks what granularities are exposed).

Comment 7

[Max Li [:maxli]]

Attachment: Patch v2

Comment 8

[Eitan Isaacson [:eeejay]]

Comment on attachment 764303 [details] [diff] [review]
Patch v2

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

It looks like this patch works outside of just editing mode. How consistent is it with the current virtual cursor? The original scope of this bug was just to have this supported in editable elements.

Comment 9

[Max Li [:maxli]]

(In reply to Eitan Isaacson [:eeejay] from comment #8)
> Comment on attachment 764303 [details] [diff] [review]
> Patch v2
> 
> Review of attachment 764303 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> It looks like this patch works outside of just editing mode. How consistent
> is it with the current virtual cursor? The original scope of this bug was
> just to have this supported in editable elements.

It doesn't work outside of editing mode. In particular, note that I check the editState and return early if we're not in editing mode.

Comment 10

[Eitan Isaacson [:eeejay]]

(In reply to Max Li [:maxli] from comment #9)
> (In reply to Eitan Isaacson [:eeejay] from comment #8)
> > Comment on attachment 764303 [details] [diff] [review]
> > Patch v2
> > 
> > Review of attachment 764303 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > It looks like this patch works outside of just editing mode. How consistent
> > is it with the current virtual cursor? The original scope of this bug was
> > just to have this supported in editable elements.
> 
> It doesn't work outside of editing mode. In particular, note that I check
> the editState and return early if we're not in editing mode.

Ah, cool. I missed that. Review coming up.

Comment 11

[Eitan Isaacson [:eeejay]]

Comment on attachment 764303 [details] [diff] [review]
Patch v2

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

Great patch, can't even find one nit. As we discussed on IRC contenteditable is broken, there should be another bug for that. Flagging kats for java changes.

Comment 12

[Kartikaya Gupta (email:[e-mail])]

Comment on attachment 764303 [details] [diff] [review]
Patch v2

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

r+ with the import fixed.

::: mobile/android/base/GeckoAccessibility.java
@@ +10,5 @@
>  import org.mozilla.gecko.util.UiAsyncTask;
>  
>  import org.json.JSONArray;
>  import org.json.JSONObject;
> +import org.json.JSONException;

Move this import up one line so it's in alphabetical order

@@ +342,2 @@
>                                  // XXX: Self brailling gives this action with a bogus argument instead of an actual click action
> +                                if (arguments.getInt(AccessibilityNodeInfo.ACTION_ARGUMENT_MOVEMENT_GRANULARITY_INT) < 0) {

nit: Might be a good idea to use a temp variable to store the result of this arguments.getInt call because you use it again below and a temp variable would make it more readable. I'll leave this up to you

Comment 13

[Max Li [:maxli]]

Attachment: Patch with nit fixed

Carrying forward r+

Comment 14

[Marco Zehe (:MarcoZ)]

Max, you have commiter access level 3 now, so you can push this to inbound yourself. Congratulations! :)

Comment 15

[Max Li [:maxli]]

Yay.

https://hg.mozilla.org/integration/mozilla-inbound/rev/46e6080dcdd4

Comment 16

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/46e6080dcdd4