ConPTY does not support black background #293
Comments
|
/cc @sbatten |
miniksa
added
Area-Rendering
|
Copying @zadjii-msft's response from #362:
Since RS4 and RS5 conhost do not support "default" colors that lie outside of the stock 0-16 palette (as explained here), ConPTY starts up with "white on black" ( This does mean that attempts to set the background to black or the foreground to white will result in ConPTY emitting "set background to default" ( In 19H1, conhost does support default background/foreground being outside of the palette, but we did not enable it by default for ConPTY consumers as it is an experimental feature with a potential impact on application compatibility. |
ghost
added
the
|
I'm tagging this The problem is, as always, backwards compatibility. Let's put together a design? |
|
Would it make sense for the theme-loader or applier to use a different foreground/background index when the given colour is in the pallette? That would restore compatibility for situations which were previously handled by updating the shortcut properties or registry key to choose a different colour, e.g., https://github.com/neilpa/cmd-colors-solarized/blob/master/Update-Link.ps1 |
|
For people landing here after wondering why highlighted backgrounds are invisible with Solarized palettes, here a summary of what does not work currently. reference Solarized colors Windows Terminal (preview) rendering
|
|
I figured out |
|
After using |
|
That just changes your foreground and background colors, it doesn’t fix the underlying issue. |
|
It's close to solving the problem at least for Solarized, since a request for However, I assume this now means most applications will get the wrong default colours, i.e. showing I'm not sure if Terminal is holding the correct pipe to mangle the stream from application to ConPTY, but if so it could work around ConPTY's behaviour here by turning any call to Since I understand the issue lies in ConPTY, any fix therein, e.g., removing this remapping behaviour, or allowing the host to specify (and update?) which colour indexes to remap into 'foreground' and 'background'. |
|
Y'all got any more of them... updates? |
|
@erossetto Not at this time, no. This bug is a pretty high priority for us, but actually requires a decent amount of work and unfortunately didn't make the cut for 1.0. Hopefully this should land soon after 1.0. When we do have updates, we'll make sure to update this thread. |
|
Wouldn't be possible to make this togglable/negotiable between client and conpty? I assume most of us are interested in this in WSL space and not in legacy cmd.exe. |
This PR reimplements the VT rendering engines to do a better job of preserving the original color types when propagating attributes over ConPTY. For the 16-color renderers it provides better support for default colors and improves the efficiency of the color narrowing conversions. It also fixes problems with the ordering of character renditions that could result in attributes being dropped. Originally the base renderer would calculate the RGB color values and legacy/extended attributes up front, passing that data on to the active engine's `UpdateDrawingBrushes` method. With this new implementation, the renderer now just passes through the original `TextAttribute` along with an `IRenderData` interface, and leaves it to the engines to extract the information they need. The GDI and DirectX engines now have to lookup the RGB colors themselves (via simple `IRenderData` calls), but have no need for the other attributes. The VT engines extract the information that they need from the `TextAttribute`, instead of having to reverse engineer it from `COLORREF`s. The process for the 256-color Xterm engine starts with a check for default colors. If both foreground and background are default, it outputs a SGR 0 reset, and clears the `_lastTextAttribute` completely to make sure any reset state is reapplied. With that out the way, the foreground and background are updated (if changed) in one of 4 ways. They can either be a default value (SGR 39 and 49), a 16-color index (using ANSI or AIX sequences), a 256-color index, or a 24-bit RGB value (both using SGR 38 and 48 sequences). Then once the colors are accounted for, there is a separate step that handles the character rendition attributes (bold, italics, underline, etc.) This step must come _after_ the color sequences, in case a SGR reset is required, which would otherwise have cleared any character rendition attributes if it came last (which is what happened in the original implementation). The process for the 16-color engines is a little different. The target client in this case (Windows telnet) is incapable of setting default colors individually, so we need to output an SGR 0 reset if _either_ color has changed to default. With that out the way, we use the `TextColor::GetLegacyIndex` method to obtain an approximate 16-color index for each color, and apply the bold attribute by brightening the foreground index (setting bit 8) if the color type permits that. However, since Windows telnet only supports the 8 basic ANSI colors, the best we can do for bright colors is to output an SGR 1 attribute to get a bright foreground. There is nothing we can do about a bright background, so after that we just have to drop the high bit from the colors. If the resulting index values have changed from what they were before, we then output ANSI 8-color SGR sequences to update them. As with the 256-color engine, there is also a final step to handle the character rendition attributes. But in this case, the only supported attributes are underline and reversed video. Since the VT engines no longer depend on the active color table and default color values, there was quite a lot of code that could now be removed. This included the `IDefaultColorProvider` interface and implementations, the `Find(Nearest)TableIndex` functions, and also the associated HLS conversion and difference calculations. VALIDATION Other than simple API parameter changes, the majority of updates required in the unit tests were to correct assumptions about the way the colors should be rendered, which were the source of the narrowing bugs this PR was trying to fix. Like passing white on black to the `UpdateDrawingBrushes` API, and expecting it to output the default `SGR 0` sequence, or passing an RGB color and expecting an indexed SGR sequence. In addition to that, I've added some VT renderer tests to make sure the rendition attributes (bold, underline, etc) are correctly retained when a default color update causes an `SGR 0` sequence to be generated (the source of bug #3076). And I've extended the VT renderer color tests (both 256-color and 16-color) to make sure we're covering all of the different color types (default, RGB, and both forms of indexed colors). I've also tried to manually verify that all of the test cases in the linked bug reports (and their associated duplicates) are now fixed when this PR is applied. Closes #2661 Closes #3076 Closes #3717 Closes #5384 Closes #5864 This is only a partial fix for #293, but I suspect the remaining cases are unfixable.
|
@j4james you mentioned that your set of color-strengthening PRs fixed most cases of this, and the ones that they didn't fix that they may not be able to. What are the remaining cases, if you know? |
|
The remaining issues (that I'm aware of) are the ones where PowerShell is using the console APIs to set its colors, so it can't differentiate between black and default. I don't think there's anything we can do about that. I think there might also have been some linked Solarized issues that were just Solarized doing its thing, but anything that was a legitimate bug should be fixed now. Bottom line is I think it's OK to close this. |
|
Is there a schedule for the next Preview release? I'd love to see the improvements for myself, and see what's left in the "Solarized doing its thing" bucket. |
|
Apart from PowerShell (will be fixed by #6807 pending an upstream fix) it all seems to be working quite nicely now. That said, I have a correctly-setup Solarized environment, including overriding the as-of-2-days-ago-default Solarized Dark theme with one that matches the spec (i.e. I've overridden the change in #6985). Per Terminal Preview 1.2.2022.0:
and to contrast with the state 12 months ago:
Screenshots taken with either MSYS2 or WSL2 Ubuntu. It might have been one of each... they both give the same results anyway. So yeah, looks good, let's mark this one done-and-dusted. Huge thank you to @j4james for driving the colour system work (AFAIK), and @DHowett for (amongst other things) telling me in another issue where I was going wrong with my initial testing. |
|
And on that note: thanks, everyone, for playing! This has been fixed and should be out in 1. the Terminal previews and 2. the next full-scale Windows update, so VSCode can take advantage of it. |
DHowett
added
the
Resolution-Fix-Available
bash repro:
echo -e '\x1b[40mabc'powershell repro:
Write-Host -BackgroundColor Black "Hello"Expected (conhost/powershell.exe):
Actual (vscode/conpty):
The text was updated successfully, but these errors were encountered: