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

Black background in PowerShell commands #6767

Closed
krzysdz opened this issue Jul 3, 2020 · 3 comments · Fixed by #6810
Closed

Black background in PowerShell commands #6767

krzysdz opened this issue Jul 3, 2020 · 3 comments · Fixed by #6810
Labels
Needs-Tag-Fix Doesn't match tag requirements Resolution-External For issues that are outside this codebase Resolution-Fix-Committed Fix is checked in, but it might be 3-4 weeks until a release.

Comments

@krzysdz
Copy link
Contributor

krzysdz commented Jul 3, 2020

Since commit ddbe370 (#6506) a black background appears after space in commands while using PowerShell.

Environment

Windows build number: 10.0.19041.329
Windows Terminal versions: all since commit ddbe370

Steps to reproduce

  1. Open PowerShell (tested in 5.1 and 7.0.2)
  2. Do either of these:
    • press space,
    • write anything and press space,
    • use the arrow up to display a command from history that contains a space character.

Expected behavior

The background colour does not change and the acrylic effect is visible.
terminal_PS_older

Actual behavior

A weird black background appears after the space character.
terminal_PS_black_bg

@ghost ghost added Needs-Triage It's a new issue that the core contributor team needs to triage at the next triage meeting Needs-Tag-Fix Doesn't match tag requirements labels Jul 3, 2020
@DHowett
Copy link
Member

DHowett commented Jul 3, 2020

/dup PowerShell/PSReadLine#830

There is going to be a very long tail of issues like this that stem from Terminal being more correct. There's a pull request out to PSReadline that fixes this, and it will take time for it to get reviewed and released.

@ghost
Copy link

ghost commented Jul 3, 2020

Hi! We've identified this issue as a duplicate of one that exists on somebody else's Issue Tracker. Please make sure you subscribe to the referenced external issue for future updates. Thanks for your report!

@ghost ghost closed this as completed Jul 3, 2020
@ghost ghost added Resolution-External For issues that are outside this codebase and removed Needs-Triage It's a new issue that the core contributor team needs to triage at the next triage meeting labels Jul 3, 2020
@ghost ghost added In-PR This issue has a related PR and removed In-PR This issue has a related PR labels Jul 7, 2020
DHowett added a commit that referenced this issue Jul 10, 2020
There is going to be a very long tail of applications that will
explicitly request VT SGR 40/37 when what they really want is to
SetConsoleTextAttribute() with a black background/white foreground.
Instead of making those applications look bad (and therefore making us
look bad, because we're releasing this as an update to something that
"looks good" already), we're introducing this compatibility quirk.
Before the color reckoning in #6698 + #6506, *every* color was subject
to being spontaneously and erroneously turned into the default color.
Now, only the 16-color palette value that matches the active console
background/foreground color will be destroyed, and only when received
from specific applications.

Removal will be tracked by #6807.

Michael and I discussed what layer this quirk really belonged in. I
originally believed it would be sufficient to detect a background color
that matched the legacy default background, but @j4james provided an
example of where that wouldn't work out (powershell setting the
foreground color to white/gray). In addition, it was too heavyhanded: it
re-broke black backgrounds for every application.

Michael thought that it should live in the server, as a small VT parser
that righted the wrongs coming directly out of the application. On
further investigation, however, I realized that we'd need to push more
information up into the server (so that it could make the decision about
which VT was wrong and which was right) than should be strictly
necessary.

The host knows which colors are right and wrong, and it gets final say
in what ends up in the buffer.

Because of that, I chose to push the quirk state down through
WriteConsole to DoWriteConsole and toggle state on the
SCREEN_INFORMATION that indicates whether the colors coming out of the
application are to be distrusted. This quirk _only applies to pwsh.exe
and powershell.exe._

NOTE: This doesn't work for PowerShell the .NET Global tool, because it
is run as an assembly through dotnet.exe. I have no opinion on how to
fix this, or whether it is worth fixing.

VALIDATION
----------
I configured my terminals to have an incredibly garish color scheme to
show exactly what's going to happen as a result of this. The _default
terminal background_ is purple or red, and the foreground green. I've
printed out a heap of test colors to see how black interacts with them.

Pull request #6810 contains the images generated from this test.

The only color lines that change are the ones where black as a
background or white as a foreground is selected out of the 16-color
palette explicitly. Reverse video still works fine (because black is in
the foreground!), and it's even possible to represent "black on default"
and reverse it into "default on black", despite the black in question
having been `40`.

Fixes #6767.
@ghost ghost added the Resolution-Fix-Committed Fix is checked in, but it might be 3-4 weeks until a release. label Jul 10, 2020
@ghost
Copy link

ghost commented Jul 22, 2020

🎉This issue was addressed in #6810, which has now been successfully released as Windows Terminal Preview v1.2.2022.0.:tada:

Handy links:

This issue was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Needs-Tag-Fix Doesn't match tag requirements Resolution-External For issues that are outside this codebase Resolution-Fix-Committed Fix is checked in, but it might be 3-4 weeks until a release.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants