From bcbf78d8ea42d01e210360450363fd1520cd6e39 Mon Sep 17 00:00:00 2001 From: Claude Date: Wed, 5 Aug 2026 23:04:09 +0000 Subject: [PATCH] fix(ime): settle the keyboard in Nav so every screen is covered MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Leaving a screen while the soft keyboard is still animating strands `imePadding()` at keyboard height for the whole app — `WindowInsets.ime` is a single shared holder, so the padding survives leaving the screen that caused it. PR #3864 fixed this for the post composers, at their call sites. That was the wrong altitude: Search strands it too, and Search has no BackHandler and no top bar of ours. Search is the clearest case: it focuses its field on arrival, so the keyboard is up before the user has done anything, and every way out is a navigation — a bottom-nav tab, a tapped result, back. Any destination that can focus a text field can strand the padding on the way out. There are 174 files with text input in this module; enumerating the screens was never going to converge. Two facts make a central fix possible: every in-app navigation goes through INav (there is not one `controller.navigate` outside navigation/navs/, and nothing touches OnBackPressedDispatcher, navigateUp or popBackStack directly), and every Nav method already runs inside `navigationScope.launch`. So Nav awaits an ImeSettler before each transition: keyboard down, it returns immediately and nothing changes; keyboard up, it clears focus, hides the IME and waits for the inset to actually reach zero, bounded, so the two animations never overlap. ObservableNav delegates to Nav and inherits it. That subsumes #3864's call-site patches, so they are removed rather than left as a second mechanism: ActionTopBar goes back to plain callbacks (which also drops the composition-scoped deferral of onPost, so posting no longer depends on the top bar staying composed), and KeyboardAwareBackHandler keeps only its imeAnimationTarget gate — the part that stops back falling through and silently dropping a draft. It is now a UX preference (let the system animate the dismissal) rather than the safety mechanism. NavImeSettleTest pins the ordering: each transition must settle before it navigates, and a settler that suspends must hold the navigation back rather than run alongside it. All four fail with the settle calls removed. Known gap: on a screen with no BackHandler the system's back pops through the NavController directly, not Nav.popBack(), so a second back landing inside the ~250ms retraction can still race. Closing it needs a shell-level handler registered after the NavHost to outrank its back callback, which is a composition-order dependency subtle enough to break silently — worth a deliberate decision rather than smuggling in here. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01LfUMGWYu2uTSyh17JJonfN --- .../ui/navigation/bottombars/KeyboardState.kt | 89 ++----------- .../amethyst/ui/navigation/navs/ImeSettler.kt | 89 +++++++++++++ .../amethyst/ui/navigation/navs/Nav.kt | 13 ++ .../ui/navigation/navs/RememberNavs.kt | 5 +- .../ui/navigation/topbars/ActionTopBar.kt | 9 +- .../ui/navigation/NavImeSettleTest.kt | 118 ++++++++++++++++++ 6 files changed, 238 insertions(+), 85 deletions(-) create mode 100644 amethyst/src/main/java/com/vitorpamplona/amethyst/ui/navigation/navs/ImeSettler.kt create mode 100644 amethyst/src/test/java/com/vitorpamplona/amethyst/ui/navigation/NavImeSettleTest.kt 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) + } +}