-
Notifications
You must be signed in to change notification settings - Fork 198
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
Fixing HTML attribute commit #10897
Fixing HTML attribute commit #10897
Conversation
TestServices.Input.Send(stringToType); | ||
} | ||
|
||
await CommitCompletionAndVerifyAsync(output); |
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.
Do we need a test/some tests that type the >
as the commit character or something?
(I'm guessing this jsut commits through the API, so essentially mirrors double clicking the item with the mouse)
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.
Added a test where we explicitly type the ">" to commit. I think it's too expensive (time-wise) to do all commit chars, but we can add more if needed.
@@ -36,6 +36,10 @@ public void ApplyCapabilities(VSInternalServerCapabilities serverCapabilities, V | |||
{ | |||
ResolveProvider = true, | |||
TriggerCharacters = _completionListProvider.AggregateTriggerCharacters.ToArray(), | |||
// This is the intersection of C# and HTML commit characters. | |||
// We need to specify it so that platform can correctly calculate ApplicableToSpan |
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.
Might be more informative to link to the appropriate code for where ApplicableToSpan
is important in the editor, as it doesn't mean anything to me, and our issue linked below doesn't shed any light on it.
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.
Added a link where we compute it using the commi character set (among other things). Do we also want a link where it gets converted to a text edit later (that was deleting the ">" mentioned in the bug? Seems like a lot of links 😄 but can add it if helpful
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.
Putting here anyway - the method that actually turns applicableToSpan into a text edit is https://devdiv.visualstudio.com/DevDiv/_git/VS-Platform?path=/src/Editor/Language/Impl/Language/AsyncCompletion/AsyncCompletionSession.cs&version=GBmain&line=1207&lineEnd=1207&lineStartColumn=29&lineEndColumn=45&lineStyle=plain&_a=contents
Summary of the changes
-VS Platform (languageserver.client code) uses various characters specified by completion capabilities, including commit characters, to figure out which text span a completion item should replace when committed. It's known as "applicable-to" span. If we don't specify commit characters HTML uses in Razor commit character set in our completion endpoint capabilities, we end up eating some of those characters if they happen to be in the document text following (or preceding) item being commited.
Fixes: #10787