Fix VoiceOver clicking the wrong button and refactor accessibility implementation (JetBrains#2720)

Author: m-sasha

compose/ui/ui/src/desktopMain/kotlin/androidx/compose/ui/awt/ComposeWindow.desktop.kt

@@ -36,7 +36,6 @@ import java.awt.event.MouseListener
 import java.awt.event.MouseMotionListener
 import java.awt.event.MouseWheelListener
 import java.util.Locale
-import javax.accessibility.Accessible
 import javax.swing.JFrame
 import org.jetbrains.skiko.GraphicsApi
 import org.jetbrains.skiko.SkiaLayerAnalytics
@@ -79,10 +78,6 @@ class ComposeWindow @ExperimentalComposeUiApi constructor(
     internal val windowContext by composePanel::windowContext
     internal var rootForTestListener by composePanel::rootForTestListener
 
-    // Don't override the accessible context of JFrame, since accessibility work through HardwareLayer
-    internal val windowAccessible: Accessible
-        get() = composePanel.windowAccessible
-
     /**
      * Returns the [SemanticsOwner]s corresponding to the roots of the semantics trees in this
      * [ComposeWindow].

compose/ui/ui/src/desktopMain/kotlin/androidx/compose/ui/awt/ComposeWindowPanel.desktop.kt

@@ -74,7 +74,6 @@ internal class ComposeWindowPanel(
         }
     private val contentComponent by composeContainer::contentComponent
 
-    val windowAccessible by composeContainer::accessible
     val windowContext by composeContainer::windowContext
     var rootForTestListener by composeContainer::rootForTestListener
     var fullscreen by composeContainer::fullscreen

compose/ui/ui/src/desktopMain/kotlin/androidx/compose/ui/platform/a11y/Accessibility.desktop.kt

@@ -17,13 +17,13 @@ import org.jetbrains.skiko.initializeCAccessible
  */
 internal class AccessibleFocusHelper(
     private val component: Component,
-    private val regularAccessible: Accessible,
+    private val sceneAccessibleContext: AccessibleContext,
 ) {
 
     private var focusedAccessible: Accessible? = null
 
     val accessibleContext: AccessibleContext
-        get() = (focusedAccessible ?: regularAccessible).accessibleContext
+        get() = focusedAccessible?.accessibleContext ?: sceneAccessibleContext
 
     private var resetFocusAccessibleJob: Job? = null
 
@@ -44,9 +44,24 @@ internal class AccessibleFocusHelper(
         // and its accessibility context. This timeout is used to deal with concurrency
         // TODO Find more reliable procedure
         resetFocusAccessibleJob?.cancel()
-        resetFocusAccessibleJob = GlobalScope.launch(MainUIDispatcher) {
-            delay(100)
-            focusedAccessible = null
+        if (focusedAccessible != null) {
+            resetFocusAccessibleJob = GlobalScope.launch(MainUIDispatcher) {
+                delay(100)
+                focusedAccessible = null
+            }
+        }
+    }
+
+    /**
+     * When [focusedAccessible] is set, for the accessible hierarchy to be correct, its parent must
+     * be reported as the scene's accessible context. This function returns it if [accessible] is
+     * [focusedAccessible].
+     */
+    fun accessibleParentOverride(accessible: Accessible): Accessible? {
+        return if (accessible == focusedAccessible) {
+            sceneAccessibleContext.accessibleParent as Accessible
+        } else {
+            null
         }
     }
 

compose/ui/ui/src/desktopMain/kotlin/androidx/compose/ui/platform/a11y/ComposeAccessible.kt

@@ -75,7 +75,7 @@ private typealias ActionKey = SemanticsPropertyKey<AccessibilityAction<() -> Boo
  */
 internal class ComposeAccessible(
     semanticsNode: SemanticsNode,
-    private val controller: AccessibilityController
+    private val ownerAccessibility: SemanticsOwnerAccessibility
 ) : Accessible,
     // Must be a subclass of java.awt.Component because CAccessible only registers property
     // listeners with the accessible context if the Accessible is an instance of java.awt.Component
@@ -171,7 +171,7 @@ internal class ComposeAccessible(
             get() = semanticsConfig.getOrNull(SemanticsProperties.Selected)
 
         private val density: Density
-            get() = controller.desktopComponent.density
+            get() = ownerAccessibility.desktopComponent.density
 
         val horizontalScroll
             get() = semanticsConfig.getOrNull(SemanticsProperties.HorizontalScrollAxisRange)
@@ -266,60 +266,61 @@ internal class ComposeAccessible(
         }
 
         override fun getAccessibleParent(): Accessible? {
-            val parentNode = semanticsNode.parent ?: return controller.parentAccessible
-            return controller.accessibleByNodeId(parentNode.id)!!
+            return ownerAccessibility.accessibleParentOf(this@ComposeAccessible)
         }
 
         override fun getAccessibleIndexInParent(): Int {
-            val parent = semanticsNode.parent ?: return controller.indexInScene()
+            val parent = semanticsNode.parent ?: return ownerAccessibility.indexInScene()
             return parent.traversalOrderedChildren().indexOfFirst { it.id == semanticsNode.id }
         }
 
         override fun getAccessibleComponent(): AccessibleComponent? {
             return this
         }
 
-        // we have to store a reference to AccessibleAction, because AWT itself uses weak
-        // references and GC could delete an object which is, in fact, in use
-        private var accessibleAction: AccessibleAction? = null
+        private var accessibleActions: List<Pair<String, ActionKey>>? = null
 
-        override fun getAccessibleAction(): AccessibleAction? {
-            val actions = mutableListOf<Pair<String?, ActionKey>>()
+        private fun updateAccessibleActions() {
+            val actions = mutableListOf<Pair<String, ActionKey>>()
 
-            fun addActionIfExist(key: SemanticsPropertyKey<AccessibilityAction<() -> Boolean>>) {
-                semanticsConfig.getOrNull(key)?.let {
-                    actions.add(Pair(it.label, key))
-                }
-            }
-            semanticsConfig.getOrNull(SemanticsActions.OnClick)?.let {
-                // AWT expects "click" label for click actions, at least on macOS...
-                actions.add(Pair("click", SemanticsActions.OnClick))
+            fun addActionIfExist(
+                key: SemanticsPropertyKey<AccessibilityAction<() -> Boolean>>,
+                label: String? = null,
+            ) {
+                val action = semanticsConfig.getOrNull(key) ?: return
+                val label = label ?: action.label ?: return  // No point to add an action without a label
+                actions.add(Pair(label, key))
             }
 
+            // AWT expects a "click" label for click actions, at least on macOS...
+            addActionIfExist(SemanticsActions.OnClick, AccessibleAction.CLICK)
             addActionIfExist(SemanticsActions.OnLongClick)
             addActionIfExist(SemanticsActions.Expand)
             addActionIfExist(SemanticsActions.Collapse)
             addActionIfExist(SemanticsActions.Dismiss)
 
-            if (actions.isEmpty()) {
-                return null
-            }
-            accessibleAction = object : AccessibleAction {
-                override fun getAccessibleActionCount(): Int = actions.size
+            accessibleActions = actions.takeIf { it.isNotEmpty() }
+        }
 
-                override fun getAccessibleActionDescription(i: Int): String? {
-                    val (label, _) = actions[i]
-                    return label
-                }
+        override fun getAccessibleAction(): AccessibleAction? {
+            updateAccessibleActions()
+            return if (accessibleActions == null) null else this
+        }
 
-                override fun doAccessibleAction(i: Int): Boolean {
-                    val (_, actionKey) = actions[i]
-                    return semanticsConfig.getOrNull(actionKey)?.let {
-                        it.action?.invoke()
-                    } ?: false
-                }
-            }
-            return accessibleAction
+        override fun getAccessibleActionCount(): Int = accessibleActions?.size ?: 0
+
+        override fun getAccessibleActionDescription(i: Int): String? {
+            val actions = accessibleActions!!
+            val (label, _) = actions[i]
+            return label
+        }
+
+        override fun doAccessibleAction(i: Int): Boolean {
+            val actions = accessibleActions!!
+            val (_, actionKey) = actions[i]
+            return semanticsConfig.getOrNull(actionKey)?.let {
+                it.action?.invoke()
+            } ?: false
         }
 
         override fun getAccessibleValue(): AccessibleValue? {
@@ -341,7 +342,7 @@ internal class ComposeAccessible(
             val regularChildren = semanticsNode.traversalOrderedChildren()
             val childrenSize = regularChildren.size
             return if (index < childrenSize) {
-                controller.accessibleByNodeId(regularChildren[index].id)
+                ownerAccessibility.accessibleByNodeId(regularChildren[index].id)
             } else {
                 auxiliaryChildren[index - childrenSize]
             }
@@ -381,7 +382,7 @@ internal class ComposeAccessible(
         override fun getAccessibleAt(p: Point): Accessible? {
             val accessibleChildren = semanticsNode.traversalOrderedChildren()
             for (child in accessibleChildren) {
-                val accessible = controller.accessibleByNodeId(child.id) as? Accessible ?: continue
+                val accessible = ownerAccessibility.accessibleByNodeId(child.id) as? Accessible ?: continue
                 val accessibleComponent = (accessible.accessibleContext as? AccessibleComponent) ?: continue
                 accessibleComponent.getAccessibleAt(p)?.let {
                     return it
@@ -464,7 +465,7 @@ internal class ComposeAccessible(
         }
 
         override fun getAccessibleRole(): AccessibleRole {
-            AccessibilityController.AccessibilityUsage.notifyInUse()
+            SemanticsOwnerAccessibility.AccessibilityUsage.notifyInUse()
             return computeAccessibleRole()
         }
 
@@ -884,20 +885,6 @@ internal class ComposeAccessible(
             TODO("Not yet implemented")
         }
 
-        // For some reasons JDK's CAccessibility does not call getAccessibleAction
-        // and performs actions on current component itself
-        override fun getAccessibleActionCount(): Int {
-            return accessibleAction?.accessibleActionCount ?: 0
-        }
-
-        override fun getAccessibleActionDescription(i: Int): String {
-            return accessibleAction?.getAccessibleActionDescription(i) ?: ""
-        }
-
-        override fun doAccessibleAction(i: Int): Boolean {
-            return accessibleAction?.doAccessibleAction(i) ?: false
-        }
-
         private fun List<CharSequence>.mergeText() = joinToString(", ")
     }
 }