Skip to content

Attempt to fix highlighting issue in monaco #12917

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 6 commits into from
Sep 30, 2025
Merged

Attempt to fix highlighting issue in monaco #12917

Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
b2f2627
attempt to fix highlight issue in monaco
jjspace Sep 24, 2025
0010d7a
expose monaco for testing
jjspace Sep 25, 2025
89d036f
wait for font load before setting monaco
jjspace Sep 25, 2025
f9212da
remove testing code
jjspace Sep 26, 2025
20aa51f
Merge remote-tracking branch 'origin/main' into monaco-highlight-fix
jjspace Sep 26, 2025
1d87ed0
Merge branch 'main' into monaco-highlight-fix
ggetz Sep 30, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
24 changes: 21 additions & 3 deletions packages/sandcastle/src/SandcastleEditor.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -83,10 +83,28 @@ function SandcastleEditor({
const {
settings: { fontFamily, fontSize, fontLigatures },
} = useContext(SettingsContext);
const documentRef = useRef(document);
useEffect(() => {
internalEditorRef.current?.updateOptions({
fontFamily: availableFonts[fontFamily]?.cssValue ?? "Droid Sans Mono",
});
const cssName = availableFonts[fontFamily]?.cssValue ?? "Droid Sans Mono";
const fontFace = [...documentRef.current.fonts.values()].find(
(font) => font.family === cssName && font.weight === "400",
);
if (fontFace?.status !== "loaded") {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For simplicity, could we instead just wait on document.fonts.ready and then call monaco.editor.remeasureFonts()?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Per MDN:

Note that the font.load() returns a promise, so we could have handled the completion of font loading by chaining then afterwards. Using document.fonts.ready can be better in some circumstances, as it is only called when all fonts in the document have been resolved and layout is complete.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ggetz Unfortunately not. That promise is a one time thing and it always returned instantly when I tried. On top of that, that promise and the other onfontloaded event handlers only trigger when the font is loaded using the fonts API and do not seem to trigger when a font is loaded because of styling changes (like how monaco updates it). I tried other arrangements of this and none worked so I settled on this as the best option.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, bummer.

// Monaco needs to check the size of the font for things like cursor position
// and variable highlighting. If it does this check before the font has loaded
// it will get the wrong size and may be horribly offset especially with ligatures
// https://github.com/microsoft/monaco-editor/issues/392
documentRef.current.fonts.load(`1rem ${cssName}`).then(() => {
internalEditorRef.current?.updateOptions({
fontFamily: cssName,
});
monaco.editor.remeasureFonts();
});
} else {
internalEditorRef.current?.updateOptions({
fontFamily: cssName,
});
}
}, [fontFamily]);
useEffect(() => {
internalEditorRef.current?.updateOptions({
Expand Down