fix(core/editor-view): snap visual cursor columns to grapheme boundaries - #1329
Open
3aKHP wants to merge 2 commits into
Open
fix(core/editor-view): snap visual cursor columns to grapheme boundaries#13293aKHP wants to merge 2 commits into
3aKHP wants to merge 2 commits into
Conversation
3aKHP
requested review from
Hona,
kommander,
msmps and
simonklee
as code owners
August 4, 2026 10:16
Member
|
|
3aKHP
force-pushed
the
vesicle/fix-1289-cursor-boundary
branch
from
August 4, 2026 10:44
5009f3b to
1ba266e
Compare
Author
Sorry for my carelessness — that file is internal to our fork and should not have been part of this PR. My mistake. The branch has been rebased onto the pristine upstream tag, and the diff now contains only the intended Zig changes. Thank you for the time you spent looking at it. |
3aKHP
force-pushed
the
vesicle/fix-1289-cursor-boundary
branch
from
August 7, 2026 12:54
b355022 to
06b9083
Compare
Author
|
Rebased onto current main (5329177, including the Zig 0.16 migration) without conflicts. The internal fork file noted earlier remains absent from the diff. This is ready for review when you have time; thank you. |
Vertical movement (moveUpVisual/moveDownVisual) preserves a desired visual column measured in display cells. When the target row's grapheme boundaries differ from the source row's (e.g. width-2 CJK graphemes offset by an odd-width ASCII char), the column can fall strictly inside a grapheme and was committed verbatim, rendering the caret between the two cells of a wide glyph and giving Shift+Up/Down an illegal selection endpoint. visualToLogicalCursor now snaps the requested column to the leading boundary of the containing grapheme cluster (grapheme-cluster- and width-method-aware, reusing utf8.findPosByWidth via the new snapColToGraphemeBoundary iterator helper). getVisualEOL and makeCursorVisible route through the same snapped mapping instead of their own unsnapped col arithmetic. Legal boundaries, column 0, end-of-line, and ASCII-only lines are unaffected (identity snap). Fixes anomalyco#1289
…ry snap The parent commit changed vertical cursor movement to snap visual columns to grapheme leading boundaries. The JS test still expected the cursor to land on visualCol 8, the second cell of the wide emoji grapheme; it now expects the snapped leading boundary 7.
3aKHP
force-pushed
the
vesicle/fix-1289-cursor-boundary
branch
from
August 27, 2026 09:34
06b9083 to
841e435
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #1289
Rebase note (2026-08-27)
Rebased onto current
mainacross the #1391 source move (packages/core/src/zig→packages/native/src) and the #1428 wrapped-layout rewrite (VirtualLine.source_col_offset→source_col_start; the unsnapped arithmetic this PR targets is unchanged, now ateditor-view.zig:655).getVisualEOLkeeps the newer viewport/occupancy computation from #1393 and only routes its final visual→logical mapping through the snapped path. One test expectation was aligned with #1393 semantics: undercelloccupancy a forward inclusive selection stores the exclusive end, so the assertion issel.end == cursor.offset + width(签)rather thansel.end == cursor.offset.Sensitivity on this base: without the fix, 8 of the 10 new tests fail; the two selection-extension tests pass because #1393's occupancy machinery already snaps the selection-sync path — the direct-move path (
moveUpVisual/moveDownVisual) remains broken. With the fix, all 10 pass.Summary
Vertical cursor movement (
moveUpVisual/moveDownVisual) preserves a desired visual column measured in display cells. When the target row's grapheme boundaries differ from the row the column was measured on — e.g. width-2 CJK graphemes shifted by an odd-width ASCII char — the desired column can fall strictly inside a grapheme.visualToLogicalCursorcommitted it verbatim:The terminal caret then renders between the two cells of a wide glyph, and Shift+Up/Down (same code path) can produce a selection endpoint inside a grapheme. All column/offset units here are display-cell widths (
coordsToOffset=line_start_weight + col), so an interior column is not a legal cursor position — it conflates display cells with grapheme boundaries.Reproduction (headless) is in #1289: buffer
的[代码签名政策](/因此签名批准者角色, cursor after代(visual col 5), move down → col 5 lands inside签(cells 4..6).Fix
Snap the requested visual column to the leading (line-start-side) boundary of the containing grapheme cluster before committing the cursor:
snapColToGraphemeBoundary(text-buffer-iterators.zig), placed next togetGraphemeWidthAt/getPrevGraphemeWidthwhose segment-walk pattern it mirrors. It reusesutf8.findPosByWidth(..., include_start_before=false, width_method), so it is grapheme-cluster-aware (combining marks, emoji/ZWJ) and width-method-aware; no new width tables. One subtlety:findPosByWidth's final-cluster tail path reports the trailing boundary when the column sits inside the last cluster of a chunk — the helper detectscolumns_used > local_coland steps back one cluster width viautf8.getPrevGraphemeStart.visualToLogicalCursorsnaps after clamping (central fix; covers Up/Down, Shift+Up/Down, and mouse/TS callers).getVisualEOLandmakeCursorVisiblehad the same unsnapped col arithmetic; both now route through the snapped mapping.Leading-boundary policy is deterministic, keeps the caret on the grapheme it was aimed at, and composes with
clampVisualColToStayOnVisualRow(#1078), whose one-cell step back from a wrap boundary can itself land inside a trailing wide grapheme — leading snap resolves that back onto the same visual row instead of drifting onto the next row's start.Legal boundaries, column 0, end-of-line, and ASCII-only text are identity mappings (verified by the unchanged pre-existing suite).
Tests
10 new tests in
packages/native/src/tests/editor-view_test.zig:签's leading boundary (col 4), plus the directvisualToLogicalCursor(1, 5)mappingmoveCursorUp/Down→getVisualCursor→setLocalSelection/updateLocalSelection→syncCursorToSelectionFocus)zig build test -Dtest-filter="EditorView": 118/118 passed. Fullzig build test: 2099/2108 (8 skipped; the one failure is an environment-dependent X11 clipboard test that fails identically on pristinemain).zig fmtclean. TS:bun test src/editor-view.test.ts73/73 against the rebuilt native library (including the aligned emoji vertical-nav expectation from the companion commit: col 8 sits inside 🌟 cells 7–8, the snapped leading boundary is 7);edit-buffer/buffer/text-bufferTS suites: 218 passed.Out of scope (deliberately; happy to follow up)
coordsToCharOffset(text-buffer-view.zig) can commit a raw interior-cell offset into a selection, andsyncCursorToSelectionFocus(editor-view.zig) can then put the cursor there when clicking the second cell of a wide grapheme. Not changed here: snapping incoordsToCharOffsetwould alter selection-range semantics (selection offsets deliberately snap at extraction time), and snapping only the cursor would decouplecursor.offsetfrom the selection focus offset the TS layer relies on. Shift+arrow selection is covered by this PR because it anchors from the already-snapped committed cursor.EditBuffer.moveUp/moveDown(edit-buffer.zig:515,538) contain the same@min(desired_col, line_width)unsnapped arithmetic.EditBufferRenderableusesmoveUpVisual/moveDownVisual, so the reported defect path is unaffected; the same one-line helper applies if you want a follow-up.