Skip to content

Allow cohosting quick info to show html tag information even when on a taghelper or component tag. #12415

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
ToddGrun merged 3 commits into from
Oct 30, 2025
Merged

Allow cohosting quick info to show html tag information even when on a taghelper or component tag. #12415

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
2ec3f12
Allow cohosting quick info to show html tag information even when on …
ToddGrun Oct 30, 2025
07815eb
1) Support VS code html/razor hover merging
ToddGrun Oct 30, 2025
2c46c25
Fix VSCode hover test
ToddGrun Oct 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
55 changes: 46 additions & 9 deletions src/Razor/src/Microsoft.CodeAnalysis.Razor.CohostingShared/Hover/CohostHoverEndpoint.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@
using Microsoft.CodeAnalysis.ExternalAccess.Razor.Features;
using Microsoft.CodeAnalysis.Razor.Cohost;
using Microsoft.CodeAnalysis.Razor.Remote;
using Roslyn.Text.Adornments;

namespace Microsoft.VisualStudio.Razor.LanguageClient.Cohost;

Expand Down Expand Up @@ -54,29 +55,65 @@ public ImmutableArray<Registration> GetRegistrations(VSInternalClientCapabilitie
{
var position = LspFactory.CreatePosition(request.Position.ToLinePosition());

var response = await _remoteServiceInvoker
var razorResponse = await _remoteServiceInvoker
.TryInvokeAsync<IRemoteHoverService, RemoteResponse<LspHover?>>(
razorDocument.Project.Solution,
(service, solutionInfo, cancellationToken) =>
service.GetHoverAsync(solutionInfo, razorDocument.Id, position, cancellationToken),
cancellationToken)
.ConfigureAwait(false);

if (response.Result is LspHover hover)
if (razorResponse.StopHandling)
{
return hover;
return razorResponse.Result;
}

if (response.StopHandling)
{
return null;
}

return await _requestInvoker.MakeHtmlLspRequestAsync<TextDocumentPositionParams, LspHover>(
var htmlHover = await _requestInvoker.MakeHtmlLspRequestAsync<TextDocumentPositionParams, LspHover>(

@ToddGrun ToddGrun Oct 30, 2025

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

MakeHtmlLspRequestAsync

I'd love some guidance on changing this to not make the html call if we aren't in an html context. It looks like this information isn't at my fingertips, and I noticed some other cohosting calls across to the remote service end up returning a flag indicating the language that is being operated in. Is that the general mechanism suggested to filter work done at this level by language?

@davidwengier davidwengier Oct 30, 2025 •
edited
Loading

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Yes, the endpoints run in devenv, and we have no knowledge of the Razor syntax tree or anything that lets us determine what language is at a position there, so we have to jump to OOP to get that info. In this case, I would imagine the OOP call above would change to return a custom type that carries the hover info, and whether or not to call Html and merge the results.

@ToddGrun ToddGrun Oct 30, 2025

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

That's what I thought when I asked for guidance. I think the small change I made in commit 2 to the stopHandling determination in the remote process prevents most of the unnecessary html lsp requests.

razorDocument,
Methods.TextDocumentHoverName,
request,
cancellationToken).ConfigureAwait(false);

return MergeHtmlAndRazorHoverResponses(razorResponse.Result, htmlHover);
}

private LspHover? MergeHtmlAndRazorHoverResponses(LspHover? razorHover, LspHover? htmlHover)
{
if (razorHover is null)
{
return htmlHover;
}

// This logic is to prepend HTML hover content to the razor hover content if both exist.
// The razor content comes through as a ContainerElement, while the html content comes
// through as MarkupContent. We need to extract the html content and insert it at the
// start of the combined ContainerElement.
if (htmlHover != null
&& htmlHover.Range == razorHover.Range
&& razorHover is VSInternalHover razorVsInternalHover
&& razorVsInternalHover.RawContent is ContainerElement razorContainerElement)

@davidwengier davidwengier Oct 30, 2025

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This will only be true in VS. In VS Code, We'd have Content that contains markdown. Though since it seems the Html server is returning markdown too, it should be easy enough to combine.

@ToddGrun ToddGrun Oct 30, 2025

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Modified VS code path to merge html/razor results with horizontal rule between:

image

@davidwengier davidwengier Oct 30, 2025

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

What does the full markdown content of this look like? Or what did this look like before your change? That first tag helper line is quite chonky.

Looks like this for me right now:
image

Could just be a bug when we have two tag helpers (in which case, can just log a follow up issue), or perhaps VS Code's markdown is poisoning ours with an unclosed or trailing symbol. Might need to log a bug there.

@ToddGrun ToddGrun Oct 30, 2025

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

It's a chunker for me without my changes too:

image

@davidwengier davidwengier Oct 30, 2025

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Awesome

{
var htmlStringResponse = htmlHover.Contents.Match(
static s => s,
static markedString => null,
static stringOrMarkedStringArray => null,
static markupContent => markupContent.Value
);

if (htmlStringResponse is not null)
{
var htmlStringClassifiedTextElement = ClassifiedTextElement.CreatePlainText(htmlStringResponse);
var verticalSpacingTextElement = ClassifiedTextElement.CreatePlainText(string.Empty);
var htmlContainerElement = new ContainerElement(
ContainerElementStyle.Stacked,
[htmlStringClassifiedTextElement, verticalSpacingTextElement]);

// Modify the existing hover's RawContent to prepend the HTML content.
razorVsInternalHover.RawContent = new ContainerElement(razorContainerElement.Style, [htmlContainerElement, .. razorContainerElement.Elements]);
}
}

return razorHover;
}

internal TestAccessor GetTestAccessor() => new(this);
Expand Down
29 changes: 26 additions & 3 deletions ...icrosoft.VisualStudioCode.RazorExtension.Test/Endpoints/Shared/CohostHoverEndpointTest.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,12 +30,32 @@ public async Task Razor()
}
""";

await VerifyHoverAsync(code, async (hover, document) =>
// This verifies Hover calls into both csharp and HTML and aggregates their results
const string htmlDescription = "html description";

@davidwengier davidwengier Oct 30, 2025

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I think this is worth reviewing after you're made the changes to the OOP service. I would have thought we'd only query html when its likely to be a real html tag, and given all Razor components must start with a capital letter, and no html elements do, I would think that's only in .cshtml files (FileKind.Legacy). So this verification code looks right, but should be in a different test. Realistically the <body> example in your screenshot is the perfect case to validate.

@ToddGrun ToddGrun Oct 30, 2025

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Thank you for this comment. I could not figure out why my attempts to use taghelpers were not working and the FileKind.Legacy was just the trick I was missing.

var htmlResponse = new VSInternalHover();

htmlResponse.Range = new Roslyn.LanguageServer.Protocol.Range()
{
Start = new Roslyn.LanguageServer.Protocol.Position(0, 1),
End = new Roslyn.LanguageServer.Protocol.Position(0, "<PageTitle".Length),
};
htmlResponse.Contents = new MarkupContent()
{
Kind = MarkupKind.Markdown,
Value = htmlDescription,
};

await VerifyHoverAsync(code, htmlResponse, async (hover, document) =>
{
await VerifyRangeAsync(hover, code.Span, document);

hover.VerifyContents(
Container(
Container(
ClassifiedText(
Text(htmlDescription)),
ClassifiedText(
Text(string.Empty))),
Container(
Image,
ClassifiedText( // class Microsoft.AspNetCore.Components.Web.PageTitle
Expand Down Expand Up @@ -310,10 +330,13 @@ await VerifyHoverAsync(code, async (hover, document) =>
});
}

private async Task VerifyHoverAsync(TestCode input, Func<Hover, TextDocument, Task> verifyHover)
private Task VerifyHoverAsync(TestCode input, Func<Hover, TextDocument, Task> verifyHover)
=> VerifyHoverAsync(input, htmlResponse: null, verifyHover);

private async Task VerifyHoverAsync(TestCode input, Hover? htmlResponse, Func<Hover, TextDocument, Task> verifyHover)
{
var document = CreateProjectAndRazorDocument(input.Text);
var result = await GetHoverResultAsync(document, input);
var result = await GetHoverResultAsync(document, input, htmlResponse);

Assert.NotNull(result);
await verifyHover(result, document);
Expand Down
Loading