Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Navigation Animation] Control composable route zIndex #1160

Closed
PhilipDukhov opened this issue May 11, 2022 · 23 comments · Fixed by #1384
Closed

[Navigation Animation] Control composable route zIndex #1160

PhilipDukhov opened this issue May 11, 2022 · 23 comments · Fixed by #1384
Assignees

Comments

@PhilipDukhov
Copy link

PhilipDukhov commented May 11, 2022

I'm trying to implement modal transition: new screen slides in from bottom and slides out back.

Expected behaviour:

Slide in animation works fine, but slide out animation doesn't - because appearing view seems to have higher zIndex, no matter wether it is pushing or popping.

Actual behaviour:

I've tried specifying it with a modifier, it didn't worked out. In my case I need zIndex to be equal to backstack index, in other words during pop disappearing view should have higher zIndex, than the appearing one.

object Destinations {
    const val Login = "login"
    const val Home = "home"
}

@Composable
fun NavTestScreen(
) {
    val navController = rememberAnimatedNavController()
    AnimatedNavHost(navController = navController,
        startDestination = Destinations.Login,
        modifier = Modifier.fillMaxSize()) {
        composable(
            Destinations.Login,
            enterTransition = { NavigationTransition.IdentityEnter },
            exitTransition = { NavigationTransition.IdentityExit },
        ) {
            Button(onClick = {
                navController.navigate(Destinations.Home)
            }) {
                Text(text = "Next")
            }
        }
        composable(
            route = Destinations.Home,
            enterTransition = { NavigationTransition.slideInBottomAnimation },
            exitTransition = { NavigationTransition.slideOutBottomAnimation },
        ) {
            Button(
                onClick = {
                    navController.popBackStack()
                },
                colors = ButtonDefaults.buttonColors(backgroundColor = Color.Yellow),
                modifier = Modifier.zIndex(100f)
            ) {
                Text(text = "label")
            }
        }
    }
}

object NavigationTransition {
    private val animation: FiniteAnimationSpec<IntOffset> = tween(
        easing = LinearOutSlowInEasing,
        durationMillis = 2000,
    )

    val IdentityEnter = slideInVertically(
        initialOffsetY = {
            -1 //fix for https://github.com/google/accompanist/issues/1159
        },
        animationSpec = animation
    )

    val IdentityExit = slideOutVertically(
        targetOffsetY = {
            -1 //fix for https://github.com/google/accompanist/issues/1159
        },
        animationSpec = animation
    )

    var slideInBottomAnimation =
        slideInVertically(initialOffsetY = { fullHeight -> fullHeight }, animationSpec = animation)

    var slideOutBottomAnimation =
        slideOutVertically(targetOffsetY = { fullHeight -> fullHeight }, animationSpec = animation)
}
@PhilipDukhov
Copy link
Author

PhilipDukhov commented May 15, 2022

I tried implementing this myself: I was able to calculate the right zIndex using navController.backQueue in AnimatedNavHost for the current entry, but if I am not mistaken, this should be applied to the internal AnimatedVisibility for each currentlyVisible item used inside Transition<S>.AnimatedContent - it works as I expect with this "fork", but the fork of androidx.compose.animation is something I would rather not do.

Am I missing something? I case I'm not - I've created a feature request on google issue tracker.

@jbw0033 jbw0033 pinned this issue Jun 13, 2022
@jbw0033 jbw0033 unpinned this issue Jun 13, 2022
@jbw0033
Copy link
Collaborator

jbw0033 commented Jun 13, 2022

Want to add in @doris4lt to give her more context on the goal here.

@doris4lt
Copy link
Collaborator

I think the functionality that you are asking for can be achieved with targetContentZIndex using AnimatedContent. Here's an example: https://cs.android.com/androidx/platform/frameworks/support/+/androidx-main:compose/animation/animation/samples/src/main/java/androidx/compose/animation/samples/AnimatedContentSamples.kt;l=242

Could you give it a try so we know whether we need to add more support for zIndex in AnimatedContent, or surface that support in navigation APIs?

@PhilipDukhov
Copy link
Author

PhilipDukhov commented Jun 14, 2022

Thanks, @doris4lt, targetContentZIndex is exactly what I was looking for!

Not sure what is the ideal API to solve this issue - in my project I just check the index in backQueue, not sure if this is what everyone else expects from this library, or if it should be more customizable.

@doris4lt
Copy link
Collaborator

@PhilipDukhov How would you like to customize the target zIndex? We could potentially support the target zIndex through Modifier.zIndex as an alternative. Or maybe you have other suggestions?

@PhilipDukhov
Copy link
Author

PhilipDukhov commented Jun 15, 2022

@doris4lt I'm using the following code in AnimatedNavHost right now:

targetContentZIndex = navController.backQueue
    .indexOf(targetState)
    .takeIf { it >= 0 }
    ?.toFloat()
    ?: Float.MAX_VALUE

I guess that the most flexible solution is to allow customizations of ContentTransform.

I actually resolve all my animations in AnimatedNavHost, because I find it quite difficult to do it in composable: I have different animations depending on the screen, and the animation actually depends not on each composable route, but on targetState/initialState depending on push/pop.

@doris4lt
Copy link
Collaborator

Have you considered

targetContentZIndex = navController.backQueue
    .indexOf(targetState)
    .takeIf { it >= 0 }
    ?.toFloat()
    ?: navController.backQueue.size

Could you give me a fanfic example of how you would like to customize ContentTransform, assuming it was supported?

@PhilipDukhov
Copy link
Author

PhilipDukhov commented Jun 19, 2022

@doris4lt For my needs targetContentZIndex seems sufficient, I can't think of any use cases for ContentTransform.

But I ran into another problem with targetContentZIndex.

Let's say I have several views in the stack. And I want to replace the stack with a new single view using slide animation. I use popUpTo as follows:

navController.navigate(route = "newRoute") {
    popUpTo(0)
}

The problem is that targetContentZIndex is only called for the new destination, and after removing the old routes from the stack navController.backQueue has only one element, so my new view has zIndex = 1 and the disappearing view has zIndex 2, which was calculated when it appeared.

Is there any way to recalculate targetContentZIndex for the disappearing view, or maybe you can think of a better way instead of checking navController.backQueue?

@x4080
Copy link

x4080 commented Jun 25, 2022

Hi, I get the same problem with zindex using slideInHorizontally, but I dont get how to set the variable targetContentZIndex , it just shows "Unresolved reference: targetContentZIndex"

@PhilipDukhov Can you kindly give a hint how you do it ? Thanks

@PhilipDukhov
Copy link
Author

PhilipDukhov commented Jun 26, 2022

@x4080 have you checked official sample on https://cs.android.com/androidx/platform/frameworks/support/+/androidx-main:compose/animation/animation/samples/src/main/java/androidx/compose/animation/samples/AnimatedContentSamples.kt;l=242?

Until this issue is resolved, you have to create a fork or copy source code to your project to make this change in AnimatedNavHost.kt.

@PhilipDukhov
Copy link
Author

Hi @doris4lt, can you confirm that the problem from my last comment is clear, or should I provide a minimal reproducible example with a video and more details?

@x4080
Copy link

x4080 commented Jun 28, 2022

@PhilipDukhov thanks, I did what you describe above and works, I think I modified inside push and pop to inc or dec z-index

I wondered why its not default :)

@doris4lt
Copy link
Collaborator

doris4lt commented Jul 2, 2022

@PhilipDukhov Thanks for the example. Sorry I didn't see the question until just now.

Is there any way to recalculate targetContentZIndex for the disappearing view

The intention is not to allow the zIndex to change for the content that is already on screen, to avoid a sudden jump. This could happen especially when you interrupt an animation, e.g. when going from destination A -> B, user changes their mind to go to C. Now you have both A and B exiting, while C entering. If we allow B's zIndex to be changed at the point of interruption, it could potentially jump above or below A.

With that said, we are looking for better ways to support zIndex customization without introducing a foot gun. We just haven't found it yet.

If I understand your use case correctly, its spacial model isn't completely dictated by the back queue. More specifically, in the example of popupTo(0) that you shared, you'd like the new content (for the bottom of the stack) to slide in with a higher zIndex.

Could you elaborate on the ideal z-order that you'd like to see for the content in the stack? Seems like there are special cases on top of a more general rule, which is perfectly valid. But I'd like to have a better understanding of the overall requirements.

@PhilipDukhov
Copy link
Author

@doris4lt I found a way to calculate it for my particular case using an auxiliary array, so being able to change targetContentZIndex seems completely sufficient to close this problem.

@doris4lt
Copy link
Collaborator

doris4lt commented Jul 8, 2022

@PhilipDukhov
If you don't mind, could you share a snippet of that solution? I'd love to understand how you did it.

@PhilipDukhov
Copy link
Author

PhilipDukhov commented Jul 8, 2022

@doris4lt Sure, it looks like this:

val allEntryIds = rememberSaveable { mutableListOf<String>() }

navController.backQueue
	.map(NavBackStackEntry::id)
	.filterNot(allEntryIds::contains)
	.forEach(allEntryIds::add)

val transition = updateTransition(backStackEntry, label = "entry")

transition.AnimatedContent(
	modifier,
	transitionSpec = {
		(finalEnter(this) with finalExit(this))
			.apply {
				targetContentZIndex = allEntryIds.indexOf(targetState.id).toFloat()
			}
	},

@serhii-pokrovskyi
Copy link

serhii-pokrovskyi commented Jul 11, 2022

What we do need here is the ability to manipulate witch state will be "on top".
Something like:

exitTransition = {
    toBeOnTop(this.targetState) // or toBeOnTop(this.initialState)
    
    // just an example of animation
    slideIntoContainer(
        towards = AnimatedContentScope.SlideDirection.Left,
        animationSpec = tween(navAnimationDuration)
    )
}

or make NavController api work with compose

navController.navigate("route") {
    anim {
        //todo: define animations here
    }
}

or make ZIndex of a destination equal to destination index in NavController.backQueue

@github-actions
Copy link

This issue is stale because it has been open 30 days with no activity. Remove stale label or comment or this will be closed in 5 days.

@github-actions github-actions bot added the stale Stale issues which are marked for closure label Aug 22, 2022
@PhilipDukhov
Copy link
Author

It's still relevant

@github-actions github-actions bot removed the stale Stale issues which are marked for closure label Aug 23, 2022
@github-actions
Copy link

This issue is stale because it has been open 30 days with no activity. Remove stale label or comment or this will be closed in 5 days.

@github-actions github-actions bot added the stale Stale issues which are marked for closure label Sep 22, 2022
@PhilipDukhov
Copy link
Author

It's still relevant

@github-actions github-actions bot removed the stale Stale issues which are marked for closure label Sep 23, 2022
@jbw0033
Copy link
Collaborator

jbw0033 commented Oct 18, 2022

We can at the very least update the z-order to match the ordering of our visible entries which reflects what we would expect from an animation stand point.

@cataling-sliide
Copy link

cataling-sliide commented Oct 27, 2022

Any update on this?

Currently a forked version of AnimatedNavHost with the following fix works fine:

targetContentZIndex = navController.backQueue
    .indexOf(targetState)
    .takeIf { it >= 0 }
    ?.toFloat()
    ?: navController.backQueue.size

But this should be a treated as a temporary fix until an official solution is pushed where the z-order matches the ordering of visible entries.

I am thinking @PhilipDukhov 's issue is similar to #1172 that we're currently facing.

jbw0033 pushed a commit to jbw0033/accompanist that referenced this issue Nov 1, 2022
NavHost should respect the z-order established by visibleEntries. This ensures that incoming Composables will be on top of exiting ones, except in the case of a pop it is expected that it is reversed.

Fixes: google#1160
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants