1465539 - Introduce automated accessibility testing

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: Testing, enhancement)

Description

[Eitan Isaacson [:eeejay]]

We currently have desktop mochitests that exercise our Android support. This is very implementation specific. It would be nice to do some black (grey?) box testing from the Android side that actually checks the platform support.

Comment 1

[Eitan Isaacson [:eeejay]]

Attachment: Introduce accessibility test for editable node.

Comment 2

[Eitan Isaacson [:eeejay]]

Attachment: Introduce accessibility test for editable node.

Comment 3

[Eitan Isaacson [:eeejay]]

Comment on attachment 8982044 [details] [diff] [review]
Introduce accessibility test for editable node.

This is my first try at a robocop test. I made a new base class I hope we can use for future a11y tests as well.

Comment 4

[Nick Alexander :nalexander [he/him]]

Comment on attachment 8982044 [details] [diff] [review]
Introduce accessibility test for editable node.

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

This is pretty nifty.

I don't understand the changes you made in SessionAccessibility -- get jchen to stamp those, please.  Otherwise, if it's green and stable, I have no complaints.

::: mobile/android/geckoview/src/main/java/org/mozilla/geckoview/SessionAccessibility.java
@@ +246,5 @@
>  
>      public static class Settings {
>          private static final Settings INSTANCE = new Settings();
>          private boolean mEnabled;
> +        private boolean mForceEnabled;

A little word on why you're doing this would help.  For testing only, right?

You have super powers when you're testing -- can we avoid a new flag and just set some private thing that you wouldn't otherwise be able to set?

::: mobile/android/tests/browser/robocop/src/org/mozilla/gecko/tests/AccessibilityTest.java
@@ +82,5 @@
> +    }
> +
> +    private void blockForReady() {
> +        final EventExpecter eventExpecter = mActions.expectGlobalEvent(
> +                                                Actions.EventType.UI, "GeckoView:AccessibilityReady");

nit: indentation looks off here.

@@ +92,5 @@
> +        eventExpecter.unregisterListener();
> +    }
> +
> +    protected int getChildId(AccessibilityNodeInfo node, int index) {
> +        long childId = 0;

Is all of this because you're invoking 18+ methods but compiling for 16+?  There might be better ways to do this, like only compiling this test for API 18+.  Although... that's tricky with the current Robocop setup.

In any case, this needs a little comment (perhaps in the class comment) to explain what assumptions and requirements you're working with.

::: mobile/android/tests/browser/robocop/src/org/mozilla/gecko/tests/testAccessibilityEditable.java
@@ +1,1 @@
> +package org.mozilla.gecko.tests;

nit: license header.  (Public domain, IIRC.)

@@ +12,5 @@
> +import android.view.accessibility.AccessibilityNodeInfo;
> +import android.view.accessibility.AccessibilityNodeProvider;
> +
> +/**
> + * This tests ensures that the toolbar in reader mode displays the original page url.

nit: this test ensures (no s).

Comment 5

[Eitan Isaacson [:eeejay]]

(In reply to Nick Alexander :nalexander from comment #4)
> Comment on attachment 8982044 [details] [diff] [review]
> Introduce accessibility test for editable node.
> 
> Review of attachment 8982044 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> This is pretty nifty.
> 
> I don't understand the changes you made in SessionAccessibility -- get jchen
> to stamp those, please.  Otherwise, if it's green and stable, I have no
> complaints.

I'll flag him for review next. Thanks!

> 
> :::
> mobile/android/geckoview/src/main/java/org/mozilla/geckoview/
> SessionAccessibility.java
> @@ +246,5 @@
> >  
> >      public static class Settings {
> >          private static final Settings INSTANCE = new Settings();
> >          private boolean mEnabled;
> > +        private boolean mForceEnabled;
> 
> A little word on why you're doing this would help.  For testing only, right?
> 
> You have super powers when you're testing -- can we avoid a new flag and
> just set some private thing that you wouldn't otherwise be able to set?

You would think our powers were super. But apparently not super enough. You can't automate turning on accessibility by design. That kind of sucks!

> 
> @@ +92,5 @@
> > +        eventExpecter.unregisterListener();
> > +    }
> > +
> > +    protected int getChildId(AccessibilityNodeInfo node, int index) {
> > +        long childId = 0;
> 
> Is all of this because you're invoking 18+ methods but compiling for 16+?

No, we are doing this because the methods are annotated as hidden in the android source code, but they are still available via introspection. Chrome does a similar trick in it's test framework.

> There might be better ways to do this, like only compiling this test for API
> 18+.  Although... that's tricky with the current Robocop setup.

Yeah, we need this working in 16, hence the nested try/catches for different revisions of this API.

Comment 6

[Eitan Isaacson [:eeejay]]

Attachment: Introduce accessibility test for editable node. r?jchen

Comment 7

[(inactive) Jim Chen [:jchen] [:darchons]]

Comment on attachment 8983095 [details] [diff] [review]
Introduce accessibility test for editable node. r?jchen

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

The future of Robocop is uncertain. Can you look into converting this to a GeckoView JUnit test? These are the tests that live under [1].

[1] https://searchfox.org/mozilla-central/source/mobile/android/geckoview/src/androidTest/java/org/mozilla/geckoview/test

Comment 8

[Eitan Isaacson [:eeejay]]

Attachment: Introduce accessibility test for editable node. r?jchen

Created a junit test instead.

Comment 9

[(inactive) Jim Chen [:jchen] [:darchons]]

Comment on attachment 8985729 [details] [diff] [review]
Introduce accessibility test for editable node. r?jchen

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

::: mobile/android/geckoview/src/androidTest/java/org/mozilla/geckoview/test/AccessibilityTest.kt
@@ +18,5 @@
> +import android.view.accessibility.AccessibilityRecord
> +import android.view.View
> +import android.view.ViewGroup
> +
> +import android.util.SparseLongArray

Not used.

@@ +27,5 @@
> +import org.junit.Before
> +import org.junit.After
> +import org.junit.runner.RunWith
> +
> +import java.lang.reflect.Method

Not used.

@@ +41,5 @@
> +        // We initialize a view with a parent and grandparent so that the
> +        // accessibility events propogate up at least to the parent.
> +        FrameLayout(InstrumentationRegistry.getTargetContext()).addView(view)
> +        FrameLayout(InstrumentationRegistry.getTargetContext()).addView(view.getParent() as View)
> +    }

Should initialize `view`, `provider`, and the layout in `setup()`

@@ +67,5 @@
> +    }
> +
> +    private interface EventDelegate {
> +        fun onAccessibilityFocused(event: AccessibilityEvent)
> +        fun onFocused(event: AccessibilityEvent)

Add default implementations so you don't need to override each method everywhere, e.g. "fun onAccessibilityFocused(event: AccessibilityEvent) {}" and "fun onFocused(event: AccessibilityEvent) {}"

@@ +74,5 @@
> +    @Before fun setup() {
> +        // Force on accessibility and assign the session's accessibility
> +        // object a view.
> +        SessionAccessibility.Settings.setForceEnabled(true)
> +        sessionRule.session.getAccessibility().setView(view)

`mainSession.accessibility.view = view`

@@ +79,5 @@
> +
> +        // Set up an external delegate that will intercept accessibility events.
> +        sessionRule.addExternalDelegateUntilTestEnd(
> +            EventDelegate::class,
> +        { newDelegate -> (view.getParent() as View).setAccessibilityDelegate(object : View.AccessibilityDelegate() {

`(view.parent as View)`

@@ +90,5 @@
> +                return false
> +            }
> +        }) },
> +        { (view.getParent() as View).setAccessibilityDelegate(null) },
> +        object : EventDelegate {

`object : EventDelegate {}`

@@ +105,5 @@
> +    @Test fun testRootNode() {
> +        assertThat("provider is not null", provider, notNullValue())
> +        var node = provider.createAccessibilityNodeInfo(AccessibilityNodeProvider.HOST_VIEW_ID)
> +        assertThat("Root node should have WebView class name",
> +            node.getClassName() as String, equalTo("android.webkit.WebView"))

`node.getClassName().toString()`

@@ +114,5 @@
> +
> +        sessionRule.waitUntilCalled(object : EventDelegate {
> +            @AssertCalled(count = 1)
> +            override fun onFocused(event: AccessibilityEvent) { }
> +            override fun onAccessibilityFocused(event: AccessibilityEvent) { }

Can remove `onAccessibilityFocused`.

@@ +127,5 @@
> +        provider.performAction(AccessibilityNodeProvider.HOST_VIEW_ID,
> +            AccessibilityNodeInfo.ACTION_ACCESSIBILITY_FOCUS, null)
> +
> +        sessionRule.waitUntilCalled(object : EventDelegate {
> +            override fun onFocused(event: AccessibilityEvent) {

I think `onFocused` needs an `@AssertCalled`? Usually you need `@AssertCalled` with each overridden method.

@@ +140,5 @@
> +        provider.performAction(nodeId,
> +            AccessibilityNodeInfo.ACTION_NEXT_HTML_ELEMENT, null)
> +
> +        sessionRule.waitUntilCalled(object : EventDelegate {
> +            override fun onFocused(event: AccessibilityEvent) { }

Can remove `onFocused`.

::: mobile/android/geckoview/src/androidTest/java/org/mozilla/geckoview/test/BaseSessionTest.kt
@@ +24,5 @@
>   * providing the test rule and other utilities.
>   */
>  open class BaseSessionTest(noErrorCollector: Boolean = false) {
>      companion object {
> +        const val ACCESSIBILITY_PATH = "/assets/www/accessibility.html"

I feel like we can reuse `SELECTION_ACTION_PATH`? Maybe rename that to something more generic.

::: mobile/android/geckoview/src/main/java/org/mozilla/geckoview/SessionAccessibility.java
@@ +150,5 @@
>                      }
>  
>                      @Override
>                      public boolean performAction(int virtualViewId, int action, Bundle arguments) {
> +                        Log.i(LOGTAG, "performAction " + virtualViewId);

Remove or move inside `if (DEBUG)`

@@ +247,5 @@
>  
>          });
>      }
>  
>      public static class Settings {

`Settings` is not really a user-facing class so it should be made private.

@@ +302,5 @@
> +         * This is for testing only. Forces our accessibility on.
> +         * This is necessary because Android does not allow accessibility
> +         * services to start without explicit user interaction.
> +         */
> +        public static void setForceEnabled(final boolean forceEnabled) {

We usually prefer a pref instead of a test-only method. Can you set a pref inside the tests, and use the "GeckoView:AccessibilityReady" event to notify Java of the pref value?

@@ +309,4 @@
>          }
>  
>          private void updateAccessibilitySettings() {
>              ThreadUtils.postToBackgroundThread(new Runnable() {

While you're at it, can you remove this `ThreadUtils.postToBackgroundThread` call and just update settings directly?

Comment 10

[Eitan Isaacson [:eeejay]]

Attachment: Introduce accessibility test for editable node. r?jchen

OK! I think I addressed all the issues in here. I switched back to not have a new HTML file. I'm a bit concerned about the selection test one has an onload() that mutates the tree and may mess with focus. This adds some a11y event noise.

Comment 11

[(inactive) Jim Chen [:jchen] [:darchons]]

Comment on attachment 8986576 [details] [diff] [review]
Introduce accessibility test for editable node. r?jchen

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

Looks good!

::: mobile/android/geckoview/src/androidTest/assets/www/inputs.html
@@ +1,2 @@
>  <html>
> +    <head><title>inputs</title></head>

Nit: capitalize

::: mobile/android/geckoview/src/androidTest/java/org/mozilla/geckoview/test/AccessibilityTest.kt
@@ +71,5 @@
> +
> +        // Force on accessibility and assign the session's accessibility
> +        // object a view.
> +        mainSession.accessibility.view = view
> +        sessionRule.setPrefsUntilTestEnd(mapOf("accessibility.force_disabled" to -1))

I think it's best to set the pref _before_ setting the view.

::: mobile/android/geckoview/src/main/java/org/mozilla/geckoview/SessionAccessibility.java
@@ +255,2 @@
>          private boolean mEnabled;
> +        private boolean mForceEnabled;

/* package */ for `mForceEnabled`. And the two booleans should now be `volatile` technically.

@@ +266,3 @@
>                  }
> +            };
> +            PrefsHelper.addObserver(new String[]{ FORCE_ACCESSIBILITY_PREF }, prefHandler);

I think this should be at the end of the method, to avoid a small race condition.

Comment 12

[Eitan Isaacson [:eeejay]]

Attachment: Introduce accessibility test for editable node. r=jchen

Fixed nits. One last try before landing:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=31eec2c0d3554d6f52957ad96b1cdb756355576b

Comment 13

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/b264a83444dd
Introduce accessibility test for editable node. r=jchen

Comment 14

[Andreea Pavel [:apavel]]

https://hg.mozilla.org/mozilla-central/rev/b264a83444dd