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(