Custom default colors are problematic for legacy apps #8205

@DHowett commented on GitHub (May 18, 2020):

Man, I hate this issue. We wrote about it in the blog post that announced the "experimental terminal features" tab (which was intended to let users make conhost more like a terminal emulator), and... actually surprisingly, nobody complained about it. I wonder if that's because nobody used it?

This is ugly, and we did have those heuristics to determine whether the application was simply setting what it thought were the defaults.

Incidentally, the treatment of "matches default colors" as "wants default" is the root cause of #293.

@DHowett commented on GitHub (May 18, 2020): Man, I hate this issue. We wrote about it in the blog post that announced the "experimental terminal features" tab (which was intended to let users make conhost more like a terminal emulator), and... actually surprisingly, nobody complained about it. I wonder if that's because nobody used it? This is ugly, and we did have those heuristics to determine whether the application was simply setting what it thought were the defaults. Incidentally, the treatment of "matches default colors" as "wants default" is the root cause of #293.

claunia commented

@DHowett commented on GitHub (May 19, 2020):

I did, at one point, try to make life horrible for applications.

d34218217c

(hax branch - no warranty - no disclaimers)

There was one unused legacy attribute flag on the API surface. I got to wondering, "What if?"

@DHowett commented on GitHub (May 19, 2020): I did, at one point, try to make life _horrible_ for applications. https://github.com/microsoft/terminal/commit/d34218217c2f253b5022c2823c62176f21d0fe7b (hax branch - no warranty - no disclaimers) There was one unused legacy attribute flag on the API surface. I got to wondering, "What if?"

claunia commented

@zadjii-msft commented on GitHub (May 19, 2020):

On a scale of 1-horrible, how bad was that?

I was considering something similar where we use that bit as "the fg and/or bg is default colored, not indexed", and then if you queried the current attributes and the FG (or BG) byte matched the FG (or BG) byte of _wFillAttribute, then the app could infer "oh, it's not index 0x7, it's default colored.". Then, at least apps could roundtrip the default attributes. They'd ask "what are the current attrs" and we'd give them (HAX | wFillAttrs) as "use the default attrs", and then setting the attributes to HAX | wFillAtrs _wouldn't change anything.

Though, if an app just wants to set 0x07 (not HAX | 0x07), then we'd still clear the default-ness of the current attributes. It's impossible to know who's roundtripping attributes vs just blindly setting them like this. So it's not a magic bullet 😕

@zadjii-msft commented on GitHub (May 19, 2020): On a scale of 1-horrible, how bad was that? I was considering something similar where we use that bit as "the fg and/or bg is _default_ colored, not indexed", and then if you queried the current attributes and the FG (or BG) byte matched the FG (or BG) byte of `_wFillAttribute`, then the app could infer "oh, it's not _index_ 0x7, it's _default_ colored.". Then, at least apps could roundtrip the default attributes. They'd ask "what are the current attrs" and we'd give them (`HAX | wFillAttrs`) as "use the default attrs", and then setting the attributes to `HAX | wFillAtrs` _wouldn't change anything. Though, if an app just wants to set `0x07` (not `HAX | 0x07`), then we'd still clear the default-ness of the current attributes. It's impossible to know who's roundtripping attributes vs just blindly setting them like this. So it's not a magic bullet 😕

claunia commented

@DHowett commented on GitHub (May 19, 2020):

It was a little horrible. PowerShell doesn't exactly round-trip them, for example: it recomposes them out of what it thinks they meant. :|

@DHowett commented on GitHub (May 19, 2020): It was a little horrible. PowerShell doesn't exactly round-trip them, for example: it _recomposes them_ out of what it thinks they meant. :|

claunia commented

@j4james commented on GitHub (May 19, 2020):

My idea was somewhat simpler than that. I was just going to suggest that a legacy background index of 0 maps to the default background, and a legacy foreground index of 7 maps to the default foreground (where 0 and 7 are whatever the legacy Screen Background and Screen Text values are set to).

Legacy apps would still just see their usual range of 16 colors, but when they use the "default" colors (typically white on black), those colors would be stored as ColorType::IsDefault and would respond to default palette updates.

I think this is actual crucial for PowerShell, which seems to mix the VT default background and legacy black indiscriminately. If we don't do something to address that, it's going to look a mess once #2661 is fixed. This is how it currently appears in my 2661 branch:

It's possible that's a result of something I've done wrong, but I'm fairly certain it's just this black/default mismatch that's the problem.

@j4james commented on GitHub (May 19, 2020): My idea was somewhat simpler than that. I was just going to suggest that a legacy background index of 0 maps to the default background, and a legacy foreground index of 7 maps to the default foreground (where 0 and 7 are whatever the legacy _Screen Background_ and _Screen Text_ values are set to). Legacy apps would still just see their usual range of 16 colors, but when they use the "default" colors (typically white on black), those colors would be stored as `ColorType::IsDefault` and would respond to default palette updates. I think this is actual crucial for PowerShell, which seems to mix the VT default background and legacy black indiscriminately. If we don't do something to address that, it's going to look a mess once #2661 is fixed. This is how it currently appears in my 2661 branch: ![image](https://user-images.githubusercontent.com/4181424/82379250-7e7c6780-9a1e-11ea-9551-4986946c379b.png) It's possible that's a result of something I've done wrong, but I'm fairly certain it's just this black/default mismatch that's the problem.

claunia commented

@j4james commented on GitHub (May 23, 2020):

OK, so I knocked together a POC of the solution I was proposing, and it's definitely an improvement, but there are still a few weird color artifacts in PowerShell. Here's what it looks like in my current branch (which includes the conpty narrowing fix).

Note how the nothing parameter, and most of the Write-Host line have the background color in black rather than the default color. That's because PS seems to query the default colors using legacy APIs (getting back 0x07), but then uses VT sequences to select those colors. Using a legacy API to set the color would have automatically mapped it back to the default background, but SGR 40 gets you a real black.
Also note the Hello produced by Write-Host has the background set to blue rather than black. In this case it is actually using a legacy API to set the color, and with my fix in place, the 0 color gets mapped back to the default background. This is also the reason why the error message background is blue, where the legacy PowerShell would have had it in black (although that seems an improvement to me).

On the plus side, though, there are definitely things that are improved by this patch. You can see the difference it has made by comparing it to a PowerShell session in conhost, with the Use Separate Background option set to a shade of blue, but with the Screen Background and Screen Text still set to the default black and white (which is more or less what you're getting in a conpty session).

The Hello has the right background color, as does the error message (assuming that's what was intended), but the nothing parameter is still wrong, and the prompt suddenly switches to the wrong color after the error message is output.

The reason for the latter issue is because most PowerShell commands seem to use legacy APIs to set their colors. So restoring the default attributes typically means setting the colors to white on black. And without my fix in place, that won't translate back to the real default colors.

Bottom line: I think what I'm proposing is a reasonable solution. The results in PowerShell aren't perfect, but that is really PowerShell's problem. A pure legacy app would probably fare much better than their current legacy-vt hybrid approach. That said, maybe these are issues that have already been fixed - I haven't tried testing with the latest PS version.

@j4james commented on GitHub (May 23, 2020): OK, so I knocked together a POC of the solution I was proposing, and it's definitely an improvement, but there are still a few weird color artifacts in PowerShell. Here's what it looks like in my current branch (which includes the conpty narrowing fix). ![image](https://user-images.githubusercontent.com/4181424/82738882-f3a7af80-9d32-11ea-8b80-c07a25307f9a.png) 1. Note how the `nothing` parameter, and most of the `Write-Host` line have the background color in black rather than the default color. That's because PS seems to query the default colors using legacy APIs (getting back 0x07), but then uses VT sequences to select those colors. Using a legacy API to set the color would have automatically mapped it back to the default background, but `SGR 40` gets you a real black. 2. Also note the `Hello` produced by `Write-Host` has the background set to blue rather than black. In this case it is actually using a legacy API to set the color, and with my fix in place, the 0 color gets mapped back to the default background. This is also the reason why the error message background is blue, where the legacy PowerShell would have had it in black (although that seems an improvement to me). On the plus side, though, there are definitely things that are improved by this patch. You can see the difference it has made by comparing it to a PowerShell session in conhost, with the _Use Separate Background_ option set to a shade of blue, but with the _Screen Background_ and _Screen Text_ still set to the default black and white (which is more or less what you're getting in a conpty session). ![image](https://user-images.githubusercontent.com/4181424/82738890-015d3500-9d33-11ea-9bb5-220fc2938442.png) The `Hello` has the right background color, as does the error message (assuming that's what was intended), but the `nothing` parameter is still wrong, and the prompt suddenly switches to the wrong color after the error message is output. The reason for the latter issue is because most PowerShell commands seem to use legacy APIs to set their colors. So restoring the default attributes typically means setting the colors to white on black. And without my fix in place, that won't translate back to the real default colors. Bottom line: I _think_ what I'm proposing is a reasonable solution. The results in PowerShell aren't perfect, but that is really PowerShell's problem. A pure legacy app would probably fare much better than their current legacy-vt hybrid approach. That said, maybe these are issues that have already been fixed - I haven't tried testing with the latest PS version.

claunia commented

@DHowett commented on GitHub (Jun 15, 2020):

I'm comfortable with us piloting the workaround that 0/7 are "default" when they come through the legacy API, yeah. It seems like we could safely(?) extend that to "active buffer defaults = default", but I'm not sure the risk there.

Unfortunately, PS6 and PS7 are not significantly better in this regard and we may need to gate #6506 on a fix for this. :/

@DHowett commented on GitHub (Jun 15, 2020): I'm comfortable with us piloting the workaround that 0/7 are "default" when they come through the legacy API, yeah. It seems like we could safely(?) extend that to "active buffer defaults = default", but I'm not sure the risk there. Unfortunately, PS6 and PS7 are not significantly better in this regard and we may need to gate #6506 on a fix for this. :/

claunia commented

@j4james commented on GitHub (Jun 16, 2020):

I'm comfortable with us piloting the workaround that 0/7 are "default" when they come through the legacy API, yeah. It seems like we could safely(?) extend that to "active buffer defaults = default", but I'm not sure the risk there.

I'd love to be able to ignore the active buffer default (assuming we're talking about _wFillAttribute), because that's a pain to access in certain parts of the code. But that will be a breaking change in conhost for anyone that sets the Screen Text and Screen Background properties the way Powershell used to do (I'm not sure if that's still the case).

Unfortunately, PS6 and PS7 are not significantly better in this regard and we may need to gate #6506 on a fix for this. :/

I figured that might be necessary.

@j4james commented on GitHub (Jun 16, 2020): > I'm comfortable with us piloting the workaround that 0/7 are "default" when they come through the legacy API, yeah. It seems like we could safely(?) extend that to "active buffer defaults = default", but I'm not sure the risk there. I'd love to be able to ignore the active buffer default (assuming we're talking about `_wFillAttribute`), because that's a pain to access in certain parts of the code. But that will be a breaking change in conhost for anyone that sets the _Screen Text_ and _Screen Background_ properties the way Powershell used to do (I'm not sure if that's still the case). > Unfortunately, PS6 and PS7 are not significantly better in this regard and we may need to gate #6506 on a fix for this. :/ I figured that might be necessary.

claunia commented

@DHowett commented on GitHub (Jun 16, 2020):

That old powershell shortcut is, unfortunately, checked into the Windows source tree and isn't going anywhere. ☹️

Now, if people enable the weird woo-woo experimental settings on that LNK file and stuff goes wrong, I legit don't care. Terminal and other PTY hosts are unimpacted.

I'm more concerned about applications that set the default attributes programmatically and then try to print in those attributes. This could be an as-yet-unexploded minefield

@DHowett commented on GitHub (Jun 16, 2020): That old powershell shortcut is, unfortunately, checked into the Windows source tree and isn't going anywhere. ☹️ Now, if people enable the weird woo-woo experimental settings _on that LNK file_ and stuff goes wrong, I legit don't care. Terminal and other PTY hosts are unimpacted. I'm more concerned about applications that set the default attributes programmatically and then try to print in those attributes. This could be an as-yet-unexploded minefield

claunia commented

@j4james commented on GitHub (Jun 16, 2020):

I'm more concerned about applications that set the default attributes programmatically and then try to print in those attributes. This could be an as-yet-unexploded minefield

I don't think that's possible, at least as far I could tell. Those attributes are initialized at startup from the CONSOLE_API_CONNECTINFO, the link settings, and the registry. And at runtime they can be manually changed via the Properties menu. But I couldn't see any console API that would enable an app to set them programatically. I might have missed something though.

@j4james commented on GitHub (Jun 16, 2020): > I'm more concerned about applications that set the default attributes programmatically and then try to print in those attributes. This could be an as-yet-unexploded minefield I don't think that's possible, at least as far I could tell. Those attributes are initialized at startup from the `CONSOLE_API_CONNECTINFO`, the link settings, and the registry. And at runtime they can be manually changed via the _Properties_ menu. But I couldn't see any console API that would enable an app to set them programatically. I might have missed something though.

claunia commented

@DHowett commented on GitHub (Jun 16, 2020):

So, hmm. I was going to remark on the split between SCREEN_INFORMATION::SetAttributes and SCREEN_INFORMATION::SetDefaultAttributes, but it looks like SetDefaultAttributes just calls SetAttributes and then triggers a redraw. It does look like the API SetConsoleScreenBufferInfoEx lets you change the "default attributes", but as far as I can tell those are actually just the active attributes?

I stand corrected! There's no API that sets the "default" attributes -- we just have really well and truly mixed up what the word "default" means in SCREEN_INFORMATION. 😄

@DHowett commented on GitHub (Jun 16, 2020): So, hmm. I was going to remark on the split between `SCREEN_INFORMATION::SetAttributes` and `SCREEN_INFORMATION::SetDefaultAttributes`, but it looks like `SetDefaultAttributes` just calls `SetAttributes` and then triggers a redraw. It does look like the API `SetConsoleScreenBufferInfoEx` lets you change the "default attributes", but as far as I can tell those are actually just the _active_ attributes? I stand corrected! There's no API that sets the "default" attributes -- we just have really well and truly mixed up what the word "default" means in SCREEN_INFORMATION. :smile:

claunia commented

@j4james commented on GitHub (Jun 16, 2020):

It does look like the API SetConsoleScreenBufferInfoEx lets you change the "default attributes", but as far as I can tell those are actually just the active attributes?

That's correct. The naming is extremely confusing. I've had to make copious notes tracking what these different attributes mean and where they're used. That's the only way I can make sense of the code.

@j4james commented on GitHub (Jun 16, 2020): > It does look like the API `SetConsoleScreenBufferInfoEx` lets you change the "default attributes", but as far as I can tell those are actually just the _active_ attributes? That's correct. The naming is extremely confusing. I've had to make copious notes tracking what these different attributes mean and where they're used. That's the only way I can make sense of the code.

claunia commented

@j4james commented on GitHub (Jun 18, 2020):

I had what I thought was an inspirational idea. What if we allow profiles to override the default legacy attributes, so PowerShell could set its default legacy background index to 1 (i.e. blue). That's essentially what make the original PowerShell work the way it does. That turned out to be more complicated than I thought, but I eventually got it working in my POC, and this was the result:

On the plus side, Write-Host with a black background actually displays as black now. On the down side, other parts of the command line that used to render with a black background are now a garish blue. The black wasn't correct either, but the blue seems somehow worse.

However, if you tweak the Campbell PowerShell color scheme so the dark blue color is the same shade as the background color, you get what I think is the perfect rendering:

But that's a whole lot of special case tweaks that all need to be in alignment for this to look right. Anyone trying to use a different color scheme is going to find things looking a mess again. Only now you're going to get bug reports about DarkBlue being rendered as black (or whatever the default background is).

In short, I'd say this isn't worth the effort, but I thought I should at least present my results here in case anything thinks this approach is worth pursuing.

@j4james commented on GitHub (Jun 18, 2020): I had what I thought was an inspirational idea. What if we allow profiles to override the default legacy attributes, so PowerShell could set its default legacy background index to 1 (i.e. blue). That's essentially what make the original PowerShell work the way it does. That turned out to be more complicated than I thought, but I eventually got it working in my POC, and this was the result: ![image](https://user-images.githubusercontent.com/4181424/85068956-29617c00-b1ab-11ea-9686-8c8a0ff1feb0.png) On the plus side, `Write-Host` with a black background actually displays as black now. On the down side, other parts of the command line that used to render with a black background are now a garish blue. The black wasn't correct either, but the blue seems somehow worse. However, if you tweak the `Campbell PowerShell` color scheme so the dark blue color is the same shade as the background color, you get what I think is the perfect rendering: ![image](https://user-images.githubusercontent.com/4181424/85069240-b3a9e000-b1ab-11ea-84cd-9d0fef6b90ee.png) But that's a whole lot of special case tweaks that all need to be in alignment for this to look right. Anyone trying to use a different color scheme is going to find things looking a mess again. Only now you're going to get bug reports about DarkBlue being rendered as black (or whatever the default background is). In short, I'd say this isn't worth the effort, but I thought I should at least present my results here in case anything thinks this approach is worth pursuing.

claunia commented

@j4james commented on GitHub (Jun 20, 2020):

One other thing I should add regarding the above test. I was a little concerned about the fact that the dir error was displayed with the default background color, since I was actually expecting that to be black. But it turns that is just a difference between powershell.exe and pwsh.exe - the former displays the error with a black background while the latter uses the default background color (technically the "startup" color).

Not that it makes much of a difference - it's just good to know that PR #6506 is actually doing the right thing here, and it is possible to get it exactly matching the conhost behaviour, assuming you're willing to jump through enough hoops.

@j4james commented on GitHub (Jun 20, 2020): One other thing I should add regarding the above test. I was a little concerned about the fact that the dir error was displayed with the default background color, since I was actually expecting that to be black. But it turns that is just a difference between powershell.exe and pwsh.exe - the former displays the error with a black background while the latter uses the default background color (technically the "startup" color). Not that it makes much of a difference - it's just good to know that PR #6506 is actually doing the right thing here, and it is possible to get it exactly matching the conhost behaviour, assuming you're willing to jump through enough hoops.

claunia commented