-
Notifications
You must be signed in to change notification settings - Fork 33.6k
Fix advanced line wrap if decorations are present #199021
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.
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
base: main
Are you sure you want to change the base?
Fix advanced line wrap if decorations are present #199021
Conversation
This also uncovers that rerendering changes text selections. When selecting text, the selection ranges within the wrapped viewport are saved. When line wrapping changes, these ranges are restored. So for example if the user selects wrapped line 2, and then line 1 becomes longer because of new decorators, the selection is suddenly at the same wrapped position on line to. I would expect the selection to retain the same content instead. Is this desirable behaviour or should it be changed? And should the change be part of this PR? I tried making a screencast to clarify, but unfortunately my screencast tool is broken. Based on this branch, you can reproduce the issue using the following code in the playground. After applying it, you have 10 seconds to select a bit of text, ideally near the beginning of a viewline. const value = `big big big big big big big big big big big big big big big big big big big big big big big big big big big big big big big big big big big big big big big big big big big big big big big big big big big big big big big big big big big big big big big big big big big big big big big big big big big big big big big big big big big big big big big big
`
const model = monaco.editor.createModel(value, 'markdown')
const editor = monaco.editor.create(document.getElementById('container'), {
automaticLayout: true,
wordWrap: 'on',
wrappingStrategy: 'advanced',
fontFamily: 'Arial',
model
})
const headerDecorationsCollection = editor.createDecorationsCollection()
function applyDecorations() {
/** @type {monaco.editor.IModelDeltaDecoration[]} */
const decorations = []
const value = model.getValue()
for (const match of value.matchAll(/big/g)) {
const start = model.getPositionAt(match.index)
const end = model.getPositionAt(match.index + match[0].length)
const range = new monaco.Range(
start.lineNumber,
start.column,
end.lineNumber,
end.column
)
decorations.push({
range,
options: {
lineHeight: 57,
inlineClassName: 'big'
}
})
}
for (const match of value.matchAll(/small/g)) {
const start = model.getPositionAt(match.index)
const end = model.getPositionAt(match.index + match[0].length)
const range = new monaco.Range(
start.lineNumber,
start.column,
end.lineNumber,
end.column
)
decorations.push({
range,
options: {
lineHeight: 10,
inlineClassName: 'small'
}
})
}
headerDecorationsCollection.set(decorations)
}
editor.onDidChangeModelContent(applyDecorations)
setTimeout(() => {
applyDecorations()
}, 10_000) .small {
font-size: 9px;
}
.big {
font-size: 55px;
} |
Can you rebase onto main (to fix merge conflicts)? And maybe squash your changes into one commit? |
23f4ec2
to
f7cfeaf
Compare
Done! |
Thanks for the PR! I understand the problem and I think it can be fixed. However, this PR has a dramatic performance impact for 99% of VS Code users (who don't use the dom-based line break algorithm, but the monospace one). I think we should handle this:
This is a bug and should be fixed. |
|
||
lowRects = lowRects || readClientRect(range, spans, charOffsets[low], charOffsets[low + 1]); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks like a significantly changed implementation.
Can you give more context on what happened here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The addition of decorators means an additional <span>
element is added. This required some significant changes, as the algorithm doesn’t deal with arbritrary DOM elements, but specifically one <span>
with another <span>
inside. It now deals with the other <span>
that’s added for the decorations.
Actually I had some trouble grasping what’s going on here, so I came up with my own implementation in a small standalone project. My own implementation turned out very similar. I then applied the new formula here while trying to keep the changes to a minimum
555d520
to
276448e
Compare
276448e
to
327b916
Compare
327b916
to
7e65ed2
Compare
996d1f9
to
bdd2911
Compare
bdd2911
to
bcc02a2
Compare
7df2bdd
to
03425a9
Compare
03425a9
to
7e710d8
Compare
I rebased onto main and added back support for line injected text. I tested the changes in the playground introduced in #249616 with the following code: /*---------------------------------------------------------------------------------------------
* Copyright (c) Microsoft Corporation. All rights reserved.
* Licensed under the MIT License. See License.txt in the project root for license information.
*--------------------------------------------------------------------------------------------*/
// Enable automatic dark mode for accessibility.
const dark = matchMedia('(prefers-color-scheme: dark)');
monaco.editor.setTheme(dark.matches ? 'vs-dark' : 'vs-light');
dark.addEventListener('change', () => {
monaco.editor.setTheme(dark.matches ? 'vs-dark' : 'vs-light');
});
const content = `The world has changed
I feel it in the water.
I feel it in the earth.
I smell it in the air.
Much that once was is lost, for none now live who remember it.
It began with the forging of the Great Rings. Three were given to the Elves, immortal, wisest and fairest of all beings. Seven to the Dwarf-Lords, great miners and craftsmen of the mountain halls. And nine, nine rings were gifted to the race of Men, who above all else desire power. For within these rings was bound the strength and the will to govern each race. But they were all of them deceived, for another ring was made. Deep in the land of Mordor, in the Fires of Mount Doom, the Dark Lord Sauron forged a master ring, and into this ring he poured his cruelty, his malice and his will to dominate all life.
One ring to rule them all.
One by one, the free lands of Middle-Earth fell to the power of the Ring, but there were some who resisted. A last alliance of men and elves marched against the armies of Mordor, and on the very slopes of Mount Doom, they fought for the freedom of Middle-Earth. Victory was near, but the power of the ring could not be undone. It was in this moment, when all hope had faded, that Isildur, son of the king, took up his father’s sword.
Sauron, enemy of the free peoples of Middle-Earth, was defeated. The Ring passed to Isildur, who had this one chance to destroy evil forever, but the hearts of men are easily corrupted. And the ring of power has a will of its own. It betrayed Isildur, to his death.
And some things that should not have been forgotten were lost. History became legend. Legend became myth. And for two and a half thousand years, the ring passed out of all knowledge. Until, when chance came, it ensnared another bearer.
It came to the creature Gollum, who took it deep into the tunnels of the Misty Mountains. And there it consumed him. The ring gave to Gollum unnatural long life. For five hundred years it poisoned his mind, and in the gloom of Gollum’s cave, it waited. Darkness crept back into the forests of the world. Rumor grew of a shadow in the East, whispers of a nameless fear, and the Ring of Power perceived its time had come. It abandoned Gollum, but then something happened that the Ring did not intend. It was picked up by the most unlikely creature imaginable: a hobbit, Bilbo Baggins, of the Shire.
For the time will soon come when hobbits will shape the fortunes of all.
`;
const model = monaco.editor.createModel(content, undefined, monaco.Uri.file('example.ts'));
const editor = monaco.editor.create(document.getElementById('editor')!, {
automaticLayout: true,
autoIndent: 'none',
accessibilitySupport: 'off',
accessibilityPageSize: 1,
autoClosingBrackets: 'never',
autoClosingComments: 'never',
autoClosingDelete: 'never',
autoClosingOvertype: 'never',
autoClosingQuotes: 'never',
autoSurround: 'never',
bracketPairColorization: { enabled: false },
columnSelection: false,
contextmenu: false,
definitionLinkOpensInPeek: false,
detectIndentation: false,
disableLayerHinting: true,
disableMonospaceOptimizations: true,
fixedOverflowWidgets: false,
inDiffEditor: false,
largeFileOptimizations: true,
linkedEditing: false,
links: false,
matchBrackets: 'never',
parameterHints: { enabled: false },
model,
minimap: { enabled: false },
fontFamily: 'Arial, Arimo, sans-serif, ui-sans-serif',
fontLigatures: false,
fontVariations: false,
fontWeight: 'normal',
fontSize: 14,
lineHeight: 0,
letterSpacing: 0,
// Wrap lines at viewport width
wordWrap: 'on',
// Advanced wrapping strategy - necessary to ensure line wraps with variable width fonts
wrappingStrategy: 'advanced',
wordBreak: 'keepAll',
wrappingIndent: 'none',
});
const boldCollection = editor.createDecorationsCollection();
const textCollection = editor.createDecorationsCollection();
editor.addAction({
keybindings: [monaco.KeyMod.CtrlCmd | monaco.KeyCode.KeyB],
id: 'bold',
label: 'Bold',
run(editor) {
const selections = editor.getSelections();
if (!selections) {
return;
}
boldCollection.set(selections.map((selection) => (
{
range: selection,
options: {
inlineClassNameAffectsLetterSpacing: true,
inlineClassName: 'cuescript bold'
}
}
)));
},
});
editor.addAction({
keybindings: [monaco.KeyMod.CtrlCmd | monaco.KeyCode.KeyG],
id: 'after',
label: 'After',
run(editor) {
const selections = editor.getSelections();
if (!selections) {
return;
}
textCollection.set(selections.map((selection) => (
{
range: selection,
options: {
inlineClassNameAffectsLetterSpacing: true,
inlineClassName: 'cuescript bold',
after: {
inlineClassName: 'foo',
content: 'XXX'
},
}
}
)));
},
});
// Make the model and editor available globally for fiddling in the console.
Object.assign(globalThis, {
editor,
model,
}); html,
body {
font-family: system-ui;
margin: 0;
height: 100%;
}
main {
display: flex;
flex-flow: column;
height: 100%;
}
h1 {
background: #00a7da;
color: #111111;
flex: 0 1 auto;
margin: 0;
overflow: hidden;
padding: 0.5rem 1rem;
text-overflow: ellipsis;
text-wrap: nowrap;
}
#editor {
margin: 1rem;
flex: 1 1 auto;
}
.cuescript {
&.bold {
color: red;
font-weight: bold;
}
} |
Decorations may affect line wrapping. For example, they make make the text bold, or increase or decrease font size. This wasn’t originally taken into account to calculate the the break offsets.
7e710d8
to
7e84da4
Compare
Hi @remcohaszing thank you for the PR! I didn't know you made this PR to take into account line break data, I just found out. Unfortunately I am currently already working on adopting variable font sizes and as part of the PR I have modified the code that calculates the line breaks to correctly take into account variable font-sizes. The PR is here: #248410. The approach we have taken is to use the DomLineBreaksComputer class when we detect there are variable font sizes on a line and inside of this class to render the line exactly as it is in the editor. |
I like that approach better than my own. This PR does one more thing of which it’s not clear to me whether #248410 does it too. For advanced text wrapping, this PR takes decorations into account that specify the option |
Hi thanks for your comment. I other PR applies all the inline decorations as they are applied in the editor so I believe it should also apply the decorations you mentioned. I will double check that. |
Decorations may affect line wrapping. For example, they make make the text bold, or increase or decrease font size. This wasn’t originally taken into account to calculate the the break offsets.
This was discovered as part of #194609, but it’s actually unrelated. This not only affects line breaking if decorations make text bigger, but also if they make it smaller, bolder, etc.
cc @hediet