From 1c7f62eaeff6e773ec8d296f4baf82cf6e8e3aa5 Mon Sep 17 00:00:00 2001 From: Vladislav Protasov Date: Wed, 5 Nov 2025 00:34:23 +0100 Subject: [PATCH] [JEWEL-1054] Fix selection race in dropdown: emit on commit; reliable mouse click after re-add MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Fix the dropdown selection race and make selection emission intentional and robust. Emit onSelectedIndexesChange synchronously exactly when selection is committed (pointer press, keyboard actions). This removes the LaunchedEffect race where the popup could close before emission. Keep hover purely visual: update only lastActiveItemIndex on move/enter; never touch selectedKeys. Retain a tiny LaunchedEffect as a secondary path to re-emit when the key→index mapping changes (reorder/header/programmatic updates). It recomputes indices from the current container, dedupes emissions, and primes lastActiveItemIndex so the first arrow after remaps works. In ListComboBox, confirm-on-click: clicking an item selects and then closes in the same call chain; outside clicks dismiss via onDismissRequest. Keyboard navigation remains centralized at the combo (inner list uses NoopListKeyActions) to avoid double handling. Follow-up (not in this patch): route all keyboard navigation to the inner SelectableLazyColumn while the popup is open. https://github.com/JetBrains/intellij-community/pull/3286 (cherry picked from commit 0be64c017c9e27d951cd83542c0b219422b6aea6) IJ-CR-181454 GitOrigin-RevId: 8c5181f29afdd6d4486bfc8c3d142f92177f4c21 --- .../jewel/foundation/lazy/Keybindings.kt | 4 +- .../foundation/lazy/SelectableLazyColumn.kt | 187 ++++++++++++++++-- .../jewel/foundation/lazy/tree/KeyActions.kt | 16 ++ ...electableLazyColumnSelectionIndicesTest.kt | 175 ++++++++++++++++ .../samples/showcase/components/ComboBoxes.kt | 67 +++++++ .../jewel/ui/component/ListComboBoxUiTest.kt | 78 ++++++++ .../jewel/ui/component/ListComboBox.kt | 123 ++++++++---- 7 files changed, 598 insertions(+), 52 deletions(-) create mode 100644 platform/jewel/foundation/src/test/kotlin/org/jetbrains/jewel/foundation/lazy/SelectableLazyColumnSelectionIndicesTest.kt diff --git a/platform/jewel/foundation/src/main/kotlin/org/jetbrains/jewel/foundation/lazy/Keybindings.kt b/platform/jewel/foundation/src/main/kotlin/org/jetbrains/jewel/foundation/lazy/Keybindings.kt index 9c28062398ad..c5fece85f195 100644 --- a/platform/jewel/foundation/src/main/kotlin/org/jetbrains/jewel/foundation/lazy/Keybindings.kt +++ b/platform/jewel/foundation/src/main/kotlin/org/jetbrains/jewel/foundation/lazy/Keybindings.kt @@ -89,10 +89,10 @@ public open class DefaultSelectableColumnKeybindings : SelectableColumnKeybindin get() = isCtrlPressed override val KeyEvent.isSelectFirstItem: Boolean - get() = key == Key.Home && !isContiguousSelectionKeyPressed + get() = (key == Key.MoveHome || key == Key.Home) && !isContiguousSelectionKeyPressed override val KeyEvent.isExtendSelectionToFirstItem: Boolean - get() = key == Key.Home && isContiguousSelectionKeyPressed + get() = (key == Key.MoveHome || key == Key.Home) && isContiguousSelectionKeyPressed override val KeyEvent.isSelectLastItem: Boolean get() = key == Key.MoveEnd && !isContiguousSelectionKeyPressed diff --git a/platform/jewel/foundation/src/main/kotlin/org/jetbrains/jewel/foundation/lazy/SelectableLazyColumn.kt b/platform/jewel/foundation/src/main/kotlin/org/jetbrains/jewel/foundation/lazy/SelectableLazyColumn.kt index 5c7098160cba..e78b75fe66e2 100644 --- a/platform/jewel/foundation/src/main/kotlin/org/jetbrains/jewel/foundation/lazy/SelectableLazyColumn.kt +++ b/platform/jewel/foundation/src/main/kotlin/org/jetbrains/jewel/foundation/lazy/SelectableLazyColumn.kt @@ -26,6 +26,7 @@ import androidx.compose.ui.focus.FocusRequester import androidx.compose.ui.focus.focusProperties import androidx.compose.ui.focus.focusRequester import androidx.compose.ui.input.key.Key +import androidx.compose.ui.input.key.KeyEvent import androidx.compose.ui.input.key.key import androidx.compose.ui.input.key.onPreviewKeyEvent import androidx.compose.ui.input.pointer.PointerEventType @@ -105,13 +106,42 @@ public fun SelectableLazyColumn( val keys = remember(container) { container.getKeys() } val isFocused by intSource.collectIsFocusedAsState() - var lastSelectedKeys by remember { mutableStateOf(state.selectedKeys) } - LaunchedEffect(state.selectedKeys, onSelectedIndexesChange, container) { - if (lastSelectedKeys == state.selectedKeys) return@LaunchedEffect + /** Tracks the last emitted indices to avoid duplicate emissions from both commit-time and effect-driven updates. */ + var lastEmittedIndices by remember { mutableStateOf>(emptyList()) } - val indices = state.selectedKeys.mapNotNull { key -> container.getKeyIndex(key) } - lastSelectedKeys = state.selectedKeys - onSelectedIndexesChange(indices) + // Keep the latest callback reference to avoid capturing a stale lambda inside effects + val latestOnSelectedIndexesChange = rememberUpdatedState(onSelectedIndexesChange) + + // Secondary emission path: mapping-change and programmatic-selection bridge + // + // We have two ways to emit selected indices to callers: + // 1) Primary (synchronous, commit-time) — the notifying wrappers around + // KeyActions/PointerEventActions (see `notifyingKeyActions`/`notifyingPointerEventActions`). + // They emit immediately when the user commits a selection via keyboard or mouse. This is + // crucial for flows where the popup disposes right after the commit (e.g., dropdown click + // or Enter), because an effect might never get a chance to run. + // 2) Secondary (this effect) — runs when the key→index mapping changes (e.g., reorder, header + // insert/remove, filtering) or when `selectedKeys` is updated programmatically. In these cases + // there’s no user "commit" event to intercept, but consumers that rely on indices must stay in + // sync. We recompute indices from keys and only notify if they differ from the last emission. + // + // Additionally, we synchronize the keyboard-navigation anchor (`lastActiveItemIndex`) after remaps + // so the very next arrow key works reliably, including across empty→non‑empty transitions. + LaunchedEffect(container, state.selectedKeys) { + val selectedKeysSnapshot = state.selectedKeys + val indices = selectedKeysSnapshot.mapNotNull { key -> container.getKeyIndex(key) } + if (indices != lastEmittedIndices) { + lastEmittedIndices = indices + + // Keep keyboard navigation gate in sync after key→index remaps, including through empty→non‑empty + // transitions. + if (indices.isNotEmpty()) { + state.lastActiveItemIndex = indices.first() + } + + // Notify using the latest callback reference to avoid capturing a stale lambda. + latestOnSelectedIndexesChange.value(indices) + } } LaunchedEffect(isFocused) { @@ -122,6 +152,110 @@ public fun SelectableLazyColumn( } } + // Synchronous commit-time emission for pointer interactions + // + // This wrapper delegates to the provided `PointerEventActions` but also emits `onSelectedIndexesChange` + // immediately after a user commit (press/toggle/extend) if the selection actually changed. We: + // - Compare `state.selectedKeys` before/after by identity to detect real changes (the state replaces the set). + // - Translate keys→indices via the current `container` so callers that want indices receive them right away. + // - Deduplicate with `lastEmittedIndices` so both this synchronous path and the effect path don’t double emit. + // + // Why a wrapper? In flows like a List/ComboBox dropdown, the popup is disposed as a consequence of the click. + // If we waited for an effect to run, the component might already be gone and the emission would be lost. + val notifyingPointerEventActions = + remember(pointerEventActions, container, state, onSelectedIndexesChange) { + object : PointerEventActions { + private fun emitIfSelectionChanged(before: Set) { + state.selectedIndicesIfChanged(before, container, lastEmittedIndices)?.let { indices -> + lastEmittedIndices = indices + onSelectedIndexesChange(indices) + } + } + + override fun handlePointerEventPress( + pointerEvent: androidx.compose.ui.input.pointer.PointerEvent, + keybindings: SelectableColumnKeybindings, + selectableLazyListState: SelectableLazyListState, + selectionMode: SelectionMode, + allKeys: List, + key: Any, + ) { + val before = state.selectedKeys + pointerEventActions.handlePointerEventPress( + pointerEvent, + keybindings, + selectableLazyListState, + selectionMode, + allKeys, + key, + ) + emitIfSelectionChanged(before) + } + + override fun toggleKeySelection( + key: Any, + allKeys: List, + selectableLazyListState: SelectableLazyListState, + selectionMode: SelectionMode, + ) { + val before = state.selectedKeys + pointerEventActions.toggleKeySelection(key, allKeys, selectableLazyListState, selectionMode) + emitIfSelectionChanged(before) + } + + override fun onExtendSelectionToKey( + key: Any, + allKeys: List, + state: SelectableLazyListState, + selectionMode: SelectionMode, + ) { + val before = state.selectedKeys + pointerEventActions.onExtendSelectionToKey(key, allKeys, state, selectionMode) + emitIfSelectionChanged(before) + } + } + } + + // Synchronous commit-time emission for keyboard interactions + // + // This wrapper decorates `KeyActions` so that, when a key event is handled and the selection actually + // changes, we emit indices on the spot. This mirrors the pointer wrapper and serves the same purpose: + // avoid losing the emission if handling the key (e.g., Enter) triggers immediate disposal of the popup. + // + // Implementation notes: + // - Only emit if the delegate reports `handled` and `selectedKeys` has changed since the event. + // - Use the current `container` to compute indices and dedupe with `lastEmittedIndices`. + val notifyingKeyActions = + remember(keyActions, container, state, onSelectedIndexesChange) { + object : KeyActions { + override val keybindings + get() = keyActions.keybindings + + override val actions + get() = keyActions.actions + + override fun handleOnKeyEvent( + event: KeyEvent, + keys: List, + state: SelectableLazyListState, + selectionMode: SelectionMode, + ): KeyEvent.() -> Boolean { + val delegate = keyActions.handleOnKeyEvent(event, keys, state, selectionMode) + return { + val before = state.selectedKeys + val handled = delegate.invoke(this) + if (handled) { + state.selectedIndicesIfChanged(before, container, lastEmittedIndices)?.let { indices -> + lastEmittedIndices = indices + onSelectedIndexesChange(indices) + } + } + handled + } + } + } + } + val focusRequester = remember { FocusRequester() } LazyColumn( modifier = @@ -134,14 +268,23 @@ public fun SelectableLazyColumn( return@onPreviewKeyEvent false } - if (state.lastActiveItemIndex != null) { - val actionHandled = keyActions.handleOnKeyEvent(event, keys, state, selectionMode).invoke(event) - if (actionHandled) { - scope.launch { state.lastActiveItemIndex?.let { state.scrollToItem(it) } } - } - return@onPreviewKeyEvent actionHandled + if (state.lastActiveItemIndex == null) { + val derivedFromSelection = keys.indexOfFirst { it.key in state.selectedKeys }.takeIf { it >= 0 } + val firstSelectable = + if (derivedFromSelection == null) { + keys.indexOfFirst { it is SelectableLazyListKey.Selectable }.takeIf { it >= 0 } + } else { + null + } + state.lastActiveItemIndex = derivedFromSelection ?: firstSelectable } - false + + val actionHandled = + notifyingKeyActions.handleOnKeyEvent(event, keys, state, selectionMode).invoke(event) + if (actionHandled) { + scope.launch { state.lastActiveItemIndex?.let { state.scrollToItem(it) } } + } + actionHandled }, state = state.lazyListState, contentPadding = contentPadding, @@ -157,8 +300,8 @@ public fun SelectableLazyColumn( isFocused, keys, focusRequester, - keyActions, - pointerEventActions, + notifyingKeyActions, + notifyingPointerEventActions, selectionMode, container::isKeySelectable, ) @@ -293,3 +436,17 @@ private fun Modifier.selectable( } } } + +// Computes selected indices if `selectedKeys` changed by identity; returns new indices or null if unchanged. +private fun SelectableLazyListState.selectedIndicesIfChanged( + before: Set, + container: SelectableLazyListScopeContainer, + lastEmitted: List, +): List? { + val after = selectedKeys + if (before !== after) { + val indices = after.mapNotNull { container.getKeyIndex(it) } + if (indices != lastEmitted) return indices + } + return null +} diff --git a/platform/jewel/foundation/src/main/kotlin/org/jetbrains/jewel/foundation/lazy/tree/KeyActions.kt b/platform/jewel/foundation/src/main/kotlin/org/jetbrains/jewel/foundation/lazy/tree/KeyActions.kt index 07c8b66ae44b..e87fce51e922 100644 --- a/platform/jewel/foundation/src/main/kotlin/org/jetbrains/jewel/foundation/lazy/tree/KeyActions.kt +++ b/platform/jewel/foundation/src/main/kotlin/org/jetbrains/jewel/foundation/lazy/tree/KeyActions.kt @@ -353,3 +353,19 @@ public open class DefaultSelectableLazyColumnKeyActions( } } } + +@ApiStatus.Internal +@InternalJewelApi +public object NoopListKeyActions : KeyActions { + override val keybindings: SelectableColumnKeybindings + get() = DefaultSelectableColumnKeybindings + + override val actions: SelectableColumnOnKeyEvent = DefaultSelectableOnKeyEvent(keybindings) + + override fun handleOnKeyEvent( + event: KeyEvent, + keys: List, + state: SelectableLazyListState, + selectionMode: SelectionMode, + ): KeyEvent.() -> Boolean = { false } +} diff --git a/platform/jewel/foundation/src/test/kotlin/org/jetbrains/jewel/foundation/lazy/SelectableLazyColumnSelectionIndicesTest.kt b/platform/jewel/foundation/src/test/kotlin/org/jetbrains/jewel/foundation/lazy/SelectableLazyColumnSelectionIndicesTest.kt new file mode 100644 index 000000000000..ba5677c79924 --- /dev/null +++ b/platform/jewel/foundation/src/test/kotlin/org/jetbrains/jewel/foundation/lazy/SelectableLazyColumnSelectionIndicesTest.kt @@ -0,0 +1,175 @@ +// Copyright 2000-2025 JetBrains s.r.o. and contributors. Use of this source code is governed by the Apache 2.0 license. +package org.jetbrains.jewel.foundation.lazy + +import androidx.compose.runtime.LaunchedEffect +import androidx.compose.runtime.MutableState +import androidx.compose.runtime.mutableStateOf +import androidx.compose.runtime.remember +import androidx.compose.ui.test.junit4.ComposeContentTestRule +import androidx.compose.ui.test.junit4.createComposeRule +import org.junit.Assert.assertEquals +import org.junit.Rule +import org.junit.Test + +/** + * Verifies that SelectableLazyColumn re-emits onSelectedIndexesChange when the list order (key→index mapping) changes + * while the selected keys remain the same. + * + * Scenario: + * - We select the item with key "B" before composition settles. + * - Initially, items are ["A", "B", "C"], so "B" is at index 1 and the callback emits [1]. + * - We then reorder items to ["C", "A", "B"]: keys are unchanged, but "B" moves to index 2. + * + * Expected: + * - onSelectedIndexesChange is invoked again with the updated index [2]. + * + * Why this matters: + * - If emission is gated only by equality of selectedKeys, a pure reorder won’t trigger the callback, leaving consumers + * that rely on indices (e.g., ListComboBox) with stale selection positions. + */ +public class SelectableLazyColumnSelectionIndicesTest { + @get:Rule public val composeTestRule: ComposeContentTestRule = createComposeRule() + + @Test + public fun `when list order changes but selected keys don't onSelectedIndexesChange is emitted with new index`() { + val initialItems = listOf("A", "B", "C") + val reorderedItems = listOf("C", "A", "B") + + var lastIndices: List? = null + var emissionCount = 0 + var itemsStateRef: MutableState>? = null + + composeTestRule.setContent { + val state = rememberSelectableLazyListState() + val itemsState: MutableState> = remember { mutableStateOf(initialItems) } + + // Select "B" by key (key = the item string) + LaunchedEffect(Unit) { state.selectedKeys = setOf("B") } + + SelectableLazyColumn( + state = state, + onSelectedIndexesChange = { indices -> + lastIndices = indices + emissionCount++ + }, + ) { + itemsIndexed( + items = itemsState.value, + key = { _, item -> item }, // keys are the item strings + ) { _, _ -> + // content not relevant for this test + } + } + // Expose items state to the test scope to mutate during assertions + itemsStateRef = itemsState + } + + // Let initial selection propagate + composeTestRule.waitForIdle() + // We expect at least one emission with index 1 ("B" at position 1) + assertEquals(listOf(1), lastIndices) + + // Change list order so that mapping key->index changes; selectedKeys stay the same (setOf("B")) + composeTestRule.runOnIdle { + itemsStateRef?.value = reorderedItems // "B" is now at index 2 + } + + // With the bug, there will be no new emission (guarded by lastSelectedKeys == state.selectedKeys) + // With the fix, we expect a new emission with the updated index 2. + composeTestRule.waitForIdle() + + // Assert we observed a second emission reflecting the new index of key "B" + assertEquals("Expected two emissions: initial and after reorder", 2, emissionCount) + assertEquals(listOf(2), lastIndices) + } + + @Test + public fun `when header is inserted but selected keys remain the same onSelectedIndexesChange is emitted with new index`() { + val initialItems = listOf("A", "B", "C") + val headerItems = listOf("HEADER", "A", "B", "C") + + var lastIndices: List? = null + var emissionCount = 0 + var itemsStateRef: MutableState>? = null + + composeTestRule.setContent { + val state = rememberSelectableLazyListState() + val itemsState: MutableState> = remember { mutableStateOf(initialItems) } + + // Select "B" by key (key = the item string) + LaunchedEffect(Unit) { state.selectedKeys = setOf("B") } + + SelectableLazyColumn( + state = state, + onSelectedIndexesChange = { indices -> + lastIndices = indices + emissionCount++ + }, + ) { + itemsIndexed(items = itemsState.value, key = { _, item -> item }) { _, _ -> + // content not relevant for this test + } + } + + itemsStateRef = itemsState + } + + // Let initial selection propagate + composeTestRule.waitForIdle() + assertEquals(listOf(1), lastIndices) + + // Insert a new item at the top; selected key remains "B" but its index shifts by +1 + composeTestRule.runOnIdle { itemsStateRef?.value = headerItems } + + composeTestRule.waitForIdle() + + // Expect a second emission with the updated index (from 1 to 2) + assertEquals("Expected two emissions: initial and after header insert", 2, emissionCount) + assertEquals(listOf(2), lastIndices) + } + + @Test + public fun `when an item before the selected key is removed onSelectedIndexesChange is emitted with new index`() { + val initialItems = listOf("A", "B", "C", "D") + val removedAItems = listOf("B", "C", "D") // remove an item before the selected one + + var lastIndices: List? = null + var emissionCount = 0 + var itemsStateRef: MutableState>? = null + + composeTestRule.setContent { + val state = rememberSelectableLazyListState() + val itemsState: MutableState> = remember { mutableStateOf(initialItems) } + + // Select "C" by key; initially at index 2 + LaunchedEffect(Unit) { state.selectedKeys = setOf("C") } + + SelectableLazyColumn( + state = state, + onSelectedIndexesChange = { indices -> + lastIndices = indices + emissionCount++ + }, + ) { + itemsIndexed(items = itemsState.value, key = { _, item -> item }) { _, _ -> + // content not relevant for this test + } + } + + itemsStateRef = itemsState + } + + // Wait for the initial emission ("C" at index 2) + composeTestRule.waitForIdle() + assertEquals(listOf(2), lastIndices) + + // Remove an item that sits before the selected key (remove "A") -> "C" shifts from 2 to 1 + composeTestRule.runOnIdle { itemsStateRef?.value = removedAItems } + + composeTestRule.waitForIdle() + + // Expect a second emission reflecting the new index of "C" + assertEquals("Expected two emissions: initial and after removal before selection", 2, emissionCount) + assertEquals(listOf(1), lastIndices) + } +} diff --git a/platform/jewel/samples/showcase/src/main/kotlin/org/jetbrains/jewel/samples/showcase/components/ComboBoxes.kt b/platform/jewel/samples/showcase/src/main/kotlin/org/jetbrains/jewel/samples/showcase/components/ComboBoxes.kt index ef94830b9aed..b0770b48ab7b 100644 --- a/platform/jewel/samples/showcase/src/main/kotlin/org/jetbrains/jewel/samples/showcase/components/ComboBoxes.kt +++ b/platform/jewel/samples/showcase/src/main/kotlin/org/jetbrains/jewel/samples/showcase/components/ComboBoxes.kt @@ -5,6 +5,7 @@ package org.jetbrains.jewel.samples.showcase.components import androidx.compose.foundation.layout.Arrangement import androidx.compose.foundation.layout.Column import androidx.compose.foundation.layout.FlowRow +import androidx.compose.foundation.layout.Row import androidx.compose.foundation.layout.Spacer import androidx.compose.foundation.layout.fillMaxWidth import androidx.compose.foundation.layout.height @@ -18,9 +19,12 @@ import androidx.compose.runtime.getValue import androidx.compose.runtime.mutableIntStateOf import androidx.compose.runtime.remember import androidx.compose.runtime.setValue +import androidx.compose.ui.Alignment import androidx.compose.ui.Modifier import androidx.compose.ui.text.style.TextOverflow import androidx.compose.ui.unit.dp +import org.jetbrains.jewel.foundation.lazy.SelectableLazyListState +import org.jetbrains.jewel.foundation.lazy.rememberSelectableLazyListState import org.jetbrains.jewel.foundation.theme.JewelTheme import org.jetbrains.jewel.samples.showcase.ShowcaseIcons import org.jetbrains.jewel.ui.component.ComboBox @@ -86,6 +90,9 @@ public fun ComboBoxes(modifier: Modifier = Modifier) { GroupHeader("Custom combo box content") CustomComboBoxes() + GroupHeader("Dynamic content") + DynamicListComboBox() + Spacer(Modifier.height(16.dp).fillMaxWidth()) } } @@ -319,3 +326,63 @@ private fun InfoText(text: String, modifier: Modifier = Modifier) { } private data class ProgrammingLanguage(val name: String, val icon: IconKey) + +@Composable +private fun DynamicListComboBox() { + Column(verticalArrangement = Arrangement.spacedBy(8.dp)) { + val itemsState = remember { androidx.compose.runtime.mutableStateOf(listOf("A", "B", "C")) } + var selectedIndex by remember { mutableIntStateOf(0) } + var reportCount by remember { mutableIntStateOf(0) } + var lastReportedIndex by remember { mutableIntStateOf(-1) } + val listState: SelectableLazyListState = rememberSelectableLazyListState() + + val itemsJoined = itemsState.value.joinToString(prefix = "[", postfix = "]") + val statusPrefix = "Items: $itemsJoined selectedIndex: $selectedIndex; " + val statusSuffix = "onSelectedItemChange count=$reportCount, last=$lastReportedIndex" + + InfoText(text = statusPrefix + statusSuffix) + + Row(horizontalArrangement = Arrangement.spacedBy(8.dp), verticalAlignment = Alignment.CenterVertically) { + ListComboBox( + items = itemsState.value, + selectedIndex = selectedIndex, + onSelectedItemChange = { idx -> + lastReportedIndex = idx + reportCount += 1 + selectedIndex = idx + }, + modifier = Modifier.widthIn(min = 100.dp, max = 200.dp), + itemKeys = { _, item -> item }, // stable keys by item value + listState = listState, + ) + DefaultButton( + onClick = { + itemsState.value = itemsState.value.filterNot { it == "B" } + selectedIndex = -1 + listState.selectedKeys = emptySet() + } + ) { + Text("Delete B, Clear Selection") + } + DefaultButton(onClick = { itemsState.value = listOf("A", "B", "C") }) { Text("Add B") } + DefaultButton( + onClick = { + itemsState.value = emptyList() + selectedIndex = -1 + listState.selectedKeys = emptySet() + } + ) { + Text("Delete All") + } + DefaultButton( + onClick = { + itemsState.value = listOf("A", "B", "C") + selectedIndex = -1 + listState.selectedKeys = emptySet() + } + ) { + Text("Add All") + } + } + } +} diff --git a/platform/jewel/ui-tests/src/test/kotlin/org/jetbrains/jewel/ui/component/ListComboBoxUiTest.kt b/platform/jewel/ui-tests/src/test/kotlin/org/jetbrains/jewel/ui/component/ListComboBoxUiTest.kt index e605000ab60f..a54df43d5b92 100644 --- a/platform/jewel/ui-tests/src/test/kotlin/org/jetbrains/jewel/ui/component/ListComboBoxUiTest.kt +++ b/platform/jewel/ui-tests/src/test/kotlin/org/jetbrains/jewel/ui/component/ListComboBoxUiTest.kt @@ -6,6 +6,7 @@ import androidx.compose.foundation.layout.size import androidx.compose.foundation.layout.width import androidx.compose.runtime.getValue import androidx.compose.runtime.mutableIntStateOf +import androidx.compose.runtime.mutableStateOf import androidx.compose.runtime.remember import androidx.compose.runtime.setValue import androidx.compose.ui.Modifier @@ -1026,6 +1027,83 @@ class ListComboBoxUiTest { popupMenu.assertHeightIsEqualTo(500.dp) } + @Test + fun `commit mapped selection to external on popup close after delete and re-add`() { + val focusRequester = FocusRequester() + // Hoist states outside composition to mutate during the test + var items by mutableStateOf((1..5).map { "Item $it" }) + var selectedIndex by mutableIntStateOf(2) // start at "Item 3" + + composeRule.setContent { + IntUiTheme { + ListComboBox( + items = items, + selectedIndex = selectedIndex, + onSelectedItemChange = { selectedIndex = it }, + modifier = Modifier.testTag("ComboBox").width(200.dp).focusRequester(focusRequester), + // Use item text as key to allow mapping across re-adds + itemKeys = { _: Int, item: String -> item }, + ) + } + } + + // Focus and open popup + focusRequester.requestFocus() + comboBox.assertIsDisplayed().assertIsFocused().performClick() + popupMenu.assertIsDisplayed() + + // Simulate delete + re-add while popup is open + composeRule.runOnUiThread { items = emptyList() } + composeRule.waitForIdle() + composeRule.runOnUiThread { items = (1..5).map { "Item $it" } } + composeRule.waitForIdle() + + // Drive: Down then Enter + comboBox.performKeyPress(Key.DirectionDown, rule = composeRule) + comboBox.performKeyPress(Key.Enter, rule = composeRule) + + // Popup should close and selection should advance to Item 4 (index 3) + popupMenu.assertDoesNotExist() + assertEquals(3, selectedIndex) + composeRule.onNode(hasTestTag("ComboBox")).assertTextEquals("Item 4", includeEditableText = false) + } + + @Test + fun `external selection is reconciled to mapped keys on popup close when changed programmatically while visible`() { + val focusRequester = FocusRequester() + val items by mutableStateOf((1..5).map { "Item $it" }) + var selectedIndex by mutableIntStateOf(2) // start at "Item 3" + + composeRule.setContent { + IntUiTheme { + ListComboBox( + items = items, + selectedIndex = selectedIndex, + onSelectedItemChange = { selectedIndex = it }, + modifier = Modifier.testTag("ComboBox").width(200.dp).focusRequester(focusRequester), + itemKeys = { _: Int, item: String -> item }, + ) + } + } + + // Focus and open popup + focusRequester.requestFocus() + comboBox.assertIsDisplayed().assertIsFocused().performClick() + popupMenu.assertIsDisplayed() + + // Programmatic external change while popup is visible (should be gated by ListComboBoxImpl) + composeRule.runOnUiThread { selectedIndex = 3 } // "Item 4" + composeRule.waitForIdle() + + // Close without selecting anything from the popup so that commit-on-close logic reconciles divergence + comboBox.performKeyPress(Key.Enter, rule = composeRule) + + // After close, external selection should be reconciled to mapped keys (back to index 2 -> "Item 3") + popupMenu.assertDoesNotExist() + assertEquals(2, selectedIndex) + composeRule.onNode(hasTestTag("ComboBox")).assertTextEquals("Item 3", includeEditableText = false) + } + private fun editableListComboBox(): SemanticsNodeInteraction { val focusRequester = FocusRequester() composeRule.setContent { diff --git a/platform/jewel/ui/src/main/kotlin/org/jetbrains/jewel/ui/component/ListComboBox.kt b/platform/jewel/ui/src/main/kotlin/org/jetbrains/jewel/ui/component/ListComboBox.kt index 71260a08fd27..5aac150a9e85 100644 --- a/platform/jewel/ui/src/main/kotlin/org/jetbrains/jewel/ui/component/ListComboBox.kt +++ b/platform/jewel/ui/src/main/kotlin/org/jetbrains/jewel/ui/component/ListComboBox.kt @@ -42,6 +42,7 @@ import org.jetbrains.jewel.foundation.lazy.SelectableLazyListState import org.jetbrains.jewel.foundation.lazy.SelectionMode import org.jetbrains.jewel.foundation.lazy.itemsIndexed import org.jetbrains.jewel.foundation.lazy.rememberSelectableLazyListState +import org.jetbrains.jewel.foundation.lazy.tree.NoopListKeyActions import org.jetbrains.jewel.foundation.lazy.visibleItemsRange import org.jetbrains.jewel.foundation.modifier.onMove import org.jetbrains.jewel.foundation.modifier.thenIf @@ -679,6 +680,44 @@ internal fun ListComboBoxImpl( hoveredItemIndex = -1 } + fun navigateDown() { + if (items.isEmpty()) return + var currentSelection = listState.selectedItemIndex(items, itemKeys) + // When there is a preview-selected item, pressing down will actually change the + // selected value to the one underneath it (unless it's the last one) + if (hoveredItemIndex >= 0 && hoveredItemIndex < items.lastIndex) { + currentSelection = hoveredItemIndex + resetPreviewSelectedIndex() + } + setSelectedItem((currentSelection + 1).coerceAtMost(items.lastIndex)) + } + + fun navigateUp() { + if (items.isEmpty()) return + var currentSelection = listState.selectedItemIndex(items, itemKeys) + // When there is a preview-selected item, pressing up will actually change the + // selected value to the one above it (unless it's the first one) + if (hoveredItemIndex > 0) { + currentSelection = hoveredItemIndex + resetPreviewSelectedIndex() + } + setSelectedItem((currentSelection - 1).coerceAtLeast(0)) + } + + fun selectHome(): Boolean { + if (items.isEmpty()) return false + setSelectedItem(0) + resetPreviewSelectedIndex() + return true + } + + fun selectEnd(): Boolean { + if (items.isEmpty()) return false + setSelectedItem(items.lastIndex) + resetPreviewSelectedIndex() + return true + } + val contentPadding = style.metrics.popupContentPadding val popupMaxHeight = maxPopupHeight.takeOrElse { style.metrics.maxPopupHeight } @@ -693,52 +732,64 @@ internal fun ListComboBoxImpl( ) } + fun commitSelectionFromHoverOrMapped() { + val mappedIndex = listState.selectedItemIndex(items, itemKeys) + val targetIndex = + when { + hoveredItemIndex >= 0 -> hoveredItemIndex + mappedIndex >= 0 -> mappedIndex + else -> null + } + if (targetIndex != null) { + setSelectedItem(targetIndex) + resetPreviewSelectedIndex() + } + } + + fun handlePopupKeyDown(event: androidx.compose.ui.input.key.KeyEvent): Boolean = + if (!popupManager.isPopupVisible.value) false + else + when (event.key) { + Key.MoveHome, + Key.Home -> selectHome() + Key.MoveEnd -> selectEnd() + Key.DirectionDown -> { + navigateDown() + true + } + Key.DirectionUp -> { + navigateUp() + true + } + Key.Enter, + Key.NumPadEnter -> { + commitSelectionFromHoverOrMapped() + popupManager.setPopupVisible(false) + true + } + else -> false + } + ComboBoxImpl( modifier = modifier .onSizeChanged { comboBoxWidth = with(density) { it.width.toDp() } } .onPreviewKeyEvent { if (it.type != KeyEventType.KeyDown) return@onPreviewKeyEvent false - - if (it.key == Key.Enter || it.key == Key.NumPadEnter) { - if (popupManager.isPopupVisible.value && hoveredItemIndex >= 0) { - setSelectedItem(hoveredItemIndex) - resetPreviewSelectedIndex() - } - popupManager.setPopupVisible(false) - true - } else { - false - } + return@onPreviewKeyEvent handlePopupKeyDown(it) }, popupModifier = popupModifier, enabled = enabled, maxPopupHeight = popupMaxHeight, maxPopupWidth = maxPopupWidth.takeOrElse { comboBoxWidth }, - onArrowDownPress = { - var currentSelection = listState.selectedItemIndex(items, itemKeys) - - // When there is a preview-selected item, pressing down will actually change the - // selected value to the one underneath it (unless it's the last one) - if (hoveredItemIndex >= 0 && hoveredItemIndex < items.lastIndex) { - currentSelection = hoveredItemIndex - resetPreviewSelectedIndex() - } - - setSelectedItem((currentSelection + 1).coerceAtMost(items.lastIndex)) - }, - onArrowUpPress = { - var currentSelection = listState.selectedItemIndex(items, itemKeys) - - // When there is a preview-selected item, pressing up will actually change the - // selected value to the one above it (unless it's the first one) - if (hoveredItemIndex > 0) { - currentSelection = hoveredItemIndex - resetPreviewSelectedIndex() - } - - setSelectedItem((currentSelection - 1).coerceAtLeast(0)) - }, + onArrowDownPress = down@{ + if (popupManager.isPopupVisible.value) return@down + navigateDown() + }, + onArrowUpPress = up@{ + if (popupManager.isPopupVisible.value) return@up + navigateUp() + }, style = style, interactionSource = interactionSource, outline = outline, @@ -788,6 +839,8 @@ private fun PopupContent( val selectedIndex = selectedItemsIndexes.firstOrNull() if (selectedIndex != null) onSelectedItemChange(selectedIndex) }, + // Disable inner list keyboard navigation while popup is visible; navigation is handled at ComboBox level + keyActions = NoopListKeyActions, ) { itemsIndexed( items = items,