diff --git a/amethyst/src/main/java/com/vitorpamplona/amethyst/ui/navigation/bottombars/KeyboardState.kt b/amethyst/src/main/java/com/vitorpamplona/amethyst/ui/navigation/bottombars/KeyboardState.kt index f576a3556c..78ea948462 100644 --- a/amethyst/src/main/java/com/vitorpamplona/amethyst/ui/navigation/bottombars/KeyboardState.kt +++ b/amethyst/src/main/java/com/vitorpamplona/amethyst/ui/navigation/bottombars/KeyboardState.kt @@ -30,15 +30,7 @@ import androidx.compose.runtime.State import androidx.compose.runtime.derivedStateOf import androidx.compose.runtime.getValue import androidx.compose.runtime.remember -import androidx.compose.runtime.rememberCoroutineScope -import androidx.compose.runtime.snapshotFlow import androidx.compose.ui.platform.LocalDensity -import androidx.compose.ui.platform.LocalFocusManager -import androidx.compose.ui.platform.LocalSoftwareKeyboardController -import kotlinx.coroutines.flow.first -import kotlinx.coroutines.launch -import kotlinx.coroutines.withTimeoutOrNull -import java.util.concurrent.atomic.AtomicBoolean enum class KeyboardState { Opened, @@ -69,80 +61,26 @@ fun keyboardAsState(): State { } } -/** How long to wait for the IME inset to reach zero before running the action anyway. */ -private const val IME_SETTLE_TIMEOUT_MS = 700L - -/** - * Returns a runner that defers an action until the soft keyboard is fully off screen. - * - * Popping a screen while the keyboard is still up races the window animation against the IME's - * close animation. On release builds — fast enough that the window animation wins — the IME - * [WindowInsetsAnimationCompat][androidx.core.view.WindowInsetsAnimationCompat] is cancelled before - * its terminal (zero) frame reaches Compose, so the shared `WindowInsets.ime` holder stays - * "animating" and every `Modifier.imePadding()` in the app freezes at the keyboard height until a - * later inset pass rebalances it (the "stuck IME padding" that survives leaving the screen). - * - * Any exit that leaves a keyboard-bearing screen has to serialize the two animations rather than - * overlap them. With the keyboard already down the action runs inline — same frame, no behavior - * change. With it up we dismiss the keyboard, wait for the inset to actually reach zero, and only - * then act, so the IME animation always completes before the window animation begins. - * - * Re-entrant calls while an action is pending are dropped: the deferral widens the window in which - * a second tap on a Post/Save button would fire the action twice. - * - * [IME_SETTLE_TIMEOUT_MS] bounds the wait — if the inset never reports zero (precisely the failure - * this guards against) the action still runs, so a stale reading can never trap the user on screen. - */ -@Composable -fun rememberAfterKeyboardCloses(): (() -> Unit) -> Unit { - val density = LocalDensity.current - val imeInsets = WindowInsets.ime - val keyboard = LocalSoftwareKeyboardController.current - val focusManager = LocalFocusManager.current - val scope = rememberCoroutineScope() - val pending = remember { AtomicBoolean(false) } - - return remember(density, imeInsets, keyboard, focusManager, scope, pending) { - { action: () -> Unit -> - if (imeInsets.getBottom(density) <= 0) { - action() - } else if (pending.compareAndSet(false, true)) { - // Clear focus first so nothing re-requests the IME as it retracts. - focusManager.clearFocus(true) - keyboard?.hide() - scope.launch { - try { - withTimeoutOrNull(IME_SETTLE_TIMEOUT_MS) { - snapshotFlow { imeInsets.getBottom(density) }.first { it <= 0 } - } - action() - } finally { - pending.set(false) - } - } - } - } - } -} - /** * A [BackHandler] that lets the system dismiss the soft keyboard before it consumes back. * - * Chat composers (and draft-saving editors) intercept back to flush a draft and pop the screen, - * which is the pop-during-IME-animation race described on [rememberAfterKeyboardCloses]. While the - * keyboard is up we do NOT consume back, so the system dismisses it first with its own animation - * (which completes cleanly, and on recent Android follows the back gesture). The next back runs - * [onBack] as before. + * Chat composers (and draft-saving editors) intercept back to flush a draft and pop the screen. + * While the keyboard is up we do NOT consume back, so the system dismisses it first with its own + * animation — which completes cleanly, and on recent Android follows the back gesture rather than + * snapping. The next back runs [onBack] as before. + * + * This is a UX preference, not the safety mechanism: the actual pop-during-IME-animation race is + * handled for every exit in the app by + * [ImeSettler][com.vitorpamplona.amethyst.ui.navigation.navs.ImeSettler] on `Nav`, so [onBack] is + * safe to run whenever it fires. * * The gate reads [WindowInsets.imeAnimationTarget] — where the IME is *heading* — not the animated * [WindowInsets.ime]. Gating on the animated value left a hole: it stays above zero for the whole * close animation, ~250ms in which the IME has already stopped consuming back but this handler was * still disabled, so a second back fell through to the NavController and popped the screen without - * ever running [onBack] — silently dropping the draft it exists to save. The target flips to zero - * the moment the hide begins, so back keeps reaching [onBack] throughout. - * - * Re-enabling that early means [onBack] can now fire mid-animation, so it is routed through - * [rememberAfterKeyboardCloses] to wait for the inset to settle before popping. + * ever running [onBack] — silently dropping the draft it exists to save, since nothing else saves + * one. The target flips to zero the moment the hide begins, so back keeps reaching [onBack] + * throughout the animation. */ @OptIn(ExperimentalLayoutApi::class) @Composable @@ -152,11 +90,10 @@ fun KeyboardAwareBackHandler( ) { val density = LocalDensity.current val imeTarget = WindowInsets.imeAnimationTarget - val afterKeyboardCloses = rememberAfterKeyboardCloses() val keyboardIsStaying by remember(density, imeTarget) { derivedStateOf { imeTarget.getBottom(density) > 0 } } - BackHandler(enabled = enabled && !keyboardIsStaying) { afterKeyboardCloses(onBack) } + BackHandler(enabled = enabled && !keyboardIsStaying, onBack = onBack) } diff --git a/amethyst/src/main/java/com/vitorpamplona/amethyst/ui/navigation/navs/ImeSettler.kt b/amethyst/src/main/java/com/vitorpamplona/amethyst/ui/navigation/navs/ImeSettler.kt new file mode 100644 index 0000000000..c36e3bc30b --- /dev/null +++ b/amethyst/src/main/java/com/vitorpamplona/amethyst/ui/navigation/navs/ImeSettler.kt @@ -0,0 +1,89 @@ +/* + * Copyright (c) 2025 Vitor Pamplona + * + * Permission is hereby granted, free of charge, to any person obtaining a copy of + * this software and associated documentation files (the "Software"), to deal in + * the Software without restriction, including without limitation the rights to use, + * copy, modify, merge, publish, distribute, sublicense, and/or sell copies of the + * Software, and to permit persons to whom the Software is furnished to do so, + * subject to the following conditions: + * + * The above copyright notice and this permission notice shall be included in all + * copies or substantial portions of the Software. + * + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY, FITNESS + * FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE AUTHORS OR + * COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER LIABILITY, WHETHER IN + * AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM, OUT OF OR IN CONNECTION + * WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE SOFTWARE. + */ +package com.vitorpamplona.amethyst.ui.navigation.navs + +import androidx.compose.foundation.layout.WindowInsets +import androidx.compose.foundation.layout.ime +import androidx.compose.runtime.Composable +import androidx.compose.runtime.remember +import androidx.compose.runtime.snapshotFlow +import androidx.compose.ui.platform.LocalDensity +import androidx.compose.ui.platform.LocalFocusManager +import androidx.compose.ui.platform.LocalSoftwareKeyboardController +import kotlinx.coroutines.flow.first +import kotlinx.coroutines.withTimeoutOrNull + +/** How long to wait for the IME inset to reach zero before navigating anyway. */ +const val IME_SETTLE_TIMEOUT_MS = 700L + +/** + * Waits for the soft keyboard to be fully off screen. Installed on [Nav] so that every navigation + * in the app serializes the IME and window animations instead of overlapping them. + * + * Navigating while the keyboard is up races the window animation against the IME's close animation. + * On release builds — fast enough that the window animation wins — the IME + * [WindowInsetsAnimationCompat][androidx.core.view.WindowInsetsAnimationCompat] is cancelled before + * its terminal (zero) frame reaches Compose. `WindowInsets.ime` is a single app-wide holder, so it + * stays "animating" and every `Modifier.imePadding()` in the app — not just the screen being left — + * freezes at the keyboard height until some later inset pass happens to rebalance it. + * + * This is not a composer-screen problem, which is why it lives here rather than in the screens. + * Any destination that can hold focus in a text field can strand the padding on the way out, by any + * exit: a back gesture, a top-bar button, a bottom-nav tab, or tapping a result. Search is the + * clearest case — it focuses its field on arrival, so the keyboard is already up before the user + * has done anything, and every way out of it is a navigation. + */ +fun interface ImeSettler { + suspend fun settle() + + companion object { + /** For [EmptyNav] and previews, where there is no window to read insets from. */ + val None = ImeSettler { } + } +} + +/** + * Reads the same animated `WindowInsets.ime` that drives `Modifier.imePadding()`, so the settler + * and the padding can never disagree about whether the keyboard is gone. + * + * Focus is cleared before hiding so nothing re-requests the IME as it retracts. The wait is bounded + * by [IME_SETTLE_TIMEOUT_MS] — if the inset never reports zero, which is precisely the failure this + * guards against, navigation still proceeds rather than stranding the user on the screen. + */ +@Composable +fun rememberImeSettler(): ImeSettler { + val density = LocalDensity.current + val imeInsets = WindowInsets.ime + val keyboard = LocalSoftwareKeyboardController.current + val focusManager = LocalFocusManager.current + + return remember(density, imeInsets, keyboard, focusManager) { + ImeSettler { + if (imeInsets.getBottom(density) > 0) { + focusManager.clearFocus(true) + keyboard?.hide() + withTimeoutOrNull(IME_SETTLE_TIMEOUT_MS) { + snapshotFlow { imeInsets.getBottom(density) }.first { it <= 0 } + } + } + } + } +} diff --git a/amethyst/src/main/java/com/vitorpamplona/amethyst/ui/navigation/navs/Nav.kt b/amethyst/src/main/java/com/vitorpamplona/amethyst/ui/navigation/navs/Nav.kt index 1526d0be72..d937109b10 100644 --- a/amethyst/src/main/java/com/vitorpamplona/amethyst/ui/navigation/navs/Nav.kt +++ b/amethyst/src/main/java/com/vitorpamplona/amethyst/ui/navigation/navs/Nav.kt @@ -44,6 +44,13 @@ import kotlin.reflect.KClass class Nav( val controller: NavHostController, override val navigationScope: CoroutineScope, + /** + * Awaited before every transition below. Leaving a screen while the soft keyboard is still + * animating strands `imePadding()` app-wide; see [ImeSettler]. Every in-app navigation goes + * through this class, so this is the one place that has to get it right — no screen, top bar + * or back handler needs to think about the keyboard on its way out. + */ + private val ime: ImeSettler = ImeSettler.None, ) : INav { override val drawerState = DrawerState(DrawerValue.Closed) @@ -63,6 +70,7 @@ class Nav( override fun nav(route: Route) { navigationScope.launch { + ime.settle() if (getRouteWithArguments(route::class, controller) != route) { controller.navigate(route) } @@ -71,6 +79,7 @@ class Nav( override fun nav(computeRoute: suspend () -> Route?) { navigationScope.launch { + ime.settle() val route = computeRoute() if (route != null && getRouteWithArguments(route::class, controller) != route) { controller.navigate(route) @@ -80,6 +89,7 @@ class Nav( override fun newStack(route: Route) { navigationScope.launch { + ime.settle() controller.navigate(route) { popUpTo(route) { inclusive = true @@ -91,6 +101,7 @@ class Nav( override fun navBottomBar(route: Route) { navigationScope.launch { + ime.settle() controller.navigate(route) { // Clear sibling bottom-nav entries but keep Home (the start // destination) below, so back-swipe from any tab returns to @@ -149,6 +160,7 @@ class Nav( override fun popBack() { navigationScope.launch { + ime.settle() controller.navigateUp() } } @@ -159,6 +171,7 @@ class Nav( klass: KClass, ) { navigationScope.launch { + ime.settle() controller.navigate(route) { popUpTo(klass) { inclusive = true } } diff --git a/amethyst/src/main/java/com/vitorpamplona/amethyst/ui/navigation/navs/RememberNavs.kt b/amethyst/src/main/java/com/vitorpamplona/amethyst/ui/navigation/navs/RememberNavs.kt index f6c42c0aa3..8b9a2d0e40 100644 --- a/amethyst/src/main/java/com/vitorpamplona/amethyst/ui/navigation/navs/RememberNavs.kt +++ b/amethyst/src/main/java/com/vitorpamplona/amethyst/ui/navigation/navs/RememberNavs.kt @@ -29,9 +29,10 @@ import androidx.navigation.compose.rememberNavController fun rememberNav(): Nav { val navController = rememberNavController() val scope = rememberCoroutineScope() + val ime = rememberImeSettler() - return remember(navController, scope) { - Nav(navController, scope) + return remember(navController, scope, ime) { + Nav(navController, scope, ime) } } diff --git a/amethyst/src/main/java/com/vitorpamplona/amethyst/ui/navigation/topbars/ActionTopBar.kt b/amethyst/src/main/java/com/vitorpamplona/amethyst/ui/navigation/topbars/ActionTopBar.kt index 4bd0aba287..84b19c084a 100644 --- a/amethyst/src/main/java/com/vitorpamplona/amethyst/ui/navigation/topbars/ActionTopBar.kt +++ b/amethyst/src/main/java/com/vitorpamplona/amethyst/ui/navigation/topbars/ActionTopBar.kt @@ -31,7 +31,6 @@ import androidx.compose.ui.Modifier import androidx.compose.ui.text.style.TextAlign import androidx.compose.ui.text.style.TextOverflow import com.vitorpamplona.amethyst.R -import com.vitorpamplona.amethyst.ui.navigation.bottombars.rememberAfterKeyboardCloses import com.vitorpamplona.amethyst.ui.note.buttons.CloseButton import com.vitorpamplona.amethyst.ui.stringRes import com.vitorpamplona.amethyst.ui.theme.HalfHorzPadding @@ -46,10 +45,6 @@ fun ActionTopBar( onPost: () -> Unit, additionalActions: @Composable (() -> Unit)? = null, ) { - // Both exits pop the screen, and on a composer the keyboard is up while typing — the same - // pop-during-IME-animation race the back gesture avoids, just reached by a tap instead. - val afterKeyboardCloses = rememberAfterKeyboardCloses() - ShorterTopAppBar( title = { if (titleRes != null) { @@ -65,7 +60,7 @@ fun ActionTopBar( navigationIcon = { CloseButton( modifier = HalfHorzPadding, - onPress = { afterKeyboardCloses(onCancel) }, + onPress = onCancel, ) }, actions = { @@ -75,7 +70,7 @@ fun ActionTopBar( Button( modifier = HalfHorzPadding, enabled = isActive(), - onClick = { afterKeyboardCloses(onPost) }, + onClick = onPost, ) { Text(text = stringRes(postRes)) } diff --git a/amethyst/src/test/java/com/vitorpamplona/amethyst/ui/navigation/NavImeSettleTest.kt b/amethyst/src/test/java/com/vitorpamplona/amethyst/ui/navigation/NavImeSettleTest.kt new file mode 100644 index 0000000000..1b362dc062 --- /dev/null +++ b/amethyst/src/test/java/com/vitorpamplona/amethyst/ui/navigation/NavImeSettleTest.kt @@ -0,0 +1,118 @@ +/* + * Copyright (c) 2025 Vitor Pamplona + * + * Permission is hereby granted, free of charge, to any person obtaining a copy of + * this software and associated documentation files (the "Software"), to deal in + * the Software without restriction, including without limitation the rights to use, + * copy, modify, merge, publish, distribute, sublicense, and/or sell copies of the + * Software, and to permit persons to whom the Software is furnished to do so, + * subject to the following conditions: + * + * The above copyright notice and this permission notice shall be included in all + * copies or substantial portions of the Software. + * + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY, FITNESS + * FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE AUTHORS OR + * COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER LIABILITY, WHETHER IN + * AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM, OUT OF OR IN CONNECTION + * WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE SOFTWARE. + */ +package com.vitorpamplona.amethyst.ui.navigation + +import androidx.navigation.NavHostController +import androidx.navigation.NavOptionsBuilder +import com.vitorpamplona.amethyst.ui.navigation.navs.ImeSettler +import com.vitorpamplona.amethyst.ui.navigation.navs.Nav +import com.vitorpamplona.amethyst.ui.navigation.routes.Route +import io.mockk.every +import io.mockk.mockk +import kotlinx.coroutines.ExperimentalCoroutinesApi +import kotlinx.coroutines.delay +import kotlinx.coroutines.test.advanceUntilIdle +import kotlinx.coroutines.test.runTest +import org.junit.Assert.assertEquals +import org.junit.Test + +/** + * Leaving a screen while the soft keyboard is still animating strands `imePadding()` at keyboard + * height for the whole app, because `WindowInsets.ime` is a single shared holder. The fix is that + * [Nav] waits for the IME to be gone before it moves, so these assert the ordering rather than any + * visual result: every transition must settle the keyboard *first*. + * + * Without the settle calls in [Nav] each of these records only "navigate" and fails. + */ +@OptIn(ExperimentalCoroutinesApi::class) +class NavImeSettleTest { + private fun controllerRecording(order: MutableList): NavHostController = + mockk(relaxed = true) { + every { navigate(any(), any Unit>()) } answers + { order.add("navigate") } + every { navigate(any()) } answers { order.add("navigate") } + every { navigateUp() } answers { + order.add("navigate") + true + } + } + + @Test + fun popBackSettlesTheKeyboardBeforeNavigating() = + runTest { + val order = mutableListOf() + val nav = Nav(controllerRecording(order), this, ImeSettler { order.add("settle") }) + + nav.popBack() + advanceUntilIdle() + + assertEquals(listOf("settle", "navigate"), order) + } + + @Test + fun bottomBarSettlesTheKeyboardBeforeNavigating() = + runTest { + // The search tab focuses its field on arrival, so the keyboard is already up when the + // user taps another tab — the exit that has no BackHandler and no top bar to guard it. + val order = mutableListOf() + val nav = Nav(controllerRecording(order), this, ImeSettler { order.add("settle") }) + + nav.navBottomBar(Route.Home) + advanceUntilIdle() + + assertEquals(listOf("settle", "navigate"), order) + } + + @Test + fun newStackSettlesTheKeyboardBeforeNavigating() = + runTest { + val order = mutableListOf() + val nav = Nav(controllerRecording(order), this, ImeSettler { order.add("settle") }) + + nav.newStack(Route.Home) + advanceUntilIdle() + + assertEquals(listOf("settle", "navigate"), order) + } + + @Test + fun aSlowKeyboardStillHoldsTheNavigationBack() = + runTest { + // The real settler suspends for the length of the IME close animation. Navigation must + // wait for it, not fire alongside it — that overlap is the bug. + val order = mutableListOf() + val nav = + Nav( + controllerRecording(order), + this, + ImeSettler { + delay(250) + order.add("settle") + }, + ) + + nav.popBack() + assertEquals(emptyList(), order) + + advanceUntilIdle() + assertEquals(listOf("settle", "navigate"), order) + } +}