Closed Bug 1042257 Opened 6 years ago Closed 6 years ago

[geckoview] Integrate GeckoAccessibility into GeckoView

Categories

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

All
Android
defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED
mozilla34

People

(Reporter: fredw, Assigned: fredw)

References

Details

Attachments

(2 files, 3 obsolete files)

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...
Attached patch Patch (untested) (obsolete) — Splinter Review
A basic patch for the "Accessibility:Event" and GeckoAccessibility.sendAccessibilityEvent call.

Not sure how to handle the events to call GeckoAccessibility.updateAccessibilitySettings.
Summary: Accessibility in GeckoView → [geckoview] Integrate GeckoAccessibility into GeckoView
Attached patch bug-1042257.patch (obsolete) — Splinter Review
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?
Assignee: nobody → fred.wang
Status: NEW → ASSIGNED
Attachment #8461052 - Flags: feedback?(mark.finkle)
Attachment #8460429 - Attachment is obsolete: true
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.
Flags: needinfo?(eitan)
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.
Attachment #8461052 - Flags: feedback?(mark.finkle)
Attachment #8461052 - Flags: feedback?(eitan)
Attachment #8461052 - Flags: feedback+
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.
Attachment #8461052 - Flags: feedback?(eitan) → feedback+
(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).
(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.
Flags: needinfo?(eitan)
(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
Attached patch Patch (obsolete) — Splinter Review
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...
Attachment #8461052 - Attachment is obsolete: true
Attachment #8464802 - Flags: review?(mark.finkle)
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?
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.
Great thanks! Yes I believe this can be dealt with in a followup bug.
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.
Attachment #8464802 - Flags: review?(mark.finkle) → review+
Flags: needinfo?(nalexander)
Blocks: 1050666
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.
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.
Note: Bug 1052841 will likely affect this work
(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.
Attachment #8473685 - Attachment description: P → Part 2: Listen when the user enables/disables accessibility
Attachment #8473684 - Flags: review?(mark.finkle)
Attachment #8473685 - Flags: review?(mark.finkle)
Attachment #8473684 - Flags: review?(mark.finkle) → review+
Attachment #8473685 - Flags: review?(mark.finkle) → review+
https://hg.mozilla.org/mozilla-central/rev/c8f6ffdeb9f2
https://hg.mozilla.org/mozilla-central/rev/f6635374c98c
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla34
Blocks: 1059427
Blocks: 1163374
Product: Core → Core Graveyard
You need to log in before you can comment on or make changes to this bug.