1449364 - Port GeckoAccessibility to GeckoView/GeckoSession

Status: RESOLVED FIXED

(GeckoView :: General, defect, P1)

Description

[Eitan Isaacson [:eeejay]]

Right now GeckoAccessibility is heavily dependent on Fennec. It should be encapsulated in GeckoView and just work when embedded in any UI.

For this bug I will port the Java side of AccessFu to GeckoView with minimal js changes. Currently AccessFu doesn't work well with more than one xul <window>, i'll follow up on that in bug 751769.

Comment 1

[Chris Peterson [:cpeterson]]

P1 because this is a blocker for shipping Klar+GeckoView.

Comment 2

[Eitan Isaacson [:eeejay]]

Attachment: Move GeckoAccessibility to GeckoView and refactor to be per-session. r?snorp

The goal here was to encapsulate the a11y bits so that embedding projects don't have to worry about it like Fennec does now.

Biggest change is that GeckoAccessibility is instantiated and associated to a GeckoSession. There is work to do in the javascript side to have it work in multiple sessions/windows, but from the java perspective, i think this is the relationship we want.

I also found out that the braille output we get is not from the self-brailling code we put in a few years back, but just general a11y API. This means we could remove those dead paths and the third-party library. It also means we will need to revisit this along with text input next.

Comment 3

[James Willcox (:snorp) ([e-mail]) (he/him)]

Comment on attachment 8963750 [details] [diff] [review]
Move GeckoAccessibility to GeckoView and refactor to be per-session. r?snorp

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

Thanks for the patch, Eitan!

I'm going on PTO here in a bit, and didn't have time to fully review this, so I'm going to move this over to jchen.

::: mobile/android/geckoview/src/main/java/org/mozilla/gecko/GeckoAccessibility.java
@@ +60,5 @@
> +            }
> +        }, "Accessibility:Event", null);
> +    }
> +
> +    public void setView(final GeckoView view) {

The fact that the GeckoSession holds an instance of this and we can then hold a GeckoView raises some red flags for me. I think most of this class probably needs to just hang off of GeckoView. It can then talk to whatever GeckoSession is attached to that GeckoView using the EventDispatcher as it does above.

::: mobile/android/geckoview/src/main/java/org/mozilla/geckoview/GeckoSession.java
@@ +794,5 @@
> +     * Get the GeckoAccessibility instance for this session.
> +     *
> +     * @return GeckoAccessibility instance.
> +     */
> +    public @NonNull GeckoAccessibility getAccessibilityDelegate() {

This doesn't seem like a delegate to me (at least not in the way it's understood here in GeckoSession).

Comment 4

[Eitan Isaacson [:eeejay]]

(In reply to James Willcox (:snorp) ([e-mail]) (PTO until April 9) from comment #3)
> Comment on attachment 8963750 [details] [diff] [review]
> Move GeckoAccessibility to GeckoView and refactor to be per-session. r?snorp
> 
> Review of attachment 8963750 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Thanks for the patch, Eitan!
> 
> I'm going on PTO here in a bit, and didn't have time to fully review this,
> so I'm going to move this over to jchen.
> 
> :::
> mobile/android/geckoview/src/main/java/org/mozilla/gecko/GeckoAccessibility.
> java
> @@ +60,5 @@
> > +            }
> > +        }, "Accessibility:Event", null);
> > +    }
> > +
> > +    public void setView(final GeckoView view) {
> 
> The fact that the GeckoSession holds an instance of this and we can then
> hold a GeckoView raises some red flags for me. I think most of this class
> probably needs to just hang off of GeckoView. It can then talk to whatever
> GeckoSession is attached to that GeckoView using the EventDispatcher as it
> does above.

I went back and fourth on this. I ultimately took inspiration from how GeckoSession holds a ref to a TextInputController that then also gets a view set to it.

I had it set up how you suggested earlier, I'll give it another shot and post a patch. The downside is that the accessibility bits are more session related (eg. the stored accessible info from the last dispatched event), but it's not a show stopper.

> 
> :::
> mobile/android/geckoview/src/main/java/org/mozilla/geckoview/GeckoSession.
> java
> @@ +794,5 @@
> > +     * Get the GeckoAccessibility instance for this session.
> > +     *
> > +     * @return GeckoAccessibility instance.
> > +     */
> > +    public @NonNull GeckoAccessibility getAccessibilityDelegate() {
> 
> This doesn't seem like a delegate to me (at least not in the way it's
> understood here in GeckoSession).

Ha. I just realized what that means in GeckoSession. I was using that term as the View's AccessibilityDelegate as it's used in the Android a11y API. I'll change that.

Comment 5

[Eitan Isaacson [:eeejay]]

Attachment: Move GeckoAccessibility to GeckoView and refactor to be per-session. r?jchen

In this revision we only store the view and not the session.

Comment 6

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

Comment on attachment 8964621 [details] [diff] [review]
Move GeckoAccessibility to GeckoView and refactor to be per-session. r?jchen

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

::: mobile/android/geckoview/src/main/java/org/mozilla/gecko/GeckoAccessibility.java
@@ +44,5 @@
> +    // Used to store the JSON message and populate the event later in the code path.
> +    private AccessibilityNodeInfo mVirtualContentNode;
> +    private AccessibilityNodeProvider mAccessibilityNodeProvider;
> +
> +    public GeckoAccessibility(final GeckoView view) {

The goal of GeckoSession is for it to work with any View, not just GeckoView, so GeckoAccessibility should accept (GeckoSession, View) rather than (GeckoView) as arguments. Looking at how `mView` is used, I think that should be fairly straightforward.

An alternative that I prefer more is for a sub-session object to have a `setView` method that tells GeckoSession which View to use [1]. We can probably adopt a similar API for accessibility. For example, a consumer would call,

> session.getAccessibility().setView(view);

to set the current accessibility View for a particular session. And when using GeckoView, GeckoView would call `setView` for you.

[1] https://searchfox.org/mozilla-central/rev/a0665934fa05158a5a943d4c8b277465910c029c/mobile/android/geckoview/src/main/java/org/mozilla/geckoview/SessionTextInput.java#178

@@ +71,5 @@
> +        private static final Settings INSTANCE = new Settings();
> +        private boolean mEnabled;
> +
> +        public Settings() {
> +            EventDispatcher.getInstance().registerUiThreadListener(this, "Accessibility:Ready", null);

You should unregister this in the event listener

@@ +115,5 @@
> +            ret.putBoolean("enabled", mEnabled);
> +            // "Accessibility:Settings" is dispatched to the Gecko thread.
> +            EventDispatcher.getInstance().dispatch("Accessibility:Settings", ret);
> +            // "Accessibility:Enabled" is dispatched to the UI thread.
> +            EventDispatcher.getInstance().dispatch("Accessibility:Enabled", ret);

This is only used in Fennec. We probably want to sort this out in a follow-up.

::: mobile/android/geckoview/src/main/java/org/mozilla/geckoview/GeckoView.java
@@ +160,5 @@
>  
>      private void init() {
>          setFocusable(true);
>          setFocusableInTouchMode(true);
> +        setImportantForAccessibility(View.IMPORTANT_FOR_ACCESSIBILITY_YES);

Ideally this call would go inside GeckoAccessibility, because we want this to work with any View, not just GeckoView.

@@ +201,5 @@
>              return null;
>          }
>  
> +        mGeckoAccessibility.unregisterSessionListener();
> +        setAccessibilityDelegate(null);

Ideally these calls would go inside GeckoAccessibility.

@@ +223,5 @@
>            return;
>          }
>  
> +        mGeckoAccessibility.registerSessionListener();
> +        setAccessibilityDelegate(mGeckoAccessibility);

Ideally these calls would go inside GeckoAccessibility.

::: mobile/android/modules/geckoview/GeckoViewAccessibility.jsm
@@ +15,5 @@
> +                       {}).AndroidLog.d.bind(null, "GeckoAccessibility"));
> +
> +class GeckoViewAccessibility extends GeckoViewModule {
> +  init() {
> +    AccessFu.attach(this.window);

Maybe only attach when accessibility is being used (which may not be the case when using GeckoSession with a View other than GeckoView).

Comment 7

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

(In reply to Jim Chen [:jchen] [:darchons] from comment #6)
> Comment on attachment 8964621 [details] [diff] [review]
> Move GeckoAccessibility to GeckoView and refactor to be per-session. r?jchen
> 
> > session.getAccessibility().setView(view);

I see that's what you did for the first version of the patch. I think the main objection there was `setView` accepting a `GeckoView` rather than a `View`, not the API itself, since like you said, we already do something similar for `session.getTextInput().setView(view)`.

Comment 8

[Eitan Isaacson [:eeejay]]

Attachment: Move GeckoAccessibility to GeckoView and refactor to be per-session. r?jchen

Comment 9

[Eitan Isaacson [:eeejay]]

Attachment: Move GeckoAccessibility to GeckoView and refactor to be per-session. r?jchen

Comment 10

[Eitan Isaacson [:eeejay]]

Attachment: Create AccessibilityBridge for GeckoSession. r?jchen

Sorry for the spam. Renamed the class to AccessibilityBridge, but kept GeckoAccessibility in the LOGTAG

Comment 11

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

Comment on attachment 8965451 [details] [diff] [review]
Create AccessibilityBridge for GeckoSession. r?jchen

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

Pretty close! Just some smaller changes to make.

::: mobile/android/geckoview/src/main/java/org/mozilla/gecko/AccessibilityBridge.java
@@ +2,5 @@
> + * This Source Code Form is subject to the terms of the Mozilla Public
> + * License, v. 2.0. If a copy of the MPL was not distributed with this
> + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
> +
> +package org.mozilla.gecko;

Should be in org.mozilla.geckoview

@@ +26,5 @@
> +import android.view.accessibility.AccessibilityManager;
> +import android.view.accessibility.AccessibilityNodeInfo;
> +import android.view.accessibility.AccessibilityNodeProvider;
> +
> +public class AccessibilityBridge extends View.AccessibilityDelegate implements BundleEventListener {

`public final class SessionAccessibility` to match other classes. Also, `View.AccessibilityDelegate` and `BundleEventListener` should be implemented by an inner class; otherwise their methods will end up in the public API.

@@ +50,5 @@
> +        mSession = session;
> +
> +        Settings.getInstance().dispatch();
> +
> +        session.getEventDispatcher().registerUiThreadListener(this, "Accessibility:Event", null);

GeckoView events all have the GeckoView prefix, e.g. "GeckoView:Accessibility"

@@ +53,5 @@
> +
> +        session.getEventDispatcher().registerUiThreadListener(this, "Accessibility:Event", null);
> +    }
> +
> +    public void setView(final View view) {

Add a `getView` method

@@ +74,5 @@
> +        private static final Settings INSTANCE = new Settings();
> +        private boolean mEnabled;
> +
> +        public Settings() {
> +            EventDispatcher.getInstance().registerUiThreadListener(this, "Accessibility:Ready", null);

Same here, e.g. "GeckoView:AccessibilityReady"

@@ +119,5 @@
> +            // "Accessibility:Settings" is dispatched to the Gecko thread.
> +            EventDispatcher.getInstance().dispatch("Accessibility:Settings", ret);
> +            // "Accessibility:Enabled" is dispatched to the UI thread.
> +            EventDispatcher.getInstance().dispatch("Accessibility:Enabled", ret);
> +      }

Indent

@@ +147,5 @@
> +
> +        return event;
> +    }
> +
> +    private static void populateEventFromJSON (AccessibilityEvent event, final GeckoBundle message) {

Remove space before `(`

@@ +172,5 @@
> +        event.setMaxScrollX(message.getInt("maxScrollX", -1));
> +        event.setMaxScrollY(message.getInt("maxScrollY", -1));
> +    }
> +
> +    private void populateNodeInfoFromJSON (AccessibilityNodeInfo node, final GeckoBundle message) {

Remove space before `(`

@@ +321,5 @@
> +                case AccessibilityNodeInfo.ACTION_ACCESSIBILITY_FOCUS:
> +                case AccessibilityNodeInfo.ACTION_CLEAR_ACCESSIBILITY_FOCUS:
> +                    final GeckoBundle data = new GeckoBundle(1);
> +                    data.putBoolean("gainFocus", action == AccessibilityNodeInfo.ACTION_ACCESSIBILITY_FOCUS);
> +                    EventDispatcher.getInstance().dispatch("Accessibility:Focus", data);

Event name. Also I feel like this should be a per-GeckoSession event rather than a global event.

@@ +345,5 @@
> +                case AccessibilityNodeInfo.ACTION_SCROLL_FORWARD:
> +                    EventDispatcher.getInstance().dispatch("Accessibility:ScrollForward", null);
> +                    return true;
> +                case AccessibilityNodeInfo.ACTION_SCROLL_BACKWARD:
> +                    EventDispatcher.getInstance().dispatch("Accessibility:ScrollBackward", null);

Same with these events.

@@ +359,5 @@
> +                    } else {
> +                        data = null;
> +                    }
> +                    EventDispatcher.getInstance().dispatch(action == AccessibilityNodeInfo.ACTION_NEXT_HTML_ELEMENT ?
> +                                                           "Accessibility:NextObject" : "Accessibility:PreviousObject", data);

Same with these events.

@@ +380,5 @@
> +                    } else if (granularity > 0) {
> +                        data = new GeckoBundle(2);
> +                        data.putString("direction", action == AccessibilityNodeInfo.ACTION_NEXT_AT_MOVEMENT_GRANULARITY ? "Next" : "Previous");
> +                        data.putInt("granularity", granularity);
> +                        EventDispatcher.getInstance().dispatch("Accessibility:MoveByGranularity", data);

Same with these events.

::: mobile/android/geckoview/src/main/java/org/mozilla/geckoview/GeckoSession.java
@@ +92,5 @@
>          new EventDispatcher(mNativeQueue);
>  
>      private final SessionTextInput mTextInput = new SessionTextInput(this, mNativeQueue);
>  
> +    private final AccessibilityBridge mAccessibilityBridge = new AccessibilityBridge(this);

Create lazily.

@@ +860,5 @@
> +      * Get the AccessibilityBridge instance for this session.
> +      *
> +      * @return AccessibilityBridge instance.
> +      */
> +    public @NonNull AccessibilityBridge getAccessibilityBridge() {

Name it `getAccessibility()`

::: mobile/android/modules/geckoview/GeckoViewAccessibility.jsm
@@ +6,5 @@
> +
> +var EXPORTED_SYMBOLS = ["GeckoViewAccessibility"];
> +
> +ChromeUtils.import("resource://gre/modules/GeckoViewModule.jsm");
> +ChromeUtils.import("resource://gre/modules/accessibility/AccessFu.jsm");

Lazy import

@@ +11,5 @@
> +ChromeUtils.import("resource://gre/modules/XPCOMUtils.jsm");
> +
> +XPCOMUtils.defineLazyGetter(this, "dump", () =>
> +    ChromeUtils.import("resource://gre/modules/AndroidLog.jsm",
> +                       {}).AndroidLog.d.bind(null, "AccessibilityBridge"));

"GeckoAccessibility"

@@ +15,5 @@
> +                       {}).AndroidLog.d.bind(null, "AccessibilityBridge"));
> +
> +class GeckoViewAccessibility extends GeckoViewModule {
> +  init() {
> +    AccessFu.attach(this.window);

Can we only attach if `setView` is called? That way we only attach if accessibility is being used.

Comment 12

[Eitan Isaacson [:eeejay]]

Attachment: Create SessionAccessibility for GeckoSession. r?jchen

Comment 13

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

Comment on attachment 8966405 [details] [diff] [review]
Create SessionAccessibility for GeckoSession. r?jchen

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

LGTM with nits.

Have you tested this under e10s?

And I assume multiple GeckoView support will be added later?

::: accessible/jsat/AccessFu.jsm
@@ +108,5 @@
>      Output.start();
>      PointerAdapter.start();
>  
>      if (Utils.MozBuildApp === "mobile/android") {
> +      Utils.win.WindowEventDispatcher.registerListener(this, [

Ideally you would use GeckoViewUtils.getDispatcherForWindow() because that supports e10s.

::: mobile/android/geckoview/src/main/java/org/mozilla/geckoview/GeckoSession.java
@@ +15,5 @@
>  
>  import org.mozilla.gecko.annotation.WrapForJNI;
>  import org.mozilla.gecko.EventDispatcher;
>  import org.mozilla.gecko.gfx.LayerSession;
> +import org.mozilla.gecko.SessionAccessibility;

Shouldn't need this after you move it to .geckoview package.

@@ +862,5 @@
> +      * @return SessionAccessibility instance.
> +      */
> +    public @NonNull SessionAccessibility getAccessibility() {
> +        if (mSessionAccessibility == null) {
> +          mSessionAccessibility = new SessionAccessibility(this);

Indent

::: mobile/android/geckoview/src/main/java/org/mozilla/geckoview/GeckoView.java
@@ +7,5 @@
>  package org.mozilla.geckoview;
>  
>  import org.mozilla.gecko.AndroidGamepadManager;
>  import org.mozilla.gecko.EventDispatcher;
> +import org.mozilla.gecko.SessionAccessibility;

Not needed after package fix.

::: mobile/android/geckoview/src/main/java/org/mozilla/geckoview/SessionAccessibility.java
@@ +2,5 @@
> + * This Source Code Form is subject to the terms of the Mozilla Public
> + * License, v. 2.0. If a copy of the MPL was not distributed with this
> + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
> +
> +package org.mozilla.gecko;

package org.mozilla.geckoview;

@@ +5,5 @@
> +
> +package org.mozilla.gecko;
> +
> +import org.json.JSONException;
> +import org.json.JSONObject;

These two are not used.

@@ +10,5 @@
> +import org.mozilla.gecko.util.BundleEventListener;
> +import org.mozilla.gecko.util.EventCallback;
> +import org.mozilla.gecko.util.GeckoBundle;
> +import org.mozilla.gecko.util.ThreadUtils;
> +import org.mozilla.geckoview.GeckoSession;

Not needed after package fix.

@@ +36,5 @@
> +    // This is the number BrailleBack uses to start indexing routing keys.
> +    private static final int BRAILLE_CLICK_BASE_INDEX = -275000000;
> +
> +    // Gecko session we are proxying
> +    private final GeckoSession mSession;

Change to,

> /* package */ final GeckoSession mSession;

@@ +44,5 @@
> +    private boolean mLastItem;
> +    // Used to store the JSON message and populate the event later in the code path.
> +    private AccessibilityNodeInfo mVirtualContentNode;
> +
> +    public SessionAccessibility(final GeckoSession session) {

Change to

> /* package */ SessionAccessibility(final GeckoSession session) {

@@ +58,5 @@
> +            }
> +        }, "GeckoView:AccessibilityEvent", null);
> +    }
> +
> +    public View getView() {

Would be nice to have some documentation.

@@ +62,5 @@
> +    public View getView() {
> +        return mView;
> +    }
> +
> +    public void setView(final View view) {

Documentation

@@ +70,5 @@
> +
> +        mView = view;
> +        mLastItem = false;
> +
> +        if (mView != null) {

> if (mView == null) {
>     return;
> }

So you have less indent.

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

Change to

> /* package */ static class Settings {

@@ +249,5 @@
> +            mEnabled = accessibilityManager.isEnabled() &&
> +                       accessibilityManager.isTouchExplorationEnabled();
> +
> +            accessibilityManager.addAccessibilityStateChangeListener(
> +            new AccessibilityManager.AccessibilityStateChangeListener() {

Indent is weird here.

@@ +277,5 @@
> +        public static boolean isEnabled() {
> +            return INSTANCE.mEnabled;
> +        }
> +
> +        private void updateAccessibilitySettings() {

Change to

> /* package */ void updateAccessibilitySettings() {

@@ +291,5 @@
> +                }
> +            });
> +        }
> +
> +        private void dispatch() {

Change to

> public void dispatch() {

::: mobile/android/modules/geckoview/GeckoViewAccessibility.jsm
@@ +8,5 @@
> +
> +ChromeUtils.import("resource://gre/modules/GeckoViewModule.jsm");
> +ChromeUtils.import("resource://gre/modules/XPCOMUtils.jsm");
> +
> +ChromeUtils.defineModuleGetter(this, "EventDispatcher",

Use the multiple-module version, ChromeUtils.defineModuleGetters({...})

Comment 14

[Eitan Isaacson [:eeejay]]

Thanks for the review! Have two questions below regarding access control.

(In reply to Jim Chen [:jchen] [:darchons] from comment #13)
> @@ +277,5 @@
> > +        public static boolean isEnabled() {
> > +            return INSTANCE.mEnabled;
> > +        }
> > +
> > +        private void updateAccessibilitySettings() {
> 
> Change to
> 
> > /* package */ void updateAccessibilitySettings() {

Why? it is only used in Settings.

> 
> @@ +291,5 @@
> > +                }
> > +            });
> > +        }
> > +
> > +        private void dispatch() {
> 
> Change to
> 
> > public void dispatch() {

Why? It used used by the containing class (SessionAccessibility), but that doesn't seem to be a problem. Is it?

Comment 15

[Eitan Isaacson [:eeejay]]

(In reply to Jim Chen [:jchen] [:darchons] from comment #13)
> Comment on attachment 8966405 [details] [diff] [review]
> Create SessionAccessibility for GeckoSession. r?jchen
> 
> Review of attachment 8966405 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> LGTM with nits.
> 
> Have you tested this under e10s?

With USE_MULTIPROCESS_EXTRA? Yeah, tested with the example GeckoViewActivity.

> 
> And I assume multiple GeckoView support will be added later?

Yes, in bug 751769.

Comment 16

[Eitan Isaacson [:eeejay]]

Attachment: Create SessionAccessibility for GeckoSession. r=jchen r?yzen

Yura, adding you for the accessfu changes.

Comment 17

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

(In reply to Eitan Isaacson [:eeejay] from comment #14)
> > 
> > Change to
> > 
> > > /* package */ void updateAccessibilitySettings() {
> 
> Why? it is only used in Settings.

It's used in an inner class of Settings, and Java will treat the inner class as an entirely separate class. Even though Java will allow you to call a private method of the enclosing class, that usage is not optimal. See [1].

> > 
> > Change to
> > 
> > > public void dispatch() {
> 
> Why? It used used by the containing class (SessionAccessibility), but that
> doesn't seem to be a problem. Is it?

Again, Java will allow you to call a private method of an inner class, but it's not optimal.

[1] https://medium.com/thoughts-overflow/effects-of-javas-synthetic-accessor-methods-in-android-bb67b3bac22e

Comment 18

[Eitan Isaacson [:eeejay]]

Attachment: Create SessionAccessibility for GeckoSession. r?jchen r?yzen

Rebased on a more recent inbound.

Comment 19

[Yura Zenevich [:yzen] (please use "needinfo")]

(In reply to Eitan Isaacson [:eeejay] from comment #18)
> Created attachment 8966807 [details] [diff] [review]
> Create SessionAccessibility for GeckoSession. r?jchen r?yzen
> 
> Rebased on a more recent inbound.

Quick eslint:

/Users/yzenevich/Developer/gecko/accessible/jsat/AccessFu.jsm
   25:43  error  Infix operators must be spaced.  space-infix-ops (eslint)
  268:13  error  'tdata' is not defined.          no-undef (eslint)

Comment 20

[Yura Zenevich [:yzen] (please use "needinfo")]

Comment on attachment 8966807 [details] [diff] [review]
Create SessionAccessibility for GeckoSession. r?jchen r?yzen

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

Looks good with some questions and nits.

::: accessible/jsat/AccessFu.jsm
@@ +30,1 @@
>      }

nit: new line after if (...) {...}

also maybe we could exit earlier:

if (aInTest) { 
  return; 
} 
...

@@ +36,5 @@
>    detach: function detach() {
>      // Avoid disabling twice.
>      if (this._enabled) {
>        this._disable();
>      }

nit: new line after if (...) { ... }

@@ +101,5 @@
>      Output.start();
>      PointerAdapter.start();
>  
>      if (Utils.MozBuildApp === "mobile/android") {
> +      Utils.win.WindowEventDispatcher.registerListener(this, [

nit: lets make a shorter getter for that:

Utils.win.WindowEventDispatcher -> Utils.eventDispatcher ?

@@ +102,5 @@
>      PointerAdapter.start();
>  
>      if (Utils.MozBuildApp === "mobile/android") {
> +      Utils.win.WindowEventDispatcher.registerListener(this, [
> +        "GeckoView:AccessibilityActivate",

Since these feel like constants (and are used more than once), they should be taken out into a const somewhere, perhaps even in Constants.jsm

something like:

const GECKO_VIEW_A11Y_EVENTS = {
  LONG_PRESS: "GeckoView:AccessibilityLongPress",
  VIEW_FOCUSED: "GeckoView:AccessibilityViewFocused",
  ...
};

Then this list will be GECKO_VIEW_A11Y_EVENTS.values()

Also I don't see a GeckoView:AccessibilitySettings in here (though I see GeckoViewAccessibility listening for it)?

@@ +264,5 @@
>  
>    onEvent(event, data, callback) {
>      switch (event) {
> +      case "GeckoView:AccessibilitySettings":
> +        if (tdata.enabled) {

as flagged by eslint tdata is meant to be data.

@@ +548,5 @@
>      const ANDROID_VIEW_TEXT_CHANGED = 0x10;
>      const ANDROID_VIEW_TEXT_SELECTION_CHANGED = 0x2000;
>  
>      for (let androidEvent of aDetails) {
> +      androidEvent.type = "GeckoView:AccessibilityEvent";

Also feels like a constant.

::: accessible/jsat/Utils.jsm
@@ +141,5 @@
>  
>    get CurrentBrowser() {
>      if (!this.BrowserApp) {
> +      // Attempt to get a content browser element.
> +      return this.win.document.querySelector("browser[type=content]");

It would be nice to have a more in depth comment here why this change (I'm guessing this changes are necessary with GeckoView) ?

Comment 21

[Eitan Isaacson [:eeejay]]

(In reply to Yura Zenevich [:yzen] from comment #20)
> Comment on attachment 8966807 [details] [diff] [review]
> Create SessionAccessibility for GeckoSession. r?jchen r?yzen
> 
> Review of attachment 8966807 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Looks good with some questions and nits.
> 
> ::: accessible/jsat/AccessFu.jsm
> @@ +30,1 @@
> >      }
> 
> nit: new line after if (...) {...}
> 
> also maybe we could exit earlier:
> 
> if (aInTest) { 
>   return; 
> } 

It is the end of the function, can't return any earlier, and a trailing line don't seem applicable here.

> ...
> 
> @@ +36,5 @@
> >    detach: function detach() {
> >      // Avoid disabling twice.
> >      if (this._enabled) {
> >        this._disable();
> >      }
> 
> nit: new line after if (...) { ... }
> 
> @@ +101,5 @@
> >      Output.start();
> >      PointerAdapter.start();
> >  
> >      if (Utils.MozBuildApp === "mobile/android") {
> > +      Utils.win.WindowEventDispatcher.registerListener(this, [
> 
> nit: lets make a shorter getter for that:
> 
> Utils.win.WindowEventDispatcher -> Utils.eventDispatcher ?

We could. I'm reluctant to because things will change when we support multiple windows..

> 
> @@ +102,5 @@
> >      PointerAdapter.start();
> >  
> >      if (Utils.MozBuildApp === "mobile/android") {
> > +      Utils.win.WindowEventDispatcher.registerListener(this, [
> > +        "GeckoView:AccessibilityActivate",
> 
> Since these feel like constants (and are used more than once), they should
> be taken out into a const somewhere, perhaps even in Constants.jsm
> 
> something like:
> 
> const GECKO_VIEW_A11Y_EVENTS = {
>   LONG_PRESS: "GeckoView:AccessibilityLongPress",
>   VIEW_FOCUSED: "GeckoView:AccessibilityViewFocused",
>   ...
> };
> 
> Then this list will be GECKO_VIEW_A11Y_EVENTS.values()
> 
> Also I don't see a GeckoView:AccessibilitySettings in here (though I see
> GeckoViewAccessibility listening for it)?

It is in GeckoViewAccessibility.jsm now.

> 
> @@ +264,5 @@
> >  
> >    onEvent(event, data, callback) {
> >      switch (event) {
> > +      case "GeckoView:AccessibilitySettings":
> > +        if (tdata.enabled) {
> 
> as flagged by eslint tdata is meant to be data.

doh! I kept on squashing that mistake into the patch.

> 
> ::: accessible/jsat/Utils.jsm
> @@ +141,5 @@
> >  
> >    get CurrentBrowser() {
> >      if (!this.BrowserApp) {
> > +      // Attempt to get a content browser element.
> > +      return this.win.document.querySelector("browser[type=content]");
> 
> It would be nice to have a more in depth comment here why this change (I'm
> guessing this changes are necessary with GeckoView) ?

I'll do a bit better in that comment.

Comment 22

[Eitan Isaacson [:eeejay]]

Attachment: Create SessionAccessibility for GeckoSession. r=jchen r=yzen

Updated nits and double checked with try:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=06d2c46f98b990380fc307b2df1d8396be5f27db

Comment 23

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/7149139c60d9
Create SessionAccessibility for GeckoSession. r=jchen r=yzen

Comment 24

[Eliza Balazs [:ebalazs_]]

Backed out 2 changesets (bug 1449364, bug 1453278) for rc4 and 42 perma failures in layout/base/tests/test_bug332655-2.html and testInputConnection

Push with failures: https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=7149139c60d98e3e5338795cb2be648136f2b9a4&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&filter-resultStatus=runnable&filter-resultStatus=success&selectedJob=173244342

Backout: https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=1f351e5132ab4f21dc117dc737c8e24e19d2e21a&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&filter-resultStatus=runnable&filter-resultStatus=success

Comment 25

[Eitan Isaacson [:eeejay]]

Attachment: Create SessionAccessibility for GeckoSession. r=jchen

Since I changed how AccessFu.attach works, i forgot to update it in fennec's browser.js.
This resulted in AccessFu being activated regardless of platform accessibility state.

Jim, the only change here is in browser.js, seeking your approval.

Comment 26

[Ryan VanderMeulen [:RyanVM]]

Needs rebasing against inbound tip.

Comment 27

[Eitan Isaacson [:eeejay]]

Attachment: Create SessionAccessibility for GeckoSession. r=jchen

Comment 28

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/0f2deb58c5d6
Create SessionAccessibility for GeckoSession. r=jchen r=yzen

Comment 29

[Arthur Iakab [arthur_iakab]]

https://hg.mozilla.org/mozilla-central/rev/0f2deb58c5d6