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

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

Open
zadjii-msft opened this issue May 26, 2023 · 14 comments
Open

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

zadjii-msft opened this issue May 26, 2023 · 14 comments
Labels
Area-Rendering Text rendering, emoji, complex glyph & font-fallback issues Issue-Bug It either shouldn't be doing this or needs an investigation. Product-Terminal The new Windows Terminal.
Milestone

Comments

@zadjii-msft
Copy link
Member

zadjii-msft commented May 26, 2023

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␍␊
  • the 37m line doesn't actually set the foreground color - I think it's relying on the 7 == "default index" thing.
  • we're emitting all the colors as 256 color indicies?? good god

Maybe this is related to #15408

@microsoft-github-policy-service microsoft-github-policy-service bot 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 May 26, 2023
@zadjii-msft
Copy link
Member Author

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

@zadjii-msft zadjii-msft changed the title Something is absolutely wack with One half light Something is, well, probably fine with One half light May 26, 2023
@lhecker
Copy link
Member

lhecker commented May 26, 2023

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
Copy link
Member Author

I mean, it probably should. MicrosoftDocs/PowerShell-Docs#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:

{
"name": "One Half Light",
"foreground": "#383A42",
"background": "#FAFAFA",
"cursorColor": "#4F525D",
"black": "#383A42",
"red": "#E45649",
"green": "#50A14F",
"yellow": "#C18301",
"blue": "#0184BC",
"purple": "#A626A4",
"cyan": "#0997B3",
"white": "#FAFAFA",
"brightBlack": "#4F525D",
"brightRed": "#DF6C75",
"brightGreen": "#98C379",
"brightYellow": "#E4C07A",
"brightBlue": "#61AFEF",
"brightPurple": "#C577DD",
"brightCyan": "#56B5C1",
"brightWhite": "#FFFFFF"
},

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
Copy link
Member

lhecker commented May 26, 2023

@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.

Footnotes

  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
Copy link
Member

DHowett commented May 26, 2023

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 case1 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 😄

Footnotes

  1. we should also skip rendering concealed sections entirely; why waste time shaping and rasterizing and blitting?

@j4james
Copy link
Collaborator

j4james commented May 26, 2023

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.

@carlos-zamora carlos-zamora added Area-Rendering Text rendering, emoji, complex glyph & font-fallback issues Issue-Bug It either shouldn't be doing this or needs an investigation. Product-Terminal The new Windows Terminal. and removed 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 May 31, 2023
@carlos-zamora carlos-zamora added this to the Terminal v1.19 milestone May 31, 2023
@lhecker
Copy link
Member

lhecker commented May 31, 2023

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
Copy link
Collaborator

j4james commented May 31, 2023

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
Copy link

citelao commented Jun 2, 2023

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
Copy link
Member

DHowett commented Jun 2, 2023

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: foreground1 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 API2 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 😉.

Footnotes

  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
Copy link
Collaborator

j4james commented Jun 2, 2023

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
Copy link

Ririshi commented Aug 6, 2023

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"
}

@zadjii-msft zadjii-msft modified the milestones: Terminal v1.19, Backlog Oct 4, 2023
@itayking1990
Copy link

itayking1990 commented Mar 19, 2024

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
Copy link
Member

@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).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area-Rendering Text rendering, emoji, complex glyph & font-fallback issues Issue-Bug It either shouldn't be doing this or needs an investigation. Product-Terminal The new Windows Terminal.
Projects
Status: To Consider
Development

No branches or pull requests

9 participants