We broke some legacy apps that said "black background" but really wanted "default background" #6807
Comments
ghost
added
Needs-Triage
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.
zadjii-msft
added
Area-Output
zadjii-msft
changed the title
zadjii-msft
removed
the
Needs-Triage
|
Thanks for handling this for me. |
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.
|
🎂 Did the updated PSReadLine make it into Windows Server 2022 Preview? If not, then the support range for keeping the hack becomes 5-10 years (Whatever the LTSC rules are now) from late 2021, and we might want to consider introducing a config option for users who do have a newer PSReadLine deployed. Otherwise, the support-range would depend on the SAC expiry cycle, once a Windows SAC release gets the update. I'm assuming the feature-updates didn't get it; my Windows PowerShell reports 'PSReadLine 2.0.0' on Windows 10 20H2, but I might have locally upgraded that to 2.0.0 GA and then forgotten about it. https://github.com/PowerShell/PSReadLine still says
which predates the fix landing in 2.1.0-rc1. |
|
That was an excellent question. I did some digging -- they backported this change into the version of PSReadline in the OS (without changing the version number! tsk!) as of Windows builds 20212+ and 20182+. If the Server previews postdate those build numbers, they should not need the workaround and we won't have to support it through LTSC 😄 |
|
Windows Server 2022 Preview is build 20348, so that's great, and Windows 11 Insider Preview is build 22000, so the only continuing issue is Windows 10 21H2, which was just announced as the third servicing-update release in a row (i.e. build 19044), and which has a 5-year LTSC version scheduled, possibly the last Windows 10 LTSC version. As a passing thought, what if the hack was modified to only be enabled on Windows builds lower than the version numbers you mentioned? That doesn't seem too intrusive, and provides a nice bit of future-proofing. I'm now mildly concerned that Windows 10 will never get a new full-system update, which means this bug persists for Windows 10 users until support expiry in October 2025. Would a PR to add a config option (per-profile perhaps, so trivial to apply only to PowerShell Core for example) to override the hack be acceptable? (This is not a commitment to create such a PR, of course, I'm just thinking out loud.) Huh, and while poking around, I noticed that PowerShell 6.x fell out of support last September, and PowerShell 7.0 expires at the end of 2022, so perhaps the In the meantime, I'm weighing up whether I just locally work around this by switching from hand-installed PowerShell 7 MSI to using the .NET Global Tool version of PowerShell. I checked, and it looks right, but I'm not sure yet if it actually has any differences I need to care about. |
|
which is the requirement for reverting the shim again? |
## Summary of the Pull Request This PR moves the color table and related render settings, which are common to both conhost and Windows Terminal, into a shared class that can be accessed directly from the renderer. This avoids the overhead of having to look up these properties via the `IRenderData` interface, which relies on inefficient virtual function calls. This also introduces the concept of color aliases, which determine the position in the color table that colors like the default foreground and background are stored. This allows the option of mapping them to one of the standard 16 colors, or to have their own separate table entries. ## References This is a continuation of the color table refactoring started in #11602 and #11784. The color alias functionality is a prerequisite for supporting a default bold color as proposed in #11939. The color aliases could also be a way for us to replace the PowerShell color quirk for #6807. ## PR Checklist * [x] Closes #12002 * [x] CLA signed. * [ ] Tests added/passed * [ ] Documentation updated. * [ ] Schema updated. * [ ] I've discussed this with core contributors already. If not checked, I'm ready to accept this work might be rejected in favor of a different grand plan. Issue number where discussion took place: #xxx ## Detailed Description of the Pull Request / Additional comments In addition to the color table, this new `RenderSettings` class manages the blinking state, the code for adjusting indistinguishable colors, and various boolean properties used in the color calculations. These boolean properties are now stored in a `til::enumset` so they can all be managed through a single `SetRenderMode` API, and easily extended with additional modes that we're likely to need in the future. In Windows Terminal we have an instance of `RenderSettings` stored in the `Terminal` class, and in conhost it's stored in the `Settings` class. In both cases, a reference to this class is passed to the `Renderer` constructor, so it now has direct access to that data. The renderer can then pass that reference to the render engines where it's needed in the `UpdateDrawingBrushes` method. This means the renderer no longer needs the `IRenderData` interface to access the `GetAttributeColors`, `GetCursorColor`, or `IsScreenReversed` methods, so those have now been removed. We still need access to `GetAttributeColors` in certain accessibility code, though, so I've kept that method in the `IUIAData` interface, but the implementation just forwards to the `RenderSettings` class. The implementation of the `RenderSettings::GetAttributeColors` method is loosely based on the original `Terminal` code, only the `CalculateRgbColors` call has now been incorporated directly into the code. This let us deduplicate some bits that were previously repeated in the section for adjusting indistinguishable colors. The last steps, where we calculate the alpha components, have now been split in to a separate `GetAttributeColorsWithAlpha` method, since that's typically not needed. ## Validation Steps Performed There were quite a lot changes needed in the unit tests, but they're mostly straightforward replacements of one method call with another. In the `TextAttributeTests`, where we were previously testing the `CalculateRgbColors` method, we're now running those tests though `RenderSettings::GetAttributeColors`, which incorporates the same functionality. The only complication is when testing the `IntenseIsBright` option, that needs to be set with an additional `SetRenderMode` call where previously it was just a parameter on `CalculateRgbColors`. In the `ScreenBufferTests` and `TextBufferTests`, calls to `LookupAttributeColors` have again been replaced by the `RenderSettings::GetAttributeColors` method, which serves the same purpose, and calls to `IsScreenReversed` have been replaced with an appropriate `GetRenderMode` call. In the `VtRendererTests`, all the calls to `UpdateDrawingBrushes` now just need to be passed a reference to a `RenderSettings` instance.
|
Okay, I've cooked this up at 32296eb. My only lingering concern is that pwsh 7.0 is still in support. We can take our medicine and ask people to upgrade, but it may not be pretty. I would rather not crack the PE file to guess the version number. That seems much worse than living with black bars for all 7.0 consumers who cannot upgrade. Thoughts? |
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)
ghost
added
Resolution-Fix-Committed
|
🎉This issue was addressed in #13352, which has now been successfully released as Handy links: |
Just go read #6810 cause that has all the details.
The text was updated successfully, but these errors were encountered: