Escape sequences behave strangely over conpty when VirtualTerminalLevel is set #1965
Comments
ghost
added
Needs-Triage
|
No, this is actually great! We've had a number of reports come through that some vt things "act weird", when it's just because they're printing escape sequences with Bit more info in this thread: #1616 (comment) I had no idea that I've always thought that if we got a raw |
|
FWIW, just to note, it is incorrect that Windows Terminal (or any conpty consumer) have a "second chance" to process the vt sequence. Since conhost is printing out what it thinks is in the screen buffer, double-processing some escape sequences can result in the cursor misaligning, vt sequences wrapping and being split into multiple lines, etc. |
|
We took a fix in 19H1 to make conpty not load user settings, because that could impact how pseudoconsole hosts work in weird and difficult to debug ways. It looks like in doing so, we dropped Thank you. |
|
I guess this is technically a duplicate then. I did wonder if it might have been related to some of the other weird behaviour bugs, but nobody else seemed to be mentioning |
DHowett-MSFT
added
Area-Server
|
We may want to just consider enabling VT processing by default for all conpty hosts. |
|
Okay, so looks like we have options here:
I'm sure I'm missing scenarios here, but I'm throwing this out for feedback. I don't think any of these changes are particularly complicated code changes, but there's not a clear solution to me. |
|
2+3 is the most correct, honestly. There's an entire range of "printable" control pictures, which provides a bunch of sweet iconography like ␛ and ␃. We could do 2 today and punt 3 for windows 2xHy, honestly. |
|
I mean, technically 1+3 is the most correct, but 2+3 is the most forward-looking... |
|
If we did PR #1964, would that not address option 3 as well as the third con in option 1, at least for the DOS code pages? As for enabling vt processing by default, I'd be in favour of doing that for conhost itself, which I assume would then make it the default for conpty as well if we went with option 1. If anyone doesn't want vt processing for some reason, they still can choose to disable it via the registry, and have that work consistently for both conhost and conpty. That said, I don't know what the downsides are. I would have thought there were more legacy applications that needed some form of ANSI support (thus the popularity of alternatives like ANSICON). So I'm curious to know what kind of legacy applications would be broken by having the vt processing enabled. |
|
It's been admittedly like 4 years since the issue came up originally, but when we first introduced VT support, we got some sort of major internal complaint that it was on by default, and it didn't break a tool, but it made is way slower. (since then, we did some optimization to the parser to make that not so much of a problem.) I honestly don't remember all the details, but the decision to change it to off by default was made, much to our dismay. I don't really want to try and fight that battle again. However, I think that conpty gives us a fair opportunity to say that this is the new world, and as much as we'd love to provide 100% backwards compatibility with it, this is also the new way of doing things. There are other minor differences that running in a conpty session introduces, so I don't think this is terribly bad. In conclusion, let's do 2+3. #1964 does something like what I was thinking for 3. I was picturing that we do it on the way out of conpty, in the vt renderer, but this seems reasonable. I'd want to compare how that behaves (esp. in regards to API buffer queries) compared to the v1 console, but that seems reasonable. I'll use this issue to do "Conpty activates VT mode by default". Apps can still request VT off when they're in conpty mode, and if they write an ESC then, it'll still be rendered to the terminal correctly. There will be behavioral differences between conhost and conpty, but if something doesn't work in the terminal, it'll be unchanged for the legacy console, so use that. We sidestep the @miniksa can @ me if this seems unreasonable or has any other arguments. |
This change enables VT processing by default for _all_ conpty clients. See #1965 for a discussion on why we believe this is a righteous change. Also mentioned in the issue was the idea of only checking the `VirtualTerminalLevel` reg key in the conpty startup. I don't think this would be a more difficult change, looks like all we'd need is a simple `reg.LoadGlobalsFromRegistry();` call instead of this change. # Validation Steps Performed Manually launched a scratch app in both the terminal and the console. The console launch's output mode was 0x3, and the terminal's was 0x7. 0x4 is the ` ENABLE_VIRTUAL_TERMINAL_PROCESSING` flag, which the client now had by default in the Terminal. Closes #1965
ghost
added
Needs-Tag-Fix
|
🎉This issue was addressed in #2824, which has now been successfully released as Handy links: |
This change enables VT processing by default for _all_ conpty clients. See #1965 for a discussion on why we believe this is a righteous change. Also mentioned in the issue was the idea of only checking the `VirtualTerminalLevel` reg key in the conpty startup. I don't think this would be a more difficult change, looks like all we'd need is a simple `reg.LoadGlobalsFromRegistry();` call instead of this change. # Validation Steps Performed Manually launched a scratch app in both the terminal and the console. The console launch's output mode was 0x3, and the terminal's was 0x7. 0x4 is the ` ENABLE_VIRTUAL_TERMINAL_PROCESSING` flag, which the client now had by default in the Terminal. Closes #1965 (cherry picked from commit 1c412d4)
- Remember last-used string in the Find dialog in conhost (GH-2845) (cherry picked from commit bfb1484) - Bugfix: TextBuffer Line Wrapping from VTRenderer (GH-2797) Change VT renderer to do erase line instead of a ton of erase chars (cherry picked from commit 8afc5b2) - Add some retry support to Renderer::PaintFrame (GH-2830) If _PaintFrameForEngine returns E_PENDING, we'll give it another two tries to get itself straight. If it continues to fail, we'll take down the application. We observed that the DX renderer was failing to present the swap chain and failfast'ing when it did so; however, there are some errors from which DXGI guidance suggests we try to recover. We'll now return E_PENDING (and destroy our device resources) when we hit those errors. Fixes GH-2265. (cherry picked from commit 277acc3) - Enable VT processing by default for ConPTY (GH-2824) This change enables VT processing by default for _all_ conpty clients. See GH-1965 for a discussion on why we believe this is a righteous change. Also mentioned in the issue was the idea of only checking the `VirtualTerminalLevel` reg key in the conpty startup. I don't think this would be a more difficult change, looks like all we'd need is a simple `reg.LoadGlobalsFromRegistry();` call instead of this change. **Validation Steps Performed** Manually launched a scratch app in both the terminal and the console. The console launch's output mode was 0x3, and the terminal's was 0x7. 0x4 is the ` ENABLE_VIRTUAL_TERMINAL_PROCESSING` flag, which the client now had by default in the Terminal. Closes GH-1965 (cherry picked from commit 1c412d4) - Remove unwanted DECSTBM clipping (GH-2764) The `DECSTBM` margins are meant to define the range of lines within which certain vertical scrolling operations take place. However, we were applying these margin restrictions in the `ScrollRegion` function, which is also used in a number of places that shouldn't be affected by `DECSTBM`. This includes the `ICH` and `DCH` escape sequences (which are only affected by the horizontal margins, which we don't yet support), the `ScrollConsoleScreenBuffer` API (which is public Console API, not meant to be affected by the VT terminal emulation), and the `CSI 3 J` erase scrollback extension (which isn't really scrolling as such, but uses the `ScrollRegion` function to manipulate the scrollback buffer). This commit moves the margin clipping out of the `ScrollRegion` function, so it can be applied exclusively in the places that need it. While testing, I also noticed that one of the `ScrollRegion` calls in the `AdjustCursorPosition` function was not setting the horizontal range correctly - the scrolling should always affect the full buffer width rather than just the viewport width - so ... Related work items: #23507749, #23563809, #23563819, #23563837, #23563841, #23563844
Environment
Steps to reproduce
Set the
HKCU\Console\VirtualTerminalLevelregistry entry to 1, to enable support for VT escape sequences by default.Makes sure the
initialColsoption in the Windows Terminal profile is set to 120.Open a cmd shell in the Windows Terminal (this can't be a WSL bash shell - it must be something that would usually not have VT support enabled without the registry setting).
Run the following Python script:
Expected behavior
I'd expect this to display 110 X's followed by the word Hello, in blue on white, on the same line. This is what it looks like in a regular console cmd shell.
Actual behavior
What actually happens in the Windows Terminal is the Hello is displayed on the following line.
I don't have a good understanding of how the conpty stuff works, but my theory is that the escape sequences aren't actually being processed initially, and are just written out to the screen as literal characters. As a result the Hello is forced to wrap onto the next line, because there isn't enough space remaining. This screen structure is then passed through conpty somehow, at which point the escape sequences are processed and the text colors changed. But by that stage the wrapping has already taken effect, so the text is in the wrong place.
It's easier to see what's going on if you apply PR #1964 first. In that case, the ESC character gets translated to a ← when it isn't initially interpreted as an escape sequence. The other side of the conpty pipe then doesn't get a second chance to process those values as an escape sequence, so you just see the raw characters being written out.
This may be a good reason to reject PR #1964 for now, since it's clearly making the situation worse, but I think the real issue is in the conpty code, and if that were fixed then the PR might well be safe to merge.
The text was updated successfully, but these errors were encountered: