fix(ace-editor): caret drift from late font loads and emoji widths#41697
Conversation
…t drift Ace caches glyph width once at editor construction via its internal FontMetrics. If the editor font settles after that measurement, the cached width no longer matches the rendered glyphs and the caret drifts further from the text the longer the line — the residual misalignment in issue apache#41664 that the font-family CSS from apache#38928 does not address (the font family is already applied consistently via setOptions and CSS; the problem is measurement timing, not the font itself). Force Ace to re-measure on load and again once document.fonts.ready resolves, so the caret realigns. A consumer-supplied onLoad is still invoked. Fixes apache#41664
Code Review Agent Run #d91e17Actionable Suggestions - 0Review Details
Bito Usage GuideCommands Type the following command in the pull request comment and save the comment.
Refer to the documentation for additional commands. Configuration This repository uses Documentation & Help AI Code Review powered by |
✅ Deploy Preview for superset-docs-preview ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #41697 +/- ##
==========================================
+ Coverage 64.73% 64.75% +0.01%
==========================================
Files 2687 2687
Lines 148743 148502 -241
Branches 34329 34277 -52
==========================================
- Hits 96295 96164 -131
+ Misses 50681 50599 -82
+ Partials 1767 1739 -28
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
…types updateFontSize() runs Ace's checkForSizeChanges, which already triggers a forced resize and full re-render when the measured character size changed, so the explicit renderer.onResize(true) was redundant — and it broke tsc since onResize is not part of ace-builds' public VirtualRenderer type. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Code Review Agent Run #478bdeActionable Suggestions - 0Review Details
Bito Usage GuideCommands Type the following command in the pull request comment and save the comment.
Refer to the documentation for additional commands. Configuration This repository uses Documentation & Help AI Code Review powered by |
EnxDev's Review Agent — #41697 · HEAD 6b7dca5comment — the change is safe and CI-green, but the vendored Ace source contradicts its root-cause premise: this very likely does not fix #41664, and 🔴 Functional
🟡 Should-fix
🔵 Nits
🙌 Praise
|
Ace's FontMetrics re-measures via a ResizeObserver on its measure node, not only once at construction; reword the comment to match the vendored ace-builds behavior. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
Good catch on the FontMetrics point, @EnxDev, the ResizeObserver re-measure does contradict the "measures once" wording, so I reworded that comment. You're right that this shouldn't merge (and auto-close #41664) until I've confirmed the caret actually lands on the repro, which I can't do headlessly. Holding on that. |
![codeant-ai-for-open-source[bot]](https://avatars.githubusercontent.com/u/151810221?s=60&v=4){kind=small}
|
The flagged issue is correct. In environments where To resolve this, you can guard the entire promise chain by checking for the existence of Proposed Fix remeasure();
if (document.fonts?.ready) {
document.fonts.ready.then(remeasure).catch(() => {});
}
onLoad?.(editor);This change ensures that the Would you like me to check the rest of the comments on this PR and implement fixes for them as well? superset-frontend/packages/superset-ui-core/src/components/AsyncAceEditor/index.tsx |
Code Review Agent Run #ab91f3Actionable Suggestions - 0Review Details
Bito Usage GuideCommands Type the following command in the pull request comment and save the comment.
Refer to the documentation for additional commands. Configuration This repository uses Documentation & Help AI Code Review powered by |
|
It's broken again. Added a screen recording to understand the issue. superset-cursor.mp4 |
rusackas
added
the
🎪 ⚡ showtime-trigger-start
github-actions
Bot
added
🎪 582c21a 🚦 building
🎪 ⌛ 48h
…onts.ready fonts.ready resolves once, and can settle before a lazily-referenced editor font even starts loading; waiting on it alone misses the swap entirely, which is the residual drift reported on the PR. Explicitly request the editor font via fonts.load() and re-measure when that load resolves, keep the fonts.ready path for loads already in flight, and re-measure on subsequent loadingdone events while mounted (unsubscribed on unmount). Also guards the promise chains with full optional chaining: the previous expression threw at .catch when document.fonts was unavailable (CodeAnt catch), and the re-measure itself now tolerates a font event firing after the editor was destroyed. Adds tests for the no-Font-Loading-API case, the fonts.load() re-measure with an already-settled fonts.ready (the reported hole), and the loadingdone subscribe/unsubscribe lifecycle. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Ace forces "full width" chars into 2×charWidth boxes and counts them as two columns, so grid math and pixels agree; its tables only cover East-Asian ranges. Astral emoji accidentally work (the renderer's surrogate-pair branch + two-code-unit counting), but BMP emoji with default emoji presentation (✨ U+2728, ⭐ …) count as one column while rendering ~1.6 cells, drifting the caret on any line containing one — the residual misalignment in apache#41664 after the font-metrics fix. Upstream ace still lacks this (ajaxorg/ace#3404). Extend the same mechanism to emoji: override the two EditSession width methods (verbatim internals plus an emoji branch; the originals close over a module-private isFullWidth) and wrap the text layer's $renderToken to emit the forced-width .ace_cjk box for emoji clusters, delegating everything else to the original. VS16 sequences top up to two columns total; ZWJ/flag sequences are out of scope (equally misaligned before). Tests cover classification, VS16 accounting, caret math on the reported repro line, astral parity, renderer box geometry, the emoji-free fast path, and patch idempotence. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
rusackas
changed the title
|
Second root cause found and fixed in 897d2f9, thanks to the ✨ screenshot: Ace's full-width tables only cover East-Asian ranges. Astral emoji (💡, surrogate pairs) accidentally align because the renderer's surrogate branch forces a 2-cell box and the column counter counts two code units, but BMP emoji like ✨ (U+2728) fall through both nets... one column in the math, ~1.6 cells on screen, and the caret drifts on exactly those lines. Upstream ace has this open as ajaxorg/ace#3404. The fix extends the mechanism ace already uses for CJK to emoji-presentation codepoints and VS16 sequences: two columns in the session math, and the same forced 2×charWidth Eight new tests, including caret math on the literal |
|
🎪 Showtime deployed environment on GHA for 897d2f9 • Environment: http://54.245.181.40:8080 (admin/admin) |
…okens The merge with master resolves the pre-commit failure (CI formats the merge result, and master's lodash-es bump had touched this file). Adds parity tests for the non-emoji branches of the copied EditSession internals (tabs, early return, break, space/punctuation/CJK tokens) to restore the core-packages-ts project coverage, which caught a real gap: VS16 after a text-presentation base (❤️) fell through to a plain CHAR, leaving the pair splittable at wrap. It now pushes CHAR_EXT. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
🎪 Showtime deployed environment on GHA for 80fdd5e • Environment: http://54.191.140.2:8080 (admin/admin) |
master bumped prettier to 3.9.3, which wraps one pre-existing template interpolation differently; CI formats the merge result so the file must match the new version. Also brings emojiWidthPatch.ts to 100% coverage (the core-packages-ts codecov flag targets 100%): the unreachable match fallback is gone in favor of matchAll, and tests now cover the leading-emoji split part and the memoized emoji-base lookup. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
🎪 Showtime deployed environment on GHA for 65bb7fb • Environment: http://34.222.98.66:8080 (admin/admin) |
|
I think it's actually fixed now! 
Merging, but happy to make touchups or even rever this if needed... onward and upward! |
|
Bito Automatic Review Skipped – PR Already Merged |
6 participants
SUMMARY
Fixes the residual caret/cursor misalignment when editing any Ace-backed text area (Markdown, SQL Lab, Explore custom SQL, CSS, etc.) reported in #41664.
Why the existing fix doesn't cover it: the font-family fix from #38928 (adding
font-familyto.ace_content/.ace_text-layer) is already present onmasterand is applied consistently —setOptions={{ fontFamily }}and the CSS both use the sameeditorFontFamily. So there is no font-family mismatch left. The remaining drift is a measurement-timing problem:Ace measures glyph width once, at editor construction, via its internal
FontMetrics. If the editor font finishes loading after that measurement, the cached character width no longer matches the rendered glyphs, and the caret drifts further from the text the longer the line — which matches the screenshot in the issue (misalignment grows with column position).Fix: wrap the editor's
onLoadto force Ace to re-measure — immediately on load and again oncedocument.fonts.readyresolves — viarenderer.updateFontSize()(which calls through toFontMetrics.checkForSizeChanges()) +renderer.onResize(true). A consumer-suppliedonLoadis still invoked. No CSS or font changes.BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
N/A (caret positioning; see the screencast in #41664). Needs a visual confirm on the repro — I don't have a way to verify the rendered caret headlessly.
TESTING INSTRUCTIONS
cd superset-frontend npx jest packages/superset-ui-core/src/components/AsyncAceEditor/AsyncAceEditor.test.tsx --no-coverageNew unit test asserts the editor re-measures after
document.fonts.readyand that a consumer-providedonLoadis still called. Manual: open a Markdown/text control in a dashboard (or SQL Lab) and type a long line — the caret should stay aligned with the glyphs.ADDITIONAL INFORMATION
Note: the earlier #38928 change is already merged and does not resolve this; re-applying it (as suggested on the issue) would be a no-op. This targets the measurement-timing root cause instead.