spec: opt-in native left-drag selection in TUIs (#10353)#10552
spec: opt-in native left-drag selection in TUIs (#10353)#10552spalagu wants to merge 1 commit intowarpdotdev:masterfrom
Conversation
Product spec + tech spec for warpdotdevGH-10353, derived from a working reference implementation (spalagu/warp:fix/left-drag-selection, f1c38c7). product.md: - One opt-in boolean (default false), backwards-compatible - Behavior matrix covering all mouse-event types in mouse-reporting TUIs - Settings UI / command palette / context flag exposure - Success criteria + manual validation across Claude Code / vim / htop / tmux tech.md: - 7 call-site enumeration with intent of each is_left_button value - One-branch addition to should_intercept_mouse, placed between Shift early-return and SGR forwarding gate - Naming: is_left_button (matches existing helper-parameter idiom) rather than bare_left_button (which would imply modifier-stripping responsibility this helper does not own) - Test coverage: existing view_tests.rs signature update + new decision-matrix tests - Risk table + comparison vs competing specs (warpdotdev#10476, warpdotdev#10455, warpdotdev#10408) The spec is grounded in a working implementation rather than speculation — every design decision maps to a verified line in f1c38c7.
|
I'm starting a first review of this spec-only pull request. You can view the conversation on Warp. I completed the review and no human review was requested for this pull request. Comment Powered by Oz |
github-actions
Bot
added
the
external-contributor
![oz-for-oss[bot]](https://avatars.githubusercontent.com/in/3450139?s=60&v=4){kind=small}
There was a problem hiding this comment.
Overview
Reviewed the product and technical specs for opt-in native left-drag selection in mouse-reporting TUIs. The specs cover the user workflow, settings surfaces, rollout, and validation, but the proposed implementation contract currently contradicts the requirement that Cmd/Ctrl/Option mouse gestures continue forwarding to the TUI.
Concerns
- The technical spec says
is_left_buttonis enough because modifier-click/drag does not flow through the left-button interception paths, but the current left-button handlers receive modifier state and onlyShiftis passed separately. Implementing the spec as written would intercept modifier-left gestures when the setting is enabled, contrary to the product spec and success criteria.
Verdict
Found: 0 critical, 1 important, 0 suggestions
Request changes
Comment /oz-review on this pull request to retrigger a review (up to 3 times on the same pull request).
Powered by Oz
|
|
||
| `should_intercept_scroll` calls `should_intercept_mouse(..., is_left_button=false, ...)` so scroll behavior is unchanged. | ||
|
|
||
| **Why `is_left_button` and not `bare_left_button`** (vs the alternative spec PR #10476): in the existing call-site model, modifier-click and modifier-drag do not currently flow through `should_intercept_mouse`'s left-button paths — they either route via the `shift` parameter (already handled) or via separate handlers (right-click, context menu). Adding a `bare_*` qualifier would imply a modifier-stripping responsibility that doesn't belong here. Keeping the name `is_left_button` matches the existing style of boolean parameters in adjacent helpers (`is_alt_screen`, `is_mouse_tracking`). |
There was a problem hiding this comment.
left_mouse_down and mouse_dragged receive Cmd/Ctrl/Option modifiers, but the proposed helper only gets shift plus is_left_button, so enabling the setting would intercept modifier-left gestures that the product spec says must still forward. Specify a bare-left predicate or pass full modifiers into should_intercept_mouse, and require tests for Cmd/Ctrl/Option forwarding.
1 participant
Summary
Product spec + tech spec for #10353, derived from a working reference implementation that has been built and manually validated across Claude Code / vim / htop / tmux on macOS.
specs/GH10353/product.mdspecs/GH10353/tech.mdReference implementation
Working code lives at
spalagu/warp:fix/left-drag-selection(commitf1c38c76). The diff is +124 −15 across 7 files:app/src/terminal/alt_screen_reporting.rs— newNativeLeftDragSelectEnabledfield in the AltScreenReporting groupapp/src/terminal/alt_screen/mod.rs—should_intercept_mousegainsis_left_button: bool; one-branch addition between the existingShiftearly-return and the SGR forwarding gateapp/src/terminal/alt_screen/alt_screen_element.rs— 4 call sites updatedapp/src/terminal/block_list_element.rs— 3 call sites updatedapp/src/settings_view/features_page.rs— toggle widget + command-palette pair + telemetry case + dispatch handlerapp/src/settings_view/mod.rs— context flag constantapp/src/terminal/view_tests.rs— existing assertion updated for new signaturecargo check -p warp --bin warp-oss --features guipasses cleanly. Manual validation matrix covered in product spec.Acknowledging the existing spec PRs
is_left_buttonoverbare_left_button(rationale in tech.md §2)cmd c copy,iterm,cjk)SPEC.md, doesn't follow the canonicalproduct.md+tech.mdshapeIf maintainers prefer to ratify a different spec, this implementation can be cherry-picked onto whichever spec wins. The spec is not the merge gate; the implementation is what users get.
Why a spec rather than just a code PR
CONTRIBUTING.md requires features (issues with
ready-to-spec) to go through a spec PR first. #10353 has bothready-to-specandenhancementlabels, so this is the spec phase. Once spec is approved, the code PR is mechanical (the implementation already exists atf1c38c76).What this PR is NOT
Shift-drag (which keeps working).Option-key transient bypass requested in Hold option to temporarily disable mouse-reporting #2990 / Allow to disable mouse report by holding option key #3280 (complementary, separate feature).cc @cla-bot — CLA already signed previously (#10352).