Skip to content
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

ConPTY does not support black background #293

Closed
Tyriar opened this issue Oct 22, 2018 · 17 comments
Closed

ConPTY does not support black background #293

Tyriar opened this issue Oct 22, 2018 · 17 comments
Labels
Area-Rendering Text rendering, emoji, complex glyph & font-fallback issues Issue-Bug It either shouldn't be doing this or needs an investigation. Issue-Task It's a feature request, but it doesn't really need a major design. Priority-1 A description (P1) Product-Conpty For console issues specifically related to conpty Resolution-Fix-Available It's available in an Insiders build or a release
Milestone

Comments

@Tyriar
Copy link
Member

Tyriar commented Oct 22, 2018

bash repro: echo -e '\x1b[40mabc'
powershell repro: Write-Host -BackgroundColor Black "Hello"

Expected (conhost/powershell.exe):

image

Actual (vscode/conpty):

image

image

@Tyriar
Copy link
Member Author

Tyriar commented Oct 22, 2018

/cc @sbatten

@miniksa miniksa added Area-Rendering Text rendering, emoji, complex glyph & font-fallback issues Product-Conpty For console issues specifically related to conpty labels Jan 18, 2019
@DHowett-MSFT
Copy link
Contributor

DHowett-MSFT commented Jan 28, 2019

Copying @zadjii-msft's response from #362:

If I had to guess, I'd think that maybe you're emitting something like 37m or 40m, and that's silently getting translated to 39m/49m (or maybe even 0m). In that case, your client app would say "I would like to write text with a foreground of dark white" (37m), but the conpty's "default foreground" color is also set to "dark white", so conpty then emits "I want default foreground text" (39m).

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" (37 on 40) set as the default.

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" (49, or 0 if no foreground was set) and "set foreground to default" (39, or 0 if no background was set.)

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 ghost added the Needs-Tag-Fix Doesn't match tag requirements label May 17, 2019
@DHowett-MSFT DHowett-MSFT added the Issue-Question For questions or discussion label May 18, 2019
@ghost ghost removed the Needs-Tag-Fix Doesn't match tag requirements label May 18, 2019
@DHowett-MSFT
Copy link
Contributor

I'm tagging this Issue-Question because we need to have a discussion around the best way to solve it. We're taking a bigger dependency on ConPTY ourselves in Windows Terminal, and we absolutely want to support the full range of colors.

The problem is, as always, backwards compatibility. Let's put together a design?

@TBBle
Copy link

TBBle commented Jun 24, 2019

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

@antoineco
Copy link
Contributor

antoineco commented Jun 26, 2019

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
reference colors

Windows Terminal (preview) rendering
win term colors

(the script)

  • the column labeled 40m has an invisible background
    Probably the most striking difference when running typical apps in the terminal because all highlights are gone (vim, tmux, status bars, ...). In the Solarized palette this color is base02, which corresponds to "black" (color 0). As already explained by @DHowett-MSFT, "black background" is always translated to the default background color, which for us is base03 a.k.a. "bright black" (color 8).

  • the row labeled 1m has white text
    This text should have the same base0 color for "foreground" (same as "bright blue" in the Solarized palette) as the row above, but making it bold automatically translates it to base3 for "bright white" (color 15), making it identical to the 1;37m row.

  • the row labeled 37m has gray text
    This text should have the base2 color for "white" (color 7), but it was translated to the base0 color for "foreground" instead, making it identical to the m row.

@hanubeki
Copy link

I figured out ESC ] 11 ; rgb:12/34/56 ESC \ can be used to separate background color and ESC ] 10 ; rgb:12/34/56 ESC \ for foreground color, but I'm not sure this is a related to this issue.

@hanubeki
Copy link

hanubeki commented Jun 27, 2019

After using ESC ] 11 ; rgb:00/2B/36 ESC \ and ESC ] 10 ; rgb:83/94/96 ESC \, My terminal just looks like this.
image

@DHowett-MSFT
Copy link
Contributor

That just changes your foreground and background colors, it doesn’t fix the underlying issue.

@TBBle
Copy link

TBBle commented Jun 27, 2019

It's close to solving the problem at least for Solarized, since a request for 37m is being translated into 39m, and 40m is being translated into 49m, and those OSC codes (10 and 11) are setting the VT100 text foreground color" and "VT100 text background color", (presumably applied by 30m and 40m) to be the colours expected by 37m and 49m? It's interesting, that these OSC codes are not listed on MS's Console Virtual Terminal Sequences listing.

However, I assume this now means most applications will get the wrong default colours, i.e. showing 40m when the screen is cleared or otherwise, instead of the desired (Solarized Dark) background, 100m.

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 ESC[37m (or compound code containing 37) into ESC]10;rgb:<themeWhite>ESC\ESC[37mESC]10;rgb:<themeFG>ESC\ (and same for ESC[40m) which seems like a pretty awful hack, but ought to work...

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'.

@erossetto
Copy link

Y'all got any more of them... updates?

@zadjii-msft
Copy link
Member

@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.

@blami
Copy link

blami commented Mar 8, 2020

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.

DHowett pushed a commit that referenced this issue Jul 1, 2020
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.
@DHowett
Copy link
Member

DHowett commented Jul 17, 2020

@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?
I'm looking to close this out -- if the remaining known issues are palatable I'll just send this one to join the choir invisible.

@j4james
Copy link
Collaborator

j4james commented Jul 17, 2020

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.

@TBBle
Copy link

TBBle commented Jul 18, 2020

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.

@TBBle
Copy link

TBBle commented Jul 24, 2020

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:

image

and to contrast with the state 12 months ago:

image

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.

@DHowett
Copy link
Member

DHowett commented Jul 24, 2020

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 DHowett closed this as completed Jul 24, 2020
@ghost ghost added the Needs-Tag-Fix Doesn't match tag requirements label Jul 24, 2020
@DHowett DHowett added the Resolution-Fix-Available It's available in an Insiders build or a release label Jul 24, 2020
@ghost ghost removed the Needs-Tag-Fix Doesn't match tag requirements label Jul 24, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area-Rendering Text rendering, emoji, complex glyph & font-fallback issues Issue-Bug It either shouldn't be doing this or needs an investigation. Issue-Task It's a feature request, but it doesn't really need a major design. Priority-1 A description (P1) Product-Conpty For console issues specifically related to conpty Resolution-Fix-Available It's available in an Insiders build or a release
Projects
None yet
Development

No branches or pull requests