1042257 - [geckoview] Integrate GeckoAccessibility into GeckoView

Status: RESOLVED FIXED

(Core Graveyard :: Embedding: GRE Core, defect)

Description

[Frédéric Wang (:fredw)]

As I said on IRC, I tried to use Google Talkback on a tablet yesterday. This works well in Fennec but not in a basic Android App with an embedded GeckoView. Nick mentioned the GeckoAccessibility class that seems to be used for that purpose. A quick find -type f | xargs grep GeckoAccessibility shows that the class is used in

1) gfx/LayerView.java, from which GeckoView derives.
2) GeckoApp.java, in the onResume function and the two handleMessage handlers.

I don't know about the Android code, but it seems that the events in 2) should be handled by GeckoView somehow...

Comment 1

[Frédéric Wang (:fredw)]

Attachment: Patch (untested)

A basic patch for the "Accessibility:Event" and GeckoAccessibility.sendAccessibilityEvent call.

Not sure how to handle the events to call GeckoAccessibility.updateAccessibilitySettings.

Comment 2

[Frédéric Wang (:fredw)]

Attachment: bug-1042257.patch

So here is the patch I wrote this morning and it seems to work, modulo two issues:

- I was not able to find how to call the function to update accessibility settings during onResume. So indeed if you enable/disable TalkBack and go back to the GeckoView-based application again, the modification is not taken into account. However, it does work if I do that in the onResume function of the main Activity of my application. (note: there are currently probably similar issues like e.g. the orientation portrait/landscape change, which does not seem to be updated automatically by the GeckoView-based application after an onResume).

- The GeckoApp also has some stuff to disable a "dynamic toolbar". I'm not sure if we want to handle that in the GeckoView case too. Perhaps with some kind of callback mechanism?

How can I write tests for this?

Comment 3

[Marco Zehe (:MarcoZ)]

Comment on attachment 8461052 [details] [diff] [review]
bug-1042257.patch

Thanks for doing this!

The dynamic toolbar stuff is definitely Fennec-specific, so I'd say this change is very appropriate.

NI'ing Eitan in case he has any input as well re the receiving of accessibility change notifications.

Comment 4

[Mark Finkle (:mfinkle) (use needinfo?)]

Comment on attachment 8461052 [details] [diff] [review]
bug-1042257.patch

Overall, I think this approach looks good. Eitan should also take a look at this too.

Comment 5

[Eitan Isaacson [:eeejay]]

Comment on attachment 8461052 [details] [diff] [review]
bug-1042257.patch

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

This looks right. Did you test and actually see that this works with TalkBack? I am surprised that our JS code doesn't need any tweaking.

Comment 6

[Frédéric Wang (:fredw)]

(In reply to Eitan Isaacson [:eeejay] from comment #5)
> This looks right. Did you test and actually see that this works with
> TalkBack? I am surprised that our JS code doesn't need any tweaking.

Yes, I tested it with TalkBack and (with my limited knowledge of the accessibility feature) that seems to work as expected. However, as I said in comment 2, I don't know where to place the code that was in GeckoApp's onResume, so the settings is not updated correctly after a resume (unless I copy the code in the onResume function of my GeckoView app's MainActivity).

Comment 7

[Eitan Isaacson [:eeejay]]

(In reply to Frédéric Wang (:fredw) from comment #6)
> (In reply to Eitan Isaacson [:eeejay] from comment #5)
> > This looks right. Did you test and actually see that this works with
> > TalkBack? I am surprised that our JS code doesn't need any tweaking.
> 
> Yes, I tested it with TalkBack and (with my limited knowledge of the
> accessibility feature) that seems to work as expected. However, as I said in
> comment 2, I don't know where to place the code that was in GeckoApp's
> onResume, so the settings is not updated correctly after a resume (unless I
> copy the code in the onResume function of my GeckoView app's MainActivity).

It has been a long time since I touched android stuff, so I have no solution for onResume. To me it seems like an acceptable bug (that should be opened separately).

To be precise: Needing to restart the app after accesssibility is toggled is a bug that could be fixed later.

Comment 8

[Mark Finkle (:mfinkle) (use needinfo?)]

(In reply to Frédéric Wang (:fredw) from comment #6)
> (In reply to Eitan Isaacson [:eeejay] from comment #5)
> > This looks right. Did you test and actually see that this works with
> > TalkBack? I am surprised that our JS code doesn't need any tweaking.
> 
> Yes, I tested it with TalkBack and (with my limited knowledge of the
> accessibility feature) that seems to work as expected. However, as I said in
> comment 2, I don't know where to place the code that was in GeckoApp's
> onResume, so the settings is not updated correctly after a resume (unless I
> copy the code in the onResume function of my GeckoView app's MainActivity).

Maybe we could try using GeckoView.onAttachedToWindow for this? I see that the native WebView does something similar. Maybe it would work for us too:
http://androidxref.com/4.1.1/xref/frameworks/base/core/java/android/webkit/WebViewClassic.java#5307

Comment 9

[Frédéric Wang (:fredw)]

Attachment: Patch

onAttachedToWindow didn't work for me, so instead I've used the addAccessibilityStateChangeListener function which seems more reliable to track changes of the accessibility setting.

I still have a problem when switching between landscape/portrait orientation: the two accessibility cursors for TalkBack & Gecko (i.e. the frames around the selected element) become out-of-sync (so the text read does not correspond to the element you select). I also see this mismatch of frame positions in Fennec, but this is corrected immediately after the orientation change. I guess this can be addressed in a follow-up bug...

Comment 10

[Marco Zehe (:MarcoZ)]

Frédéric, is this mismatch purely visual, or does the wrong item also get activated when double-tapping? E. g., you touch or swipe to a link, and then double-tap anywhere on the screen to activate. Does the link TalkBack has spoken get activated, or whatever is in the visual rectangle?

Comment 11

[Frédéric Wang (:fredw)]

I see the (visual) frame mismatch for both Fennec and a GeckoView-based app, when I change the screen orientation. On Fennec, when I touch or swipe to select another element the two cursors are then immediately synchronized again and Tallback reads the new element selected. On a GeckoView-based app, that does not happen and Tallback keeps reading the wrong element. So with the current patch, everything is ok as long as you don't change the screen orientation of your GeckoView-based app...

When I tried last week, I think I had a purely visual issue with screen orientation changes (the GeckoView orientation could become different to the GeckoView-based app), so I was assuming it was not something specific to accessibility (I was not able to reproduce this today). GeckoApp.java seems to take care of updating the orientation in several places, so probably some stuff should be copied into GeckoView too. However, my quick attempt this morning didn't help fixing the sync bug.

Comment 12

[Marco Zehe (:MarcoZ)]

Great thanks! Yes I believe this can be dealt with in a followup bug.

Comment 13

[Mark Finkle (:mfinkle) (use needinfo?)]

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

Looks OK to me. I won't be around to help land this, in case you need some help. So I am NI'ing Nick to keep an eye on it.

Comment 14

[Frédéric Wang (:fredw)]

I'm not sure I understand the error, but the try server returns:

00:47:32     INFO -  08-08 00:41:12.375 I/dalvikvm(  419): Failed resolving Lorg/mozilla/gecko/GeckoView; interface 1045 'Landroid/view/accessibility/AccessibilityManager$AccessibilityStateChangeListener;'
00:47:32     INFO -  08-08 00:41:12.375 W/dalvikvm(  419): Link of class 'Lorg/mozilla/gecko/GeckoView;' failed

So it looks like AccessibilityStateChangeListener is not found. It was added in API level 14, what does the test bots use? If we can't use that, we'll probably need to go back to attachment 8461052 [details] [diff] [review] instead.

Comment 15

[Marco Zehe (:MarcoZ)]

That's strange indeed, we conditionally use stuff from API Level 14, and even stuff exclusive to API level 16 and above, too. And it never gave us problems.

Comment 16

[Mark Finkle (:mfinkle) (use needinfo?)]

Note: Bug 1052841 will likely affect this work

Comment 17

[Frédéric Wang (:fredw)]

Attachment: Part 1: Integrate GeckoAccessibility into GeckoView

Comment 18

[Frédéric Wang (:fredw)]

Attachment: Part 2: Listen when the user enables/disables accessibility

(In reply to Marco Zehe (:MarcoZ) from comment #15)
> That's strange indeed, we conditionally use stuff from API Level 14, and
> even stuff exclusive to API level 16 and above, too. And it never gave us
> problems.

Indeed, that's because my previous patch didn't use that stuff "conditionally". This one seems to make the Try bots happier:

https://tbpl.mozilla.org/?tree=Try&rev=5f14c528ffc3

Right now, I don't have an Android device to test if the patches still work as expected, so I'll come back to that bug next week.

Comment 19

[Frédéric Wang (:fredw)]

https://tbpl.mozilla.org/?tree=Try&rev=5f14c528ffc3

Comment 20

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/c8f6ffdeb9f2
https://hg.mozilla.org/integration/mozilla-inbound/rev/f6635374c98c

Comment 21

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/c8f6ffdeb9f2
https://hg.mozilla.org/mozilla-central/rev/f6635374c98c