-
Notifications
You must be signed in to change notification settings - Fork 3.1k
feat(ui): word-wise drag after double-click, line-wise extension after triple-click #8739
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
5c99107
01404d5
99af051
24ae25c
797f345
a6c516e
7cf9520
b5cbd2f
4fd9868
a03e911
de9c6a1
133e7ff
b32dbbe
d5999ed
aaaae0d
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||
|---|---|---|---|---|---|---|---|---|
|
|
@@ -208,11 +208,291 @@ describe('TextSelectionController', () => { | |||||||
| selectHello(handler); | ||||||||
|
|
||||||||
| handler(makeEvent('left-press', 1)); | ||||||||
| handler(makeEvent('left-release', 1)); | ||||||||
|
|
||||||||
| expect(copyToClipboard).toHaveBeenCalledTimes(1); | ||||||||
| expect(copyToClipboard).toHaveBeenLastCalledWith('hello'); | ||||||||
| }); | ||||||||
|
|
||||||||
| it('does not highlight a bare char-mode click', () => { | ||||||||
| const handler = mount(); | ||||||||
| handler(makeEvent('left-press', 1)); | ||||||||
|
|
||||||||
| // Assert on press: release clears the highlight either way, so only the | ||||||||
| // press call pins the bare-click suppression. | ||||||||
| expect(setSelection).toHaveBeenLastCalledWith(null); | ||||||||
|
|
||||||||
| handler(makeEvent('left-release', 1)); | ||||||||
|
|
||||||||
| expect(setSelection).toHaveBeenLastCalledWith(null); | ||||||||
| expect(copyToClipboard).not.toHaveBeenCalled(); | ||||||||
| }); | ||||||||
|
|
||||||||
| it('extends a double-click word selection word-wise on drag', () => { | ||||||||
| frame = makeFrame('foo bar baz'); | ||||||||
| viewportRect = { x: 0, y: 0, width: 11, height: 1 }; | ||||||||
| const nowSpy = vi.spyOn(Date, 'now').mockReturnValue(1000); | ||||||||
| const handler = mount(); | ||||||||
| handler(makeEvent('left-press', 2)); // first click on "foo" | ||||||||
| handler(makeEvent('left-press', 2)); // double-click -> selects "foo" | ||||||||
| expect(setSelection).toHaveBeenLastCalledWith({ | ||||||||
| sx: 0, | ||||||||
| sy: 0, | ||||||||
| ex: 2, | ||||||||
| ey: 0, | ||||||||
| }); | ||||||||
| handler(makeEvent('move', 10)); // drag to "baz" | ||||||||
| handler(makeEvent('left-release', 10)); | ||||||||
| nowSpy.mockRestore(); | ||||||||
|
|
||||||||
| expect(setSelection).toHaveBeenLastCalledWith({ | ||||||||
| sx: 0, | ||||||||
| sy: 0, | ||||||||
| ex: 10, | ||||||||
| ey: 0, | ||||||||
| }); | ||||||||
| // The press-time copy survives so a repaint before release cannot lose | ||||||||
| // the word; the release overwrites it with the grown range. | ||||||||
| expect(copyToClipboard).toHaveBeenCalledWith('foo'); | ||||||||
| expect(copyToClipboard).toHaveBeenCalledWith('foo bar baz'); | ||||||||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [Suggestion] The tests never pin that the copy happens only on release — the central behavior change of this PR (the diff deletes Failure scenario: a mutant that re-adds
Suggested change
中文说明[建议] 测试没有钉住“复制只发生在释放时”这一行为——而这正是本 PR 的核心行为变更(diff 从多点点击的按下分支删掉了 失败场景:在 — qwen3.8-max via Qwen Code /review (v0.21.7) |
||||||||
| expect(copyToClipboard).toHaveBeenCalledTimes(2); | ||||||||
| }); | ||||||||
|
|
||||||||
| it('extends a triple-click line selection line-wise on drag', () => { | ||||||||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [Suggestion] The line-mode collapsed branch of the new release condition has no test: nothing ever produces a collapsed line selection (triple-click on a one-character line). Probe-verified at HEAD: the mutant it('copies a one-cell line on a no-drag triple-click', () => {
frame = makeFrame('x');
viewportRect = { x: 0, y: 0, width: 1, height: 1 };
const nowSpy = vi.spyOn(Date, 'now').mockReturnValue(1000);
const handler = mount();
handler(makeEvent('left-press', 1));
handler(makeEvent('left-press', 1));
handler(makeEvent('left-press', 1)); // triple-click -> selects the line
handler(makeEvent('left-release', 1));
nowSpy.mockRestore();
expect(setSelection).toHaveBeenLastCalledWith({
sx: 0,
sy: 0,
ex: 0,
ey: 0,
});
expect(copyToClipboard).toHaveBeenLastCalledWith('x');
});中文说明(建议) 新释放条件的 line 模式折叠分支没有测试:没有任何测试产生过折叠的 line 选区(对单字符行三击)。已在 HEAD 用探针验证:变异体 — qwen3.8-max via Qwen Code /review (v0.21.8) |
||||||||
| frame = makeTwoLineFrame('hello', 'world!'); | ||||||||
| viewportRect = { x: 0, y: 0, width: 6, height: 2 }; | ||||||||
|
Comment on lines
+261
to
+263
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [Suggestion] The line-wise drag tests only use single-word lines ('hello', 'world!'), where it('extends a triple-click line selection across multi-word lines', () => {
frame = makeTwoLineFrame('foo bar', 'baz qux');
viewportRect = { x: 0, y: 0, width: 7, height: 2 };
const nowSpy = vi.spyOn(Date, 'now').mockReturnValue(1000);
const handler = mount();
handler(makeEvent('left-press', 2, 1));
handler(makeEvent('left-press', 2, 1));
handler(makeEvent('left-press', 2, 1)); // triple-click -> line 0
handler(makeEvent('move', 2, 2)); // drag into 'baz' on line 1
handler(makeEvent('left-release', 2, 2));
nowSpy.mockRestore();
expect(setSelection).toHaveBeenLastCalledWith({
sx: 0,
sy: 0,
ex: 6,
ey: 1,
});
expect(copyToClipboard).toHaveBeenCalledWith('foo bar\nbaz qux');
});中文说明(建议) 按行拖动的测试只使用单词行('hello'、'world!'),此时 — qwen3.8-max via Qwen Code /review (v0.21.8) |
||||||||
| const nowSpy = vi.spyOn(Date, 'now').mockReturnValue(1000); | ||||||||
| const handler = mount(); | ||||||||
| handler(makeEvent('left-press', 2, 1)); | ||||||||
| handler(makeEvent('left-release', 2, 1)); | ||||||||
| handler(makeEvent('left-press', 2, 1)); | ||||||||
| handler(makeEvent('left-release', 2, 1)); // double-click -> word "hello" | ||||||||
| handler(makeEvent('left-press', 2, 1)); // triple-click -> line 0 | ||||||||
| handler(makeEvent('move', 3, 2)); // drag into the middle of line 1 | ||||||||
| handler(makeEvent('left-release', 3, 2)); | ||||||||
|
Comment on lines
+266
to
+272
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [Suggestion] The triple-click tests emit three consecutive 中文说明(建议) 三击测试连发三个 — qwen3.8-max via Qwen Code /review (v0.21.8) |
||||||||
| nowSpy.mockRestore(); | ||||||||
|
|
||||||||
| expect(setSelection).toHaveBeenLastCalledWith({ | ||||||||
| sx: 0, | ||||||||
| sy: 0, | ||||||||
| ex: 5, | ||||||||
| ey: 1, | ||||||||
| }); | ||||||||
| expect(copyToClipboard).toHaveBeenLastCalledWith('hello\nworld!'); | ||||||||
| }); | ||||||||
|
|
||||||||
| it('extends a triple-click line selection across multi-word lines', () => { | ||||||||
| frame = makeTwoLineFrame('foo bar', 'baz qux'); | ||||||||
| viewportRect = { x: 0, y: 0, width: 7, height: 2 }; | ||||||||
| const nowSpy = vi.spyOn(Date, 'now').mockReturnValue(1000); | ||||||||
| const handler = mount(); | ||||||||
| handler(makeEvent('left-press', 2, 1)); | ||||||||
| handler(makeEvent('left-release', 2, 1)); | ||||||||
| handler(makeEvent('left-press', 2, 1)); | ||||||||
| handler(makeEvent('left-release', 2, 1)); // double-click -> word "foo" | ||||||||
| handler(makeEvent('left-press', 2, 1)); // triple-click -> line 0 | ||||||||
| handler(makeEvent('move', 2, 2)); // drag into 'baz' on line 1 | ||||||||
| handler(makeEvent('left-release', 2, 2)); | ||||||||
| nowSpy.mockRestore(); | ||||||||
|
|
||||||||
| expect(setSelection).toHaveBeenLastCalledWith({ | ||||||||
| sx: 0, | ||||||||
| sy: 0, | ||||||||
| ex: 6, | ||||||||
| ey: 1, | ||||||||
| }); | ||||||||
| expect(copyToClipboard).toHaveBeenLastCalledWith('foo bar\nbaz qux'); | ||||||||
| }); | ||||||||
|
|
||||||||
| it('copies a single-character word on a no-drag double-click', () => { | ||||||||
| frame = makeFrame('a b'); | ||||||||
| viewportRect = { x: 0, y: 0, width: 3, height: 1 }; | ||||||||
| const nowSpy = vi.spyOn(Date, 'now').mockReturnValue(1000); | ||||||||
| const handler = mount(); | ||||||||
| handler(makeEvent('left-press', 1)); | ||||||||
| handler(makeEvent('left-release', 1)); | ||||||||
| handler(makeEvent('left-press', 1)); // double-click -> selects "a" | ||||||||
| handler(makeEvent('left-release', 1)); | ||||||||
| nowSpy.mockRestore(); | ||||||||
|
|
||||||||
| expect(setSelection).toHaveBeenLastCalledWith({ | ||||||||
| sx: 0, | ||||||||
| sy: 0, | ||||||||
| ex: 0, | ||||||||
| ey: 0, | ||||||||
| }); | ||||||||
| expect(copyToClipboard).toHaveBeenCalledWith('a'); | ||||||||
| expect(copyToClipboard).toHaveBeenCalledTimes(2); | ||||||||
| }); | ||||||||
|
|
||||||||
| it('keeps the double-click copy when streaming clears the selection before release', () => { | ||||||||
| frame = makeFrame('foo bar'); | ||||||||
| viewportRect = { x: 0, y: 0, width: 7, height: 1 }; | ||||||||
| const nowSpy = vi.spyOn(Date, 'now').mockReturnValue(1000); | ||||||||
| const handler = mount(); | ||||||||
| handler(makeEvent('left-press', 2)); | ||||||||
| handler(makeEvent('left-release', 2)); | ||||||||
| handler(makeEvent('left-press', 2)); // double-click -> copies "foo" | ||||||||
| listener!(makeFrame('foo baz')); // streaming repaint clears the selection | ||||||||
| handler(makeEvent('left-release', 2)); // release arrives after the clear | ||||||||
| nowSpy.mockRestore(); | ||||||||
|
|
||||||||
| expect(copyToClipboard).toHaveBeenCalledTimes(1); | ||||||||
| expect(copyToClipboard).toHaveBeenCalledWith('foo'); | ||||||||
| }); | ||||||||
|
|
||||||||
| it('copies a one-cell line on a no-drag triple-click', () => { | ||||||||
| frame = makeFrame('x'); | ||||||||
| viewportRect = { x: 0, y: 0, width: 1, height: 1 }; | ||||||||
| const nowSpy = vi.spyOn(Date, 'now').mockReturnValue(1000); | ||||||||
| const handler = mount(); | ||||||||
| handler(makeEvent('left-press', 1)); | ||||||||
| handler(makeEvent('left-release', 1)); | ||||||||
| handler(makeEvent('left-press', 1)); | ||||||||
| handler(makeEvent('left-release', 1)); // double-click -> word "x" | ||||||||
| handler(makeEvent('left-press', 1)); // triple-click -> line "x" | ||||||||
| handler(makeEvent('left-release', 1)); | ||||||||
| nowSpy.mockRestore(); | ||||||||
|
|
||||||||
| expect(setSelection).toHaveBeenLastCalledWith({ | ||||||||
| sx: 0, | ||||||||
| sy: 0, | ||||||||
| ex: 0, | ||||||||
| ey: 0, | ||||||||
| }); | ||||||||
| expect(copyToClipboard).toHaveBeenLastCalledWith('x'); | ||||||||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [Suggestion] The press-time copy guarantee this PR introduces is pinned only for double-click; the line-mode (triple-click) twin is missing. Probe-verified at HEAD: a mutant gating the press-time copy on Failure scenario: a regression restricting the press-time copy to word mode ships green; then a triple-click whose streaming repaint lands between press and release ( Add 中文说明(建议) 本 PR 引入的"按压时复制"保证只为双击固定了测试;行模式(三击)的对应测试缺失。已在 HEAD 用探针验证:把按压复制限定为 触发场景:若回归把按压复制限制为词模式,会在绿灯下发布;此后三击时若流式重绘落在按压与释放之间( 建议仿照现有双击测试补一条 — qwen3.8-max via Qwen Code /review (v0.21.9) |
||||||||
| }); | ||||||||
|
|
||||||||
| it('extends a word drag to the release cell when no move event is emitted', () => { | ||||||||
| frame = makeFrame('foo bar baz'); | ||||||||
| viewportRect = { x: 0, y: 0, width: 11, height: 1 }; | ||||||||
| const nowSpy = vi.spyOn(Date, 'now').mockReturnValue(1000); | ||||||||
| const handler = mount(); | ||||||||
| handler(makeEvent('left-press', 2)); | ||||||||
| handler(makeEvent('left-press', 2)); // double-click -> selects "foo" | ||||||||
| handler(makeEvent('left-release', 10)); // release over "baz" with no move | ||||||||
| nowSpy.mockRestore(); | ||||||||
|
|
||||||||
| expect(setSelection).toHaveBeenLastCalledWith({ | ||||||||
| sx: 0, | ||||||||
| sy: 0, | ||||||||
| ex: 10, | ||||||||
| ey: 0, | ||||||||
| }); | ||||||||
| expect(copyToClipboard).toHaveBeenCalledWith('foo bar baz'); | ||||||||
| }); | ||||||||
|
|
||||||||
| it('extends a double-click word selection backward when dragging left', () => { | ||||||||
| frame = makeFrame('foo bar baz'); | ||||||||
| viewportRect = { x: 0, y: 0, width: 11, height: 1 }; | ||||||||
| const nowSpy = vi.spyOn(Date, 'now').mockReturnValue(1000); | ||||||||
| const handler = mount(); | ||||||||
| handler(makeEvent('left-press', 9)); // first click on "baz" | ||||||||
| handler(makeEvent('left-press', 9)); // double-click -> selects "baz" | ||||||||
| handler(makeEvent('move', 1)); // drag back onto "foo" | ||||||||
| handler(makeEvent('left-release', 1)); | ||||||||
| nowSpy.mockRestore(); | ||||||||
|
|
||||||||
| expect(setSelection).toHaveBeenLastCalledWith({ | ||||||||
| sx: 0, | ||||||||
| sy: 0, | ||||||||
| ex: 10, | ||||||||
| ey: 0, | ||||||||
| }); | ||||||||
| expect(copyToClipboard).toHaveBeenCalledWith('foo bar baz'); | ||||||||
| }); | ||||||||
|
|
||||||||
| it('extends a triple-click line selection backward when dragging up', () => { | ||||||||
| frame = makeTwoLineFrame('hello', 'world!'); | ||||||||
| viewportRect = { x: 0, y: 0, width: 6, height: 2 }; | ||||||||
| const nowSpy = vi.spyOn(Date, 'now').mockReturnValue(1000); | ||||||||
| const handler = mount(); | ||||||||
| handler(makeEvent('left-press', 2, 2)); | ||||||||
| handler(makeEvent('left-release', 2, 2)); | ||||||||
| handler(makeEvent('left-press', 2, 2)); | ||||||||
| handler(makeEvent('left-release', 2, 2)); // double-click -> word "world!" | ||||||||
| handler(makeEvent('left-press', 2, 2)); // triple-click -> line 1 | ||||||||
| handler(makeEvent('move', 2, 1)); // drag up onto line 0 | ||||||||
| handler(makeEvent('left-release', 2, 1)); | ||||||||
| nowSpy.mockRestore(); | ||||||||
|
|
||||||||
| expect(setSelection).toHaveBeenLastCalledWith({ | ||||||||
| sx: 0, | ||||||||
| sy: 0, | ||||||||
| ex: 5, | ||||||||
| ey: 1, | ||||||||
| }); | ||||||||
| expect(copyToClipboard).toHaveBeenLastCalledWith('hello\nworld!'); | ||||||||
| }); | ||||||||
|
|
||||||||
| it('keeps covered-row trailing spaces in a multi-row line drag', () => { | ||||||||
| frame = makeTwoLineFrame('aaa ', 'bbb'); | ||||||||
| viewportRect = { x: 0, y: 0, width: 4, height: 2 }; | ||||||||
| const nowSpy = vi.spyOn(Date, 'now').mockReturnValue(1000); | ||||||||
| const handler = mount(); | ||||||||
| handler(makeEvent('left-press', 2, 1)); | ||||||||
| handler(makeEvent('left-release', 2, 1)); | ||||||||
| handler(makeEvent('left-press', 2, 1)); | ||||||||
| handler(makeEvent('left-release', 2, 1)); // double-click -> word "aaa" | ||||||||
| handler(makeEvent('left-press', 2, 1)); // triple-click -> line 0 | ||||||||
| handler(makeEvent('move', 2, 2)); // drag onto line 1 | ||||||||
| handler(makeEvent('left-release', 2, 2)); | ||||||||
| nowSpy.mockRestore(); | ||||||||
|
|
||||||||
| expect(setSelection).toHaveBeenLastCalledWith({ | ||||||||
| sx: 0, | ||||||||
| sy: 0, | ||||||||
| ex: 2, | ||||||||
| ey: 1, | ||||||||
| }); | ||||||||
| // Covered rows keep written trailing spaces (getSelectedText contract); | ||||||||
| // only the final row ends at the line span's trimmed last content column. | ||||||||
| expect(copyToClipboard).toHaveBeenLastCalledWith('aaa \nbbb'); | ||||||||
| }); | ||||||||
|
|
||||||||
| it('falls back to the cursor cell when a word drag lands on whitespace', () => { | ||||||||
| frame = makeFrame('foo bar baz'); | ||||||||
| viewportRect = { x: 0, y: 0, width: 11, height: 1 }; | ||||||||
| const nowSpy = vi.spyOn(Date, 'now').mockReturnValue(1000); | ||||||||
| const handler = mount(); | ||||||||
| handler(makeEvent('left-press', 2)); | ||||||||
| handler(makeEvent('left-press', 2)); // double-click -> selects "foo" | ||||||||
| handler(makeEvent('move', 4)); // drag onto the gap after "foo" | ||||||||
| handler(makeEvent('left-release', 4)); | ||||||||
| nowSpy.mockRestore(); | ||||||||
|
|
||||||||
| expect(setSelection).toHaveBeenLastCalledWith({ | ||||||||
| sx: 0, | ||||||||
| sy: 0, | ||||||||
| ex: 3, | ||||||||
| ey: 0, | ||||||||
| }); | ||||||||
| expect(copyToClipboard).toHaveBeenCalledWith('foo '); | ||||||||
| }); | ||||||||
|
|
||||||||
| it('keeps the triple-click chain across drift during a held double-click', () => { | ||||||||
| frame = makeFrame('foo bar baz'); | ||||||||
| viewportRect = { x: 0, y: 0, width: 11, height: 1 }; | ||||||||
| const nowSpy = vi.spyOn(Date, 'now').mockReturnValue(1000); | ||||||||
| const handler = mount(); | ||||||||
| handler(makeEvent('left-press', 2)); | ||||||||
| handler(makeEvent('left-release', 2)); | ||||||||
| handler(makeEvent('left-press', 2)); // double-click -> selects "foo" | ||||||||
| handler(makeEvent('move', 4)); // drift off the word while held | ||||||||
| handler(makeEvent('left-release', 4)); | ||||||||
| handler(makeEvent('left-press', 2)); // third click -> selects the line | ||||||||
| handler(makeEvent('left-release', 2)); | ||||||||
| nowSpy.mockRestore(); | ||||||||
|
|
||||||||
| expect(setSelection).toHaveBeenLastCalledWith({ | ||||||||
| sx: 0, | ||||||||
| sy: 0, | ||||||||
| ex: 10, | ||||||||
| ey: 0, | ||||||||
| }); | ||||||||
| expect(copyToClipboard).toHaveBeenLastCalledWith('foo bar baz'); | ||||||||
| }); | ||||||||
|
|
||||||||
| it('snaps a wide-character spacer to the leading cell', () => { | ||||||||
| frame = makeWideFrame(); | ||||||||
| const handler = mount(); | ||||||||
|
|
||||||||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[Suggestion]
spanAtForModeaccepts the fullSelectionModeunion but only implements two of its three values:'char'is silently routed tolineSpanAt(a whole-line span), contradicting the doc comment ("word/line selection mode"). The multi-click call site is statically safe (literal'word' | 'line'), butextendSpanDragpassesselection.modethrough un-narrowed, guarded only by the comment-documented invariant thatanchorSpanRefnon-null implies word/line mode. — Failure scenario: a future caller forwardingselection.modefrom a char-mode path (e.g. lifting theanchorSpanRefguard while simplifying, or reusing this helper for a new snap feature) gets a whole-line span for a char-mode selection with zero compiler diagnostics — a character-level operation silently selects and copies an entire line. Suggested fix: narrow the parameter toExclude<SelectionMode, 'char'>;extendSpanDragthen has to narrowselection.modeat the call site, surfacing the invariant at compile time instead of in a comment.中文说明
(建议)
spanAtForMode接受完整的SelectionMode联合类型,但只实现了其中两个值:'char'会被静默路由到lineSpanAt(整行 span),与文档注释("word/line selection mode")矛盾。多点点击调用点是静态安全的(字面量'word' | 'line'),但extendSpanDrag未收窄就传入selection.mode,仅靠注释记载的不变式(anchorSpanRef非空 ⟹ word/line 模式)保护。触发场景:未来若有调用方从 char 模式路径转发selection.mode(例如简化时移除anchorSpanRef守卫,或为新吸附功能复用此 helper),char 模式选区会拿到整行 span 且编译器零报错——字符级操作会静默选中并复制整行。建议修复:把参数收窄为Exclude<SelectionMode, 'char'>;extendSpanDrag就必须在调用点显式收窄selection.mode,让不变式由编译器而不是注释来保证。— qwen3.8-max via Qwen Code /review (v0.21.8)