From 1b0dd5a4d2c594ac7e91f8ef2fb5ec7f310900e0 Mon Sep 17 00:00:00 2001 From: Sebastiano Poggi Date: Thu, 20 Aug 2026 16:56:31 +0200 Subject: [PATCH] [JEWEL-1396] Take popup dismissal policy from PopupProperties The custom popup renderers decided both whether to dismiss a popup and whether to consume the Escape key from the shape of `onDismissRequest`: non-null meant "dismiss and consume", null meant "do neither". A caller wanting to suppress dismissal had to withhold the callback, which suppressed every dismissal path at once. `ComboBox` did exactly that while the pointer was over it, which is a pointer concern, and so Escape was consumed while nothing closed and the key never reached the surrounding dialog. Compose's own popup keeps these three concerns apart, and Jewel now does the same: `dismissOnClickOutside` governs the pointer path, `dismissOnBackPress` governs Escape, and `onDismissRequest` says only whether there is any way to act at all. Changes: * `ComboBox` and `EditableComboBox` suppress only the pointer path, via `dismissOnClickOutside`, and always pass a dismissal callback. Hovering no longer affects Escape. * Both renderers gate Escape dismissal on `dismissOnBackPress`, which neither honoured before. * Both renderers consume Escape whenever the popup is focusable, whether or not it dismissed, matching Compose's rule that any focusable popup consumes back events. A non-focusable popup no longer swallows a key it did not act on. * `JBPopupRenderer` pushes property changes onto the live `AbstractPopup`. It captured them once at creation, so a popup whose properties change could not be honoured at all. * `GotItTooltip` sets `dismissOnBackPress = false`, since it handles Escape on its anchor and the renderer must not claim the key. Without this it was measurably broken against the bridge renderer: Escape was swallowed and the tooltip stayed on screen. * `SpeedSearchArea` likewise expresses `dismissOnLoseFocus` as `dismissOnClickOutside`, so Escape always hides the search input whatever the outside-dismissal policy is. * Adds a Spectre parity test asserting the custom renderer is indistinguishable from Compose's own popup across nine scenarios, including popups with no component logic behind them, where the renderer's own decision is what is being observed. closes https://github.com/JetBrains/intellij-community/pull/3622 (cherry picked from commit 71c304a9fe976a8ac48f3ba6e7d1d6e343fb92d5) IJ-MR-220560 GitOrigin-RevId: 0a3b2988e04eac2ac1e3f50cc4c2b67364eb1772 --- platform/jewel/BUILD.bazel | 4 + .../jewel/bridge/component/JBPopupRenderer.kt | 34 +- .../popup/CustomPopupRendererSpectreTest.kt | 159 +++++-- .../popup/PopupRendererParitySpectreTest.kt | 394 ++++++++++++++++++ .../intui/standalone/popup/JDialogRenderer.kt | 13 +- platform/jewel/jewel_deps.MODULE.bazel | 28 ++ .../component/ComboBoxDismissRequestTest.kt | 147 +++++++ .../ui/component/RecordingPopupRenderer.kt | 64 +++ .../SpeedSearchAreaDismissRequestTest.kt | 73 +--- .../gotit/GotItTooltipDismissPolicyTest.kt | 57 +++ .../jetbrains/jewel/ui/component/ComboBox.kt | 11 +- .../jewel/ui/component/EditableComboBox.kt | 10 +- .../jewel/ui/component/SpeedSearchArea.kt | 6 +- .../jewel/ui/component/gotit/GotItTooltip.kt | 4 +- 14 files changed, 896 insertions(+), 108 deletions(-) create mode 100644 platform/jewel/int-ui/int-ui-standalone-tests/src/spectreTest/kotlin/org/jetbrains/jewel/intui/standalone/popup/PopupRendererParitySpectreTest.kt create mode 100644 platform/jewel/ui-tests/src/test/kotlin/org/jetbrains/jewel/ui/component/ComboBoxDismissRequestTest.kt create mode 100644 platform/jewel/ui-tests/src/test/kotlin/org/jetbrains/jewel/ui/component/RecordingPopupRenderer.kt create mode 100644 platform/jewel/ui-tests/src/test/kotlin/org/jetbrains/jewel/ui/component/gotit/GotItTooltipDismissPolicyTest.kt diff --git a/platform/jewel/BUILD.bazel b/platform/jewel/BUILD.bazel index 83fec6a65b3d..77c911e36ff2 100644 --- a/platform/jewel/BUILD.bazel +++ b/platform/jewel/BUILD.bazel @@ -17,6 +17,7 @@ kt_jvm_import( name = "spectre-core", testonly = True, jar = "@jewel_deps_spectre_core//file", + srcjar = "@jewel_deps_spectre_core_sources//file", visibility = ["//visibility:private"], ) @@ -24,6 +25,7 @@ kt_jvm_import( name = "spectre-testing", testonly = True, jar = "@jewel_deps_spectre_testing//file", + srcjar = "@jewel_deps_spectre_testing_sources//file", visibility = ["//visibility:private"], ) @@ -31,6 +33,7 @@ kt_jvm_import( name = "spectre-agent", testonly = True, jar = "@jewel_deps_spectre_agent//file", + srcjar = "@jewel_deps_spectre_agent_sources//file", visibility = ["//visibility:private"], ) @@ -38,6 +41,7 @@ kt_jvm_import( name = "spectre-recording", testonly = True, jar = "@jewel_deps_spectre_recording//file", + srcjar = "@jewel_deps_spectre_recording_sources//file", visibility = ["//visibility:private"], ) diff --git a/platform/jewel/ide-laf-bridge/src/main/kotlin/org/jetbrains/jewel/bridge/component/JBPopupRenderer.kt b/platform/jewel/ide-laf-bridge/src/main/kotlin/org/jetbrains/jewel/bridge/component/JBPopupRenderer.kt index 19801f1618bb..c28b8f98afdc 100644 --- a/platform/jewel/ide-laf-bridge/src/main/kotlin/org/jetbrains/jewel/bridge/component/JBPopupRenderer.kt +++ b/platform/jewel/ide-laf-bridge/src/main/kotlin/org/jetbrains/jewel/bridge/component/JBPopupRenderer.kt @@ -45,6 +45,7 @@ import com.intellij.openapi.ui.popup.JBPopupListener import com.intellij.openapi.ui.popup.LightweightWindowEvent import com.intellij.ui.ScreenUtil import com.intellij.ui.awt.RelativePoint +import com.intellij.ui.popup.AbstractPopup import com.intellij.ui.scale.JBUIScale import java.awt.Component import java.awt.Dimension @@ -118,6 +119,8 @@ private fun JBPopup( val currentContent = rememberUpdatedState(content) val currentPopupPositionProvider by rememberUpdatedState(popupPositionProvider) val currentOnDismissRequest by rememberUpdatedState(onDismissRequest) + val currentOnPreviewKeyEvent by rememberUpdatedState(onPreviewKeyEvent) + val currentOnKeyEvent by rememberUpdatedState(onKeyEvent) val currentProperties by rememberUpdatedState(properties) val owner = LocalComponent.current @@ -182,17 +185,29 @@ private fun JBPopup( .setCancelOnClickOutside(currentProperties.dismissOnClickOutside) .setCancelOnWindowDeactivation(currentProperties.dismissOnClickOutside) .setLocateWithinScreenBounds(false) + // AbstractPopup cancels itself on Escape whenever the handler below declines the event, which would + // close the popup behind Compose's back and leave the two out of sync. Take sole ownership instead. + .setCancelKeyEnabled(false) .setKeyEventHandler { event -> val composeEvent = event.toComposeKeyEvent() val consumed = - onPreviewKeyEvent?.invoke(composeEvent) == true || onKeyEvent?.invoke(composeEvent) == true + currentOnPreviewKeyEvent?.invoke(composeEvent) == true || + currentOnKeyEvent?.invoke(composeEvent) == true + // Whether Escape dismisses is the popup's declared policy, not something inferred from the shape + // of its callbacks. A null callback means we have no way to dismiss, so we must not claim the key. + val dismissed = + !consumed && + composeEvent.isDismissRequest() && + currentProperties.dismissOnBackPress && + currentOnDismissRequest != null - if (!consumed && composeEvent.isDismissRequest()) { + if (dismissed) { isVisible = false - true - } else { - consumed } + // A focusable popup swallows Escape whether or not it dismissed, matching Compose. A + // non-focusable one must let a key it did not act on reach whatever owns it. + val claimsDismissRequest = composeEvent.isDismissRequest() && currentProperties.focusable + consumed || dismissed || claimsDismissRequest } .addListener( object : JBPopupListener { @@ -208,6 +223,15 @@ private fun JBPopup( .createPopup() } + // The builder above only captures the properties as they were on first composition. AbstractPopup reads these + // flags live on every event, so pushing changes keeps a popup whose properties vary (a ComboBox suppressing + // click-outside dismissal while its chevron is hovered) in step without recreating the native window. + LaunchedEffect(popup, currentProperties) { + val abstractPopup = popup as? AbstractPopup ?: return@LaunchedEffect + abstractPopup.setCancelOnClickOutside(currentProperties.dismissOnClickOutside) + abstractPopup.setCancelOnWindowDeactivation(currentProperties.dismissOnClickOutside) + } + val rectValue = popupRectangle.value LaunchedEffect(rectValue) { val rectangle = rectValue ?: return@LaunchedEffect diff --git a/platform/jewel/int-ui/int-ui-standalone-tests/src/spectreTest/kotlin/org/jetbrains/jewel/intui/standalone/popup/CustomPopupRendererSpectreTest.kt b/platform/jewel/int-ui/int-ui-standalone-tests/src/spectreTest/kotlin/org/jetbrains/jewel/intui/standalone/popup/CustomPopupRendererSpectreTest.kt index d37af476b23e..ad6f77034996 100644 --- a/platform/jewel/int-ui/int-ui-standalone-tests/src/spectreTest/kotlin/org/jetbrains/jewel/intui/standalone/popup/CustomPopupRendererSpectreTest.kt +++ b/platform/jewel/int-ui/int-ui-standalone-tests/src/spectreTest/kotlin/org/jetbrains/jewel/intui/standalone/popup/CustomPopupRendererSpectreTest.kt @@ -7,6 +7,7 @@ import androidx.compose.foundation.layout.Box import androidx.compose.foundation.layout.Column import androidx.compose.foundation.layout.padding import androidx.compose.foundation.layout.width +import androidx.compose.foundation.text.input.TextFieldState import androidx.compose.runtime.Composable import androidx.compose.runtime.getValue import androidx.compose.runtime.mutableStateOf @@ -18,13 +19,17 @@ import androidx.compose.ui.awt.ComposeWindow import androidx.compose.ui.platform.testTag import androidx.compose.ui.unit.dp import androidx.compose.ui.window.application +import dev.sebastiano.spectre.core.AutomatorNode import dev.sebastiano.spectre.core.ComposeAutomator import dev.sebastiano.spectre.core.RobotDriver import dev.sebastiano.spectre.testing.runSpectreTest import java.awt.event.KeyEvent import java.util.concurrent.atomic.AtomicReference import kotlin.concurrent.thread +import kotlin.test.assertEquals +import kotlin.test.assertFalse import kotlin.test.assertTrue +import kotlin.time.Duration.Companion.milliseconds import kotlinx.coroutines.CoroutineScope import kotlinx.coroutines.delay import org.jetbrains.jewel.foundation.ExperimentalJewelApi @@ -33,7 +38,7 @@ import org.jetbrains.jewel.intui.standalone.theme.IntUiTheme import org.jetbrains.jewel.intui.standalone.window.Window as JewelWindow import org.jetbrains.jewel.ui.component.ComboBox import org.jetbrains.jewel.ui.component.DefaultButton -import org.jetbrains.jewel.ui.component.PopupManager +import org.jetbrains.jewel.ui.component.EditableComboBox import org.jetbrains.jewel.ui.component.PopupMenu import org.jetbrains.jewel.ui.component.SpeedSearchArea import org.jetbrains.jewel.ui.component.Text @@ -44,6 +49,11 @@ import org.junit.jupiter.api.Test // Headful, and deliberately not a jps_test: the app under test must keep a standalone-only runtime closure, // with no IntelliJ Platform classes on the classpath. Runs in CI on any agent with a display: // bazel test //platform/jewel/int-ui/int-ui-standalone-tests:jewel-intUi-standalone-spectre-tests +// +// Every assertion here is on what is actually on screen. The renderers under test put popups in their own native +// windows, so a popup can be gone while Compose still believes it is showing, or the reverse: asserting on the +// components' own visibility state alone would pass straight through the bug class these tests exist to catch. +// The state is still checked, but only ever as a cross-check that it agrees with the screen. class CustomPopupRendererSpectreTest { @Test fun `escape closes a hovered combo box`(): Unit = runSpectreTestWithCustomPopupRenderer { @@ -53,12 +63,12 @@ class CustomPopupRendererSpectreTest { val automator = ComposeAutomator.inProcess(RobotDriver.synthetic(app.awaitWindow())) automator.click(automator.waitForNode(tag = REGULAR_COMBO_TAG)) - automator.waitForNode(tag = REGULAR_POPUP_VISIBLE_TAG, text = "true") + automator.waitForNode(tag = COMBO_BOX_POPUP_TAG) // The pointer remains inside the ComboBox after click, reproducing the hover path that // previously let the popup's JDialogRenderer consume Escape as a no-op dismissal. automator.pressKey(KeyEvent.VK_ESCAPE) - automator.waitForNode(tag = REGULAR_POPUP_VISIBLE_TAG, text = "false") + automator.waitUntilGone("The ComboBox popup") { findByTestTag(COMBO_BOX_POPUP_TAG) } } finally { app.stop() } @@ -74,7 +84,7 @@ class CustomPopupRendererSpectreTest { val menuButton = automator.waitForNode(tag = MENU_BUTTON_TAG) automator.click(menuButton) - automator.waitForNode(tag = MENU_VISIBLE_TAG, text = "true") + automator.waitForNode(tag = MENU_POPUP_TAG) // Spectre routes synthetic key events to the window under its injected pointer. Move that pointer back // to the owner window to cover focusable popups that have not taken native focus yet. @@ -82,7 +92,7 @@ class CustomPopupRendererSpectreTest { automator.waitForIdle() automator.pressKey(KeyEvent.VK_ESCAPE) - automator.waitForNode(tag = MENU_VISIBLE_TAG, text = "false") + automator.waitUntilGone("The menu popup") { findByTestTag(MENU_POPUP_TAG) } } finally { app.stop() } @@ -97,21 +107,101 @@ class CustomPopupRendererSpectreTest { val automator = ComposeAutomator.inProcess(RobotDriver.synthetic(app.awaitWindow())) automator.click(automator.waitForNode(tag = SPEED_SEARCH_COMBO_TAG)) - automator.waitForNode(tag = SPEED_SEARCH_POPUP_VISIBLE_TAG, text = "true") + automator.waitForNode(tag = COMBO_BOX_POPUP_TAG) + automator.typeText("Alpha") - automator.waitForNode(tag = SPEED_SEARCH_QUERY_TAG, text = "Alpha") - automator.waitForNode(tag = SPEED_SEARCH_VISIBLE_TAG, text = "true") + val searchInput = automator.waitForNode(tag = SPEED_SEARCH_INPUT_TAG) + assertEquals("Alpha", searchInput.editableText, "The speed search field should show what was typed") + // The first Escape belongs to speed search, and must not reach the popup behind it. automator.pressKey(KeyEvent.VK_ESCAPE) - automator.waitForNode(tag = SPEED_SEARCH_VISIBLE_TAG, text = "false") - automator.waitForNode(tag = SPEED_SEARCH_POPUP_VISIBLE_TAG, text = "true") + automator.waitUntilGone("The speed search field") { findByTestTag(SPEED_SEARCH_INPUT_TAG) } + assertTrue( + automator.isPresent { findByTestTag(COMBO_BOX_POPUP_TAG) }, + "The first Escape dismissed speed search, so the popup behind it must still be open", + ) + // Only once speed search is gone does Escape reach the popup. automator.pressKey(KeyEvent.VK_ESCAPE) - automator.waitForNode(tag = SPEED_SEARCH_POPUP_VISIBLE_TAG, text = "false") + automator.waitUntilGone("The speed searchable ComboBox popup") { findByTestTag(COMBO_BOX_POPUP_TAG) } } finally { app.stop() } } + + @Test + fun `clicking the chevron of an open combo box closes it without reopening it`(): Unit = + runSpectreTestWithCustomPopupRenderer { + val app = SpectreTestApplication() + app.start() + try { + val driver = RobotDriver.synthetic(app.awaitWindow()) + val automator = ComposeAutomator.inProcess(driver) + + val comboBox = automator.waitForNode(tag = REGULAR_COMBO_TAG) + automator.click(comboBox) + automator.waitForNode(tag = COMBO_BOX_POPUP_TAG) + + // Suppressing dismissal while the ComboBox is hovered is what stops the click-outside dismissal + // from closing the popup before the chevron's own handler runs and immediately reopens it. + val bounds = comboBox.boundsOnScreen + driver.click(bounds.x + bounds.width - CHEVRON_INSET, bounds.y + bounds.height / 2) + + automator.waitUntilGone("The ComboBox popup") { findByTestTag(COMBO_BOX_POPUP_TAG) } + automator.waitForIdle() + assertFalse( + automator.isPresent { findByTestTag(COMBO_BOX_POPUP_TAG) }, + "The chevron click must not reopen the popup it just closed", + ) + } finally { + app.stop() + } + } + + @Test + fun `escape closes a hovered editable combo box`(): Unit = runSpectreTestWithCustomPopupRenderer { + val app = SpectreTestApplication() + app.start() + try { + val driver = RobotDriver.synthetic(app.awaitWindow()) + val automator = ComposeAutomator.inProcess(driver) + + // Only the chevron opens the popup here; clicking the text field just focuses it. + val comboBox = automator.waitForNode(tag = EDITABLE_COMBO_TAG) + val bounds = comboBox.boundsOnScreen + driver.click(bounds.x + bounds.width - CHEVRON_INSET, bounds.y + bounds.height / 2) + automator.waitForNode(tag = COMBO_BOX_POPUP_TAG) + + // The pointer is left on the chevron, which is the hover path that previously had the renderer + // consume Escape as a no-op dismissal. + automator.pressKey(KeyEvent.VK_ESCAPE) + automator.waitUntilGone("The EditableComboBox popup") { findByTestTag(COMBO_BOX_POPUP_TAG) } + } finally { + app.stop() + } + } +} + +/** + * Waits until [find] matches nothing, across every window Spectre tracks. + * + * Spectre only ships a wait-for-presence helper, but a popup lives in its own native window, and these tests need to + * watch that window go away. + */ +private suspend fun ComposeAutomator.waitUntilGone( + description: String, + find: ComposeAutomator.() -> List, +) { + repeat(POLL_ATTEMPTS) { + if (!isPresent(find)) return + delay(POLL_INTERVAL_MS.milliseconds) + } + error("$description was still on screen after ${POLL_ATTEMPTS * POLL_INTERVAL_MS} ms") +} + +private fun ComposeAutomator.isPresent(find: ComposeAutomator.() -> List): Boolean { + refreshWindows() + return find().isNotEmpty() } private fun runSpectreTestWithCustomPopupRenderer(block: suspend CoroutineScope.() -> Unit): Unit = runSpectreTest { @@ -121,25 +211,22 @@ private fun runSpectreTestWithCustomPopupRenderer(block: suspend CoroutineScope. @Composable private fun PopupEscapeScreen() { - val regularPopupManager = remember { PopupManager() } + val editableTextFieldState = remember { TextFieldState("Editable") } var menuVisible by remember { mutableStateOf(false) } - var speedSearchPopupVisible by remember { mutableStateOf(false) } val speedSearchState = rememberSpeedSearchState() Column(modifier = Modifier.padding(24.dp), verticalArrangement = Arrangement.spacedBy(12.dp)) { - Text( - text = regularPopupManager.isPopupVisible.value.toString(), - modifier = Modifier.testTag(REGULAR_POPUP_VISIBLE_TAG), - ) - ComboBox( - labelText = "Regular ComboBox", - modifier = Modifier.testTag(REGULAR_COMBO_TAG).width(240.dp), - popupManager = regularPopupManager, - ) { + ComboBox(labelText = "Regular ComboBox", modifier = Modifier.testTag(REGULAR_COMBO_TAG).width(240.dp)) { Text("Regular popup content") } - Text(text = menuVisible.toString(), modifier = Modifier.testTag(MENU_VISIBLE_TAG)) + EditableComboBox( + textFieldState = editableTextFieldState, + modifier = Modifier.testTag(EDITABLE_COMBO_TAG).width(240.dp), + ) { + Text("Editable popup content") + } + Box { DefaultButton(onClick = { menuVisible = true }, modifier = Modifier.testTag(MENU_BUTTON_TAG)) { Text("Show menu") @@ -152,22 +239,19 @@ private fun PopupEscapeScreen() { true }, horizontalAlignment = Alignment.Start, + modifier = Modifier.testTag(MENU_POPUP_TAG), ) { selectableItem(selected = false, onClick = {}) { Text("Menu item") } } } } - Text(text = speedSearchPopupVisible.toString(), modifier = Modifier.testTag(SPEED_SEARCH_POPUP_VISIBLE_TAG)) - Text(text = speedSearchState.isVisible.toString(), modifier = Modifier.testTag(SPEED_SEARCH_VISIBLE_TAG)) - Text(text = speedSearchState.searchText, modifier = Modifier.testTag(SPEED_SEARCH_QUERY_TAG)) SpeedSearchArea(state = speedSearchState, dismissOnLoseFocus = false) { SpeedSearchableComboBox( items = listOf("Alpha", "Beta", "Gamma"), selectedIndex = 0, onSelectedItemChange = {}, modifier = Modifier.testTag(SPEED_SEARCH_COMBO_TAG).width(240.dp), - onPopupVisibleChange = { speedSearchPopupVisible = it }, ) } } @@ -198,17 +282,26 @@ private class SpectreTestApplication(private val content: @Composable () -> Unit window.get()?.let { return it } - delay(100) + delay(100.milliseconds) } error("The Compose test window was not created") } } +/** Set by both ComboBox and EditableComboBox on their popup content, including the speed searchable variant. */ +private const val COMBO_BOX_POPUP_TAG = "Jewel.ComboBox.Popup" + +/** Set by SpeedSearchArea on its search field. */ +private const val SPEED_SEARCH_INPUT_TAG = "SpeedSearchArea.Input" + private const val REGULAR_COMBO_TAG = "spectre.regularCombo" -private const val REGULAR_POPUP_VISIBLE_TAG = "spectre.regularPopupVisible" +private const val EDITABLE_COMBO_TAG = "spectre.editableCombo" private const val MENU_BUTTON_TAG = "spectre.menuButton" -private const val MENU_VISIBLE_TAG = "spectre.menuVisible" +private const val MENU_POPUP_TAG = "spectre.menuPopup" private const val SPEED_SEARCH_COMBO_TAG = "spectre.speedSearchCombo" -private const val SPEED_SEARCH_POPUP_VISIBLE_TAG = "spectre.speedSearchPopupVisible" -private const val SPEED_SEARCH_VISIBLE_TAG = "spectre.speedSearchVisible" -private const val SPEED_SEARCH_QUERY_TAG = "spectre.speedSearchQuery" + +/** Distance from the ComboBox's trailing edge that reliably lands on the chevron rather than the label. */ +private const val CHEVRON_INSET = 8 + +private const val POLL_ATTEMPTS = 100 +private const val POLL_INTERVAL_MS = 100L diff --git a/platform/jewel/int-ui/int-ui-standalone-tests/src/spectreTest/kotlin/org/jetbrains/jewel/intui/standalone/popup/PopupRendererParitySpectreTest.kt b/platform/jewel/int-ui/int-ui-standalone-tests/src/spectreTest/kotlin/org/jetbrains/jewel/intui/standalone/popup/PopupRendererParitySpectreTest.kt new file mode 100644 index 000000000000..a86fe97fa3e7 --- /dev/null +++ b/platform/jewel/int-ui/int-ui-standalone-tests/src/spectreTest/kotlin/org/jetbrains/jewel/intui/standalone/popup/PopupRendererParitySpectreTest.kt @@ -0,0 +1,394 @@ +@file:OptIn(ExperimentalJewelApi::class) + +package org.jetbrains.jewel.intui.standalone.popup + +// Differential test: the custom renderer must behave exactly as Compose's own popup does, for every component +// that opens a popup. Compose's renderer is the oracle here, so a divergence is a bug in JDialogRenderer even +// when both outcomes look individually reasonable. + +import androidx.compose.foundation.layout.Box +import androidx.compose.foundation.layout.Column +import androidx.compose.foundation.layout.padding +import androidx.compose.foundation.layout.width +import androidx.compose.foundation.text.input.TextFieldState +import androidx.compose.runtime.Composable +import androidx.compose.runtime.getValue +import androidx.compose.runtime.mutableStateOf +import androidx.compose.runtime.remember +import androidx.compose.runtime.setValue +import androidx.compose.ui.Alignment +import androidx.compose.ui.Modifier +import androidx.compose.ui.awt.ComposeWindow +import androidx.compose.ui.input.key.Key +import androidx.compose.ui.input.key.KeyEventType +import androidx.compose.ui.input.key.key +import androidx.compose.ui.input.key.onKeyEvent +import androidx.compose.ui.input.key.type +import androidx.compose.ui.platform.testTag +import androidx.compose.ui.unit.dp +import androidx.compose.ui.window.PopupProperties +import androidx.compose.ui.window.application +import dev.sebastiano.spectre.core.ComposeAutomator +import dev.sebastiano.spectre.core.RobotDriver +import dev.sebastiano.spectre.testing.runSpectreTest +import java.awt.event.KeyEvent +import java.util.concurrent.atomic.AtomicBoolean +import java.util.concurrent.atomic.AtomicReference +import kotlin.concurrent.thread +import kotlin.test.assertEquals +import kotlin.time.Duration.Companion.milliseconds +import kotlinx.coroutines.delay +import org.jetbrains.jewel.foundation.ExperimentalJewelApi +import org.jetbrains.jewel.foundation.JewelFlags +import org.jetbrains.jewel.intui.standalone.theme.IntUiTheme +import org.jetbrains.jewel.intui.standalone.window.Window as JewelWindow +import org.jetbrains.jewel.ui.component.ComboBox +import org.jetbrains.jewel.ui.component.DefaultButton +import org.jetbrains.jewel.ui.component.EditableComboBox +import org.jetbrains.jewel.ui.component.PopupContainer +import org.jetbrains.jewel.ui.component.PopupMenu +import org.jetbrains.jewel.ui.component.SpeedSearchArea +import org.jetbrains.jewel.ui.component.Text +import org.jetbrains.jewel.ui.component.gotit.GotItButtons +import org.jetbrains.jewel.ui.component.gotit.GotItTooltip +import org.jetbrains.jewel.ui.component.rememberSpeedSearchState +import org.jetbrains.jewel.ui.component.search.SpeedSearchableComboBox +import org.junit.jupiter.api.Test + +private const val OPEN_TAG = "matrix.open" +private const val POPUP_TAG = "Jewel.ComboBox.Popup" +private const val MENU_TAG = "matrix.menuPopup" +private const val SEARCH_INPUT_TAG = "SpeedSearchArea.Input" +private const val RAW_POPUP_TAG = "parity.rawPopup" + +/** Set when Escape reaches the owner window's Compose tree, i.e. nothing above it consumed the key. */ +private val hostSawEscape = AtomicBoolean(false) + +private class Case( + val name: String, + val open: suspend (ComposeAutomator, RobotDriver) -> Unit, + val measure: suspend (ComposeAutomator) -> String, + /** + * The absolute measurement both renderers must produce, for scenarios with no component logic masking the + * renderer's own decision. Differential parity alone cannot catch both renderers being wrong the same way. + */ + val expect: String? = null, + val content: @Composable () -> Unit, +) + +private fun ComposeAutomator.hasTag(tag: String) = findByTestTag(tag).isNotEmpty() + +private fun ComposeAutomator.hasText(text: String) = findByText(text).isNotEmpty() + +private fun host() = "hostSawEscape=" + hostSawEscape.get() + +private val openByClick: suspend (ComposeAutomator, RobotDriver) -> Unit = { a, _ -> + a.click(a.waitForNode(tag = OPEN_TAG)) + a.waitForNode(tag = POPUP_TAG) +} + +/** Clicks the chevron rather than the centre, which on an EditableComboBox only focuses the text field. */ +private val openByChevron: suspend (ComposeAutomator, RobotDriver) -> Unit = { a, driver -> + val bounds = a.waitForNode(tag = OPEN_TAG).boundsOnScreen + driver.click(bounds.x + bounds.width - 8, bounds.y + bounds.height / 2) + a.waitForNode(tag = POPUP_TAG) +} + +private val CONTROL_CASE = + // Control: no popup at all. If this does not report hostSawEscape=true, the probe itself is broken + // and every other hostSawEscape reading in this run is meaningless. + Case( + name = "control(noPopup)", + open = { a, _ -> a.click(a.waitForNode(tag = OPEN_TAG)) }, + measure = { "hostSawEscape=" + hostSawEscape.get() }, + ) { + DefaultButton(onClick = {}, modifier = Modifier.testTag(OPEN_TAG)) { Text("Plain button") } + } + +private val CASES = + listOf( + Case(name = "comboBox", open = openByClick, measure = { "popupOpen=" + it.hasTag(POPUP_TAG) + " " + host() }) { + ComboBox(labelText = "Combo", modifier = Modifier.testTag(OPEN_TAG).width(240.dp)) { Text("Content") } + }, + Case( + name = "editableComboBox", + open = openByChevron, + measure = { "popupOpen=" + it.hasTag(POPUP_TAG) + " " + host() }, + ) { + EditableComboBox( + textFieldState = remember { TextFieldState("Editable") }, + modifier = Modifier.testTag(OPEN_TAG).width(240.dp), + ) { + Text("Content") + } + }, + Case( + name = "popupMenu(focusable=true)", + open = { a, _ -> + a.click(a.waitForNode(tag = OPEN_TAG)) + a.waitForNode(tag = MENU_TAG) + }, + measure = { "menuOpen=" + it.hasTag(MENU_TAG) + " " + host() }, + ) { + MenuWithSubmenu() + }, + Case( + name = "submenu(nested)", + open = { a, _ -> + a.click(a.waitForNode(tag = OPEN_TAG)) + a.waitForNode(tag = MENU_TAG) + a.click(a.waitForNode(text = "Open submenu")) + a.waitForNode(text = "Sub item") + }, + measure = { + "parentMenuOpen=" + it.hasTag(MENU_TAG) + " submenuOpen=" + it.hasText("Sub item") + " " + host() + }, + ) { + MenuWithSubmenu() + }, + Case( + name = "gotIt(withButtons)", + open = { a, _ -> + a.click(a.waitForNode(tag = OPEN_TAG)) + a.waitForNode(text = "Tooltip body") + }, + measure = { "tooltipOpen=" + it.hasText("Tooltip body") + " " + host() }, + ) { + GotIt(buttons = GotItButtons.default()) + }, + Case( + name = "gotIt(noButtons)", + open = { a, _ -> + a.click(a.waitForNode(tag = OPEN_TAG)) + a.waitForNode(text = "Tooltip body") + }, + measure = { "tooltipOpen=" + it.hasText("Tooltip body") + " " + host() }, + ) { + GotIt(buttons = GotItButtons.None) + }, + Case( + name = "rawPopup(dismissOnBackPress=true)", + open = { a, _ -> + a.click(a.waitForNode(tag = OPEN_TAG)) + a.waitForNode(tag = RAW_POPUP_TAG) + }, + measure = { "popupOpen=" + it.hasTag(RAW_POPUP_TAG) + " " + host() }, + // Escape dismisses, and the focusable popup consumes the key whether it dismissed or not. + expect = "popupOpen=false hostSawEscape=false", + ) { + RawPopup(dismissOnBackPress = true) + }, + Case( + name = "rawPopup(nonFocusable)", + open = { a, _ -> + a.click(a.waitForNode(tag = OPEN_TAG)) + a.waitForNode(tag = RAW_POPUP_TAG) + }, + measure = { "popupOpen=" + it.hasTag(RAW_POPUP_TAG) + " " + host() }, + // A non-focusable popup cannot act on Escape at all: it must neither dismiss nor consume, so the + // key reaches the host. Proves unconsumed Escape is forwarded while a native popup window exists. + expect = "popupOpen=true hostSawEscape=true", + ) { + RawPopup(focusable = false) + }, + Case( + name = "rawPopup(dismissOnBackPress=false)", + open = { a, _ -> + a.click(a.waitForNode(tag = OPEN_TAG)) + a.waitForNode(tag = RAW_POPUP_TAG) + }, + measure = { "popupOpen=" + it.hasTag(RAW_POPUP_TAG) + " " + host() }, + // No dismissal, but the focusable popup still consumes the key. + expect = "popupOpen=true hostSawEscape=false", + ) { + RawPopup(dismissOnBackPress = false) + }, + Case( + name = "speedSearchCombo(escape#1)", + open = { a, _ -> + a.click(a.waitForNode(tag = OPEN_TAG)) + a.waitForNode(tag = POPUP_TAG) + a.typeText("Alpha") + a.waitForNode(tag = SEARCH_INPUT_TAG) + }, + measure = { + "popupOpen=" + it.hasTag(POPUP_TAG) + " searchOpen=" + it.hasTag(SEARCH_INPUT_TAG) + " " + host() + }, + ) { + SpeedSearchCombo() + }, + ) + +class PopupRendererParitySpectreTest { + @Test + fun `the custom renderer matches Compose's own popup for every component`(): Unit = runSpectreTest { + // The control doubles as the probe's canary: if Escape does not reach the host window with no popup + // involved, every other hostSawEscape reading is meaningless, and the parity comparison below would + // pass vacuously on a dead probe. Gate on it explicitly. + for (custom in listOf(false, true)) { + assertEquals( + expected = "hostSawEscape=true", + actual = measure(custom = custom, case = CONTROL_CASE), + message = + "Escape probe is broken on ${label(custom)}: the host window did not see Escape with no popup showing", + ) + } + + val divergences = mutableListOf() + for (case in CASES) { + val withCompose = measure(custom = false, case = case) + val withJDialog = measure(custom = true, case = case) + if (withCompose != withJDialog) { + divergences += "${case.name}: defaultCompose[$withCompose] != JDialogRenderer[$withJDialog]" + } + val expected = case.expect + if (expected != null) { + if (withCompose != expected) { + divergences += "${case.name}: defaultCompose[$withCompose] != expected[$expected]" + } + if (withJDialog != expected) { + divergences += "${case.name}: JDialogRenderer[$withJDialog] != expected[$expected]" + } + } + } + assertEquals( + emptyList(), + divergences, + "JDialogRenderer must be indistinguishable from Compose's own popup for these components", + ) + } + + private suspend fun measure(custom: Boolean, case: Case): String { + JewelFlags.useCustomPopupRenderer = custom + val app = MatrixApp(case.content) + app.start() + try { + val driver = RobotDriver.synthetic(app.awaitWindow()) + val automator = ComposeAutomator.inProcess(driver) + case.open(automator, driver) + automator.waitForIdle() + hostSawEscape.set(false) + + automator.pressKey(KeyEvent.VK_ESCAPE) + delay(1500.milliseconds) + automator.refreshWindows() + + val result = case.measure(automator) + println("PARITY renderer=" + label(custom) + " case=" + case.name + " " + result) + return result + } finally { + app.stop() + delay(500.milliseconds) + } + } + + private fun label(custom: Boolean) = if (custom) "JDialogRenderer" else "defaultCompose" +} + +@Composable +private fun MenuWithSubmenu() { + var visible by remember { mutableStateOf(false) } + Box { + DefaultButton(onClick = { visible = true }, modifier = Modifier.testTag(OPEN_TAG)) { Text("Menu") } + if (visible) { + PopupMenu( + onDismissRequest = { + visible = false + true + }, + horizontalAlignment = Alignment.Start, + modifier = Modifier.testTag(MENU_TAG), + ) { + submenu(submenu = { selectableItem(false, onClick = {}) { Text("Sub item") } }) { Text("Open submenu") } + } + } + } +} + +@Composable +private fun GotIt(buttons: GotItButtons) { + var visible by remember { mutableStateOf(false) } + GotItTooltip(text = "Tooltip body", visible = visible, onDismiss = { visible = false }, buttons = buttons) { + DefaultButton(onClick = { visible = true }, modifier = Modifier.testTag(OPEN_TAG)) { Text("Show") } + } +} + +/** + * A popup with no component logic behind it: nothing else handles Escape, so what happens is entirely the renderer's + * decision. Every other case in this matrix has a component that closes itself on Escape, which masks whatever the + * renderer does. + */ +@Composable +private fun RawPopup(focusable: Boolean = true, dismissOnBackPress: Boolean = true) { + var visible by remember { mutableStateOf(false) } + Box { + DefaultButton(onClick = { visible = true }, modifier = Modifier.testTag(OPEN_TAG)) { Text("Open raw popup") } + if (visible) { + PopupContainer( + onDismissRequest = { visible = false }, + horizontalAlignment = Alignment.Start, + modifier = Modifier.testTag(RAW_POPUP_TAG), + popupProperties = PopupProperties(focusable = focusable, dismissOnBackPress = dismissOnBackPress), + ) { + Text("Raw popup content") + } + } + } +} + +@Composable +private fun SpeedSearchCombo() { + val state = rememberSpeedSearchState() + SpeedSearchArea(state = state, dismissOnLoseFocus = false) { + SpeedSearchableComboBox( + items = listOf("Alpha", "Beta", "Gamma"), + selectedIndex = 0, + onSelectedItemChange = {}, + modifier = Modifier.testTag(OPEN_TAG).width(240.dp), + ) + } +} + +private class MatrixApp(private val content: @Composable () -> Unit) { + private val exitApplication = AtomicReference<(() -> Unit)?>(null) + private val window = AtomicReference(null) + + fun start() { + thread(name = "spectre-matrix-window", isDaemon = true) { + application(exitProcessOnExit = false) { + exitApplication.set(::exitApplication) + JewelWindow(onCloseRequest = ::exitApplication, title = "Jewel popup matrix") { + this@MatrixApp.window.compareAndSet(null, window) + IntUiTheme { + Column( + modifier = + Modifier.padding(24.dp).onKeyEvent { event -> + if (event.type == KeyEventType.KeyDown && event.key == Key.Escape) { + hostSawEscape.set(true) + } + false + } + ) { + content() + } + } + } + } + } + } + + fun stop() { + exitApplication.get()?.invoke() + window.set(null) + } + + suspend fun awaitWindow(): ComposeWindow { + repeat(100) { + window.get()?.let { + return it + } + delay(100.milliseconds) + } + error("The Compose test window was not created") + } +} diff --git a/platform/jewel/int-ui/int-ui-standalone/src/main/kotlin/org/jetbrains/jewel/intui/standalone/popup/JDialogRenderer.kt b/platform/jewel/int-ui/int-ui-standalone/src/main/kotlin/org/jetbrains/jewel/intui/standalone/popup/JDialogRenderer.kt index 73c8a50892e7..0ca99cc07bec 100644 --- a/platform/jewel/int-ui/int-ui-standalone/src/main/kotlin/org/jetbrains/jewel/intui/standalone/popup/JDialogRenderer.kt +++ b/platform/jewel/int-ui/int-ui-standalone/src/main/kotlin/org/jetbrains/jewel/intui/standalone/popup/JDialogRenderer.kt @@ -430,12 +430,21 @@ private fun JPopupImpl( currentOnPreviewKeyEvent?.invoke(composeEvent) == true || currentOnKeyEvent?.invoke(composeEvent) == true val dismissRequest = currentOnDismissRequest - val dismissed = !consumed && composeEvent.isDismissRequest() && dismissRequest != null + // Whether Escape dismisses is the popup's declared policy; a null callback just means we have + // no way to act on it, so we must not claim the key either. + val dismissed = + !consumed && + composeEvent.isDismissRequest() && + currentProperties.dismissOnBackPress && + dismissRequest != null if (dismissed) { dismissRequest.invoke() } - if (consumed || dismissed) { + // A focusable popup swallows Escape whether or not it dismissed, matching Compose. A + // non-focusable one must let a key it did not act on reach whatever owns it. + val claimsDismissRequest = composeEvent.isDismissRequest() && currentProperties.focusable + if (consumed || dismissed || claimsDismissRequest) { event.consume() } } diff --git a/platform/jewel/jewel_deps.MODULE.bazel b/platform/jewel/jewel_deps.MODULE.bazel index 2cb82ada997a..3b34d97c8da4 100644 --- a/platform/jewel/jewel_deps.MODULE.bazel +++ b/platform/jewel/jewel_deps.MODULE.bazel @@ -30,6 +30,13 @@ http_file( url = "https://cache-redirector.jetbrains.com/repo1.maven.org/maven2/dev/sebastiano/spectre/spectre-core/0.5.0/spectre-core-0.5.0.jar", ) +http_file( + name = "jewel_deps_spectre_core_sources", + downloaded_file_path = "spectre-core-0.5.0-sources.jar", + sha256 = "798968ef359092d51beca43dbd2707792a21405761bf4f817a98addac038e1f9", + url = "https://cache-redirector.jetbrains.com/repo1.maven.org/maven2/dev/sebastiano/spectre/spectre-core/0.5.0/spectre-core-0.5.0-sources.jar", +) + http_file( name = "jewel_deps_spectre_testing", downloaded_file_path = "spectre-testing-0.5.0.jar", @@ -37,6 +44,13 @@ http_file( url = "https://cache-redirector.jetbrains.com/repo1.maven.org/maven2/dev/sebastiano/spectre/spectre-testing/0.5.0/spectre-testing-0.5.0.jar", ) +http_file( + name = "jewel_deps_spectre_testing_sources", + downloaded_file_path = "spectre-testing-0.5.0-sources.jar", + sha256 = "2f95170eb876feeb284261077257a019eb0073e89f91e24322fef07730972917", + url = "https://cache-redirector.jetbrains.com/repo1.maven.org/maven2/dev/sebastiano/spectre/spectre-testing/0.5.0/spectre-testing-0.5.0-sources.jar", +) + # Compile dependency of spectre-testing; unused by the in-process lane, but must resolve. http_file( name = "jewel_deps_spectre_agent", @@ -45,6 +59,13 @@ http_file( url = "https://cache-redirector.jetbrains.com/repo1.maven.org/maven2/dev/sebastiano/spectre/spectre-agent/0.5.0/spectre-agent-0.5.0.jar", ) +http_file( + name = "jewel_deps_spectre_agent_sources", + downloaded_file_path = "spectre-agent-0.5.0-sources.jar", + sha256 = "8117ee97f119d1fae12ee44a467ceb6d91f6c552edaea1a3d32d033e43e7a6e4", + url = "https://cache-redirector.jetbrains.com/repo1.maven.org/maven2/dev/sebastiano/spectre/spectre-agent/0.5.0/spectre-agent-0.5.0-sources.jar", +) + # Runtime dependency of spectre-testing. http_file( name = "jewel_deps_spectre_recording", @@ -53,6 +74,13 @@ http_file( url = "https://cache-redirector.jetbrains.com/repo1.maven.org/maven2/dev/sebastiano/spectre/spectre-recording/0.5.0/spectre-recording-0.5.0.jar", ) +http_file( + name = "jewel_deps_spectre_recording_sources", + downloaded_file_path = "spectre-recording-0.5.0-sources.jar", + sha256 = "7c84c5c3e9bd2774d06183d802fbad5a8fe4bb3bd2bb2ea271b7e5d5b1bb61b8", + url = "https://cache-redirector.jetbrains.com/repo1.maven.org/maven2/dev/sebastiano/spectre/spectre-recording/0.5.0/spectre-recording-0.5.0-sources.jar", +) + # Runtime dependency of spectre-core, not otherwise in the monorepo. http_file( name = "jewel_deps_androidx_tracing_wire_desktop", diff --git a/platform/jewel/ui-tests/src/test/kotlin/org/jetbrains/jewel/ui/component/ComboBoxDismissRequestTest.kt b/platform/jewel/ui-tests/src/test/kotlin/org/jetbrains/jewel/ui/component/ComboBoxDismissRequestTest.kt new file mode 100644 index 000000000000..d350d8656f35 --- /dev/null +++ b/platform/jewel/ui-tests/src/test/kotlin/org/jetbrains/jewel/ui/component/ComboBoxDismissRequestTest.kt @@ -0,0 +1,147 @@ +// Copyright 2000-2026 JetBrains s.r.o. and contributors. Use of this source code is governed by the Apache 2.0 license. +package org.jetbrains.jewel.ui.component + +import androidx.compose.foundation.layout.width +import androidx.compose.foundation.text.input.TextFieldState +import androidx.compose.runtime.Composable +import androidx.compose.runtime.CompositionLocalProvider +import androidx.compose.runtime.remember +import androidx.compose.ui.Modifier +import androidx.compose.ui.geometry.Offset +import androidx.compose.ui.platform.testTag +import androidx.compose.ui.test.MouseInjectionScope +import androidx.compose.ui.test.junit4.v2.createComposeRule +import androidx.compose.ui.test.onNodeWithTag +import androidx.compose.ui.test.performMouseInput +import androidx.compose.ui.unit.dp +import kotlin.test.assertFalse +import kotlin.test.assertTrue +import org.jetbrains.jewel.foundation.ExperimentalJewelApi +import org.jetbrains.jewel.foundation.JewelFlags +import org.jetbrains.jewel.intui.standalone.theme.IntUiTheme +import org.junit.Rule +import org.junit.Test + +/** + * Both combo boxes suppress the *pointer* dismissal path while the pointer is over them, so that clicking the chevron + * to close an open popup does not have the click-outside dismissal close it first and the chevron's own handler + * immediately reopen it. + * + * That suppression belongs in `PopupProperties.dismissOnClickOutside`, which is what renderers consult for the pointer + * path, and it must leave Escape alone. Expressing it by withholding `onDismissRequest` instead would suppress every + * dismissal path at once, so Escape would be swallowed with nothing closing. + */ +@OptIn(ExperimentalJewelApi::class) +class ComboBoxDismissRequestTest { + @get:Rule val composeRule = createComposeRule() + + @Test + fun `combo box allows pointer dismissal when the pointer is elsewhere`() { + val renderer = recordPopup(hoverAt = null) { popupManager -> ComboBox(popupManager) } + + assertTrue(renderer.properties.isNotEmpty(), "The popup was never rendered") + assertTrue(renderer.properties.last().dismissOnClickOutside, "An unhovered combo box must be dismissable") + assertTrue(renderer.dismissRequests.last() != null, "A combo box must always be able to dismiss") + } + + @Test + fun `combo box suppresses only pointer dismissal while it is hovered`() { + // The whole ComboBox drives the hover flag, so its centre is as good a hover target as the chevron. + val renderer = recordPopup(hoverAt = { center }) { popupManager -> ComboBox(popupManager) } + + assertFalse( + renderer.properties.last().dismissOnClickOutside, + "A hovered combo box must opt out of the pointer dismissal path", + ) + assertTrue( + renderer.properties.last().dismissOnBackPress, + "Hovering must not disable Escape: it is a separate dismissal path", + ) + assertTrue(renderer.dismissRequests.last() != null, "A combo box must always be able to dismiss") + } + + @Test + fun `editable combo box allows pointer dismissal when the pointer is elsewhere`() { + val renderer = recordPopup(hoverAt = null) { popupManager -> EditableComboBox(popupManager) } + + assertTrue(renderer.properties.isNotEmpty(), "The popup was never rendered") + assertTrue(renderer.properties.last().dismissOnClickOutside, "An unhovered editable combo box is dismissable") + } + + @Test + fun `editable combo box suppresses only pointer dismissal while its chevron is hovered`() { + // Unlike ComboBox, only the text field and the chevron drive the hover flags here, and the tagged node also + // spans the popup, so aim at the chevron: the right-hand end of the first row. + val renderer = + recordPopup(hoverAt = { Offset(width - CHEVRON_INSET, CHEVRON_INSET) }) { popupManager -> + EditableComboBox(popupManager) + } + + assertFalse( + renderer.properties.last().dismissOnClickOutside, + "An editable combo box with a hovered chevron must opt out of the pointer dismissal path", + ) + assertTrue( + renderer.properties.last().dismissOnBackPress, + "Hovering must not disable Escape: it is a separate dismissal path", + ) + } + + @Composable + private fun ComboBox(popupManager: PopupManager) { + ComboBox(labelText = "Label", modifier = comboBoxModifier, popupManager = popupManager) { + Text("Popup content") + } + } + + @Composable + private fun EditableComboBox(popupManager: PopupManager) { + EditableComboBox( + textFieldState = remember { TextFieldState("Value") }, + modifier = comboBoxModifier, + popupManager = popupManager, + ) { + Text("Popup content") + } + } + + private fun recordPopup( + hoverAt: (MouseInjectionScope.() -> Offset)?, + content: @Composable (PopupManager) -> Unit, + ): RecordingPopupRenderer { + val renderer = RecordingPopupRenderer() + lateinit var popupManager: PopupManager + val oldUseCustomPopupRenderer = JewelFlags.useCustomPopupRenderer + JewelFlags.useCustomPopupRenderer = true + try { + composeRule.setContent { + IntUiTheme { + CompositionLocalProvider(LocalPopupRenderer provides renderer) { + popupManager = remember { PopupManager() } + content(popupManager) + } + } + } + composeRule.runOnIdle { popupManager.setPopupVisible(true) } + composeRule.waitForIdle() + + if (hoverAt != null) { + composeRule.onNodeWithTag(COMBO_BOX_TAG).performMouseInput { + // moveTo, not updatePointerTo: onHover keys off Enter, which only a dispatched move produces. + moveTo(hoverAt()) + advanceEventTime() + } + composeRule.waitForIdle() + } + } finally { + JewelFlags.useCustomPopupRenderer = oldUseCustomPopupRenderer + } + return renderer + } + + private val comboBoxModifier + get() = Modifier.testTag(COMBO_BOX_TAG).width(240.dp) +} + +private const val COMBO_BOX_TAG = "Jewel.Test.ComboBox" +private const val CHEVRON_INSET = 8f diff --git a/platform/jewel/ui-tests/src/test/kotlin/org/jetbrains/jewel/ui/component/RecordingPopupRenderer.kt b/platform/jewel/ui-tests/src/test/kotlin/org/jetbrains/jewel/ui/component/RecordingPopupRenderer.kt new file mode 100644 index 000000000000..ae9aa8ca3e1d --- /dev/null +++ b/platform/jewel/ui-tests/src/test/kotlin/org/jetbrains/jewel/ui/component/RecordingPopupRenderer.kt @@ -0,0 +1,64 @@ +// Copyright 2000-2026 JetBrains s.r.o. and contributors. Use of this source code is governed by the Apache 2.0 license. +package org.jetbrains.jewel.ui.component + +import androidx.compose.foundation.shape.CornerSize +import androidx.compose.runtime.Composable +import androidx.compose.ui.input.key.KeyEvent +import androidx.compose.ui.unit.IntSize +import androidx.compose.ui.window.PopupPositionProvider +import androidx.compose.ui.window.PopupProperties + +/** + * A [PopupRenderer] that records the `onDismissRequest` it is handed, and renders nothing. + * + * Renderers treat a null `onDismissRequest` as "this popup does not want renderer-driven dismissal", and must then + * neither dismiss nor consume the key. Components that instead pass a callback which decides whether to dismiss look + * identical from here, so the renderer swallows Escape while nothing closes: this renderer makes that distinction + * observable in a headless test. + * + * The content is deliberately dropped rather than composed inline. A real popup is its own window, so composing it into + * the caller's layout instead would let it overlap the component that opened it and swallow the pointer events a hover + * test depends on. + */ +internal class RecordingPopupRenderer : PopupRenderer { + val dismissRequests: MutableList<(() -> Unit)?> = mutableListOf() + val properties: MutableList = mutableListOf() + + @Suppress("OVERRIDE_DEPRECATION") + @Composable + override fun Popup( + popupPositionProvider: PopupPositionProvider, + properties: PopupProperties, + onDismissRequest: (() -> Unit)?, + onPreviewKeyEvent: ((KeyEvent) -> Boolean)?, + onKeyEvent: ((KeyEvent) -> Boolean)?, + cornerSize: CornerSize, + content: @Composable () -> Unit, + ) { + Popup( + popupPositionProvider = popupPositionProvider, + properties = properties, + onDismissRequest = onDismissRequest, + onPreviewKeyEvent = onPreviewKeyEvent, + onKeyEvent = onKeyEvent, + cornerSize = cornerSize, + windowShape = null, + content = content, + ) + } + + @Composable + override fun Popup( + popupPositionProvider: PopupPositionProvider, + properties: PopupProperties, + onDismissRequest: (() -> Unit)?, + onPreviewKeyEvent: ((KeyEvent) -> Boolean)?, + onKeyEvent: ((KeyEvent) -> Boolean)?, + cornerSize: CornerSize, + windowShape: ((IntSize) -> java.awt.Shape)?, + content: @Composable () -> Unit, + ) { + dismissRequests += onDismissRequest + this.properties += properties + } +} diff --git a/platform/jewel/ui-tests/src/test/kotlin/org/jetbrains/jewel/ui/component/SpeedSearchAreaDismissRequestTest.kt b/platform/jewel/ui-tests/src/test/kotlin/org/jetbrains/jewel/ui/component/SpeedSearchAreaDismissRequestTest.kt index b83d0150d571..7678939e4ac2 100644 --- a/platform/jewel/ui-tests/src/test/kotlin/org/jetbrains/jewel/ui/component/SpeedSearchAreaDismissRequestTest.kt +++ b/platform/jewel/ui-tests/src/test/kotlin/org/jetbrains/jewel/ui/component/SpeedSearchAreaDismissRequestTest.kt @@ -1,15 +1,9 @@ // Copyright 2000-2026 JetBrains s.r.o. and contributors. Use of this source code is governed by the Apache 2.0 license. package org.jetbrains.jewel.ui.component -import androidx.compose.foundation.shape.CornerSize -import androidx.compose.runtime.Composable import androidx.compose.runtime.CompositionLocalProvider -import androidx.compose.ui.input.key.KeyEvent import androidx.compose.ui.test.junit4.v2.createComposeRule -import androidx.compose.ui.unit.IntSize -import androidx.compose.ui.window.PopupPositionProvider -import androidx.compose.ui.window.PopupProperties -import kotlin.test.assertNull +import kotlin.test.assertFalse import kotlin.test.assertTrue import org.jetbrains.jewel.foundation.ExperimentalJewelApi import org.jetbrains.jewel.foundation.JewelFlags @@ -22,18 +16,31 @@ class SpeedSearchAreaDismissRequestTest { @get:Rule val composeRule = createComposeRule() @Test - fun `passes null dismiss request when dismiss on lose focus is disabled`() { + fun `suppresses outside dismissal when dismiss on lose focus is disabled`() { val renderer = recordSpeedSearchPopup(dismissOnLoseFocus = false) - assertTrue(renderer.dismissRequests.isNotEmpty()) - assertNull(renderer.dismissRequests.last()) + assertTrue(renderer.properties.isNotEmpty(), "The popup was never rendered") + assertFalse(renderer.properties.last().dismissOnClickOutside, "Outside dismissal should be suppressed") } @Test - fun `passes dismiss request when dismiss on lose focus is enabled`() { + fun `allows outside dismissal when dismiss on lose focus is enabled`() { val renderer = recordSpeedSearchPopup(dismissOnLoseFocus = true) - assertTrue(renderer.dismissRequests.any { it != null }) + assertTrue(renderer.properties.last().dismissOnClickOutside, "Outside dismissal should be allowed") + } + + @Test + fun `keeps escape enabled regardless of dismiss on lose focus`() { + for (dismissOnLoseFocus in listOf(false, true)) { + val renderer = recordSpeedSearchPopup(dismissOnLoseFocus = dismissOnLoseFocus) + + assertTrue( + renderer.properties.last().dismissOnBackPress, + "Escape must always hide the search input, whatever the outside-dismissal policy is", + ) + assertTrue(renderer.dismissRequests.last() != null, "Speed search must always be able to hide its input") + } } private fun recordSpeedSearchPopup(dismissOnLoseFocus: Boolean): RecordingPopupRenderer { @@ -62,45 +69,3 @@ class SpeedSearchAreaDismissRequestTest { return renderer } } - -private class RecordingPopupRenderer : PopupRenderer { - val dismissRequests = mutableListOf<(() -> Unit)?>() - - @Suppress("OVERRIDE_DEPRECATION") - @Composable - override fun Popup( - popupPositionProvider: PopupPositionProvider, - properties: PopupProperties, - onDismissRequest: (() -> Unit)?, - onPreviewKeyEvent: ((KeyEvent) -> Boolean)?, - onKeyEvent: ((KeyEvent) -> Boolean)?, - cornerSize: CornerSize, - content: @Composable () -> Unit, - ) { - Popup( - popupPositionProvider = popupPositionProvider, - properties = properties, - onDismissRequest = onDismissRequest, - onPreviewKeyEvent = onPreviewKeyEvent, - onKeyEvent = onKeyEvent, - cornerSize = cornerSize, - windowShape = null, - content = content, - ) - } - - @Composable - override fun Popup( - popupPositionProvider: PopupPositionProvider, - properties: PopupProperties, - onDismissRequest: (() -> Unit)?, - onPreviewKeyEvent: ((KeyEvent) -> Boolean)?, - onKeyEvent: ((KeyEvent) -> Boolean)?, - cornerSize: CornerSize, - windowShape: ((IntSize) -> java.awt.Shape)?, - content: @Composable () -> Unit, - ) { - dismissRequests += onDismissRequest - content() - } -} diff --git a/platform/jewel/ui-tests/src/test/kotlin/org/jetbrains/jewel/ui/component/gotit/GotItTooltipDismissPolicyTest.kt b/platform/jewel/ui-tests/src/test/kotlin/org/jetbrains/jewel/ui/component/gotit/GotItTooltipDismissPolicyTest.kt new file mode 100644 index 000000000000..236171a1130f --- /dev/null +++ b/platform/jewel/ui-tests/src/test/kotlin/org/jetbrains/jewel/ui/component/gotit/GotItTooltipDismissPolicyTest.kt @@ -0,0 +1,57 @@ +// Copyright 2000-2026 JetBrains s.r.o. and contributors. Use of this source code is governed by the Apache 2.0 license. +package org.jetbrains.jewel.ui.component.gotit + +import androidx.compose.runtime.CompositionLocalProvider +import androidx.compose.ui.test.junit4.v2.createComposeRule +import kotlin.test.assertFalse +import kotlin.test.assertTrue +import org.jetbrains.jewel.foundation.ExperimentalJewelApi +import org.jetbrains.jewel.foundation.JewelFlags +import org.jetbrains.jewel.intui.standalone.theme.IntUiTheme +import org.jetbrains.jewel.ui.component.LocalPopupRenderer +import org.jetbrains.jewel.ui.component.RecordingPopupRenderer +import org.junit.Rule +import org.junit.Test + +/** + * `GotItTooltip` handles Escape itself, on its anchor rather than inside the popup, so it has to declare that the + * renderer should keep its hands off the key. + * + * Without this, a renderer that honours the default `dismissOnBackPress = true` will dismiss the popup — invoking a + * callback that does nothing, since the tooltip's visibility is owned by the caller — and consume Escape on the way, so + * the anchor's own handler never runs and the tooltip stays on screen. That was measured against the IDE bridge + * renderer, which is the one with no window-ownership check to fall back on. + */ +@OptIn(ExperimentalJewelApi::class) +class GotItTooltipDismissPolicyTest { + @get:Rule val composeRule = createComposeRule() + + @Test + fun `opts out of renderer-driven escape dismissal`() { + val renderer = RecordingPopupRenderer() + val oldUseCustomPopupRenderer = JewelFlags.useCustomPopupRenderer + JewelFlags.useCustomPopupRenderer = true + try { + composeRule.setContent { + IntUiTheme { + CompositionLocalProvider(LocalPopupRenderer provides renderer) { + GotItTooltip(text = "Body", visible = true, onDismiss = {}) {} + } + } + } + composeRule.waitForIdle() + } finally { + JewelFlags.useCustomPopupRenderer = oldUseCustomPopupRenderer + } + + assertTrue(renderer.properties.isNotEmpty(), "The tooltip popup was never rendered") + assertFalse( + renderer.properties.last().dismissOnBackPress, + "GotItTooltip handles Escape on its anchor, so the renderer must neither dismiss nor consume it", + ) + assertFalse( + renderer.properties.last().dismissOnClickOutside, + "GotItTooltip is dismissed by its own buttons or timeout, not by clicking away", + ) + } +} diff --git a/platform/jewel/ui/src/main/kotlin/org/jetbrains/jewel/ui/component/ComboBox.kt b/platform/jewel/ui/src/main/kotlin/org/jetbrains/jewel/ui/component/ComboBox.kt index 3b7a024ce4d6..467effee2dcb 100644 --- a/platform/jewel/ui/src/main/kotlin/org/jetbrains/jewel/ui/component/ComboBox.kt +++ b/platform/jewel/ui/src/main/kotlin/org/jetbrains/jewel/ui/component/ComboBox.kt @@ -389,11 +389,7 @@ internal fun ComboBoxImpl( val maxHeight = maxPopupHeight.takeOrElse { style.metrics.maxPopupHeight } PopupContainer( - onDismissRequest = { - if (!chevronHovered) { - popupManager.setPopupVisible(false) - } - }, + onDismissRequest = { popupManager.setPopupVisible(false) }, modifier = Modifier.testTag("Jewel.ComboBox.Popup") .heightIn(max = maxHeight) @@ -401,7 +397,10 @@ internal fun ComboBoxImpl( .then(popupModifier) .onClick { popupManager.setPopupVisible(false) }, horizontalAlignment = horizontalPopupAlignment, - popupProperties = PopupProperties(focusable = false), + // Suppressing the pointer path while the ComboBox is hovered stops the click-outside + // dismissal from firing on pointer down, before the chevron's own toggle handler runs and + // reopens the popup it just closed. Escape is governed separately, and stays enabled. + popupProperties = PopupProperties(focusable = false, dismissOnClickOutside = !chevronHovered), style = popupStyle, popupPositionProvider = popupPositionProvider, content = popupContent, diff --git a/platform/jewel/ui/src/main/kotlin/org/jetbrains/jewel/ui/component/EditableComboBox.kt b/platform/jewel/ui/src/main/kotlin/org/jetbrains/jewel/ui/component/EditableComboBox.kt index bcdc9614d1fc..44c14153c4c9 100644 --- a/platform/jewel/ui/src/main/kotlin/org/jetbrains/jewel/ui/component/EditableComboBox.kt +++ b/platform/jewel/ui/src/main/kotlin/org/jetbrains/jewel/ui/component/EditableComboBox.kt @@ -191,11 +191,7 @@ public fun EditableComboBox( val popupVisible by popupManager.isPopupVisible if (popupVisible) { PopupContainer( - onDismissRequest = { - if (!chevronHovered && !textFieldHovered) { - popupManager.setPopupVisible(false) - } - }, + onDismissRequest = { popupManager.setPopupVisible(false) }, modifier = Modifier.testTag("Jewel.ComboBox.Popup") .semantics { contentDescription = "Jewel.EditableComboBox.Popup" } @@ -204,7 +200,9 @@ public fun EditableComboBox( .then(popupModifier) .onClick { popupManager.setPopupVisible(false) }, horizontalAlignment = Alignment.Start, - popupProperties = PopupProperties(focusable = false), + // See ComboBox: only the pointer path is suppressed while hovered; Escape stays enabled. + popupProperties = + PopupProperties(focusable = false, dismissOnClickOutside = !chevronHovered && !textFieldHovered), content = popupContent, ) } diff --git a/platform/jewel/ui/src/main/kotlin/org/jetbrains/jewel/ui/component/SpeedSearchArea.kt b/platform/jewel/ui/src/main/kotlin/org/jetbrains/jewel/ui/component/SpeedSearchArea.kt index 33bf9d55fa83..36d9744f91f3 100644 --- a/platform/jewel/ui/src/main/kotlin/org/jetbrains/jewel/ui/component/SpeedSearchArea.kt +++ b/platform/jewel/ui/src/main/kotlin/org/jetbrains/jewel/ui/component/SpeedSearchArea.kt @@ -49,6 +49,7 @@ import androidx.compose.ui.layout.onFirstVisible import androidx.compose.ui.platform.testTag import androidx.compose.ui.text.TextStyle import androidx.compose.ui.unit.dp +import androidx.compose.ui.window.PopupProperties import androidx.compose.ui.window.rememberComponentRectPositionProvider import java.awt.event.KeyEvent as AWTKeyEvent import kotlinx.coroutines.CoroutineDispatcher @@ -415,7 +416,10 @@ private fun SpeedSearchInput( Popup( popupPositionProvider = rememberComponentRectPositionProvider(anchor, alignment), - onDismissRequest = if (dismissOnLoseFocus) ({ speedSearchState.hideSearch() }) else null, + onDismissRequest = { speedSearchState.hideSearch() }, + // Only the outside-click path is conditional. Escape always hides the search input, which is what makes + // the first Escape belong to speed search and the second to whatever the search is layered over. + properties = PopupProperties(dismissOnClickOutside = dismissOnLoseFocus), ) { val focusRequester = remember { FocusRequester() } diff --git a/platform/jewel/ui/src/main/kotlin/org/jetbrains/jewel/ui/component/gotit/GotItTooltip.kt b/platform/jewel/ui/src/main/kotlin/org/jetbrains/jewel/ui/component/gotit/GotItTooltip.kt index 6f1675fe451e..61204368a2e9 100644 --- a/platform/jewel/ui/src/main/kotlin/org/jetbrains/jewel/ui/component/gotit/GotItTooltip.kt +++ b/platform/jewel/ui/src/main/kotlin/org/jetbrains/jewel/ui/component/gotit/GotItTooltip.kt @@ -275,7 +275,9 @@ public fun GotItTooltip( popupPositionProvider = rememberGotItTooltipBalloonPopupPositionProvider(gotItBalloonPosition, anchor, padding = offset), cornerSize = CornerSize(style.metrics.cornerRadius), - properties = PopupProperties(focusable = false, dismissOnClickOutside = false), + // Escape is handled on the anchor above, so the renderer must neither dismiss nor consume it. + properties = + PopupProperties(focusable = false, dismissOnBackPress = false, dismissOnClickOutside = false), onDismissRequest = {}, windowShape = { logicalSize -> createBalloonAwtShape(