Skip to content

Making the whitespaces align vertically with variable font text#298252

Open
aiday-mar wants to merge 16 commits intomainfrom
religious-crawdad
Open

Making the whitespaces align vertically with variable font text#298252
aiday-mar wants to merge 16 commits intomainfrom
religious-crawdad

Conversation

@aiday-mar
Copy link
Contributor

@aiday-mar aiday-mar commented Feb 27, 2026

fixes #298249

  • The spans are bottom aligned to the bottom of the larger line height. A padding of 3px was added to not have the text glued to the bottom of the line.
  • The ascent and descent are calculated in the FontInfo to form the line height
  • The getFontInfoAtPosition returns a FontInfo object now instead of { fontSize: number, fontFamily: string}
  • To avoid layer importing issues I had to pass in the FontMeasurements.readFontInfo method into the constructor of the ViewModelImpl
  • fontSize on the ModelDecorations has been changed to a number for several reasons:
    • it mirrors the lineHeight option which is a number multiplier of the default line height
    • it makes it easier to compute the bare font info using the number fontSize
    • I need it to be a number so the font tokenization provider can return the multiplier without making it into a string (which is not possible since we don't have access to the configuration object in the provider)

Copilot AI review requested due to automatic review settings February 27, 2026 14:11
@aiday-mar aiday-mar self-assigned this Feb 27, 2026
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

This PR adjusts editor whitespace rendering so that whitespace markers are rendered via the experimental whitespace overlay even on lines that contain variable-font-affecting decorations, improving alignment with the actual rendered text.

Changes:

  • Enable WhitespaceOverlay rendering for lines flagged with lineData.hasVariableFonts (remove the early-return skip).
  • Ensure view-line DOM rendering does not render whitespace when experimentalWhitespaceRendering !== 'off', regardless of hasVariableFonts, so the overlay is the single whitespace-rendering mechanism in experimental modes.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.

File Description
src/vs/editor/browser/viewParts/whitespace/whitespace.ts Removes the variable-font short-circuit so the overlay can render whitespace markers on those lines.
src/vs/editor/browser/viewParts/viewLines/viewLine.ts Simplifies the whitespace rendering decision so experimental modes always delegate whitespace rendering to the overlay.

@aiday-mar aiday-mar requested a review from alexdima March 3, 2026 17:03
@aiday-mar aiday-mar marked this pull request as ready for review March 3, 2026 17:03
@vs-code-engineering vs-code-engineering bot added this to the March 2026 milestone Mar 3, 2026
Copy link
Member

@alexdima alexdima left a comment

Choose a reason for hiding this comment

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

Nice! I just have these 2 questions / observations around:

  • what is the advantage of the super complex formula?
  • if we do need ascent and descent, can we enhance the existing infrastructure we have around DOM read font information

this._options = new WhitespaceOptions(this._context.configuration);
this._selection = [];
this._renderResult = null;
this._fontMetricsCache = new FontMetricsCache();
Copy link
Member

Choose a reason for hiding this comment

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

Would we want to share this instance across multiple editor views? Otherwise it needs to be recreated every time. Come to think of it, shouldn't we simply enhance FontInfo to contain also these two properties -- ascent and descent?

There is a lot of infrastructure in place to store that across editors, to reset the cache when the DPI changes (like plugging in a monitor or moving the window to another monitor), as well as to cache the values across vscode restarts to avoid flickering or slowness on startup.

This is all implemented at

export class FontMeasurementsImpl extends Disposable {
and I think you should be able to enhance it to add these extra props?

@alexdima alexdima modified the milestones: 1.111.0, 1.112.0 Mar 5, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Whitespace rendered at incorrect height when using variable line heights and font sizes

3 participants