Skip to content
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
19 commits
Select commit Hold shift + click to select a range
a6b2bd6
Simplify helper method
davidwengier Dec 24, 2025
71c5305
Drop Html indentation changes unless they're in script or style tags
davidwengier Dec 24, 2025
c06c22e
Drop all html formatting changes before the first non-whitespace char…
davidwengier Dec 24, 2025
94d066b
Indent attributes in line with the first attribute on an element
davidwengier Dec 24, 2025
d8a3fca
Add basic Html attribute test, and fix bug with generic type params
davidwengier Dec 24, 2025
eb6d9a2
Format RenderFragments that start in explicit expressions
davidwengier Dec 24, 2025
e19c614
Copy indentation from the previous line when ignoring an open brace
davidwengier Dec 24, 2025
e9795e9
Code blocks are always at column 0
davidwengier Dec 24, 2025
60b6257
Text outside of Html elements if formatted to column 0
davidwengier Dec 24, 2025
0b2b168
Update implicit and explicit expression test outputs
davidwengier Dec 24, 2025
b4b9a6d
Update "bad input" test output
davidwengier Dec 24, 2025
143126c
Update RenderFragment test outputs
davidwengier Dec 24, 2025
8ac743c
Add a few more tests to demonstrate fixed issues
davidwengier Dec 24, 2025
4b6666c
Don't do any formatting in Html comments
davidwengier Dec 24, 2025
a12f688
Update doc comment to represent the new behaviour
davidwengier Dec 24, 2025
df04f2a
Fix first attribute alignment calculation, add more test coverage
davidwengier Dec 24, 2025
7a3f323
Fix test baseline
davidwengier Dec 24, 2025
e3ef464
Fix formatting in release mode
davidwengier Dec 25, 2025
61a0354
Unskip test, because I accidentally fixed it :)
davidwengier Dec 25, 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
Original file line number Diff line number Diff line change
Expand Up @@ -255,7 +255,7 @@ static bool IsInScriptOrStyleOrHtmlComment(AspNetCore.Razor.Language.Syntax.Synt
{
if (node is MarkupElementSyntax elementNode)
{
if (RazorSyntaxFacts.IsStyleBlock(elementNode) || RazorSyntaxFacts.IsScriptBlock(elementNode))
if (RazorSyntaxFacts.IsScriptOrStyleBlock(elementNode))
{
return true;
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -172,6 +172,11 @@ private static ImmutableArray<FormattingSpan> GetFormattingSpans(RazorSyntaxTree
/// <returns>A whitespace string representing the indentation level based on the configuration.</returns>
public string GetIndentationLevelString(int indentationLevel)
{
if (indentationLevel == 0)
{
return "";
}

var indentation = GetIndentationOffsetForLevel(indentationLevel);
var indentationString = FormattingUtilities.GetIndentationString(indentation, Options.InsertSpaces, Options.TabSize);
return indentationString;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -152,9 +152,7 @@ public static int GetIndentationLevel(TextLine line, int firstNonWhitespaceChara
var currentIndentationWidth = firstNonWhitespaceCharacterPosition - line.Start;
if (insertSpaces)
{
var indentationLevel = currentIndentationWidth / tabSize;
additionalIndentation = new string(' ', currentIndentationWidth % tabSize);
return indentationLevel;
return GetIndentationLevel(currentIndentationWidth, tabSize, out additionalIndentation);
}

// For tabs, we just count the tabs, and additional is any spaces at the end.
Expand All @@ -178,6 +176,16 @@ public static int GetIndentationLevel(TextLine line, int firstNonWhitespaceChara
return tabCount;
}

public static int GetIndentationLevel(int length, int tabSize, out string additionalIndentation)
{
var indentationLevel = length / tabSize;
var additionalIndentationLength = length % tabSize;
additionalIndentation = additionalIndentationLength == 0
? ""
: new string(' ', additionalIndentationLength);
return indentationLevel;
}

/// <summary>
/// Given a <paramref name="indentation"/> amount of characters, generate a string representing the configured indentation.
/// </summary>
Expand Down

Large diffs are not rendered by default.

Original file line number Diff line number Diff line change
Expand Up @@ -73,6 +73,8 @@ public async Task<ImmutableArray<TextChange>> ExecuteAsync(FormattingContext con
break;
}

string? indentationString = null;

var formattedLine = formattedCSharpText.Lines[iFormatted];
if (lineInfo.ProcessIndentation &&
formattedLine.GetFirstNonWhitespaceOffset() is { } formattedIndentation)
Expand All @@ -85,7 +87,7 @@ public async Task<ImmutableArray<TextChange>> ExecuteAsync(FormattingContext con
// First up, we take the indentation from the formatted file, and add on the Html indentation level from the line info, and
// replace whatever was in the original file with it.
var htmlIndentString = context.GetIndentationLevelString(lineInfo.HtmlIndentLevel);
var indentationString = formattedCSharpText.ToString(new TextSpan(formattedLine.Start, formattedIndentation))
indentationString = formattedCSharpText.ToString(new TextSpan(formattedLine.Start, formattedIndentation))
+ htmlIndentString
+ lineInfo.AdditionalIndentation;
formattingChanges.Add(new TextChange(new TextSpan(originalLine.Start, originalLineOffset), indentationString));
Expand Down Expand Up @@ -173,6 +175,21 @@ public async Task<ImmutableArray<TextChange>> ExecuteAsync(FormattingContext con
if (NextLineIsOnlyAnOpenBrace(changedText, iOriginal))
{
iOriginal++;

// We're skipping a line in the original document, because Roslyn brought it up to the previous
// line, but the fact is the opening brace was in the original document, and might need its indentation
// adjusted. Since we can't reason about this line in any way, because Roslyn has changed it, we just
// apply the indentation from the previous line. Previously the indentation on this line would have been
// adjusted by the Html formatter and we wouldn't have needed to worry.
//
// If we didn't adjust the indentation of the previous line, then we really have no information to go
// on at all, so hopefully the user is happy with where their open brace is.
if (indentationString is not null)
{
var originalLine = changedText.Lines[iOriginal];
var originalLineOffset = originalLine.GetFirstNonWhitespaceOffset().GetValueOrDefault();
formattingChanges.Add(new TextChange(new TextSpan(originalLine.Start, originalLineOffset), indentationString));
}
}
}
}
Expand Down
Copy link
Member Author

Choose a reason for hiding this comment

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

It probably goes without saying, but make sure you're viewing this diff with whitespace on :)

Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
// Licensed to the .NET Foundation under one or more agreements.
// The .NET Foundation licenses this file to you under the MIT license.

using System;
using System.Collections.Immutable;
using System.Linq;
using System.Threading;
Expand Down Expand Up @@ -87,13 +88,26 @@ private async Task<ImmutableArray<TextChange>> FilterIncomingChangesAsync(Format
var syntaxRoot = codeDocument.GetRequiredSyntaxRoot();
var sourceText = codeDocument.Source.Text;
SyntaxNode? csharpSyntaxRoot = null;
ImmutableArray<int> scriptAndStyleElementBounds = default;

using var changesToKeep = new PooledArrayBuilder<TextChange>(capacity: changes.Length);

foreach (var change in changes)
{
// We don't keep changes that start inside of a razor comment block.
var node = syntaxRoot.FindInnermostNode(change.Span.Start);

// We don't keep indentation changes that aren't in a script or style block
// As a quick check, we only care about dropping edits that affect indentation - ie, are before the first
// whitespace char on a line
var line = sourceText.Lines.GetLineFromPosition(change.Span.Start);
if (change.Span.Start < line.GetFirstNonWhitespacePosition() &&
!ChangeIsInsideScriptOrStyleElement(change))
{
context.Logger?.LogMessage($"Dropping change {change} because it's not in a script or style block");
continue;
}

// We don't keep changes that start inside of a razor comment block.
var comment = node?.FirstAncestorOrSelf<RazorCommentBlockSyntax>();
if (comment is not null && change.Span.Start > comment.SpanStart)
{
Expand Down Expand Up @@ -190,6 +204,60 @@ async Task<bool> ChangeIsInStringLiteralAsync(FormattingContext context, RazorCS

return false;
}

bool ChangeIsInsideScriptOrStyleElement(TextChange change)
{
// Rather than ascend up the tree for every change, and look for script/style elements, we build up an index
// first and just reuse it for every subsequent edit.
InitializeElementBoundsArray();

// If there aren't any elements we're interested in, we don't need to do anything
if (scriptAndStyleElementBounds.Length == 0)
{
return false;
}

var index = scriptAndStyleElementBounds.BinarySearch(change.Span.Start);

// If we got a hit, then we're on the tag itself, which is not inside the tag
if (index >= 0)
{
return false;
}

// If we don't get a hit, the complement of the result is the index to the next largest item in the array. Since we add
// things to the array in pairs, we know that if that index is even, we're outside an element, so we want to ignore identation.
// This only works if the array is ordered, but we can assume it is because we built it in order, and it makes no sense to nest
// script tags within style tags, or vice versa.
index = ~index;
if (index % 2 == 0)
{
return false;
}

return true;
}

void InitializeElementBoundsArray()
{
if (!scriptAndStyleElementBounds.IsDefault)
{
return;
}

using var boundsBuilder = new PooledArrayBuilder<int>();

foreach (var element in syntaxRoot.DescendantNodes(static node => node is RazorDocumentSyntax or MarkupBlockSyntax or MarkupElementSyntax or CSharpCodeBlockSyntax).OfType<MarkupElementSyntax>())
{
if (RazorSyntaxFacts.IsScriptOrStyleBlock(element))
{
boundsBuilder.Add(element.SpanStart);
boundsBuilder.Add(element.Span.End);
}
}

scriptAndStyleElementBounds = boundsBuilder.ToImmutableAndClear();
}
}

internal TestAccessor GetTestAccessor() => new(this);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -84,10 +84,12 @@ public static bool TryGetFullAttributeNameSpan(RazorCodeDocument codeDocument, i
return attributeNameSpan != default;
}

private static TextSpan GetFullAttributeNameSpan(RazorSyntaxNode? node)
public static TextSpan GetFullAttributeNameSpan(RazorSyntaxNode? node)
{
return node switch
{
MarkupAttributeBlockSyntax att => att.Name.Span,
MarkupMinimizedAttributeBlockSyntax att => att.Name.Span,
MarkupTagHelperAttributeSyntax att => att.Name.Span,
MarkupMinimizedTagHelperAttributeSyntax att => att.Name.Span,
MarkupTagHelperDirectiveAttributeSyntax att => CalculateFullSpan(att.Name, att.ParameterName, att.Transition),
Expand Down Expand Up @@ -208,18 +210,29 @@ internal static bool IsInUsingDirective(RazorSyntaxNode node)
return false;
}

internal static bool IsElementWithName(MarkupElementSyntax? element, string name)
internal static bool IsScriptOrStyleBlock(MarkupElementSyntax? element)
{
return string.Equals(element?.StartTag.Name.Content, name, StringComparison.OrdinalIgnoreCase);
}
// StartTag is annotated as not nullable, but on invalid documents it can be. The 'Format_DocumentWithDiagnostics' test
// illustrates this.
if (element?.StartTag?.Name.Content is not { } tagName)
{
return false;
}

internal static bool IsStyleBlock(MarkupElementSyntax? node)
{
return IsElementWithName(node, "style");
return string.Equals(tagName, "script", StringComparison.OrdinalIgnoreCase) ||
string.Equals(tagName, "style", StringComparison.OrdinalIgnoreCase);
}

internal static bool IsScriptBlock(MarkupElementSyntax? node)
internal static bool IsAttributeName(RazorSyntaxNode node, [NotNullWhen(true)] out BaseMarkupStartTagSyntax? startTag)
{
return IsElementWithName(node, "script");
startTag = null;

if (node.Parent.IsAnyAttributeSyntax() &&
GetFullAttributeNameSpan(node.Parent).Start == node.SpanStart)
{
startTag = node.Parent.Parent as BaseMarkupStartTagSyntax;
}

return startTag is not null;
}
}
Loading