fix(selection): hit-test line selection against resolved line rects#10
Conversation
Tap/drag line selection mapped the touch point to a line arithmetically (font.lineHeight + codeLineSpacing per row), while the selection highlight draws at the resolved CoreText line rects. The real per-line advance differs from the assumed one by fractions of a point (~0.5pt for SF Mono 14 + 4pt spacing), which accumulates with the line number — several lines of finger/highlight misalignment a few hundred rows into a diff or code block. Lines containing fallback-font glyphs (emoji, CJK) are taller still and add a full extra jump. Hit-test in the selection overlay's coordinate space against the same rects it draws, so the picked line always matches the rendered highlight. Each line owns the spacing gap below it, mirroring the old rounding. The arithmetic path remains as a fallback for gestures that land before the first layout pass resolves rects. Applies to DiffView.rowIndex(at:) and CodeView.lineIndex(at:) on both UIKit and AppKit. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
There was a problem hiding this comment.
Pull request overview
This PR fixes line-selection hit-testing in code and diff blocks by mapping touch/mouse Y positions to the same resolved CoreText line rects used to draw the selection highlight, eliminating drift from per-row arithmetic estimates (especially in long blocks and with fallback-font glyphs).
Changes:
- Add rect-based line index hit-testing (
lineIndex(atY:trailingGap:)) toLineSelectionOverlayView(UIKit + AppKit). - Update DiffView and CodeView line/row selection to prefer rect-based hit-testing when rects are available, with an arithmetic fallback before first layout.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| Sources/MarkdownView/Components/LineSelection/LineSelectionOverlayView.swift | Adds shared rect-based hit-testing API over resolved line rects and exposes hasLineRects. |
| Sources/MarkdownView/Components/DiffView/DiffView.swift | Switches row hit-testing to use overlay rects (with fallback), for both UIKit and AppKit implementations. |
| Sources/MarkdownView/Components/CodeView/CodeView.swift | Switches line hit-testing to use overlay rects (with fallback), for both UIKit and AppKit implementations. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| if selectionOverlay.hasLineRects { | ||
| let overlayPoint = selectionOverlay.convert(point, from: nil) | ||
| guard let line = selectionOverlay.lineIndex( | ||
| atY: overlayPoint.y, | ||
| trailingGap: CodeViewConfiguration.codeLineSpacing |
There was a problem hiding this comment.
Good catch — the AppKit mouse handlers do convert to view-local coordinates before calling. Fixed in b60efc9: the overlay hit-test now converts from: self, and the arithmetic fallback (which had the same pre-existing double conversion) uses the local point directly.
| if selectionOverlay.hasLineRects { | ||
| let overlayPoint = selectionOverlay.convert(point, from: nil) | ||
| guard let row = selectionOverlay.lineIndex( | ||
| atY: overlayPoint.y, | ||
| trailingGap: CodeViewConfiguration.codeLineSpacing |
There was a problem hiding this comment.
Good catch — same as the CodeView comment. Fixed in b60efc9: overlay hit-test converts from: self, and the arithmetic fallback's pre-existing double conversion is removed too.
| /// Maps a y in this overlay's coordinate space to a 1-based line index | ||
| /// using the same rects the highlight draws, so a hit-tested line always | ||
| /// matches the rendered selection. Each line owns the spacing gap below | ||
| /// it; nil above the first line or below the last (plus one trailing gap). | ||
| func lineIndex(atY y: CGFloat, trailingGap: CGFloat) -> Int? { | ||
| guard let first = lineRects.first, let last = lineRects.last else { return nil } | ||
| guard y >= first.minY, y < last.maxY + trailingGap else { return nil } | ||
| for index in 0 ..< (lineRects.count - 1) where y < lineRects[index + 1].minY { | ||
| return index + 1 | ||
| } | ||
| return lineRects.count | ||
| } |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 6103fe9afd
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| /// it; nil above the first line or below the last (plus one trailing gap). | ||
| func lineIndex(atY y: CGFloat, trailingGap: CGFloat) -> Int? { | ||
| guard let first = lineRects.first, let last = lineRects.last else { return nil } | ||
| guard y >= first.minY, y < last.maxY + trailingGap else { return nil } |
There was a problem hiding this comment.
When the final logical line has no glyphs, such as a code fence or diff row ending in a blank line, CoreText can provide no rect or a zero-height rect for that row while cachedLineCount/diffRowCount() and the intrinsic height still include it. Once hasLineRects is true, this guard caps hit-testing at the last resolved rect plus only codeLineSpacing, so taps in the visible trailing blank row return nil instead of falling back to the arithmetic row mapping that previously selected it. Consider synthesizing/falling back for logical rows beyond the resolved rects.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 409c033: lineIndex(atY:) now extrapolates past the last resolved rect at the last line's advance instead of returning nil, so the trailing blank row stays selectable. The callers' existing row <= count guard still rejects points beyond the logical content, preserving the old arithmetic mapping's bounds.
…oint Review follow-up: the AppKit mouse handlers convert event.locationInWindow into view-local coordinates before calling rowIndex(at:)/lineIndex(at:), so converting again with `from: nil` treated a local point as window coordinates. Convert from `self` instead. The pre-existing arithmetic fallback had the same double conversion; use the local point directly there too. Also adds unit coverage for LineSelectionOverlayView.lineIndex(atY:): inside lines, gap ownership, above-first / trailing-gap bounds, single line, and uneven line heights. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Review follow-up (P2): CoreText emits no line for a trailing blank row (text ending in a newline), so logical rows can outnumber resolved rects and the rect-capped hit-test made the visible trailing blank row unselectable. Extrapolate past the last rect at its own advance; the callers' existing logical-line-count guard still rejects points beyond the real content. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Tap/drag line selection mapped the touch point to a line arithmetically (font.lineHeight + codeLineSpacing per row), while the selection highlight draws at the resolved CoreText line rects. The real per-line advance differs from the assumed one by fractions of a point (~0.5pt for SF Mono 14 + 4pt spacing), which accumulates with the line number — several lines of finger/highlight misalignment a few hundred rows into a diff or code block. Lines containing fallback-font glyphs (emoji, CJK) are taller still and add a full extra jump.
Hit-test in the selection overlay's coordinate space against the same rects it draws, so the picked line always matches the rendered highlight. Each line owns the spacing gap below it, mirroring the old rounding. The arithmetic path remains as a fallback for gestures that land before the first layout pass resolves rects.
Applies to DiffView.rowIndex(at:) and CodeView.lineIndex(at:) on both UIKit and AppKit.