Conversation
| return if (i < replacedChildrenSize) { | ||
| controller.accessibleByNodeId(replacedChildren[i].id) | ||
| override fun getAccessibleChild(index: Int): Accessible? { | ||
| val regularChildren = semanticsNode.traversalOrderedChildren() |
There was a problem hiding this comment.
Sorting nodes on every getAccessibleChild looks expensive. Can we cache traversalOrderedChildren?
There was a problem hiding this comment.
Fetching the nodes themselves is even more expensive.
I do plan to look at improving a11y performance at some point, but it's a big and separate task.
There was a problem hiding this comment.
Fetching the nodes themselves is even more expensive.
Looks like fetching the nodes is just iterating over the children, so it is O(n)? Sorting on the other hand is O(n logn).
I measured and have these results:
100 getAccessibleChild, new code: 5.868800ms
100 accessibleIndexInParent, new code: 2.606ms
100 getAccessibleChild, old code: 2.978900ms
100 accessibleIndexInParent, old code: 972.9us
500 getAccessibleChild, new code: 62.596700ms
500 accessibleIndexInParent, new code: 50.972800ms
500 getAccessibleChild, old code: 10.660500ms
500 accessibleIndexInParent, old code: 3.967100ms
Code:
@Test
fun traversalIndexIsRespected() = runDesktopA11yTest {
test.setContent {
Column(Modifier
.testTag("container")
.semantics {
isTraversalGroup = true
}
) {
val random = Random(245)
repeat(500) {
Text(
"Item $it",
Modifier
.semantics {
traversalIndex = random.nextInt(500).toFloat()
contentDescription = "Item $it"
}
.testTag("item$it")
)
}
}
}
test.onNodeWithTag("container").fetchAccessible().accessibleContext.let { context ->
assertNotNull(context)
assertThat(context.accessibleChildrenCount).isEqualTo(500)
// warm up
assertThat(context.getAccessibleChild(0).accessibleContext.accessibleDescription.isNotEmpty()).isEqualTo(true)
assertThat(context.getAccessibleChild(1).accessibleContext.accessibleDescription.isNotEmpty()).isEqualTo(true)
assertThat(context.getAccessibleChild(2).accessibleContext.accessibleDescription.isNotEmpty()).isEqualTo(true)
println("500 getAccessibleChild:")
println(measureTime {
for (i in 0..<500) {
assertThat(context.getAccessibleChild(i).accessibleContext.accessibleDescription.isNotEmpty()).isEqualTo(true)
}
})
}
val accessibles = (0 ..< 500).map { test.onNodeWithTag("item$it").fetchAccessible().accessibleContext!! }
// warm up
assertThat(accessibles[0].accessibleIndexInParent >= 0).isEqualTo(true)
assertThat(accessibles[1].accessibleIndexInParent >= 0).isEqualTo(true)
assertThat(accessibles[2].accessibleIndexInParent >= 0).isEqualTo(true)
println("500 accessibleIndexInParent:")
println(measureTime {
for (i in 0..<500) {
assertThat(accessibles[i].accessibleIndexInParent >= 0).isEqualTo(true)
}
})
}
}
If AWT just calls it once, not iterates over them - it is fine. If it iterates over them in some cases, we might add a significant freeze with new code. Can you tell if it iterates (at least on macOs/Windows)?
There was a problem hiding this comment.
It does iterate over them in some cases because there is no getAccessibleChildren() method, only getAccessibleChild(index) method.
However,
- Using traversal index is uncommon, and even more uncommon with a large number of children, and even then the order will not be random, in which case the sort will probably be O(n).
- I will look into the performance, just not in this PR.
There was a problem hiding this comment.
I did fix getAccessibleAt which was calling getAccessibleChild multiple times.
But it also needs additional fixing, as it's just incorrect.
There was a problem hiding this comment.
There was a problem hiding this comment.
Using traversal index is uncommon, and even more uncommon with a large number of children
Makes sense. So, we only degrade the case when users set traversalIndex. It also didn't work before, but at least works now.
I will look into the performance, just not in this PR.
Please create an issue if you don't have it already.
compose/ui/ui/src/desktopTest/kotlin/androidx/compose/ui/platform/AccessibilityTest.kt
Outdated
Show resolved
Hide resolved
m-sasha
force-pushed
the
m-sasha/apply-traversalOrder
branch
from
4ebb2be to
a665482
Compare
2 participants
The children of nodes that have
isTraversalGroupsemantics are now ordered according to theirtraversalIndex.Fixes https://youtrack.jetbrains.com/issue/CMP-8520/A11y-should-respect-traversalIndex
Unfortunately some screen readers, namely VoiceOver, ignore the order of children and only use the geometry to decide the order. I've created a JBR ticket, but I'm not sure it can even be fixed on their end.
Add: according to the JBR folks, this is expected behavior, as the corresponding commands in VoiceOver are "move up/down/left/right" and there's no "move to next/previous" command. The item chooser (VO+I) does show the correct item order, so the information is being sent to VoiceOver correctly.
It does work correctly on Windows with NVDA.
Testing
Tested manually and added a unit test
This should be tested by QA
Release Notes
Fixes - Desktop
isTraversalNodesemantics are now ordered according to theirtraversalIndex.