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,