Conversation
The caret and the hit test measured the text before them as a string on
its own, through Canvas.MeasureText. The renderer never lays text out
that way: scene.Scene.DrawText and the GPU glyph paths shape the whole
string and position every glyph from that one run. Measuring a prefix
alone loses two things — the kerning pair spanning the cut, and the
advance source itself, since text.Measure sums grid-fitted advances
where shaping uses the font's own plus GPOS.
Inter Regular at 14px: the caret sits up to 3.366px right of the glyph
it should precede ("AV Wave"), of which 1.094px is kerning ("To The
Top" at index 1). "common sense" has no kerning pairs at all and still
drifts 2.898px, from the advance source alone. Click-to-position
carried the same error with the opposite sign — a click on the exact
start of a character resolved to the character before it.
widget.TextCaretPositioner is a new optional Canvas interface, in the
shape of ArcStroker and StyledTextDrawer, returning the caret offset
before every rune of a string from ShapedGlyph.Cluster over the whole
shaped run — the use that field's own doc comment describes. Canvas and
SceneCanvas implement it; a canvas that does not keeps the previous
prefix measurement, so no implementor has to change and the interface
stays additive.
It is also less work than what it replaces: one shaping per paint,
memoized on textmetrics.Metrics, instead of a prefix measurement per
cursor, selection edge and scroll clamp — and one pass for a hit test
where RuneIndexFromX measured up to two prefixes per character.
CursorX, CursorRect, SelectionRect, RuneIndexFromX and the TextField's
horizontal scroll clamp all read the same offsets, so the caret, the
highlight and the scroll cannot drift apart.
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
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.
The defect
core/textfieldplaces the caret about a pixel — sometimes three — to the right of where the character actually begins, so it overlaps the glyph instead of sitting in the gap before it. It looks intermittent because the size depends on which letters sit either side of the caret.internal/textmetrics.Metricsmeasures the text before the caret as a string on its own:The renderer never lays text out that way.
scene.Scene.DrawText(the RepaintBoundary recorder, and therefore the compositor) shapes the whole string withtext.Shapeand encodes every glyph at its shaped X; the GPU glyph-mask and MSDF paths thatgg.Context.DrawStringselects do the same. Measuring a prefix on its own loses two things:To,AV,Yakern negative: in context the second glyph is pulled left and the caret does not follow.Canvas.MeasureText→text.Measure→Face.Advance, which sums grid-fitted per-glyph advances and applies no GPOS at all. Shaping uses the font's own advances plus GPOS. The two disagree even on a string with no kerning pairs, and the error accumulates along the string.Hit-testing has the same root cause with the opposite sign:
RuneIndexFromXcompares the click against prefix widths the renderer never used, so a click on the exact start of a character can resolve to the character before it.Measured
Inter Regular (the embedded default) at 14px, caret position against the pen position of the same cluster in the shaped run:
AV WaveVy Ya P.common senseYo, Tavi.To The Topcommon senseis the control: it has no kerning pairs at all and still drifts 2.9 px, which is term 2 on its own.Standalone repro
No toolkit needed — this is
gg/textagainst any TTF:The fix
text.ShapedGlyph.Clusteris the rune index the glyph came from, and its own doc comment says what it is for: "Used for hit testing and cursor positioning." Nothing used it. The caret before runeiis the sum ofXAdvanceover the glyphs of the whole shaped string whoseClusteris belowi.The textfield cannot reach shaping:
widget.Canvasexposes onlyMeasureText(text string, fontSize float32, bold bool) float32. So this adds an optional interface in the same shape as the ones already there (ArcStroker,SVGFiller,TextModeController,StyledTextDrawer):One method rather than a caret-offset and a hit-test method, because one shaping answers both and
RuneIndexFromXthen scans a slice instead of re-measuring.internal/render.Canvasandinternal/render.SceneCanvasimplement it through one shared helper; they resolve the same default font, so they cannot drift apart (there is a test for exactly that).internal/textmetrics.Metricstype-asserts for it and falls back to the existing prefix measurement when the canvas does not implement it, souitest.MockCanvasand every downstreamwidget.Canvaskeep working unchanged. I checked every implementor in the tree first:internal/render.Canvas,internal/render.SceneCanvas,uitest.MockCanvas, and a hand-written canvas in eight test packages. Adding a method toCanvasitself would have broken all of them plus every downstream implementor, whichdocs/VERSIONING.mddoes not allow.CursorX,CursorRect,SelectionRect,RuneIndexFromXand the TextField's horizontal scroll clamp all read the same offsets, so the caret, the selection highlight and the scroll cannot drift apart.uitest.MockCanvasis deliberately not updated: it has no font, so it would only restate its own0.5 * fontSizewidth formula, and the fallback already produces that exactly. A test asserts the mock stays on the fallback path.It is also cheaper
One shaping per paint, memoized on
Metrics, replaces a prefix measurement per cursor, per selection edge and per scroll clamp.RuneIndexFromXwent from up to twoMeasureTextcalls per character — each of which allocates a prefix string and walks it — to one shaping and a scan.Tests
internal/textmetrics/shaping_test.gois the regression test and uses only pre-existing API, so it runs againstmainunchanged.Before (on
main, 4d78f0d, abridged — 49 failing assertions across the two tests):After:
TestSamplesDivergeFromAPrefixMeasurementguards the fixture rather than the code: if the default font ever stopped kerning these pairs, the two tests above would pass against a prefix measurement and prove nothing. It logs5 of 5 samples diverge from a prefix measurement, worst 3.366 px.The new gates were also run against a deliberate violation — an off-by-one in the cluster walk (
Cluster <= i) — and all three packages go red, including an existingcore/textfieldscroll test.Full suite,
go test ./... -count=1: all 62 test packages pass (8 further packages have no test files).go build ./...,go build ./examples/...,go vet ./...andgofmt -l .are clean.Two notes on the local checks, so they are not overstated:
go test ./... -racefails inapponTestWindow_AnimPumper_StartsOnInvalidation(mockWindowProvider.RequestRedrawvs the test goroutine). It reproduces identically on unmodifiedmain, so it is pre-existing and unrelated; every other package is race-clean.golangci-lintreports0 issueshere withstaticcheck,gosec,nilerranderrorlintdisabled. With those enabled it cannot run on unmodifiedmaineither on my machine — its IR builder (honnef.co/go/tools v0.7.0) panics on the Go 1.27 standard library. CI pins Go 1.25, where this does not arise.The sub-pixel rounding term, and why it is not in here
There is a second, smaller disagreement:
DrawTextsnaps the text origin withx = math.Round(x)after the canvas transform, whileCursorXroundscontentRect.Min.Xbefore the widget addsscrollOffsetX. #211 removed the unscrolled half of this; what is left isRound(a+s) - Round(a) - s, which is zero when the field is not scrolled and at most half a pixel when it is. There is also a third layer — the caret is drawn throughDrawLine, which is transformed but not rounded, so widget-local rounding cannot match a rounding that happens after a fractional transform.I left it out on purpose. It is a rounding-order question in how
core/textfieldapplies its scroll offset and in where the canvas rounds, not a measurement error, and fixing it means changing which rect the caret is computed against — the current code comments that choice deliberately ("compute cursor/selection in UNSHIFTED content rect ... they never drift apart"). That deserves its own change with the scroll tests re-derived, rather than riding along with a fix that is exact for the term it addresses. Happy to follow up if you want it.Observation, not a request
gg's CPU bitmap fallback (text.Draw→Face.Glyphs) positions glyphs from grid-fitted advances and applies no GPOS, so text rendered through it is itself un-kerned and disagrees with every GPU and scene path ingg. That is why the old prefix measurement matched that path. This change follows the shaped layout, which is what adesktop.Runapplication and the compositor actually draw. If the CPU fallback should shape too, that belongs inggrather than here.