Skip to content

feat(dom): lvt-fx:text-select — character-range selection over diff text - #140

Merged
adnaan merged 3 commits into
mainfrom
text-select
Jul 1, 2026
Merged

feat(dom): lvt-fx:text-select — character-range selection over diff text#140
adnaan merged 3 commits into
mainfrom
text-select

Conversation

@adnaan

@adnaan adnaan commented Jul 1, 2026

Copy link
Copy Markdown
Contributor

Adds lvt-fx:text-select, a native-selection capture directive so a host app can let users comment on a word / phrase / multi-line span — not just whole lines. Mirrors the existing region-select arm/sweep/teardown lifecycle.

What it does

  • textOffsetsFromSelection (exported, unit-tested): resolves a native selection to rune-offset {fromLine,fromCol,toLine,toCol,side,text} against [data-line]/[data-side] rows, using a [data-line-text] descendant as the gutter-excluding text container. Rejects cross-side and out-of-content boundaries.
  • Mouse/touch: debounced selectionchange → floating Comment button, positioned below the selection on coarse pointers so iOS's native selection callout (an OS layer that beats z-index) doesn't cover it.
  • Keyboard: a client-only block caret — visible on load, ←/→ move the column, ↑/↓ follow the host's line cursor, Shift+arrows extend a selection via getSelection().modify() on the non-editable text (no contenteditable), Ctrl/Alt = word, Enter commits.
  • Capture-phase click guard so a drag-select's completing click never fires the host's line action.

Offsets are rune counts (matches Go's []rune), relying on the host keeping [data-line-text]'s textContent equal to the raw line. Styling is overridable via --lvt-text-select-* / --lvt-text-caret-* custom properties.

Tests

tests/text-select.test.ts (jsdom): word / cross-token / multi-line / emoji-rune / gutter-exclusion / cross-side / collapsed / outside-host. Full suite green (781).

🤖 Generated with Claude Code

Adds a native-selection capture directive so a host can let users comment on
a word / phrase / multi-line span (not just whole lines). Mirrors
region-select's arm/sweep/teardown lifecycle.

- textOffsetsFromSelection: resolves a native selection to rune-offset
  {fromLine,fromCol,toLine,toCol,side,text} against [data-line]/[data-side]
  rows with a [data-line-text] gutter-excluding container; rejects cross-side
  and out-of-content boundaries.
- Debounced selectionchange trigger → floating Comment button (below the
  selection on coarse pointers so iOS's native callout doesn't cover it).
- Client-only keyboard block caret: visible on load, ←/→ move column, ↑/↓
  follow the host's line cursor, Shift+arrows extend via getSelection().modify()
  on non-editable text (no contenteditable), Ctrl/Alt = word, Enter commits.
- Capture-phase click guard so a drag-select's click never fires the host's
  line action.

Styling is overridable via --lvt-text-select-* / --lvt-text-caret-* custom
properties. Covered by tests/text-select.test.ts (jsdom).

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Ey41dRwDpRgLtVHZjhYLKt
@claude

claude Bot commented Jul 1, 2026

Copy link
Copy Markdown

Reviewed the diff (dom/directives.ts, livetemplate-client.ts, tests/text-select.test.ts). The selection-resolution logic (textOffsetsFromSelection, colWithin, nodePointAtRune, rune-vs-UTF16 handling) is solid and well covered by unit tests, including the gutter-exclusion and old/new-side-boundary edge cases.

One real bug in the keyboard path:

onKeydown's Enter/Escape branches don't check isEditableFocus() (dom/directives.ts around the new onKeydown). The handler is registered as a window-level capture listener, so it runs before any other Enter handler regardless of focus. isEditableFocus() is only consulted for the arrow-key branch, further down. Concretely: a user selects code text via Shift+Arrow (keyboard selection, which this PR explicitly adds support for), then Tabs focus into an unrelated <textarea> (e.g. the comment composer) without pressing Escape or clicking the Comment button — Tab does not collapse the document selection in most browsers. If they then press Enter to submit/newline in that textarea, this capture-phase handler sees haveSel === true, calls preventDefault()/stopPropagation(), and fires confirmSelection() instead of letting the textarea handle Enter. Worth gating the Enter/Escape branches on !isEditableFocus() (or scoping them to only fire when focus/selection is actually relevant to the host) the same way the arrow-key branch already does.

Minor/architectural note (not blocking): attachTextSelect attaches one window-level keydown capture listener and one document-level selectionchange listener per armed element, and activeCursorLine()/caret state is entirely local to each instance. handleTextSelectDirectives is written to support multiple simultaneously-armed elements (it loops over querySelectorAll matches), but if two elements are ever armed at once, every keystroke drives both instances' carets independently since stopPropagation() doesn't stop sibling listeners on the same window target. Probably fine given the current single-diff-view use case, but worth a comment noting the assumption if it's load-bearing.

Nothing else stood out — no XSS/injection risk (button text set via textContent, no innerHTML), cleanup/teardown mirrors the existing area/region-select lifecycle pattern correctly.

…cused

Addresses the claude-review finding on #140. onKeydown is a window-CAPTURE
listener; only the arrow branch checked isEditableFocus(), so a live document
selection + Tab into the composer textarea meant Enter fired confirmSelection()
instead of a newline. Bail on isEditableFocus() before the Enter/Escape branches.
Adds a comment on the single-armed-host assumption and jsdom regression tests.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Ey41dRwDpRgLtVHZjhYLKt
@claude

claude Bot commented Jul 1, 2026

Copy link
Copy Markdown

Reviewed the text-select directive (dom/directives.ts). The rune-offset resolution logic (textOffsetsFromSelection, colWithin, nodePointAtRune) is careful and well-tested — good coverage of emoji/surrogate pairs, gutter exclusion, and cross-side rejection.

One correctness concern worth addressing before merge:

Global keydown capture isn't scoped to the diff. onKeydown is registered via window.addEventListener("keydown", onKeydown, true) and only bails when isEditableFocus() (focus in an INPUT/TEXTAREA/SELECT/contentEditable). It does not check whether the user has actually interacted with or focused the diff. Since activeCursorLine() falls back to the first [data-line] element when there's no .is-cursor line, this means: as long as a text-select host is mounted anywhere on the page and focus isn't in a form field, every ArrowLeft/ArrowRight keypress app-wide gets preventDefault() + stopPropagation() and silently moves the (possibly invisible/off-screen) diff caret — even if the user is interacting with an unrelated widget (custom dropdown, carousel, etc.) that also listens for arrow keys. Enter/Escape are similarly hijacked whenever a selection happens to be live. Consider gating on the host having DOM focus or a hover/active state, not just "focus isn't in a form field."

Self-acknowledged multi-instance issue. The comment above the window.addEventListener call (line ~487) already flags that this assumes exactly one armed text-select host at a time — each armed element adds its own window-capture listener, so if two are ever armed simultaneously (e.g., split-diff view, multiple open files), every keystroke drives both carets since stopPropagation doesn't stop sibling listeners on the same target. Since this is a real constraint rather than a hypothetical, it'd be good to either enforce it (warn/no-op on a second arm) or confirm the host app guarantees single-instance today and will continue to.

Everything else — the button positioning (coarse-pointer offset for iOS's native callout), the click-capture guard for drag-select, and the cleanup/sweep lifecycle mirroring region-select — looks solid.

Round-2 review follow-up (#140): the window-capture keydown listener swallowed
Left/Right/Enter/Escape page-wide whenever a host was mounted and no form field
was focused, so an unrelated arrow-key widget would lose its keys. Now handle
keys only when the interaction belongs to the diff — nothing else focused
(page-load/body, so the on-load caret stays drivable) or focus within the host.
Regression tests added.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Ey41dRwDpRgLtVHZjhYLKt
@claude

claude Bot commented Jul 1, 2026

Copy link
Copy Markdown

Reviewed the diff (dom/directives.ts text-select feature + wiring + tests). Solid implementation with good test coverage of the selection-resolution logic and the two keyboard-scoping regressions. A few things worth a look:

  1. Global arrow-key hijack when nothing is focused (dom/directives.ts onKeydown, ~line 464-467). scoped is true whenever document.activeElement is body/documentElement/null — i.e. the common "nothing focused yet" state on page load. In that state, any ArrowLeft/ArrowRight/Enter/Escape keydown anywhere on the page gets preventDefault() + stopPropagation() by this window-capture listener, even if the user's interaction has nothing to do with the diff view. On a host page with other keyboard-driven widgets that rely on document-level bubbling without requiring focus (carousels, custom dropdowns, etc.), this would silently break them whenever the diff view happens to be mounted. Worth scoping this further (e.g. only when the host is visible/in viewport, or requiring at least a prior click/focus into the host) rather than defaulting to "capture everything until something else grabs focus."

  2. Single-instance assumption is unguarded (line ~497-501). The code comments call out that two armed text-select hosts would each add their own window keydown listener and "every keystroke would drive both carets," but there's no runtime check/warning if that assumption is violated. Since handleTextSelectDirectives is written generically (arms every match of the selector), it'd be easy for a future caller to arm two hosts and get silently-broken caret behavior. Consider at least a console.warn when textSelectArmed.size > 1.

  3. Click-capture guard is coarser than the comment suggests (onClickCapture, line ~434). It swallows any click on the host whenever a non-collapsed selection exists inside it, not just the completing click of a drag-select. So a stray lingering selection (e.g. from a previous interaction) could suppress clicks on unrelated interactive elements inside the host until the selection is cleared. Probably fine in practice given the debounced button flow, but worth double-checking against real usage with clickable content inside line rows.

Nothing blocking — the rune/offset math, side-boundary handling, and cleanup/sweep logic all look correct, and the new tests exercise the tricky parts (surrogate pairs, gutter exclusion, old/new boundary, keyboard focus gating) well.

@adnaan
adnaan merged commit 392c829 into main Jul 1, 2026
6 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant