Semantics not being merged in dev15 [161979921]

Fixed

Bug

Status Update

No update yet.

Description

ja...@google.com

created issue #1

Jul 23, 2020 03:15PM

In dev15, some tests are broken in Jetnews as a result of semantics not being merged anymore.

For example Box(Modifier.clickable) { Text("This text does not appear on the tree") }

The workaround is to useUnmergedTree = true

@Test
fun app_opensArticle() {
onRoot(useUnmergedTree = true).printToLog("jetnews")
onAllNodes(hasSubstring("Manuel Vivo"), useUnmergedTree = true)[0].performClick()
onAllNodes(hasSubstring("3 min read"), useUnmergedTree = true)[0].assertIsDisplayed()
}

Filip believes the regression happened in aosp/1360099

Comments

pa...@google.com <pa...@google.com> #2Jul 23, 2020 03:43PM

Project: platform/frameworks/support
Branch: androidx-master-dev

commit c60f33e229e31ab328ef6b59dab63b264954831c
Author: Alexandre Elias <aelias@google.com>
Date: Fri Jul 10 16:23:09 2020

Semantics no-op cleanups

Partly in response to lmr's broad code review, I did a pass of
superficial API/implementation cleanup. The main changes are:

- I changed each Boolean SemanticsProperty where false is equivalent to
not being present to take "Unit" instead. This is conceptually
clearer: it avoids questions like "can I cancel out a semantics from a
merged child by setting it to false?" Because "property = Unit" looks
weird, I also changed the style of these to "property()".

- I moved the Semantics id generator closer to where it's used, in
SemanticsModifierCore. I made it internal and an AtomicInt.
(Note that integer ids are heavily used in the Android
AccessibilityNodeInfo APIs so I can't simply remove them entirely.)

- I deleted dead code. Some examples include SemanticsHintOverrides,
a public API not connected to anything, and SemanticsPropertyKey
merge() open method which is never called. (In both cases I have
a different plan in mind for accessibility.)

Fixes: 145951226
Fixes: 145955412
Test: existing tests
Relnote: "Single-value semantics properties now use a calling style.
For example, 'semantics { hidden = true }' is now written as:
'semantics { hidden() }'."

Change-Id: Ic1afd12ea22c926babc9662f1804d80b33aa0cfc

M ui/integration-tests/benchmark/src/androidTest/java/androidx/ui/benchmark/test/LayoutNodeModifierBenchmark.kt
M ui/ui-core/api/0.1.0-dev15.txt
M ui/ui-core/api/current.txt
M ui/ui-core/api/public_plus_experimental_0.1.0-dev15.txt
M ui/ui-core/api/public_plus_experimental_current.txt
M ui/ui-core/api/restricted_0.1.0-dev15.txt
M ui/ui-core/api/restricted_current.txt
M ui/ui-core/src/androidAndroidTest/kotlin/androidx/ui/graphics/vector/VectorTest.kt
M ui/ui-core/src/androidAndroidTest/kotlin/androidx/ui/semantics/SemanticsTests.kt
M ui/ui-core/src/androidMain/kotlin/androidx/ui/core/AndroidActuals.kt
M ui/ui-core/src/androidMain/kotlin/androidx/ui/core/AndroidComposeView.kt
M ui/ui-core/src/androidMain/kotlin/androidx/ui/core/AndroidComposeViewAccessibilityDelegateCompat.kt
M ui/ui-core/src/androidMain/kotlin/androidx/ui/core/AndroidPopup.kt
M ui/ui-core/src/commonMain/kotlin/androidx/ui/core/Expect.kt
M ui/ui-core/src/commonMain/kotlin/androidx/ui/core/semantics/SemanticsConfiguration.kt
D ui/ui-core/src/commonMain/kotlin/androidx/ui/core/semantics/SemanticsHintOverrides.kt
M ui/ui-core/src/commonMain/kotlin/androidx/ui/core/semantics/SemanticsModifier.kt
M ui/ui-core/src/commonMain/kotlin/androidx/ui/core/semantics/SemanticsNode.kt
M ui/ui-core/src/commonMain/kotlin/androidx/ui/core/semantics/SemanticsOwner.kt
M ui/ui-core/src/commonMain/kotlin/androidx/ui/core/semantics/SemanticsWrapper.kt
M ui/ui-core/src/commonMain/kotlin/androidx/ui/semantics/SemanticsProperties.kt
M ui/ui-foundation/api/0.1.0-dev15.txt
M ui/ui-foundation/api/current.txt
M ui/ui-foundation/api/public_plus_experimental_0.1.0-dev15.txt
M ui/ui-foundation/api/public_plus_experimental_current.txt
M ui/ui-foundation/api/restricted_0.1.0-dev15.txt
M ui/ui-foundation/api/restricted_current.txt
M ui/ui-foundation/src/main/java/androidx/ui/foundation/Clickable.kt
M ui/ui-foundation/src/main/java/androidx/ui/foundation/Dialog.kt
M ui/ui-foundation/src/main/java/androidx/ui/foundation/Scroller.kt
M ui/ui-foundation/src/main/java/androidx/ui/foundation/selection/Selectable.kt
M ui/ui-foundation/src/main/java/androidx/ui/foundation/selection/Toggleable.kt
M ui/ui-foundation/src/main/java/androidx/ui/foundation/semantics/FoundationSemanticsProperties.kt
M ui/ui-material/src/androidTest/java/androidx/ui/material/ButtonTest.kt
M ui/ui-material/src/androidTest/java/androidx/ui/material/CardTest.kt
M ui/ui-material/src/androidTest/java/androidx/ui/material/CheckboxScreenshotTest.kt
M ui/ui-material/src/androidTest/java/androidx/ui/material/RadioButtonScreenshotTest.kt
M ui/ui-material/src/androidTest/java/androidx/ui/material/ScaffoldTest.kt
M ui/ui-material/src/androidTest/java/androidx/ui/material/SnackbarTest.kt
M ui/ui-material/src/androidTest/java/androidx/ui/material/SurfaceTest.kt
M ui/ui-material/src/androidTest/java/androidx/ui/material/ripple/RippleIndicationTest.kt
M ui/ui-material/src/androidTest/java/androidx/ui/material/textfield/TextFieldScreenshotTest.kt
M ui/ui-material/src/main/java/androidx/ui/material/AppBar.kt
M ui/ui-material/src/main/java/androidx/ui/material/TextFieldImpl.kt
M ui/ui-test/src/androidTest/java/androidx/ui/test/AssertsTest.kt
M ui/ui-test/src/androidTest/java/androidx/ui/test/CallSemanticsActionTest.kt
M ui/ui-test/src/androidTest/java/androidx/ui/test/ErrorMessagesTest.kt
M ui/ui-test/src/androidTest/java/androidx/ui/test/FindersTest.kt
M ui/ui-test/src/androidTest/java/androidx/ui/test/PrintToStringTest.kt
M ui/ui-test/src/androidTest/java/androidx/ui/test/ScrollToTest.kt
M ui/ui-test/src/androidTest/java/androidx/ui/test/TextActionsTest.kt
M ui/ui-test/src/main/java/androidx/ui/test/Actions.kt
M ui/ui-test/src/main/java/androidx/ui/test/Filters.kt
M ui/ui-text/api/0.1.0-dev15.txt
M ui/ui-text/api/current.txt
M ui/ui-text/api/public_plus_experimental_0.1.0-dev15.txt
M ui/ui-text/api/public_plus_experimental_current.txt
M ui/ui-text/api/restricted_0.1.0-dev15.txt
M ui/ui-text/api/restricted_current.txt
M ui/ui-text/src/commonMain/kotlin/androidx/ui/text/CoreTextField.kt
M ui/ui-text/src/commonMain/kotlin/androidx/ui/text/TextSemanticsProperties.kt

https://android-review.googlesource.com/1360099

ae...@google.com <ae...@google.com> Jul 23, 2020 08:42PM

Accepted by ae...@google.com.

ae...@google.com <ae...@google.com> #3Jul 27, 2020 08:34PM

I'm working on it this week. Note that the culprit is actually the big substantive patch I landed on Jul 3, http://aosp/1354246

ae...@google.com <ae...@google.com> #4Jul 29, 2020 07:19AM

I started a WIP patch to allow non-unique keys: http://aosp/1375563 . It doesn't fix any bug so far because everything on the semantics client side (accessibility, tests) is just taking the first key/value pair, reproducing the old behavior. Semantics tests pass, next I'll check nothing else is broken and then make it work as expected with duplicate TestTags and Text semantics especially.

ap...@google.com <ap...@google.com> #5Aug 11, 2020 11:23PM

Marked as fixed.

Project: platform/frameworks/support
Branch: androidx-master-dev

commit 1531c13353d4caa1aa6a8f41a58adf67d911531f
Author: Alexandre Elias <aelias@google.com>
Date: Fri Aug 07 20:03:55 2020

Implement per-property merge policy.

This changes the current merging behavior into the default
implementation of an open function called "merge()", and adds different
implementations for the following:
- Text and Label: concatenates strings with comma in between (same as
TalkBack's native merging behavior). This behavior will probably
evolve further in future patches to merge text layout for accessibility.
- TestTag and Hidden: does not merge at all.
- Popup and Dialog: throws an exception.

Note that the latter two behaviors require carefully distinguishing
between collapsing and merging so that, for example, collapsed TestTags
are not dropped as well.

Also switch ListItem to have mergeAllDescendants (should've had that
before anyway), which has more multiple-Text stress tests than
Button/Toggleable. As an example of how TalkBack behaves with a two-line
ListItem, see

https://screenshot.googleplex.com/06KZaX6VfsG

Fixes: 161979921
Test: depthFirstLabelConcat, clickableTest_clickOnChildText
Relnote: "Added mergePolicy lambda to SemanticsPropertyKey. This can be
used to define a custom policy for mergeAllDescendants semantics
merging. The default policy is to use the parent value if already
present, otherwise the child value."

Change-Id: Iaf6c4cc327017ee492f4d8334c8df5167d33df58

M ui/ui-core/api/current.txt
M ui/ui-core/api/public_plus_experimental_current.txt
M ui/ui-core/api/restricted_current.txt
M ui/ui-core/src/androidAndroidTest/kotlin/androidx/compose/ui/semantics/SemanticsTests.kt
M ui/ui-core/src/commonMain/kotlin/androidx/compose/ui/semantics/SemanticsConfiguration.kt
M ui/ui-core/src/commonMain/kotlin/androidx/compose/ui/semantics/SemanticsNode.kt
M ui/ui-core/src/commonMain/kotlin/androidx/compose/ui/semantics/SemanticsProperties.kt
M ui/ui-core/src/commonMain/kotlin/androidx/compose/ui/semantics/SemanticsWrapper.kt
M ui/ui-foundation/src/androidAndroidTest/kotlin/androidx/compose/foundation/ClickableTest.kt
M ui/ui-material/integration-tests/material-demos/src/main/java/androidx/compose/material/demos/ButtonDemo.kt
M ui/ui-material/src/androidAndroidTest/kotlin/androidx/compose/material/TabTest.kt
M ui/ui-material/src/commonMain/kotlin/androidx/compose/material/ListItem.kt

https://android-review.googlesource.com/1394332

Issue 161979921