microsoft / terminal

The new Windows Terminal and the original Windows console host, all in the same place!
MIT License
95.43k stars 8.3k forks source link

Something is, well, probably fine with One half light #15452

Open zadjii-msft opened 1 year ago

zadjii-msft commented 1 year ago

From a mail thread with @jazzdelightsme and @oldnewthing

image

Repro:

  1. set theme for cmd to “One Half Light”
  2. set the “automatically adjust lightness” feature to “always”
  3. open a new cmd tab
  4. run “pwsh” or “powershell”
  5. run “Get-PSReadLineOption”, and notice that several of the colors are invisible (notably the DefaultTokenColor).

This is Terminal 1.18, with One Half Light. I’ve typed git do the thing. image

Left: "adjustIndistinguishableColors": "never", right: "adjustIndistinguishableColors": "always",

wait, what’s going on here?

image

And the output of conpty looks pretty normal, too. After typing the o in git do, Terminal gets: ␛[?25l␛[93m␛[36;27Hgit␣␛[37mdo␣␣␛[36;33H␛[?25h␛[m

Which is basically: • Turn cursor off • Use bright yellow FG • Cursor to the start of the prompt • “git “ • Use dark white FG • “do “ (two spaces) • Move the cursor back a space, turn it back on, and reset the colors.

Which is exactly what I’d expect. So why is that appearing as white, and not black (which is what bright white is defined as) from the scheme?

Maybe I haven’t had enough coffee yet this morning.


Trawling through the colortool output, I noticed:

1;36m␛[38;5;14m␛[3C␣␣gYw␣␣␣␣␣gYw␣␣␣␛[48;5;1m␣␣gYw␣␣␛[49m␣␛[48;5;2m␣␣gYw␣␣␛[49m␣␛[48;5;3m␣␣gYw␣␣␛[49m␣␛[48;5;4m␣␣gYw␣␣␛[49m␣␛[?25l␛[m␛[38;5;14m␛[48;5;5m␣␣gYw␣␣␛[49m␣␛[48;5;6m␣␣gYw␣␣␛[49m␣␛[48;5;7m␣␣gYw␣␣␛[m␍␊

37m␛[5C␣␣gYw␣␣␣␣␣gYw␣␣␣␛[48;5;1m␣␣gYw␣␣␛[m␣␛[48;5;2m␣␣gYw␣␣␛[m␣␛[48;5;3m␣␣gYw␣␣␛[m␣␛[48;5;4m␣␣gYw␣␣␛[m␣␛[48;5;5m␣␣gYw␣␣␛[m␣␛[48;5;6m␣␣gYw␣␣␛[m␣␛[48;5;7m␣␣gYw␣␣␛[m␍␊

1;37m␛[38;5;15m␛[3C␣␣gYw␣␣␣␣␣gYw␣␣␣␛[48;5;1m␣␣gYw␣␣␛[49m␣␛[48;5;2m␣␣gYw␣␣␛[49m␣␛[48;5;3m␣␣gYw␣␣␛[49m␣␛[48;5;4m␣␣gYw␣␣␛[49m␣␛[48;5;5m␣␣gYw␣␣␛[49m␣␛[48;5;6m␣␣gYw␣␣␛[49m␣␛[48;5;7m␣␣gYw␣␣␛[21;1H␛]0;OHL,␣no␣adjust␇␛[?25h␛[m␛[30m␛[107m␍␊

Maybe this is related to #15408

zadjii-msft commented 1 year ago

I knew I hadn't had enough coffee:

image

My version of ColorTool is wrong here - dark white in One Half Light is white. Colortool is doing the "oh yea sure dark white is just the default color" thing cause it was written for the Console API.

Configuring a light colored theme from the powershell docs is probably the solution here

lhecker commented 1 year ago

If PSReadLine implicitly assumes that "white" = white and "black" = black, why doesn't it implicitly assume that the default foreground "`e[39m" is equivalent to white "`e[37m"? In other words: Should it use "`e[39m"? While it would be a slight regression for a theme light Solarized Dark, it would fix the general issue fairly consistently in combination with color nudging/contrasting.

It can be hot fixed by adding this to profile.ps1:

Set-PSReadLineOption -Colors @{
    ContinuationPrompt = "`e[39m"
    Default = "`e[39m"
    Type = "`e[39m"
}

and enabling Defaults > Appearance > Automatically adjust brightness of indistinguishable text in 1.18.

zadjii-msft commented 1 year ago

I mean, it probably should. https://github.com/MicrosoftDocs/PowerShell-Docs/issues/9358 was one of the earlier discussions. I forget why we came to the conclusion that couldn't be the default in PsReadline.

For reference: https://github.com/microsoft/terminal/blob/3c3b1aac02320f49f6887f08e6d6d1d56132b1ef/src/cascadia/TerminalSettingsModel/defaults.json#L172-L193

Are we horrifically morally opposed to manually setting the white value to #fbfbfb instead of #fafafa, so that the auto adjust algorithm does something to it? It seems like a frequent enough footgun that a rgb(1,1,1) delta would be worth it. Even if it's not technically our problem, I think this is something in our power to fix

lhecker commented 1 year ago

@j4james A long time ago you pointed out that we shouldn't adjust the contrast of colors when they're identical (like with the concealed attribute). Would this still work if we used a hardcoded 16 color index table that isn't influenced by the user's theme? [^1] That way, when a light color scheme is used, we'd still consider \e[37m "white" to be a color that's different from the (white) background color and thus apply color contrasting to make it visible.

[^1]: I believe we could implement it without lookup table, if we map default-background to color index 0 and default foreground to index 7 for the comparison. There are probably some more clever ways it could be done.

DHowett commented 1 year ago

I think there's value in separating out the "conceal" vs "colors are the same" cases. We no longer have to act like the only data we have at the point of color selection is the raw values of foreground and background (thanks to James fixing #2661), so we could totally understand this case[^1] separately from the same color case.

Now, I really do wonder how many applications meaningfully display actual text in the same foreground as background (rather than spaces or whatever) and why on earth they're doing that :smile:

[^1]: we should also skip rendering concealed sections entirely; why waste time shaping and rasterizing and blitting?

j4james commented 1 year ago

I really do wonder how many applications meaningfully display actual text in the same foreground as background

I'm not sure I've seen this in practice, but I can certainly think of hypothetical reasons for it. For example, you might write out content as black-on-black to hide spoilers, and then later reveal that content with DECCARA (i.e. change attributes in rectangular area). You could do the same sort of thing with the Win32 console APIs I think.

lhecker commented 1 year ago

We've been discussing this and felt like it'd be better to only conceal text if it has the conceal attribute - Even though we think you're actually completely right @j4james. 😅 It's just that only handling \e[8m exclusively is a bit more consistent with how other terminals work, but more importantly, it works correctly even if you accidentally set the fg/bg to the same color and is a lot easier to implement correctly.

j4james commented 1 year ago

That's cool. I don't feel strongly about it, as long as it's not enabled by default.

Back when we were first discussing this feature, I had hoped we could get by with a very light adjustment - just enough to make the crap color schemes usable - and for that I thought it best to try and remain as "correct" as possible. But now it's clearly an accessibility feature with a very noticeable change. So I think if you're enabling the color adjustment now, you've got to expect that there could be edge cases where things may not look quite right.

citelao commented 1 year ago

I know I'm kinda blundering into a long-standing discussion, but I've been observing this conversation from the sidelines for a bit -

Is this an issue that affects basically every light theme across basically every terminal emulator?

In my experience, color schemes define ANSI colors like blue or bright red in the context of the background color: they're designed to be visible & sensible for the theme. I'd always assumed, then, that colors like black or white would also fall into this. In particular, since most terminals use a dark mode, I assumed that white is typically "foreground text color" and black is typically "background color."

So I'd expect a light-mode color scheme to actually lie: white would be a black color, to show up against the background, and black would be a white color, to match the background.

That isn't the case here, for any of the builtin themes: white is a literal white and black is a literal black, and I was prepared to point that out here. But then I investigated, and it seems like that's the case everywhere?

Terminal color What I thought it would mean What it means in dark mode What it means in light mode
White "foreground text color" literally white (happens to be foreground text color) literally white (happens to be invisible)
Black "background color" literally black (happens to be invisible) literally black (happens to be foreground text color)

So is this an issue that has affected every light theme across every terminal emulator that uses colors?

DHowett commented 1 year ago

So is this an issue that has affected every light theme across every terminal emulator that uses colors?

It absolutely is! But there's some nuance.

There are two (occasionally three) additional colors in the standard color palette for compliant terminal emulators: foreground[^1] and background. Those are intended to be the ground truth: the user's preferred foreground and background. Almost all terminal emulators that ship light color schemes by default set their foregrounds to dark.

Those are the colors you see in the m row (unadorned, no attribute). They are also accessible via \e[39m (foreground) and \e[49m (background).

The issue arises when applications request ANSI white by name. It's arguably wrong to make that anything but white. PSReadline is in a weird position, where it is trying to bridge the gap between the Windows Console API[^2] and modern terminal emulation and color schemes. It doesn't always know that the palette is 18 colors (all 16 plus fg, bg)... and sometimes limits its choice to 16. That worked perfectly fine when the Windows Console only supported 16 😉.

[^1]: The occasional third is "bright foreground", which applies to \e[1m or \e[1;39m when SGR 1 is set to enable "bright" colors. It might also be accessible via \e[99m (AIX SGR), but that is wild speculation on my part.

[^2]: SetConsoleTextAttribute only supports the 16-color palette of the Windows Console.

j4james commented 1 year ago

It's worth noting that some apps will try to detect whether you're using a light color scheme (by querying the palette values), and then adjust their colors appropriately. This is not yet supported in Windows Terminal though.

Ririshi commented 1 year ago

Can someone suggest a workaround for people who often switch between light and dark themes and schemes? I use AutoDarkMode to switch my Windows theme between dark and light based on time of day. I set up my WT to switch between a dark and light colour scheme (Material Darker and One Half Light), and I would like to be able to read all text in both modes. The two issues I'm running into are as follows:

  1. PSReadLine shows white text on a white background with default settings (as mentioned in this issue).

  2. On WSL with Ubuntu 22.04, some output from ls -al (with ls aliased to ls --colors=auto) is coloured white, because my ~/.dircolors contains this:

    FILE 37 # regular file
    MULTIHARDLINK 1;37 # regular file with more than one link

    Apparently those lines are not default (they're not in the output of dircolors --print-database), but I cannot remember why I ever set them like that. Even without this, though, the PSReadLine issue still remains.

Both of these happen even when "adjustIndistinguishableColors": "always" is set, which does not seem to realise that white on white is unreadable? I'm probably missing something when it comes to how this option is implemented.

Regardless, is there something I can do in my WT setup that will fix (or work around) both issues? While individual workarounds (e.g. PSReadLine colour settings, and changing my ~/.dircolors) are fine for my personal setup, it will not fix invisible text when SSHing into other machines with similar setups.

Edit: for now I've used the hotfix suggested by lhecker, updating some PSReadLine colors to `e[39m, and updated my ~/.dircolors to remove the explicit white. I'm guessing it now implicitly uses regular foreground colours instead, which works fine.

Another edit: I've added Member and Number to PSReadLine hotfix list as well. For me, Member was also set to `e[39m, and Number was set to `e[97m, which is bright white.

Set-PSReadLineOption -Colors @{
    ContinuationPrompt = "`e[39m"
    Default            = "`e[39m"
    Type               = "`e[39m"
    Member             = "`e[39m"
    Number             = "`e[39m"
}
itayking1990 commented 7 months ago

I face the same issue with light mode & one half light any eta for this bug?

Moreover, could someone explain where exactly to put this:

Set-PSReadLineOption -Colors @{
    ContinuationPrompt = "`e[39m"
    Default            = "`e[39m"
    Type               = "`e[39m"
    Member             = "`e[39m"
    Number             = "`e[39m"
}

I tried to $Profile.CurrentUserCurrentHost

But I can not find the profile.ps1 file (I have a new pc with a fresh install of Windows 11 latest stable build)

jazzdelightsme commented 7 months ago

@itayasaf2002xd

I can not find the profile.ps1 file

It does not exist by default (it is optional, for user customizations).

Note that legacy powershell.exe (5.1) and modern pwsh.exe each have separate $profile scripts (most people I know do something to "unify" them--either they both dot-source a common script, or one is a link to the other, something like that).