From 1250ed56efb6dbe9f865d1d3bde7c582bebbc714 Mon Sep 17 00:00:00 2001 From: Adam Brown Date: Fri, 12 Jun 2026 00:16:44 -0700 Subject: [PATCH 1/2] Support shift+click to extend the selection Shift+click now extends the selection from the cursor (or the existing selection's far anchor) to the clicked position instead of clearing it. A shared extendSelection() unifies the logic with shift+arrow, and the parallel click handlers leave the selection alone while shift is held, fixing the intermittent failure caused by them racing to clear the extension. Fixes #23 Co-Authored-By: Claude Opus 4.8 --- .../input/TextEditorKeyCommandHandler.kt | 25 +--- .../state/TextEditorSelectionManager.kt | 22 ++++ .../textEditorPointerInputHandling.kt | 43 +++++-- .../ShiftClickExtendSelectionTest.kt | 119 ++++++++++++++++++ 4 files changed, 174 insertions(+), 35 deletions(-) create mode 100644 ComposeTextEditor/src/desktopTest/kotlin/selection/ShiftClickExtendSelectionTest.kt diff --git a/ComposeTextEditor/src/commonMain/kotlin/com/darkrockstudios/texteditor/input/TextEditorKeyCommandHandler.kt b/ComposeTextEditor/src/commonMain/kotlin/com/darkrockstudios/texteditor/input/TextEditorKeyCommandHandler.kt index 23f3bcd..1f0bcc5 100644 --- a/ComposeTextEditor/src/commonMain/kotlin/com/darkrockstudios/texteditor/input/TextEditorKeyCommandHandler.kt +++ b/ComposeTextEditor/src/commonMain/kotlin/com/darkrockstudios/texteditor/input/TextEditorKeyCommandHandler.kt @@ -325,30 +325,7 @@ internal class TextEditorKeyCommandHandler { state: TextEditorState, initialPosition: CharLineOffset ) { - val currentSelection = state.selector.selection - when { - // No existing selection - start a new one - currentSelection == null -> { - state.selector.updateSelection(initialPosition, state.cursorPosition) - } - // Extend/modify existing selection - else -> { - // If we were at the start of the selection, keep the end fixed - when (initialPosition) { - currentSelection.start -> { - state.selector.updateSelection(state.cursorPosition, currentSelection.end) - } - // If we were at the end of the selection, keep the start fixed - currentSelection.end -> { - state.selector.updateSelection(currentSelection.start, state.cursorPosition) - } - // If cursor was outside selection, create new selection - else -> { - state.selector.updateSelection(initialPosition, state.cursorPosition) - } - } - } - } + state.selector.extendSelection(initialPosition, state.cursorPosition) } /** diff --git a/ComposeTextEditor/src/commonMain/kotlin/com/darkrockstudios/texteditor/state/TextEditorSelectionManager.kt b/ComposeTextEditor/src/commonMain/kotlin/com/darkrockstudios/texteditor/state/TextEditorSelectionManager.kt index 4bcd747..967038c 100644 --- a/ComposeTextEditor/src/commonMain/kotlin/com/darkrockstudios/texteditor/state/TextEditorSelectionManager.kt +++ b/ComposeTextEditor/src/commonMain/kotlin/com/darkrockstudios/texteditor/state/TextEditorSelectionManager.kt @@ -64,6 +64,28 @@ class TextEditorSelectionManager( updateSelectionRange(range) } + /** + * Extends the selection to [newPosition], holding the anchor fixed, and returns the fixed + * anchor. The anchor is the end of the existing selection opposite to [anchor]; with no + * selection, [anchor] itself becomes the anchor. Collapsing onto the anchor clears the + * selection. Shared by shift+arrow keys and shift+click so both extend identically. + */ + fun extendSelection(anchor: CharLineOffset, newPosition: CharLineOffset): CharLineOffset { + val currentSelection = _selection + val fixedAnchor = when { + currentSelection == null -> anchor + anchor == currentSelection.start -> currentSelection.end + anchor == currentSelection.end -> currentSelection.start + else -> anchor + } + if (fixedAnchor == newPosition) { + clearSelection() + } else { + updateSelection(fixedAnchor, newPosition) + } + return fixedAnchor + } + private fun makeRange(start: CharLineOffset, end: CharLineOffset): TextEditorRange { return if (isBeforeInDocument(start, end)) { TextEditorRange(start, end) diff --git a/ComposeTextEditor/src/commonMain/kotlin/com/darkrockstudios/texteditor/textEditorPointerInputHandling.kt b/ComposeTextEditor/src/commonMain/kotlin/com/darkrockstudios/texteditor/textEditorPointerInputHandling.kt index 3dcd271..5258e0b 100644 --- a/ComposeTextEditor/src/commonMain/kotlin/com/darkrockstudios/texteditor/textEditorPointerInputHandling.kt +++ b/ComposeTextEditor/src/commonMain/kotlin/com/darkrockstudios/texteditor/textEditorPointerInputHandling.kt @@ -28,12 +28,16 @@ internal fun Modifier.textEditorPointerInputHandling( .handleDragInput(state, readOnly) .handleTextInteractions(state, onSpanClick, onContextMenuRequest, readOnly) .detectMouseClicksImperatively( - onClick = { offset: Offset -> - val position = state.getOffsetAtPosition(offset) - if (!readOnly) { - state.cursor.updatePosition(position) + onClick = { offset: Offset, isShiftPressed: Boolean -> + // On shift+click handleDragInput extends the selection; clearing it here + // (handlers run in parallel) would race away the extension. + if (!isShiftPressed) { + val position = state.getOffsetAtPosition(offset) + if (!readOnly) { + state.cursor.updatePosition(position) + } + state.selector.clearSelection() } - state.selector.clearSelection() }, onDoubleClick = { offset: Offset -> val position = state.getOffsetAtPosition(offset) @@ -60,6 +64,7 @@ private fun Modifier.handleDragInput(state: TextEditorState, readOnly: Boolean): !currentEvent.buttons.isSecondaryPressed val isMouseLike = down.type == PointerType.Mouse || hasPrimaryButton val isFingerTouch = down.type == PointerType.Touch && !hasPrimaryButton + val isShiftPressed = currentEvent.keyboardModifiers.isShiftPressed val initialPosition = down.position @@ -73,8 +78,20 @@ private fun Modifier.handleDragInput(state: TextEditorState, readOnly: Boolean): } else if (isMouseLike && hasPrimaryButton) { // Only start selection drag on primary (left) mouse button // Secondary (right) click should preserve existing selection for context menu - mouseSelectionAnchor = state.getOffsetAtPosition(initialPosition) - state.selector.startSelection(position = mouseSelectionAnchor, isTouch = false) + val clickedPosition = state.getOffsetAtPosition(initialPosition) + if (isShiftPressed) { + // Shift+click extends from the cursor (or the existing selection's far end) + // to the click; this handler owns the extension so the other two pointer + // handlers must leave the selection alone while shift is held. The cursor is + // the moving end, so track it even when readOnly (as shift+arrow does), or a + // later shift+click would re-anchor from a stale cursor. + val anchor = state.cursorPosition + mouseSelectionAnchor = state.selector.extendSelection(anchor, clickedPosition) + state.cursor.updatePosition(clickedPosition) + } else { + mouseSelectionAnchor = clickedPosition + state.selector.startSelection(position = clickedPosition, isTouch = false) + } } val pointerId = down.id @@ -155,6 +172,7 @@ private fun handleSpanInteraction( clickType: SpanClickType, onSpanClick: RichSpanClickListener?, readOnly: Boolean, + isShiftPressed: Boolean = false, ): Boolean { if (findHandleAtPosition(offset, state) != null) { return true @@ -163,14 +181,16 @@ private fun handleSpanInteraction( val position = state.getOffsetAtPosition(offset) val clickedSpan = state.findSpanAtPosition(position) - if (clickType == SpanClickType.PRIMARY_CLICK || clickType == SpanClickType.TAP) { + // A shift+click extends the selection; handleDragInput owns that, so leave the + // cursor and selection untouched here rather than collapsing them. + if (!isShiftPressed && (clickType == SpanClickType.PRIMARY_CLICK || clickType == SpanClickType.TAP)) { if (!readOnly) { state.cursor.updatePosition(position) } state.selector.clearSelection() } - return if (clickedSpan != null && onSpanClick != null) { + return if (!isShiftPressed && clickedSpan != null && onSpanClick != null) { onSpanClick.invoke(clickedSpan, clickType, offset) } else { true @@ -226,6 +246,7 @@ private fun Modifier.handleTextInteractions( SpanClickType.PRIMARY_CLICK, onSpanClick, readOnly, + isShiftPressed = event.keyboardModifiers.isShiftPressed, ) didHandlePress = true } else if (hasSecondaryButton) { @@ -312,7 +333,7 @@ private fun Modifier.handleTextInteractions( @OptIn(ExperimentalTime::class) private fun Modifier.detectMouseClicksImperatively( - onClick: (Offset) -> Unit, + onClick: (Offset, Boolean) -> Unit, onDoubleClick: (Offset) -> Unit, onTripleClick: (Offset) -> Unit, ): Modifier { @@ -343,7 +364,7 @@ private fun Modifier.detectMouseClicksImperatively( val downTime = kotlin.time.Clock.System.now().toEpochMilliseconds() val downPosition = down.position - onClick(downPosition) + onClick(downPosition, currentEvent.keyboardModifiers.isShiftPressed) do { val event = awaitPointerEvent() diff --git a/ComposeTextEditor/src/desktopTest/kotlin/selection/ShiftClickExtendSelectionTest.kt b/ComposeTextEditor/src/desktopTest/kotlin/selection/ShiftClickExtendSelectionTest.kt new file mode 100644 index 0000000..f7d6254 --- /dev/null +++ b/ComposeTextEditor/src/desktopTest/kotlin/selection/ShiftClickExtendSelectionTest.kt @@ -0,0 +1,119 @@ +package selection + +import androidx.compose.ui.text.AnnotatedString +import com.darkrockstudios.texteditor.CharLineOffset +import com.darkrockstudios.texteditor.state.TextEditorState +import io.mockk.mockk +import kotlinx.coroutines.test.TestScope +import kotlin.test.BeforeTest +import kotlin.test.Test +import kotlin.test.assertEquals +import kotlin.test.assertNull + +/** + * Regression for issue #23 — shift+click should reliably extend the selection. + * + * Exercises the selection math the pointer handlers drive on a shift+click: a plain + * click positions the cursor and clears any selection; a shift+click extends from the + * cursor to the click while holding the original anchor fixed. + */ +class ShiftClickExtendSelectionTest { + + private lateinit var state: TextEditorState + + @BeforeTest + fun setup() { + state = TextEditorState( + scope = TestScope(), + measurer = mockk(relaxed = true), + initialText = AnnotatedString(""), + ) + state.setText("The quick brown fox jumps over the lazy dog") + } + + private fun offset(char: Int) = CharLineOffset(0, char) + + /** Mirrors a plain primary click: move cursor, drop any selection. */ + private fun click(char: Int) { + state.cursor.updatePosition(offset(char)) + state.selector.clearSelection() + } + + /** Mirrors a shift+primary click: extend from the cursor to the click, then move the cursor. */ + private fun shiftClick(char: Int) { + val anchor = state.cursorPosition + state.selector.extendSelection(anchor, offset(char)) + state.cursor.updatePosition(offset(char)) + } + + @Test + fun `shift+click with no selection selects from cursor to click`() { + click(4) + shiftClick(15) + + val selection = state.selector.selection + assertEquals(offset(4), selection?.start) + assertEquals(offset(15), selection?.end) + } + + @Test + fun `shift+click works the same selecting backward`() { + click(15) + shiftClick(4) + + val selection = state.selector.selection + assertEquals(offset(4), selection?.start) + assertEquals(offset(15), selection?.end) + } + + @Test + fun `a second shift+click keeps the original anchor fixed`() { + click(4) + shiftClick(9) + shiftClick(19) + + val selection = state.selector.selection + assertEquals(offset(4), selection?.start, "anchor from the first click must stay put") + assertEquals(offset(19), selection?.end) + } + + @Test + fun `shift+click can shrink the selection back toward the anchor`() { + click(4) + shiftClick(19) + shiftClick(9) + + val selection = state.selector.selection + assertEquals(offset(4), selection?.start) + assertEquals(offset(9), selection?.end) + } + + @Test + fun `shift+click crossing the anchor flips the selection around it`() { + click(10) + shiftClick(19) + shiftClick(4) + + val selection = state.selector.selection + assertEquals(offset(4), selection?.start) + assertEquals(offset(10), selection?.end, "anchor stays at the original click") + } + + @Test + fun `shift+click back onto the anchor collapses the selection`() { + click(4) + shiftClick(19) + shiftClick(4) + + assertNull(state.selector.selection) + } + + @Test + fun `a plain click after extending clears the selection`() { + click(4) + shiftClick(19) + click(9) + + assertNull(state.selector.selection) + } +} From ccd4b6633dce1095d1ee34adaae57210cd922712 Mon Sep 17 00:00:00 2001 From: Adam Brown Date: Fri, 12 Jun 2026 00:50:32 -0700 Subject: [PATCH 2/2] Address review on shift+click: readOnly cursor consistency and UI test Gate the shift+click cursor move with !readOnly so mouse-driven shift+click matches plain-click and drag behavior; selection still extends in readOnly. Add a Compose UI test that drives the real pointer-input handlers with a held Shift modifier, guarding the parallel-handler race that the existing selection-math test cannot reach. Co-Authored-By: Claude Opus 4.8 --- ComposeTextEditor/build.gradle.kts | 3 + .../textEditorPointerInputHandling.kt | 11 +- .../selection/ShiftClickPointerInputTest.kt | 102 ++++++++++++++++++ 3 files changed, 110 insertions(+), 6 deletions(-) create mode 100644 ComposeTextEditor/src/desktopTest/kotlin/selection/ShiftClickPointerInputTest.kt diff --git a/ComposeTextEditor/build.gradle.kts b/ComposeTextEditor/build.gradle.kts index 89bd04d..5076e51 100644 --- a/ComposeTextEditor/build.gradle.kts +++ b/ComposeTextEditor/build.gradle.kts @@ -65,6 +65,9 @@ kotlin { implementation(libs.mockk) implementation(libs.kotlinx.coroutines.test) implementation(libs.kotlinx.coroutines.test.jvm) + @OptIn(org.jetbrains.compose.ExperimentalComposeLibrary::class) + implementation(compose.uiTest) + implementation(compose.desktop.currentOs) } } } diff --git a/ComposeTextEditor/src/commonMain/kotlin/com/darkrockstudios/texteditor/textEditorPointerInputHandling.kt b/ComposeTextEditor/src/commonMain/kotlin/com/darkrockstudios/texteditor/textEditorPointerInputHandling.kt index 5258e0b..2b0dd4d 100644 --- a/ComposeTextEditor/src/commonMain/kotlin/com/darkrockstudios/texteditor/textEditorPointerInputHandling.kt +++ b/ComposeTextEditor/src/commonMain/kotlin/com/darkrockstudios/texteditor/textEditorPointerInputHandling.kt @@ -80,14 +80,13 @@ private fun Modifier.handleDragInput(state: TextEditorState, readOnly: Boolean): // Secondary (right) click should preserve existing selection for context menu val clickedPosition = state.getOffsetAtPosition(initialPosition) if (isShiftPressed) { - // Shift+click extends from the cursor (or the existing selection's far end) - // to the click; this handler owns the extension so the other two pointer - // handlers must leave the selection alone while shift is held. The cursor is - // the moving end, so track it even when readOnly (as shift+arrow does), or a - // later shift+click would re-anchor from a stale cursor. + // This handler owns the shift+click extension; the other two pointer + // handlers must leave the selection alone while shift is held. val anchor = state.cursorPosition mouseSelectionAnchor = state.selector.extendSelection(anchor, clickedPosition) - state.cursor.updatePosition(clickedPosition) + if (!readOnly) { + state.cursor.updatePosition(clickedPosition) + } } else { mouseSelectionAnchor = clickedPosition state.selector.startSelection(position = clickedPosition, isTouch = false) diff --git a/ComposeTextEditor/src/desktopTest/kotlin/selection/ShiftClickPointerInputTest.kt b/ComposeTextEditor/src/desktopTest/kotlin/selection/ShiftClickPointerInputTest.kt new file mode 100644 index 0000000..7e4c2c6 --- /dev/null +++ b/ComposeTextEditor/src/desktopTest/kotlin/selection/ShiftClickPointerInputTest.kt @@ -0,0 +1,102 @@ +package selection + +import androidx.compose.foundation.layout.size +import androidx.compose.ui.Modifier +import androidx.compose.ui.geometry.Offset +import androidx.compose.ui.input.key.Key +import androidx.compose.ui.test.ExperimentalTestApi +import androidx.compose.ui.test.click +import androidx.compose.ui.test.onRoot +import androidx.compose.ui.test.performKeyInput +import androidx.compose.ui.test.performMouseInput +import androidx.compose.ui.test.runComposeUiTest +import androidx.compose.ui.text.AnnotatedString +import androidx.compose.ui.unit.dp +import com.darkrockstudios.texteditor.BasicTextEditor +import com.darkrockstudios.texteditor.state.TextEditorState +import com.darkrockstudios.texteditor.state.rememberTextEditorState +import kotlin.test.Test +import kotlin.test.assertNotNull +import kotlin.test.assertTrue + +/** + * Drives the real [Modifier.textEditorPointerInputHandling] through [BasicTextEditor] to guard + * the handler race in issue #23: three parallel pointerInput handlers must not clear the + * selection a shift+click extends. Asserting on real mouse input catches a regression that the + * selection-math unit test cannot. + */ +@OptIn(ExperimentalTestApi::class) +class ShiftClickPointerInputTest { + + @Test + fun `click then shift+click extends a selection that survives the parallel handlers`() = + runComposeUiTest { + lateinit var state: TextEditorState + setContent { + state = rememberTextEditorState( + initialText = AnnotatedString("The quick brown fox jumps over the lazy dog") + ) + BasicTextEditor( + state = state, + modifier = Modifier.size(width = 400.dp, height = 200.dp), + autoFocus = true, + ) + } + waitForIdle() + + onRoot().performMouseInput { click(Offset(20f, 10f)) } + waitForIdle() + + onRoot().performKeyInput { keyDown(Key.ShiftLeft) } + onRoot().performMouseInput { + moveTo(Offset(220f, 10f)) + press() + release() + } + onRoot().performKeyInput { keyUp(Key.ShiftLeft) } + waitForIdle() + + val selection = state.selector.selection + assertNotNull(selection, "shift+click must leave a selection in place") + assertTrue( + selection.start != selection.end, + "selection must span the gap between the two clicks, not be collapsed by a racing handler", + ) + } + + @Test + fun `shift+click then drag keeps extending the selection`() = runComposeUiTest { + lateinit var state: TextEditorState + setContent { + state = rememberTextEditorState( + initialText = AnnotatedString("The quick brown fox jumps over the lazy dog") + ) + BasicTextEditor( + state = state, + modifier = Modifier.size(width = 400.dp, height = 200.dp), + autoFocus = true, + ) + } + waitForIdle() + + onRoot().performMouseInput { click(Offset(20f, 10f)) } + waitForIdle() + + onRoot().performKeyInput { keyDown(Key.ShiftLeft) } + onRoot().performMouseInput { + moveTo(Offset(120f, 10f)) + press() + moveTo(Offset(240f, 10f)) + release() + } + onRoot().performKeyInput { keyUp(Key.ShiftLeft) } + waitForIdle() + + val selection = state.selector.selection + assertNotNull(selection, "shift+click then drag must leave a selection in place") + assertTrue( + selection.start != selection.end, + "dragging after a shift+click must keep the selection spanning", + ) + } +}