AndroidView does not set ViewTree*Owners from composition locals [179708470]

Fixed

Bug

Status Update

No update yet.

Description

za...@gmail.com

created issue #1

Feb 9, 2021 08:14PM

Jetpack Compose release version: 1.0.0-alpha11

Android Studio Build:

Android Studio Arctic Fox | 2020.3.1 Canary 4
Build #AI-202.7319.50.2031.7049475, built on December 23, 2020
Runtime version: 11.0.8+10-b944.6842174 x86_64
VM: OpenJDK 64-Bit Server VM by N/A
macOS 10.16
GC: G1 Young Generation, G1 Old Generation
Memory: 16384M
Cores: 16
Registry: analyze.exceptions.on.the.fly=true, external.system.auto.import.disabled=true, debugger.watches.in.variables=false, vcs.log.index.git=off, caches.indexerThreadsCount=16
Non-Bundled Plugins: carbon-now-sh, com.intellij.marketplace, com.squareup.android.library.generator, org.jetbrains.kotlin

This requires a bit of setup to reproduce, and there are a number of issues that require similar setup, so I have created a dedicated repo that demonstrates and has UI tests for them here.

The README in the root of that repo describes the setup, the issues, and the fixes to them that make sense to me.

These issues will probably affect the Compose integration for the Jetpack Navigation library, as well as other navigation libraries that are written in Compose.

Comments

an...@google.com <an...@google.com> Feb 9, 2021 08:20PM

Assigned to an...@google.com.

za...@gmail.com <za...@gmail.com> #2Feb 10, 2021 12:49AM

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

za...@gmail.com <za...@gmail.com> #3Aug 26, 2021 08:59PM

It looks like at least some of these issues are still present in 1.0.1, although I need to update the repro project to verify. I’ve been chatting with Ian about this on the Kotlin Slack. He said it seems like something in Wrapper.android.it might need to be changed, but I’m not sure what.

Ian pointed out that composition locals that are set by a parent composition are generally visible to/inherited by subcompositions, even if there are Android views in between. However, in this particular case, that does not matter because the Wrapper will always explicitly set the values of the lifecycle and state registry locals to the values it got from the view tree, overriding any values coming in from the parent composition.

As far as I can tell from reading the code myself, I think the solution is just that AndroidView needs to set the ViewTreeLifecycleOwner and ViewTreeSavedStateRegistryOwner from the LocalLifecycleOwner and LocalSavedStateRegistry, respectively, on the Android view it creates. Then, any descendant views will see the correct owners if they were overridden in the composition, and any nested compositions will also get those owners and set them as the local values for their compositions.

an...@google.com <an...@google.com> Aug 27, 2021 10:29AM

Reassigned to po...@google.com.

za...@gmail.com <za...@gmail.com> #4Sep 6, 2021 04:55PM

Confirming the issue affects Jetpack Navigation as well.

I just updated the repro project to Compose 1.0.1 and added a demonstration of the issue affecting Jetpack Navigation.

po...@google.com <po...@google.com> Sep 7, 2021 05:34PM

Reassigned to kl...@google.com.

kl...@google.com <kl...@google.com> Sep 13, 2021 08:37PM

Accepted by kl...@google.com.

kl...@google.com <kl...@google.com> #5Sep 17, 2021 06:00PM

Started working on this, but it looks like navigation-compose might need a change as well.

kl...@google.com <kl...@google.com> #6Sep 23, 2021 04:36PM

Filed b/200953430 to track the issue in Jetpack Navigation.

ap...@google.com <ap...@google.com> #7Oct 7, 2021 12:50AM

Marked as fixed.

Project: platform/frameworks/support
Branch: androidx-main

commit 11a26697f42c3a6d4a248eb35f52d2c26bd6d9cf
Author: Zach Klippenstein <klippenstein@google.com>
Date: Thu Sep 16 16:46:51 2021

Propagate local owners as view tree owners in AndroidView.

Propagates the LocalLifecycleOwner and LocalSavedStateRegistryOwner from
an AndroidView to the managed View by setting ViewTreeLifecycleOwner and
ViewTreeSavedStateRegistryOwner, respectively, on the AndroidViewHolder.
This ensures that, if something in the composition (e.g. a navigation
library like Jetpack Navigation Compose) sets those locals to
owners that are different from the ones used to create the composition
itself, any Android Views nested beneath them will see those owners and
not the ones used by the composition.

Test: Added tests to AndroidViewTest
Fixes:

b/179708470

Relnote: "AndroidView now propagates LocalLifecycleOwner and
LocalSavedStateRegistryOwner to its view via ViewTreeLifecycleOwner
and ViewTreeSavedStateRegistryOwner."

Change-Id: I38f966576085db66e35b0106f49275ef5bb31adc

M compose/ui/ui/src/androidAndroidTest/kotlin/androidx/compose/ui/viewinterop/AndroidViewTest.kt
M compose/ui/ui/src/androidMain/kotlin/androidx/compose/ui/platform/AndroidCompositionLocals.android.kt
M compose/ui/ui/src/androidMain/kotlin/androidx/compose/ui/viewinterop/AndroidViewHolder.android.kt
M compose/ui/ui/src/androidMain/kotlin/androidx/compose/ui/viewinterop/AndroidView.android.kt

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