dialog's lifecycle is wrong [286371387]

Fixed

Bug

Status Update

No update yet.

Description

zh...@gmail.com

created issue #1

Jun 9, 2023 02:24AM

Component used: Navigation Version used: Devices/Android versions reproduced on:

If this is a bug in the library, we would appreciate if you could attach:

Sample project to trigger the issue.
A screenrecord or screenshots showing the issue (if UI related).

When the user clicks the button twice in a row, the button event is a pop-up window, and then two pop-up windows will appear

The interesting thing is that when I close the pop-up window at the top of the stack, the two pop-up windows will be added to NavigatorState#_transitionsInProgress, and then the pop-up window at the top of the stack will be destroyed from compose and execute dialogNavigator#onTransitionComplete, but the second pop-up If the window does not execute dialogNavigator#onTransitionComplete, the second lifeCycle will always be in the START state, which will cause NavController data exception

I compared screen and DialogHost in the composable function of NavHost, and found that Screen will execute onTransitionComplete after the state changes, while Dialog does not

Comments

jb...@google.com <jb...@google.com> #2Jun 9, 2023 02:54PM

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

zh...@gmail.com <zh...@gmail.com> #3Jun 11, 2023 11:03AM

Yes, of course

@Composable
private fun DemoNavHost() {
    val controller = rememberNavController()
    NavHost(
        navController = controller,
        startDestination = "mainRoute",
        modifier = Modifier.fillMaxSize()
    ) {

        composable("mainRoute") {
            Surface() {
                Box(Modifier.fillMaxSize(), contentAlignment = Alignment.Center) {
                    Button(
                        onClick = {
                            controller.navigate("demoDialog")
                            controller.navigate("demoDialog")   // Simulate a quick click by the user
                        }
                    ) {
                        Text(text = "show dialog")
                    }
                }
            }
        }

        dialog("demoDialog") {
            val context = LocalContext.current
            val lifecycle = LocalLifecycleOwner.current
            Surface() {
                Column() {
                    Text(text = "I'm demo dialog")
                    Button(
                        onClick = {
                            Toast
                                .makeText(
                                    context,
                                    "${lifecycle.lifecycle.currentState}",
                                    Toast.LENGTH_SHORT
                                )
                                .show()
                        }
                    ) {
                        Text(text = "check lifecycle")
                    }
                    Button(onClick = controller::popBackStack) {
                        Text(text = "close")
                    }
                }
            }
        }
    }
}

Steps:

Step 1: Click the show dialog button on mainRoute

Step 2: Because 2 pop-up windows pop up, we close the top one

Step 3: Click the check lifecycle button on the pop-up window, you can see STARTED instead of RESUMED

I have also checked the cause, which is consistent with the description in my question.

zh...@gmail.com <zh...@gmail.com> #4Jun 11, 2023 11:18AM

In fact, I found another problem. When the tester frequently clicks the button, the button event is to display a Dialog, Dialog is set to be destroyed by external click, so Dialog is always displayed and destroyed

Interesting thing, because it may be too fast, causing DialogHost to not notice that Dialog appears and disappears, but it has actually joined NavController#_visibleEntries

But because NavHost#DialogHost did not process DisposableEffect, it will not execute dialogNavigator#onTransitionComplete, so it will not be removed from NavController#_visibleEntries, and a NavBackStackEntry leak occurs

The reason may be that the pop-up window appears and disappears within one frame

jb...@google.com <jb...@google.com> Jun 15, 2023 11:06PM

Assigned to cl...@google.com.

cl...@google.com <cl...@google.com> #5Jun 19, 2023 09:47PM

Hi, can you please file a separate bug for the issue you see in comment#4?

On initial bug - when multiple dialogs are open at the same time and the top one is dismissed, the second dialog in backstack is marked as transitioning to hold it in STARTED state until the top dialog has completed transitioning. But the second dialog was not marked as complete when composed so it stays in STARTED state instead of moving to expected RESUMED state.

zh...@gmail.com <zh...@gmail.com> #6Jun 20, 2023 02:09AM

Thank you for your answer, I will open a separate post to ask a separate question;

separate question

Message last modified on Jun 20, 2023 06:03AM

cl...@google.com <cl...@google.com> #7Jun 20, 2023 05:32PM

Thanks for filing the new bug! I was thinking a separate bug for the issue you see in commment#4 with NavBackStackEntry possibly leaking. I'm gonna update the new bug's description to match that.

cl...@google.com <cl...@google.com> #8Aug 29, 2023 09:52PM

Marked as fixed.

Fixed internally and will be available in navigation 2.7.2.

ap...@google.com <ap...@google.com> #9Aug 29, 2023 09:53PM

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

commit b624e50960eb6601efe2cd3f5d81b95a12ff56f2
Author: Clara Fok <clarafok@google.com>
Date: Thu Jun 22 14:11:43 2023

Fix Dialog not marked complete

When multiple dialogs are open at the same time and the top one is dismissed, the second dialog in backstack is marked as transitioning to hold it in STARTED state until the top dialog has completed transitioning. But the second dialog was never marked as complete so it stays in STARTED state instead of moving to expected RESUMED state.

Now we make sure to mark Dialogs as complete when Dialog has been commposed.

Test: ./gradlew navigation:navigation-compose:cC
Bug: 286371387
Change-Id: I48da82d4c0db3fecd40b4357b152e807078066c6

M navigation/navigation-compose/src/androidTest/java/androidx/navigation/compose/DialogNavigatorTest.kt
M navigation/navigation-compose/src/main/java/androidx/navigation/compose/DialogNavigator.kt

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

pr...@google.com <pr...@google.com> #10Sep 6, 2023 07:55PM

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

androidx.navigation:navigation-compose:2.7.2

na...@google.com <na...@google.com> #11Jan 24, 2024 07:53PM