Status Update
Comments
so...@google.com <so...@google.com>
so...@google.com <so...@google.com>
pa...@google.com <pa...@google.com> #2
After some discussion in chat we find the following policy to be quite a good match: if parent is defined use parent otherwise merge all the children in there into a list
However the current merging policy does not allow that. It acts more like a zip function. So one can't know if parent was defined or not.
The possible solution would be to rewrite merging policy in the following places:
Anastasia mentioned that there are no cycles available on semantics side. So me or Jelle could pick it up.
Alex, could you let me know your opinion on this?
ae...@google.com <ae...@google.com> #3
I've felt in the past it would be useful to distinguish parent/child and peer merging in merging policy because they are qualitatively different. But if the current idea is to have a global policy where parent always overrides child without customizability, I fear that would break a different set of use cases. For accessibility action labels in aosp/1569421 we explicitly had a parent/child merging in mind. One idea to cover more use cases is to have a second lambda peerPolicy: leftPeer, rightPeer
that would be used for children. It could also be used for the "collapsing" identical semantics on the same layout node, which is an area where we're unable to apply any policy right now, and might eventually come up as a complaint.
But also, more specific to contentDescription
, I noticed yesterday an issue where an innocuous-looking contentDescription
on a higher-level parent destroys a lot of child information in the Owl app: contentDescription
instead of just encouraging them to sprinkle it around whenever they have a useful wording to add.
More generally contentDescription
and text
have been really straining the merging system because of the need to combine both types of properties together in the final TalkBack string and also to provide Text layout rects for select-to-speak. Even the second peerPolicy
lambda would not be enough to actually use the merging system for everything (and avoid additional algorithm using the unmerged tree on the accessibility side). The situation is a bit unfortunate in the sense that the testing side is no longer truly testing what accessibility "sees" but a simulacrum. It's maybe worth biting the bullet and move to a complicated+rich mega-SemanticsProperty containing contentDescription
, text
and text-layout-rects like we've discussed a few times?
pa...@google.com <pa...@google.com> #4
Re: But if the current idea is to have a global policy where parent always overrides child without customizability, I fear that would break a different set of use cases.
=> Nope. The idea is to still have merge policy per property but instead of being defined as (parent, child) -> result
it would be (parent, child[]) -> result
. If that makes sense.
ae...@google.com <ae...@google.com> #5
Ah, okay, I see. I spent an hour thinking hard about this, as while we revisit this we need to fix
Your idea sounded good at first in theory, but one problem with it is that it bakes bottom-up list construction into our API. It's work that will usually be thrown out and it'll be impossible to optimize away. A top-down merging with no mandated allocations is a better fit for most of the SemanticsProperty types. And purely looking at the testing use case, it seems like we could keep things simple because the default merging policy of parent ?: child
would be a good fit (in fact we have seen comma-merging of children is actively problematic with exact match filters).
The comma-merging is indeed not for testing but for accessibility. But here the real complexity comes in because contentDescription
and text
are tangled up from the POV of accessibility. In AndroidComposeViewAccessibilityDelegateCompat.android.kt
we are evolving two quite similar merging algorithms for both text
and contentDescription
neither of which are the ones defined in SemanticsProperty.kt
! It made some sense to let this evolve bugfix by bugfix while we weren't sure of the final accessibility requirements yet, but now that we've seen a lot of use cases it's probably time for a more unified approach. Let me write a short design doc with my current ideas for this and I'll link it here when I'm done.
so...@google.com <so...@google.com> #6
we need to fix
at the same time, and there are a lot of different problems tangled up together here b/157474582
IIUC we also need to have a text layout result associated with the contentDescription
to be able to traverse it similar to text.
so...@google.com <so...@google.com>
ap...@google.com <ap...@google.com> #8
Branch: androidx-main
commit e8f93a08af0f03ccb5e86b0bd157935a609737e8
Author: Filip Pavlis <pavlis@google.com>
Date: Thu Apr 29 16:31:33 2021
Use list for content description and text.
This CL changes semantics properties of contentDescrtiption and text to
be lists instead of single values. This avoids the need having to join
them via comma during merging which erases useful information.
Bug: 184825850
Test: Added
Relnote: "ContentDescription and Text semantics properties are no longer
single values but lists. This enables to merge them as they are instead
of concatenations. Also provided better testing APIs to utilize these
changes"
Change-Id: Ica6bf4236d05b97357c18fb306a6305877a349f7
M compose/foundation/foundation/src/androidAndroidTest/kotlin/androidx/compose/foundation/ImageTest.kt
M compose/foundation/foundation/src/androidAndroidTest/kotlin/androidx/compose/foundation/textfield/TextFieldTest.kt
M compose/material/material/src/androidAndroidTest/kotlin/androidx/compose/material/IconTest.kt
M compose/ui/ui-inspection/src/androidTest/java/androidx/compose/ui/inspection/ParametersTest.kt
M compose/ui/ui-inspection/src/androidTest/java/androidx/compose/ui/inspection/inspector/LayoutInspectorTreeTest.kt
M compose/ui/ui-test/api/1.0.0-beta08.txt
M compose/ui/ui-test/api/current.ignore
M compose/ui/ui-test/api/current.txt
M compose/ui/ui-test/api/public_plus_experimental_1.0.0-beta08.txt
M compose/ui/ui-test/api/public_plus_experimental_current.txt
M compose/ui/ui-test/api/restricted_1.0.0-beta08.txt
M compose/ui/ui-test/api/restricted_current.ignore
M compose/ui/ui-test/api/restricted_current.txt
M compose/ui/ui-test/src/androidAndroidTest/kotlin/androidx/compose/ui/test/AssertsTest.kt
M compose/ui/ui-test/src/androidAndroidTest/kotlin/androidx/compose/ui/test/ErrorMessagesTest.kt
M compose/ui/ui-test/src/androidAndroidTest/kotlin/androidx/compose/ui/test/FindersTest.kt
M compose/ui/ui-test/src/androidAndroidTest/kotlin/androidx/compose/ui/test/PrintToStringTest.kt
A compose/ui/ui-test/src/androidAndroidTest/kotlin/androidx/compose/ui/test/assertions/AssertContentDescription.kt
A compose/ui/ui-test/src/androidAndroidTest/kotlin/androidx/compose/ui/test/assertions/AssertText.kt
M compose/ui/ui-test/src/commonMain/kotlin/androidx/compose/ui/test/Assertions.kt
M compose/ui/ui-test/src/commonMain/kotlin/androidx/compose/ui/test/Filters.kt
M compose/ui/ui/api/1.0.0-beta08.txt
M compose/ui/ui/api/current.ignore
M compose/ui/ui/api/current.txt
M compose/ui/ui/api/public_plus_experimental_1.0.0-beta08.txt
M compose/ui/ui/api/public_plus_experimental_current.txt
M compose/ui/ui/api/restricted_1.0.0-beta08.txt
M compose/ui/ui/api/restricted_current.ignore
M compose/ui/ui/api/restricted_current.txt
M compose/ui/ui/src/androidAndroidTest/kotlin/androidx/compose/ui/AndroidAccessibilityTest.kt
M compose/ui/ui/src/androidAndroidTest/kotlin/androidx/compose/ui/semantics/SemanticsTests.kt
M compose/ui/ui/src/androidMain/kotlin/androidx/compose/ui/platform/AndroidComposeViewAccessibilityDelegateCompat.android.kt
M compose/ui/ui/src/commonMain/kotlin/androidx/compose/ui/semantics/SemanticsProperties.kt
pa...@google.com <pa...@google.com> #9
This was now fixed by using lists for content description & text. Test API's are now a bit smarter as they can use the lists.
This changes is breaking but was pre-approved in
I did several experiments locally and also waited for our CI to trigger any alerts afterwards and concluded that this change had no measurable impact on our benchmarks.
so...@google.com <so...@google.com> #10
Oops, I kept this ticket to track a11y work that we're currently doing. I'll create a new one :)
pa...@google.com <pa...@google.com> #11
Ops, sorry about that.
Description
In changed changing testing behavior. For example this test no longer passes:
beta04
semantics mergingComposable
With the changes in
beta04
theIcon
s content description is no longer merged to the parentFloatingActionButton
so it does not appear in the merged semantics tree.