Navigation should prioritize duplicate destinations from the current graph [352006850]

Fixed

Bug

Status Update

No update yet.

Description

do...@google.com

created issue #1

Jul 9, 2024 12:35PM

Component used: Navigation Version used: 2.8.0-beta04 Devices/Android versions reproduced on: all

Duplicate destinations in a navigation graph are allowed but can lead to indeterminate behavior at run time. Consider the following code which creates two nested nav graphs that share a destination: DetailRoute.

@Serializable data object HomeNavRoute
@Serializable data object HomeRoute
@Serializable data object ListNavRoute
@Serializable data object ListRoute 
@Serializable data class DetailRoute(val id: Int)

@Composable
fun MyNavHost(navController: NavHostController) {
    Column(modifier = Modifier.fillMaxHeight()) {
        NavHost(
            navController = navController,
            startDestination = HomeNavRoute
        ) {
            navigation<ListNavRoute>(startDestination = ListRoute) {
                composable<ListRoute> {
                    ListScreen(onClickItem = { 
                        navController.navigate(route = DetailRoute(id = 123)) 
                    })
                }
                composable<DetailRoute> { backStackEntry -> 
                    DetailScreen(
                        id = backStackEntry.toRoute<DetailRoute>().id,
                        parent = backStackEntry.destination.parent) 
                }
            }
            navigation<HomeNavRoute>(startDestination = HomeRoute) {
                composable<HomeRoute> {
                    HomeScreen(onClickItem = {
                        navController.navigate(route = DetailRoute(id = 456))
                    })
                }
                composable<DetailRoute> { backStackEntry ->
                    DetailScreen(
                        id = backStackEntry.toRoute<DetailRoute>().id,
                        parent = backStackEntry.destination.parent)
                }
            }
        }
    }
}

@Composable
private fun HomeScreen(onClickItem: () -> Unit) {
    Column {
        Text("Home screen")
        Button(onClick = onClickItem){
            Text("Go to detail screen")
        }
    }
}

@Composable
fun ListScreen(onClickItem: () -> Unit){
    Column {
        Text("I am a list screen")
        Button(onClick = onClickItem){
            Text("Go to detail screen")
        }
    }
}

@Composable
fun DetailScreen(id: Int, parent: NavDestination? = null) {
    Text("I am a detail screen with ID $id, my parent is $parent")
}

If you navigate from ListRoute to DetailRoute it is undetermined whether the resulting destination will have a parent of HomeNavRoute or ListNavRoute. If we follow the rule of "whichever destination is defined first in the navigation graph will be used" then destination with parent ListNavRoute should be used, however, in my tests, DetailRoute's parent is HomeNavRoute (which is defined second in the nav graph).

Perhaps adding a duplicate destination should be a compile-time error or warning.

Comments

il...@google.com <il...@google.com> #2Jul 25, 2024 11:45PM

Assigned to cl...@google.com.

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

ap...@google.com <ap...@google.com> #3Jul 31, 2024 10:14PM

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

commit 52f5c98ca6820cdeb8cfb071278929746ce7d527
Author: Clara Fok <clarafok@google.com>
Date: Wed Jul 31 13:49:58 2024

Add matchDeepLinkComprehensive helper

Refactor current uses of matchDeepLink to leverage new internal helper

Test: existing tests
Bug: 352006850
Change-Id: I48964c4d83bc62fcdc0ff995597e4289a0f7ac19

M navigation/navigation-common/src/main/java/androidx/navigation/NavDestination.kt
M navigation/navigation-common/src/main/java/androidx/navigation/NavGraph.kt

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

ap...@google.com <ap...@google.com> #4Jul 31, 2024 10:39PM

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

commit f397b74f2c8620c3ecd33253af117b6246931d1d
Author: Clara Fok <clarafok@google.com>
Date: Wed Jul 31 14:31:23 2024

Refactor navigate to use matchDeepLinkComprehensive

Now when we search for navigate target, instead of starting from base graph, we start from the closest parent NavGraph of current destination. This ensures that when navigating to shared/duplicated destinations, the one closest to the current destination will be prioritized.

Matching that only needs to recurse downwards (match children) will keep using the existing matchDeepLink function. Matching that needs to also recurse up the parents will directly call matchDeepLinkComprehensive. The latter has only been applied to uses cases where we used to match starting from _graph.

Test: ./gradlew navigation:navigation-runtime:cC
Bug: 352006850
Relnote: "When navigating to duplicate or shared destinations, navigation will prioritize the destination from current graph"
Change-Id: Ic89a41d2a54f36fca6c41f2f7ea948f8c13b2bd5

M navigation/navigation-common/src/main/java/androidx/navigation/NavGraph.kt
M navigation/navigation-runtime/src/androidTest/java/androidx/navigation/NavControllerRouteTest.kt
M navigation/navigation-runtime/src/main/java/androidx/navigation/NavController.kt

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

cl...@google.com <cl...@google.com> #5Aug 1, 2024 12:02AM

Marked as fixed.

Fixed internally and will be available in navigation 2.8.0-beta07.

pr...@google.com <pr...@google.com> #6Aug 13, 2024 06:58PM

The following release(s) address this bug.It is possible this bug has only been partially addressed:

androidx.navigation:navigation-common:2.8.0-beta07
androidx.navigation:navigation-runtime:2.8.0-beta07

Issue 352006850