fix: keep terminal grid and glyph size stable across canvas zoom - #551
fix: keep terminal grid and glyph size stable across canvas zoom#551superkim0610 wants to merge 4 commits into
Conversation
Canvas zoom CSS-scales the world div, which blurs xterm's pre-rasterized glyph atlas. TerminalPanel compensated by raising fontSize to base × renderScale and counter-scaling the render box by 1/renderScale. But xterm ceils its cell pixels, so cell(k·fs) ≠ k·cell(fs) and the compensation drifted apart from the thing it was compensating for. Measured at 13px / DPR 1: renderScale 1.25 wants an 8.75px cell and gets 9 (+2.9%); 2.5 wants 17.5 and gets 19 (+8.6%). Sizing the box by renderScale while the cell grew by more than that dropped columns — 90 → 87 → 86, up to 9 columns at the top step — and every drop reached the PTY as a SIGWINCH, reflowing the shell and stacking duplicate frames in Ink-style TUIs. The vertical size wobbled separately because width and height ceil independently. Layout now follows the measured cell rather than the intended ratio: box = container × (cell / baseCell) → columns = container / baseCell (the scale cancels out) - Counter-scale per axis. Width and height quantize independently (7:15 → 9:19 → 11:23), so a uniform width-driven scale left the height 1.5-2.4% short depending on the step. - Capture the baseline only at renderScale 1, measuring it directly when a panel is first laid out already zoomed in. A scaled cell cannot be divided back down — 11/1.5 yields 7.33 for a cell that is really 7 — and that 4.8% error shrank the box at every scale, including 1. - Halve the snap-step spacing to 0.25. 125% was both the blurriest point in the range (a 25% upscale) and a tie between two steps, so a hair past it the scale jumped a full 1.5× in one frame. - Scale the scrollbar track with the cell. It sits inside the counter-scaled box, so a fixed 8px track shrank on screen as the terminal zoomed in, down to -36% at the top step. - Write the resulting track width back into xterm's cached scrollBarWidth. xterm measures it once in the Viewport constructor and never again; FitAddon is its only reader (xterm's core never touches the field), so a stale value meant FitAddon subtracting 8 from a 17px track and returning a column count whose grid overflowed its viewport, clipping the right-hand columns. preserveGrid was the earlier mitigation for this: it discarded ±1 column/row deltas so the PTY never saw a phantom SIGWINCH. The real deltas turned out to be 3-9 columns, which went straight through it. It now has no callers and is removed; snapRenderScale moves to its own module so the snap boundaries can be unit-tested without pulling in xterm's WebGL addon, which needs a browser global at import time. Verified by instrumenting a running app: column count holds at 101x27 across renderScale 1 → 1.25 → 1.5 → 2 → 2.5, and the measured scrollbar track matches the cached value at every step. Note that all measurements come from a single DPR 1 machine — the arithmetic is DPR-independent, but the confirmation is not. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01WsMgXwPNJJ4SgXgRff5wJk
oh-my-claudecode writes session scratch to .omc/state, which showed up as untracked on every status. Same category as the .agents/ and .cate/ entries next to it — per-machine tooling state, not project state. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01WsMgXwPNJJ4SgXgRff5wJk
|
One issue before this lands: if the terminal mounts while the canvas is already zoomed, the scaling effect can run before xterm is attached. It applies the larger font but never measures the matching cell scale, so the first fit can still shrink the grid. Could we rerun the measurement after attach and add a regression test for delayed creation at zoom above 1? |
Review catch (thanks @Anton-Horn): a panel mounted while the canvas is ALREADY zoomed could still end up with a shrunken grid. getOrCreate is async, so the render-scale effect can run before attach() has opened xterm into the render box. terminal.element is undefined at that point, so nothing is measurable — but the effect raised fontSize to base × renderScale anyway, growing the cell while cellScale stayed at 1. That is exactly the box/cell mismatch this effect exists to prevent, and it was permanent: renderScale had already settled at its target, so the effect never ran again on its own and no later fit could recover the lost columns. Two changes: - Bail before touching fontSize, not after. The font is now only ever raised together with the measurement that justifies it, so there is no window where the two disagree. - Re-run the effect after attach, via an attachEpoch counter bumped where the xterm element is created. Without it the guard above would simply leave a zoomed panel rendering at the base font forever. The regression test drives the real component and pins the ordering that actually breaks: let renderScale settle to 2.0 while the spawn is still in flight, then attach. Reverting either change fails it — the font goes to 26 with the render box stuck at 100% instead of 200%. An earlier version of that test released the spawn before flushing the debounce frames, which meant xterm was already attached by the time the scale committed; it passed with the fix removed and so proved nothing. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01WsMgXwPNJJ4SgXgRff5wJk
|
Good catch — confirmed and fixed in 40759e3. You were right about the mechanism. const measurable = !!screenEl && cols > 0 && rows > 0
...
entry.terminal.options.fontSize = base * renderScale // ← ran regardless
if (measurable && ...) { /* cellScale measurement */ } // ← guardedSo the cell grew while Two changes:
The regression test drives the real component and pins the ordering that actually breaks — let Worth flagging: my first version of that test released the spawn before flushing the debounce frames, so xterm was already attached when the scale committed. It passed with the fix removed and proved nothing. The committed version is the one I verified fails without the fix. |
…sity baseCellRef caches the cell measured at the unscaled font, and every later cellScale is that cache divided into a fresh measurement. But xterm rounds the cell to DEVICE pixels and hands it back in CSS pixels: css.cell = round(char × dpr) / dpr so the rounding grid is 1/dpr — whole pixels at DPR 1, half pixels at DPR 2. The same font measures differently per display, and a baseline captured on one screen is simply wrong on another. Dragging the window to a second monitor left every subsequent cellScale computed from a mismatched pair, so the render box no longer matched the cell and the column count drifted — the exact symptom this whole change set exists to remove. Nothing else would have noticed. The render box keeps its CSS size, so the ResizeObserver stays quiet; renderScale is already at its target, so the measuring effect has no reason to re-run. The grid would have stayed wrong until the user happened to zoom or resize by hand — the same "wrong until touched by accident" shape as the PTY-size bug. Record the DPR alongside the baseline and drop the cache when a resolution media query fires. Such a query only matches the DPR it was constructed with, so it is re-armed at the new value on each change. attachEpoch becomes measureEpoch: it is no longer only about attach, it is the general "something outside this component invalidated the measurement" signal. Found by review (thanks @Anton-Horn's reviewer pass for prompting the DPR question). Note the review classified this as pre-existing — it is not. The baseline cache does not exist before this branch; the previous code recomputed from renderScale every time and so followed a DPR change on its own. This is a regression introduced here. The test models xterm's device-pixel rounding rather than fixed numbers, using a deliberately fractional glyph advance — with a whole-number cell the DPR 1 and DPR 2 grids coincide and the test cannot tell a stale baseline from a fresh one. Reverting the invalidation fails it (214% held over from DPR 1 where 193% is correct) while the other two cases still pass. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01WsMgXwPNJJ4SgXgRff5wJk
|
Follow-up: found and fixed a second hole in the same area — 5d2f97e.
The rounding grid is Nothing would have corrected it, either. The render box keeps its CSS size so the ResizeObserver stays quiet, and Fix: record the DPR alongside the baseline and drop the cache when a resolution media query fires. Such a query only matches the DPR it was constructed with, so it is re-armed at the new value on each change. Verified on real hardware, dragging the window between a 1080p external display (DPR 1) and the built-in retina panel (DPR 2), with per-panel instrumentation so before/after could be matched to the same panel: 7 → 7.5 is exactly the DPR 2 half-pixel grid. A stale baseline would have been off by ~7%. The reverse direction (retina → external) fires too, and the re-measurement picked up the new value rather than the old one, so xterm had already re-rasterized by the time the invalidation ran. That ordering held on every panel and in both directions here — though it is a timing observation on one machine, not a guarantee. On scope: this is a regression introduced by this PR, not a pre-existing issue. The unit test models xterm's device-pixel rounding with a deliberately fractional glyph advance — with a whole-number cell the DPR 1 and DPR 2 grids coincide and it cannot tell a stale baseline from a fresh one. Reverting the invalidation fails it while the other two cases still pass. |
Symptoms
Three things went wrong whenever the canvas was zoomed:
Cause
Canvas zoom CSS-scales the world div, which stretches xterm's pre-rasterized glyph atlas and blurs it.
TerminalPanelcompensated by raisingfontSizetobase × renderScaleand counter-scaling the box by1/renderScale.The problem is that xterm ceils its cell pixels, so
cell(k·fs) ≠ k·cell(fs)and the compensation drifts away from the thing it is compensating for.Measured at 13px / DPR 1:
The box grows by
renderScalewhile the cell grows by more, so columns disappear — and each drop reaches the PTY as a SIGWINCH. Zoom stopped being a zoom and became a resize.Fix
Drive layout off the measured cell instead of the intended ratio:
With the scale gone from the arithmetic, the column count is fixed by construction at any zoom.
7:15 → 9:19 → 11:23), so a uniform width-driven scale left the height 1.5–2.4% short.11/1.5 = 7.33for a cell that is really 7). That 4.8% error shrank the box at every scale including 1, which is what broke sizes below 100%. A panel first laid out already zoomed now drops to the base font, measures, and returns in the same frame (no visible flicker).scrollBarWidth— it is measured once in theViewportconstructor and never again. FitAddon is its only reader (xterm's core never touches the field), so a stale value meant subtracting 8 from a 17px track and returning a column count whose grid overflowed its viewport, clipping right-hand columns.Removing
preserveGridIt was the earlier mitigation: discard ±1 column/row deltas so the PTY never saw a phantom SIGWINCH. The real deltas turned out to be 3–9 columns, which went straight through it.
It now has no callers and is removed. Its regression coverage is replaced by three tests for the scrollbar cache sync (writes the measurement, keeps the last good value when the track reports 0, degrades quietly if a future xterm drops the field).
snapRenderScalemoves to its own module — left in place, testing the snap boundaries pulls in xterm's WebGL addon, which needs a browser global at import time and dies under the node test environment.Verification
Instrumented a running app:
plain fit changed gridlogged zero times during zoom. 237 tests pass (5 + 3 new).Limits of that verification
All measurements come from a single DPR 1 machine. The arithmetic is DPR-independent but the confirmation is not — the rounding unit is
1/DPR, so on a retina display the error halves and may be zero at some steps.This is the same trap the
preserveGridcommit fell into: it was tuned to "flipped rows by 1" as observed in one environment, and broke where the deltas were 3–9. A second pair of eyes on a retina display would be valuable.Left alone
renderScale1 — the atlas is being downsampled anyway, so rasterizing it larger buys nothing.glyph:cellratio differs per scale; the cell itself is fixed, so layout does not move._core.viewport.scrollBarWidthis an internal. The only debt here. Every hop is optional-chained so a future xterm turns scaling off rather than throwing — but it fails silently, which is why the tests above stand guard.Also adds
.omc/(agent tooling state) to.gitignore— same category as the.agents/and.cate/entries beside it.🤖 Generated with Claude Code
https://claude.ai/code/session_01WsMgXwPNJJ4SgXgRff5wJk