fix(shiki-editor): use DOM measurement for cursor positioning#61
fix(shiki-editor): use DOM measurement for cursor positioning#61babarot wants to merge 1 commit into
Conversation
Replace ch-based CSS calculation with pixel-precise DOM measurement for cursor positioning. The previous approach assumed all characters are 1ch wide, causing cursor misalignment on lines containing CJK (East Asian Wide) characters. Changes: - Vim.tsx: Add hidden measurement span to code area. Use useLayoutEffect + getBoundingClientRect() to measure text width before cursor, character width at cursor, and gutter width. - Cursor.tsx: Accept pixel-based leftPx/widthPx props instead of ch-based visualCol/gutterWidth. Block cursor width now matches the actual rendered character width. - styles.css: Remove ch-based left/width from cursor rules (now set via inline style). This approach also lays groundwork for future word-wrap support, since DOM measurement naturally handles wrapped lines.
|
@babarot I will also review this later. |
|
Found 2 issues:
vimee/packages/shiki-editor/src/__tests__/Cursor.test.tsx Lines 11 to 21 in b909883
vimee/packages/shiki-editor/src/Vim.tsx Lines 170 to 175 in b909883 |
There was a problem hiding this comment.
Pull request overview
This PR updates @vimee/shiki-editor cursor positioning to use pixel-precise DOM measurement (via getBoundingClientRect) instead of ch-based CSS calculations, fixing misalignment on CJK (and other non-1ch-width) glyphs.
Changes:
- Measure rendered text width before the cursor + current character width using a hidden
<span>and applyleft/widthas inline pixel styles. - Refactor
Cursorprops to accept pixel offsets (leftPx,widthPx) and simplerline/colvalues. - Update cursor CSS to no longer rely on
ch-basedleft/widthrules.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| packages/shiki-editor/src/Vim.tsx | Adds hidden measurement span + layout-effect measurement to compute pixel cursor offsets and passes them to Cursor. |
| packages/shiki-editor/src/components/Cursor.tsx | Changes cursor API to consume pixel offsets and applies inline left/width styles. |
| packages/shiki-editor/src/styles.css | Removes ch-based cursor positioning/width so inline pixel styles control placement/size. |
Comments suppressed due to low confidence (2)
packages/shiki-editor/src/Vim.tsx:174
- The cursor pixel measurement depends on whether line numbers are currently rendered, but the effect dependencies don’t include the line-number toggle (
engine.options.number/showLineNumbers). If line numbers are turned on/off without moving the cursor or changing content,leftPxcan stay stale and the cursor will be misaligned. Include the relevant dependency (or otherwise trigger re-measurement when line-number visibility changes).
// Measure gutter offset from first line-number element
const gutterEl = area.querySelector(".sv-line-number");
const gutterWidth = gutterEl ? gutterEl.getBoundingClientRect().width : 0;
setCursorPx({ left: gutterWidth + textWidth, width: charWidth });
}, [engine.content, engine.cursor.line, engine.cursor.col]);
packages/shiki-editor/src/Vim.tsx:149
- This introduces DOM-based cursor measurement (getBoundingClientRect + hidden span), but there are no automated tests covering the computed
leftPx/widthPxbehavior. Since this is a key correctness fix (CJK alignment, gutter offset), consider adding a happy-dom test that stubsgetBoundingClientRect()to assert the inlineleft/widthstyles update for representative lines (ASCII vs CJK, with/without line numbers).
// --- Measure cursor position in pixels via DOM (handles CJK, tabs, etc.) ---
const measureRef = useRef<HTMLSpanElement>(null);
const [cursorPx, setCursorPx] = useState({ left: 0, width: 0 });
useLayoutEffect(() => {
const measure = measureRef.current;
const area = codeAreaRef.current;
if (!measure || !area) return;
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // --- Calculate gutter width for line numbers --- | ||
| const totalLines = tokenLines.length; | ||
| const gutterWidth = String(totalLines).length; | ||
|
|
||
| // --- Tab size for rendering and cursor calculation --- | ||
| const tabSize = indentWidth ?? 4; |
| export interface CursorProps { | ||
| /** Cursor position (0-based) */ | ||
| position: CursorPosition; | ||
| /** Visual column (accounting for tab width) */ | ||
| visualCol: number; | ||
| /** Cursor line (0-based) */ | ||
| line: number; | ||
| /** Cursor column (0-based, for blink restart detection) */ | ||
| col: number; | ||
| /** Left offset in pixels (measured from DOM) */ | ||
| leftPx: number; | ||
| /** Character width in pixels (measured from DOM) */ | ||
| widthPx: number; |
|
Since more than 30 days have passed since the PR was created, the CI approval button has disappeared. Therefore, I will close and then reopen the PR once. |
|
The CI is failing, so please fix it. |
Merging this PR will improve performance by 14.47%
Warning Please fix the performance issues or acknowledge them on CodSpeed. Performance Changes
Tip Investigate this regression by commenting Comparing Footnotes
|
What
Replace ch-based CSS cursor positioning with pixel-precise DOM measurement using getBoundingClientRect(). A hidden measurement span in the code area measures the actual rendered width of text before the cursor, the character at the cursor, and the line number gutter. Cursor left and width are now set as inline pixel values instead of CSS calc(N * 1ch).
Why
The previous approach calculated cursor position as character_index * 1ch, assuming every character occupies exactly one ch unit. CJK (East Asian Wide) characters render at roughly 2ch in monospace fonts, but the exact width varies by font and never equals precisely 2 * 1ch. This caused the block cursor to drift leftward on lines containing Japanese/Chinese/Korean characters. The more CJK characters before the cursor, the larger the offset.
DOM measurement eliminates the width assumption entirely by asking the browser for actual rendered dimensions. This also lays groundwork for future word-wrap support, since DOM measurement naturally handles wrapped lines.