1479034 - Make GeckoView's SessionAccessibility a JNIObject associated with a nsWindow.

Status: RESOLVED FIXED

(Core :: Disability Access APIs, enhancement)

Description

[Eitan Isaacson [:eeejay]]

Each SessionAccessibility instance should be associated with an nsWindow that references the session's root accessible.

Comment 1

[Eitan Isaacson [:eeejay]]

Attachment: Make GeckoView's SessionAccessibility a JNIObject associated with a nsWindow. r?jchen

We could remove the attach functions if SessionAccessibility wasn't lazily instantiated.

Comment 2

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

Comment on attachment 9009288 [details] [diff] [review]
Make GeckoView's SessionAccessibility a JNIObject associated with a nsWindow. r?jchen

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

Looks good with only a few nits.

::: accessible/android/SessionAccessibility.cpp
@@ +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/. */
> +
> +#include <stdio.h>

Not used?

::: accessible/android/SessionAccessibility.h
@@ +7,5 @@
> +#define mozilla_a11y_SessionAccessibility_h_
> +
> +#include "AndroidUiThread.h"
> +#include "GeneratedJNINatives.h"
> +#include "nsAppShell.h"

Not used?

@@ +12,5 @@
> +#include "nsThreadUtils.h"
> +#include "nsWindow.h"
> +
> +#include <android/log.h>
> +#define AALOG(args...)                                                         \

`#if DEBUG`. Also please define this in a .cpp file rather than in a header.

@@ +33,5 @@
> +  {
> +    if (RefPtr<nsThread> uiThread = GetAndroidUiThread()) {
> +      java::SessionAccessibility::GlobalRef sa(mSessionAccessibility);
> +      uiThread->Dispatch(NS_NewRunnableFunction(
> +        "SessionAccessibility::Attach", [sa] { sa->SetAttached(true); }));

`[sa = java::SessionAccessibility::GlobalRef(mSessionAccessibility)]` and no need for separate `sa` declaration above

@@ +42,5 @@
> +  {
> +    if (RefPtr<nsThread> uiThread = GetAndroidUiThread()) {
> +      java::SessionAccessibility::GlobalRef sa(mSessionAccessibility);
> +      uiThread->Dispatch(NS_NewRunnableFunction(
> +        "SessionAccessibility::OnDetach", [sa] { sa->SetAttached(false); }));

Same here. It's probably worth combining this function and the constructor since they share much of the code.

@@ +56,5 @@
> +private:
> +  ~SessionAccessibility() {}
> +
> +  nsWindow::WindowPtr<SessionAccessibility> mWindow; // Parent only
> +  java::SessionAccessibility::GlobalRef mSessionAccessibility;

Probably need to switch to a weak pointer later, with the work :snorp is doing (bug 1486596).

::: mobile/android/geckoview/src/main/java/org/mozilla/geckoview/GeckoSession.java
@@ +792,5 @@
>          public native void attachEditable(IGeckoEditableParent parent,
>                                            GeckoEditableChild child);
>  
> +        @WrapForJNI(dispatchTo = "proxy")
> +        private native void attachAccessibility(SessionAccessibility SessionAccessibility);

`public`. Also parameter name should be lowercase (sessionAccessibility)

@@ +1070,5 @@
>        *
>        * @return SessionAccessibility instance.
>        */
>      public @NonNull SessionAccessibility getAccessibility() {
>          if (mAccessibility == null) {

Early return here, i.e. `if (mAccessibility != null) { return mAccessibility; }`

Should also add a `ThreadUtils.assertOnUiThread();` above.

::: mobile/android/geckoview/src/main/java/org/mozilla/geckoview/SessionAccessibility.java
@@ +32,5 @@
>  import android.view.accessibility.AccessibilityManager;
>  import android.view.accessibility.AccessibilityNodeInfo;
>  import android.view.accessibility.AccessibilityNodeProvider;
>  
> +public class SessionAccessibility extends JNIObject {

Because `SessionAccessibility` is public API, instead of having it directly extend JNIObject, we normally prefer having a private inner class that extends JNIObject.

::: widget/android/nsWindow.cpp
@@ +296,5 @@
>      // Close and destroy the nsWindow.
>      void Close();
>  
> +    void BindAccessibility(const GeckoSession::Window::LocalRef& inst,
> +                           jni::Object::Param aSessionAccessibility);

Not used? Seems like this would do the same thing as `AttachAccessibility`

@@ +1145,5 @@
>          window.mLayerViewSupport.Detach();
>      }
> +
> +    if (window.mSessionAccessibility) {
> +      window.mSessionAccessibility.Detach();

Four space indent

@@ +1265,5 @@
>      MOZ_ASSERT(window.mAndroidView);
>      window.mAndroidView->mEventDispatcher->Attach(
>              java::EventDispatcher::Ref::From(aDispatcher), mDOMWindow);
>  
> +    if (aSessionAccessibility) {

We should call `window.mSessionAccessibility.Detach()` if needed, even if `aSessionAccessibility` is null here.

@@ +1302,5 @@
>      window.mEditableParent = aEditableParent;
>  }
>  
> +void
> +nsWindow::GeckoViewSupport::AttachAccessibility(const GeckoSession::Window::LocalRef& inst,

I would just incorporate this into `GeckoViewSupport::Transfer` itself

::: widget/android/nsWindow.h
@@ +189,5 @@
>      // Strong referenced by the Java instance.
>      NativePtr<mozilla::widget::GeckoEditableSupport> mEditableSupport;
>      mozilla::jni::Object::GlobalRef mEditableParent;
>  
> +    NativePtr<mozilla::a11y::SessionAccessibility> mSessionAccessibility;

Please add a comment about ownership (similar to the other members above)

Comment 3

[Eitan Isaacson [:eeejay]]

(In reply to Jim Chen [:jchen] [:darchons] from comment #2)
> @@ +1302,5 @@
> >      window.mEditableParent = aEditableParent;
> >  }
> >  
> > +void
> > +nsWindow::GeckoViewSupport::AttachAccessibility(const GeckoSession::Window::LocalRef& inst,
> 
> I would just incorporate this into `GeckoViewSupport::Transfer` itself


I'm not sure what you mean. Get rid of AttachAccessibility? We need it for when the window is opened before a SessionAccessibility is created.

Comment 4

[Eitan Isaacson [:eeejay]]

Attachment: Make GeckoView's SessionAccessibility a JNIObject associated with a nsWindow. r?jchen

Here is another attempt!

Comment 5

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

Comment on attachment 9010794 [details] [diff] [review]
Make GeckoView's SessionAccessibility a JNIObject associated with a nsWindow. r?jchen

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

Looks good! Thanks!

::: accessible/android/SessionAccessibility.h
@@ +12,5 @@
> +namespace mozilla {
> +namespace a11y {
> +
> +class SessionAccessibility final
> +  : public java::SessionAccessibility::NativeProvider::Natives<

Usually you would put these in one line; same below.

::: mobile/android/geckoview/src/main/java/org/mozilla/geckoview/GeckoSession.java
@@ +1092,5 @@
> +                mWindow.attachAccessibility(mAccessibility.getNativeProvider());
> +            } else {
> +                GeckoThread.queueNativeCallUntil(GeckoThread.State.PROFILE_READY,
> +                        mWindow, "attachAccessibility",
> +                        SessionAccessibility.class, mAccessibility.getNativeProvider());

`SessionAccessibility.NativeProvider.class`

::: mobile/android/geckoview/src/main/java/org/mozilla/geckoview/SessionAccessibility.java
@@ +234,5 @@
>      /* package */  final GeckoSession mSession;
>      // This is the view that delegates accessibility to us. We also sends event through it.
>      private View mView;
> +    // The native portion of the node provider.
> +    final private NativeProvider mNativeProvider = new NativeProvider();

`/* package */ final NativeProvider nativeProvider` would also be fine if you want to get rid of `getNativeProvider()`.

::: widget/android/nsWindow.cpp
@@ +1265,5 @@
>      window.mAndroidView->mEventDispatcher->Attach(
>              java::EventDispatcher::Ref::From(aDispatcher), mDOMWindow);
>  
> +    if (window.mSessionAccessibility) {
> +      window.mSessionAccessibility.Detach();

Four space indent

@@ +1312,5 @@
> +      inst.Env());
> +    sessionAccessibility = java::SessionAccessibility::NativeProvider::Ref::From(
> +      aSessionAccessibility);
> +
> +    if (window.mSessionAccessibility) {

I think you can `MOZ_ASSERT(!window.mSessionAccessibility)` here?

Comment 6

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/6c784e692474
Make GeckoView's SessionAccessibility a JNIObject associated with a nsWindow. r=jchen

Comment 7

[Andreea Pavel [:apavel]]

https://hg.mozilla.org/mozilla-central/rev/6c784e692474