navigation-compose 2.4.0-alpha05 does not keep a subgraph's BackStackEntry until the transition animation completes [194313238]

Fixed

Bug

adexe s nau

Status Update

No update yet.

Description

ma...@gmail.com

created issue #1

Jul 22, 2021 02:28PM

Component used: navigation-compose Version used: 2.4.0-alpha05

Steps to reproduce:

Create a new compose project using Android Studio Arctic Fox rc01 wizard.
Replace the code below in MainActivity
Run the app, click the button "Go to B" then click the button "Go back".

Ref to Slack convo: https://kotlinlang.slack.com/archives/CJLTWPH7S/p1626963158280100?thread_ts=1626960240.270300&cid=CJLTWPH7S

Sample code to reproduce the issue:

class MainActivity : ComponentActivity() {
    override fun onCreate(savedInstanceState: Bundle?) {
        super.onCreate(savedInstanceState)
        setContent {
            val navController = rememberNavController()
            NavHost(
                navController = navController,
                startDestination = "a",
                modifier = Modifier.fillMaxSize(),
                route = "mainGraph",
            ) {
                composable(route = "a") {
                    ScreenA(navToB = { navController.navigate("b") })
                }
                navigation(
                    startDestination = "b",
                    route = "subGraph",
                ) {
                    composable(route = "b") {
                        val vm = viewModel<SomeViewModel>(
                            viewModelStoreOwner = navController.getBackStackEntry("subGraph")
                        )
                        ScreenB(navBack = { navController.navigateUp() })
                    }
                }
            }
        }
    }
}

@Composable
fun ScreenA(
    navToB: () -> Unit
) {
    Column {
        Text("I am screen A")
        Button(onClick = navToB) {
            Text("Go to B")
        }
    }
}

@Composable
fun ScreenB(
    navBack: () -> Unit
) {
    Column {
        Text("I am screen B")
        Button(onClick = navBack) {
            Text("Go back")
        }
    }
}

Comments

il...@google.com <il...@google.com> Jul 22, 2021 06:18PM

Assigned to jb...@google.com.

ma...@gmail.com <ma...@gmail.com> #2Aug 5, 2021 06:54AM

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

ma...@gmail.com <ma...@gmail.com> #3Aug 5, 2021 06:56AM

This is still happening with navigation-compose 2.4.0-alpha06

ap...@google.com <ap...@google.com> #4Aug 24, 2021 11:10PM

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

commit d7af5243adb70d92a0104005ff353c83933c97ed
Author: Jeremy Woods <jbwoods@google.com>
Date: Mon Aug 23 22:03:09 2021

Ensure NavGraph Navigators are destroyed properly

When navigating with transitions, we properly hold destination entries
until the transition completes, but not for NavGraph entries. This is a
particular issue for popping since it results in NavGraph entries will
be moved to DESTROYED before their child destinations.

We need to make NavGraph back stack entries keep the maximum lifecycle
state of all of the entries in the graph. This means that as long as one
entry in the graph is CREATED, the graph itself will be CREATED and only
after all entries have been DESTROYED, will the NavGraph entry also be
destroyed.

RelNote: "Using getBackStackEntry and previousBackStackEntry inside
composable(), in conjuction with remember() will no longer cause an
exception for no destination being on the back stack."
Test: add new test
Bug: 194313238

Change-Id: I75138d24d27dac83b5301507161578ac811454e3

M navigation/navigation-compose/src/androidTest/java/androidx/navigation/compose/NavHostTest.kt
M navigation/navigation-runtime/src/androidTest/java/androidx/navigation/NavBackStackEntryLifecycleTest.kt
M navigation/navigation-runtime/src/main/java/androidx/navigation/NavController.kt

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

jb...@google.com <jb...@google.com> #5Aug 26, 2021 11:31PM

Marked as fixed.

This has been fixed internally and will be available in the Navigation 2.4.0-alpha08 release.

This also includes a lint check to ensure you surround any calls to getBackStackEntry within composition with a remember, which you should always be doing anyway, and that is the only way this actually works.

ri...@gmail.com <ri...@gmail.com> #6Sep 27, 2021 11:01PM

I don't understand why and how do we need to surround getBackStackEntry with a remember.

Even the docs does not do that https://developer.android.com/jetpack/compose/libraries#hilt-navigation

Issue 194313238