Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Improve iOS a11y sync behavior #1170

Merged
merged 15 commits into from
Mar 8, 2024
Original file line number Diff line number Diff line change
Expand Up @@ -35,7 +35,7 @@ import kotlinx.cinterop.ExportObjCClass
import kotlinx.cinterop.readValue
import kotlinx.coroutines.CoroutineScope
import kotlinx.coroutines.Job
import kotlinx.coroutines.delay
import kotlinx.coroutines.channels.Channel
import kotlinx.coroutines.launch
import platform.CoreGraphics.CGRect
import platform.CoreGraphics.CGRectMake
Expand Down Expand Up @@ -85,6 +85,57 @@ interface AccessibilityDebugLogger {
fun log(message: Any?)
}

/**
* Enum class representing different kinds of accessibility invalidation.
*/
private sealed interface SemanticsTreeInvalidation {
/**
* Represents a complete invalidation of the semantics tree.
*/
object SemanticsChanged : SemanticsTreeInvalidation

/**
* Represents a change in the bounds of a node with the given [id].
*/
class BoundsChanged(val id: Int) : SemanticsTreeInvalidation
}

private sealed interface SemanticsTreeSyncStrategy {
/**
* Returns the updated strategy with the given [invalidation] applied.
*/
fun reduce(invalidation: SemanticsTreeInvalidation): SemanticsTreeSyncStrategy

object Complete : SemanticsTreeSyncStrategy {
override fun reduce(invalidation: SemanticsTreeInvalidation): Complete {
return this
}
}

class BoundsOnly(id: Int) : SemanticsTreeSyncStrategy {
val ids = mutableSetOf(id)

override fun reduce(invalidation: SemanticsTreeInvalidation): SemanticsTreeSyncStrategy {
when (invalidation) {
is SemanticsTreeInvalidation.SemanticsChanged -> return Complete
is SemanticsTreeInvalidation.BoundsChanged -> {
ids.add(invalidation.id)
return this
}
}
}
}

companion object {
fun from(invalidation: SemanticsTreeInvalidation): SemanticsTreeSyncStrategy {
return when (invalidation) {
is SemanticsTreeInvalidation.SemanticsChanged -> Complete
is SemanticsTreeInvalidation.BoundsChanged -> BoundsOnly(invalidation.id)
}
}
}
}

private class CachedAccessibilityPropertyKey<V>

private object CachedAccessibilityPropertyKeys {
Expand Down Expand Up @@ -226,11 +277,27 @@ private class AccessibilityElement(
return null
}

/**
* Discards the cache for the properties that are computed from the [SemanticsNode.config].
*/
fun discardCache() {
_cachedConfig = null
cachedProperties.clear()
}

/**
* Discards the cached accessibilityFrame for this element and all its descendants recursively.
*/
fun discardCachedAccessibilityFrameRecursively() {
if (cachedProperties.remove(CachedAccessibilityPropertyKeys.accessibilityFrame) != null) {
for (child in children) {
child.discardCachedAccessibilityFrameRecursively()
}
} else {
// Not calculated yet, or the subtree was already discarded. Do nothing.
}
}

fun dispose() {
check(isAlive) {
"AccessibilityElement is already disposed"
Expand Down Expand Up @@ -816,10 +883,9 @@ private class AccessibilityContainer(
}
}

private sealed interface NodesSyncResult {
object NoChanges : NodesSyncResult
data class Success(val newElementToFocus: Any?) : NodesSyncResult
}
private class NodesSyncResult(
val newElementToFocus: Any?
ASalavei marked this conversation as resolved.
Show resolved Hide resolved
)

/**
* A sealed class that represents the options for syncing the Compose SemanticsNode tree with the iOS UIAccessibility tree.
Expand Down Expand Up @@ -895,6 +961,8 @@ internal class AccessibilityMediator(
private var needsInitialRefocusing = true
private var isAlive = true

private val invalidationChannel = Channel<SemanticsTreeInvalidation>(capacity = Channel.UNLIMITED)

/**
* Remembered [AccessibilityDebugLogger] after last sync, if logging is enabled according to
* [AccessibilitySyncOptions].
Expand All @@ -904,12 +972,6 @@ internal class AccessibilityMediator(

var rootSemanticsNodeId: Int = -1

/**
* A value of true indicates that the Compose accessible tree is dirty, meaning that compose
* semantics tree was modified since last sync, false otherwise.
*/
private var isCurrentComposeAccessibleTreeDirty = false

/**
* Job to cancel tree syncing when the mediator is disposed.
*/
Expand All @@ -929,13 +991,18 @@ internal class AccessibilityMediator(
init {
getAccessibilitySyncOptions().debugLoggerIfEnabled?.log("AccessibilityMediator for ${view} created")

val updateIntervalMillis = 50L
// TODO: this approach was copied from desktop implementation, obviously it has a [updateIntervalMillis] lag
// between the actual change in the semantics tree and the change in the accessibility tree.
// should we use some other approach?
coroutineScope.launch {
while (isAlive) {
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think this isAlive is needed. When you cancel the Job, a CancellationException is thrown inside the coroutine.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It makes sense, will change it

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Dome

var result: NodesSyncResult
var strategy = SemanticsTreeSyncStrategy.from(
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's not necessary to send this data through the channel.

The channel is just used as a synchronization primitive to trigger the coroutine to awake. The information about what to do when awaking can just be put in a regular data structure by onSemanticsChange and onLayoutChange.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This approach has more straightforward data-flow though instead of scattering the states all around the class.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It could affect performance, and IMO is not a good idea in general (mixing a synchronization utility with logic). Tomorrow there could be a better/faster way to trigger a coroutine to wake up (like Java's notify) that doesn't allow data to pass through.

But your call.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So you'd insist to revert e8de6ee?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reverted

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't insist, just suggesting.

invalidationChannel.receive()
)

while (true) {
val result = invalidationChannel.tryReceive()

val invalidation = result.getOrNull() ?: break
strategy = strategy.reduce(invalidation)
}

val syncOptions = getAccessibilitySyncOptions()

Expand All @@ -948,24 +1015,16 @@ internal class AccessibilityMediator(
}

if (shouldPerformSync) {
var result: NodesSyncResult

val time = measureTime {
result = sync()
result = sync(strategy)
}

when (val immutableResult = result) {
is NodesSyncResult.NoChanges -> {
// Do nothing
}

is NodesSyncResult.Success -> {
debugLogger?.log("AccessibilityMediator.sync took $time")
debugLogger?.log("AccessibilityMediator.sync took $time")

UIAccessibilityPostNotification(UIAccessibilityLayoutChangedNotification, immutableResult.newElementToFocus)
}
}
UIAccessibilityPostNotification(UIAccessibilityLayoutChangedNotification, result.newElementToFocus)
}

delay(updateIntervalMillis)
}
}
}
Expand All @@ -979,15 +1038,17 @@ internal class AccessibilityMediator(
fun onSemanticsChange() {
debugLogger?.log("onSemanticsChange")

isCurrentComposeAccessibleTreeDirty = true
invalidationKind = SemanticsTreeInvalidationKind.COMPLETE
invalidationChannel.trySend(Unit)
}

fun onLayoutChange(nodeId: Int) {
debugLogger?.log("onLayoutChange (nodeId=$nodeId)")

// TODO: Only recompute the layout-related properties of the node
isCurrentComposeAccessibleTreeDirty = true
invalidatedBoundsNodeIds.add(nodeId)

// unprocessedInvalidationKind will be set to BOUNDS in sync(), it's a strict subset of COMPLETE
invalidationChannel.trySend(Unit)
}

fun dispose() {
Expand Down Expand Up @@ -1075,22 +1136,37 @@ internal class AccessibilityMediator(

/**
* Syncs the accessibility tree with the current semantics tree.
* TODO: Does a full tree traversal on every sync, expect changes from Google, they are also aware
* of the issue and associated performance overhead.
*/
private fun sync(): NodesSyncResult {
// TODO: investigate what needs to be done to reflect that this hierarchy is probably covered
// by sibling overlay
private fun sync(strategy: SemanticsTreeSyncStrategy): NodesSyncResult {
when (strategy) {
is SemanticsTreeSyncStrategy.Complete -> {
return completeSync()
}

if (!isCurrentComposeAccessibleTreeDirty) {
return NodesSyncResult.NoChanges
is SemanticsTreeSyncStrategy.BoundsOnly -> {
for (id in strategy.ids) {
val element = accessibilityElementsMap[id]
element?.discardCachedAccessibilityFrameRecursively()
}

return NodesSyncResult(null)
}
}
}

/**
* Performs a complete sync of the accessibility tree with the current semantics tree.
*
* TODO: Does a full tree traversal on every sync, expect changes from Google, they are also aware
* of the issue and associated performance overhead.
*/

private fun completeSync(): NodesSyncResult {
// TODO: investigate what needs to be done to reflect that this hierarchy is probably covered
// by sibling overlay or another UIView hierarchy represented by other mediator
val rootSemanticsNode = owner.rootSemanticsNode
rootSemanticsNodeId = rootSemanticsNode.id

isCurrentComposeAccessibleTreeDirty = false

check(!view.isAccessibilityElement) {
"Root view must not be an accessibility element"
}
Expand All @@ -1108,11 +1184,13 @@ internal class AccessibilityMediator(
// TODO: in future the focused element could be the interop UIView that is detached from the
// hierarchy, but still maintains the focus until the GC collects it, or AX services detect
// that it's not reachable anymore through containment chain
val isFocusedElementDead = focusedElement?.let {
val isFocusedElementAlive = focusedElement?.let {
val accessibilityElement = it as? AccessibilityElement
accessibilityElement?.isAlive ?: false
} ?: false

val isFocusedElementDead = !isFocusedElementAlive

val needsRefocusing = needsInitialRefocusing || isFocusedElementDead

val newElementToFocus = if (needsRefocusing) {
Expand All @@ -1132,7 +1210,7 @@ internal class AccessibilityMediator(
null
}

return NodesSyncResult.Success(newElementToFocus)
return NodesSyncResult(newElementToFocus)
}
}

Expand Down
Loading