[HTML] Auto tag-closing doesn't work with undo/redo and programmatic changes

[lostintangent]

We had a user report an issue with Visual Studio Live Share, where their HTML tag-closing behavior would insert duplicate closing tags as soon as a guest connected to their session.

After looking into it briefly, it appears like the HTML tag-closing behavior is listening to the workspace.onDidChangeTextDocument event, and therefore, generating the closing tag on every participants machine, as soon as the / or > is synchronized across the session. If five guests joined the session, then you'd see six copies of the closing tag.

It isn't immediately clear what the best fix for this is, so I was hoping to get some advice. Initially, I thought we could set the html.autoClosingTags setting to false in the guest's workspace file. However, since the tag-closing behavior relies on the activeEditor, this would break tag-closing for guests if they weren't focused on the same file as the host (maybe that's a better short-term solution?).

At a high-level, it seems like there are two possible ways to address to:

1. Run the tag-closing logic on the host - Live Share already takes "exclusive" ownership of language services for vsls: documents, but the tag-closing logic runs on the guest's machine since it appears to be always initialized regardless of the document scheme. We could explore ways to run this logic on the host, however, the reliance on the activeEditor would need to be refactored.

2. Run the tag-closing logic local to each participant - Since the tag-closing logic could operate entirely on a document buffer, it could technically just run on each guest's machine, without needing file system access. However, since it relies on the workspace.onDidChangeChangeTextDocument event, it seems like it would need to identify "local" edits, and only generate closing tags as appropriate. That way, each guest doesn't create the same edit that everyone else is already making.

Any thoughts would be greatly appreciated, since the current behavior is definitely a little wonky. Thanks!

[vscodebot[bot]]

(Experimental duplicate detection) Thanks for submitting this issue. Please also check if it is already covered by an existing one, like:

• Running Visual Studio Code as guest (#36500)

[crstamps2]

It should also be noted that it moved the cursor of the other person as well.

[aeschli]

Yes, we're aware of that problem, but currently have no solution in hand. We currently rely on the document listener but have no information from where the document changes originated.

[mjbvz]

@aeschli I am going to have the same problem in js/ts once we add tag completion in July. Do you think we need a new API to better support this? Something that lets extensions reliably trigger a text insertion when a given character is typed. Quick sketch:

interface ClosingProvider {
     provideClosing(document, position, triggerCharacter, cancellation): TextEdit[];
}

function registerClosingProvider(selector, provider, ...triggerChracters: string[]);

/cc @jrieken

[aeschli]

@mjbvz Yes, that would be the cleanest approach. Basically language-server driven autoClosingPairs @alexandrudima FYI

[jrieken]

Yeah, maybe... I think today that internal editor logic is synchronous and will require some reworking.

[alexdima]

:+1: yes, new APIs would be needed to support this. I'm not sure ClosingProvider is the best name, but yeah, something along those lines.

[lostintangent]

@mjbvz Did I notice that you shipped the auto-closing support for JSX in JS/TS? If so, has there been any updates on this discussion? We temporarily disabled the HTML auto-closing support for guests in a Live Share session (disabling the setting in their generated workspace file), so we may need to do the same for JS/TS. Otherwise, the duplicated closing tag behavior is pretty bizarre 😄

[mjbvz]

Yes, no progress on this likely this iteration since it requires a new API. This can be disabled by the javascript.autoClosingTags and typescript.autoClosingTags settings

[lostintangent]

@mjbvz Sounds good. Thanks for the update! We'll disable these two settings in the meantime 👍

[angelozerr]

Yes, we're aware of that problem, but currently have no solution in hand. We currently rely on the document listener but have no information from where the document changes originated.

If HTML Language Server supports incremental TextDocumentSyncKind.Incremental it should resolve your problem, no? IMHO I think HTML Language Server should support incremental feature because when you have a big HTML file, the client sends the full HTML content which can freeze the editor each time you type character.

Incremental support is hard to implement but you could implement with a basic strategy: when https://microsoft.github.io/language-server-protocol/specification#textDocument_didChange is called you update the TextDocument content instance from TextDocuments by using the contentChanges, in other words:

1° client sends only the character which changes. You have 2 benefits:

• the client send a little HTML content insead of sending the full HTML document content (performance)
• you can retrieve the changed content easily 2° the server which stores the HTML content in the TextDocument instance update the HTML content of TextDocument instance by using the character which changes.

[aeschli]

@angelozerr Incremental document update is a good thing, but the main problem here is that we need to know where the change came from. If it's from user input (e.g. keyboard, paste from clipboard, code assist) it's ok to close the tag. But if came from a 'Undo' or programmatically from life share, we don't want to perform the auto closing of tags.

[jjoselv]

Any updates on this?

[aeschli]

There's now a fix for undo: #34484 We still don't know if a change came in from a programmatic change.

[aeschli]

@hediet I'd also need a way if a document change came in by keyboard interaction vs. a programmatic change (applying edits)

[hediet]

vs. a programmatic change (applying edits)

This is not so easy to figure out. The undo flag was already passed with the event, but the actual reason is lost.