Reintroduce a color compatibility hack, but only for PowerShells #6810
Conversation
There is going to be a very long tail of applications that will explicitly request VT SGR 40 when what they really want is to SetConsoleTextAttribute() with a black background. 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 hack. 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 color will be destroyed. This is not intended to be a long-term solution. This comment will be discovered in forty years(*) time and people will laugh at my hubris. Removal, or final remediation, will be tracked by #6807. *it doesn't matter when you're reading this, it will always be 40 years from now.
|
/cc @j4james, as I can't request a review from him right now. With apologies. |
|
Obviously I hate this, but I understand the reasons for needing it. I'd have been happy to block #6506 for the same reasons. But definitely only as a temporary solution. The mention of "defer the hard work until later -- if ever" sounds scary to me. Because with this solution we're just trading one set of bug reports for another. And it's the apps that are doing the right thing that are going to get buggy results, while the buggy apps get what they want. That just doesn't seem right. So +1 on doing whatever we have to do to keep PowerShell happy for now. But I'd definitely want us to make this a PowerShell-only mode ASAP. I don't think it really needs to be anything fancy - just exactly what you're doing now, but tied to a flag that is only set for PowerShell. Possibly a profile option that's default true for the PowerShell profiles, so that way users can still turn it off if PowerShell ever gets fixed. |
|
So, my motivation behind saying “if ever” is less because we might keep it forever and more because I’m hoping to get PowerShell (Core) serviced with the fix. If we fail to identify any other applications that need a workaround like this, we can move it into a shim that only applies to PowerShell Inbox or simply throw up our hands and say “no, we aren’t going to make a concession for this one.” If we never have to do the shim work because all our reproducers disappear, that’s better than having to carry around a better version of the fix forever. |
|
And with regards to blocking the original PR, I’m really glad we didn’t. I wanted this improvement, and its already paying dividends in how easily you could implement overline and fix DECSCNM. We would have had merge conflicts for months while we shook it all out. At least this way we have the architectural fix and a place to document our known failings and foibles. I much prefer that over a beautiful architecture that never lands. 😄 |
There was a problem hiding this comment.
I'm mildly concerned that by adding this hack for apps that are buggy, we'll have a harder time of identifying the misbehaving apps. We've obviously already identified powershell, but with this hack, the bugged apps will remain bugged, without an easy way to identify them.
I don't have a solution now for this problem, but it's something to consider. Maybe more coffee will help.
|
To throw another spanner in the works, I've just realised that this patch actually makes things worse with light color schemes. For example, here's the before and after of
|
|
Alright, wow, ouch. The hack pretty much has to move into host (it can’t live in server because server doesn’t know the state of the global “legacy defaults” and it can’t live in the renderer because I absolutely don’t want to break 37 for anyone but powershell). @zadjii-msft the only real way we truly ferret out all the broken applications is to ship with the visual regression, but I have so far failed to convince @cinnamon-msft that it’s worth our reputation to do so. |
|
I know this isn't an ideal situation. Considering majority of people use PowerShell in Terminal, I'd be okay with having the hack only for PowerShell and the other broken applications will surface eventually. |
|
Have you considered maybe hacking this on the Terminal side as just a simple color scheme patch? For example, if the command line is powershell.exe or pwsh.exe, then automatically patch the initial scheme so black is the same color as background, and white is the same color as foreground. Doesn't require any run-time overhead, and people could still override the palette with an escape sequence if they really wanted to. |
|
I'm not sure that's feasible, for a couple reasons.
|
|
|
|
And a money shot for acrylic:
|
DHowett
changed the title
|
The tests will fail to compile. Don't be daft, Dustin, fix them! |
|
I'll have you all know that i hate this so much. 😄 |
|
Okay, this check needs to be smarter about |
|
I've just been trying out this branch, and my initial impression with I don't know if this has got anything to do with it, but when starting a conhost session in the debugger with I don't have time to dig more tonight, but just wanted to let you know that there may be a problem somewhere. |
|
Huh, that's actually really interesting. Thanks for testing! |
|
So, wow, okay. When PowerShell's been installed as a dotnet global tool it's spawned via |
DHowett
changed the title
|
For the record I'm "happy" with this. I just want to note that black-on-black and white-on-white are no longer invisible if your defaults aren't white-on-black, which might trigger some bug reports. But I still think it's an improvement on the current situation, and hopefully this quirk won't have to last forever. |
DHowett
changed the title
DHowett
deleted the
dev/duhowett/i_am_sad_about_this_but_we_are_at_an_impasse
branch
|
🎉 Handy links: |
ghost
mentioned this pull request
|
Thanks for all the work on this issue. Am I correct in my understanding that:
|
|
@jantari very close. Since the workaround is happening in conhost, which receives a "console connection packet" for every commandline application that starts up, it's not doing any process tree inspection. But, yes, on all your other points:
|
|
(@jantari: why do you ask? I love talking about this stuff, so am happy to dig through a bunch of questions about it :)) |
|
@DHowett sorry for not getting back to you, the reason is I've had to read up on and utilize the console APIs in some of my own projects (like this or here) and am generally interested in applications and platform architecture and the odd ins and outs of how something really works and comes together, it's what I enjoy most at my job when I get to do it 😊 Good to know I'm not the only one! |
|
@DHowett so the issue stands that in order to fix we would need to push a fix to the included Windows PowerShell 5.1 or PSReadLine included in Windows 10? |
This is fixed in PSReadline in Insider builds of Windows. Problem is, though, that this is a bug with a very limited impact and an easy workaround... so there’s no reason to service it to earlier versions. 🤷🏻 |
In #6810, we introduced a "quirk" for all known versions of PowerShell that suppressed their requests for black background/gray foreground. This was done to avoid an [issue in PSReadline] where it would paint black bars all over the screen if the default background color wasn't the same as the ANSI black color. Years have passed since that quirk was introduced. The underlying bug was fixed, and the fix was released broadly long ago. It's time for us to remove the quirk... almost. Terminal still runs on versions of Windows that ship a broken version of PSReadline. We must maintain the quirk there -- the user can't do anything about it, and we would make their experience worse if we removed the quirk entirely. PowerShell 7.0 also ships a broken version of PSReadline. It is still in support for another 6 months, but updates have been available for some time. We can encourage users to update. Therefore, we only need the quirk for Windows PowerShell, and then only for specific versions of Windows. _Inside Windows_, we don't even need that: we're guaranteed to be built alongside a fixed version of PowerShell! Closes #6807 [issue in PSReadline]: PowerShell/PSReadLine#830 (comment)
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.
PR Checklist
Validation
I've 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.
One the left, Terminal Preview without this fix. On the right, Terminal
Dev with this fix.
DEFAULT=39or49WHITE=37or47BLACK=30or40AIX WHT=97or107AIX BLK=90or100INT=1The 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.