Skip to content

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

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

remcohaszing
Copy link
Contributor

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

@hediet hediet added this to the December 2023 milestone Nov 28, 2023
@remcohaszing
Copy link
Contributor Author

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;
}

@hediet
Copy link
Member

hediet commented Jan 16, 2024

Can you rebase onto main (to fix merge conflicts)? And maybe squash your changes into one commit?

@remcohaszing remcohaszing force-pushed the fix-advanced-line-wrapping-with-decorations branch from 23f4ec2 to f7cfeaf Compare January 16, 2024 15:10
@remcohaszing
Copy link
Contributor Author

Done!

@aiday-mar aiday-mar modified the milestones: December / January 2024, February 2024 Jan 24, 2024
@hediet
Copy link
Member

hediet commented Feb 7, 2024

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).
For them, whenever someone sets a decoration, all view lines would be reconstructed and the view would be flushed. This is a blocker for this PR.

I think we should handle this:

  • When the default line break algorithm is used, setting decorations should only rerender affected lines (current behavior)
  • Even with the advanced algorithm, only certain decorations should cause re-computing the layout. I think there is already "decorationAffectsSpacing" on a decoration to mark such decorations. Computing the layout should be incremental, i.e. if a decoration increases the width a little bit and this doesn't cause a new line break, the view should not be rerendered.

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.

This is a bug and should be fixed.
It works for injected text though, which also affects line breaks (demo).


lowRects = lowRects || readClientRect(range, spans, charOffsets[low], charOffsets[low + 1]);
Copy link
Member

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?

Copy link
Contributor Author

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

@hediet hediet removed this from the February 2024 milestone Feb 19, 2024
@remcohaszing remcohaszing force-pushed the fix-advanced-line-wrapping-with-decorations branch 5 times, most recently from 555d520 to 276448e Compare May 27, 2024 09:27
@remcohaszing remcohaszing force-pushed the fix-advanced-line-wrapping-with-decorations branch from 276448e to 327b916 Compare June 11, 2024 16:56
@remcohaszing remcohaszing force-pushed the fix-advanced-line-wrapping-with-decorations branch from 327b916 to 7e65ed2 Compare July 5, 2024 12:00
@remcohaszing remcohaszing force-pushed the fix-advanced-line-wrapping-with-decorations branch 2 times, most recently from 996d1f9 to bdd2911 Compare August 8, 2024 13:32
@remcohaszing remcohaszing force-pushed the fix-advanced-line-wrapping-with-decorations branch from bdd2911 to bcc02a2 Compare August 13, 2024 09:19
@remcohaszing remcohaszing force-pushed the fix-advanced-line-wrapping-with-decorations branch 2 times, most recently from 7df2bdd to 03425a9 Compare September 5, 2024 11:17
@remcohaszing remcohaszing force-pushed the fix-advanced-line-wrapping-with-decorations branch from 03425a9 to 7e710d8 Compare May 23, 2025 13:33
@remcohaszing
Copy link
Contributor Author

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.
@remcohaszing remcohaszing force-pushed the fix-advanced-line-wrapping-with-decorations branch from 7e710d8 to 7e84da4 Compare May 28, 2025 15:31
@hediet hediet requested a review from aiday-mar June 13, 2025 16:41
@aiday-mar
Copy link
Contributor

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.

@aiday-mar aiday-mar self-assigned this Jun 17, 2025
@remcohaszing
Copy link
Contributor Author

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 inlineClassNameAffectsLetterSpacing. This is used to support bold and italic text, or even different fonts.

@aiday-mar
Copy link
Contributor

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants