microsoft / terminal

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

Faint attribute works backwards in Light themes #16493

Open chrisant996 opened 8 months ago

chrisant996 commented 8 months ago

Windows Terminal version

1.20.231215001-llm, and 1.19.3172.0, and 1.18.3181.0

Windows build number

Windows 10 Pro, 22H2, 19045.3803

Other Software

No response

Steps to reproduce

The SGR 2m (Faint) attribute blends with Black, making it work backwards from its name in Light themes, where the default background color is white (or whitish).

It should instead blend with the current background color. Or an argument could be made for it to blend with the default background color.

(I recognize there's a possibility that it's intentionally matching behavior of bugs in other terminals for compatibility purposes, e.g. for compatibility with some Linux terminals.)

Repro Steps

  1. Download this file: faint_sample.txt
  2. Launch Windows Terminal.
  3. Open or select a tab that uses a Light theme, i.e. a theme where the default background color is white (or whitish).
  4. Optionally change the font weight to Bold to make it easier to see the color problems.
  5. Run type %USERPROFILE%\Downloads\faint_sample.txt

Why This Matters

Most "Light" themes use the same color layout as "Dark" themes (i.e. 30m..37m being defined as darker than 90m..97m). Given that, the only way for an app to "support Light themes" is to either bypass the terminal theme entirely (require users to customize each app's colors separately in addition to customizing the terminal colors), or to only use non-color attributes like 1m (bold), 2m (faint), 3m (italic), 4m (underline).

I was trying to find a way to not require users to configure every single color individually. I expected 2m (Faint) to actually produce faint text, i.e. lower contrast versus the background.

As things stand, apps cannot use 2m (Faint) if they want to behave reasonably when a Light theme is active. Combined with the fact that most Light themes are defined backwards with respect to contrast, and it means the only feasible option seems to be requiring users to manually configure the app colors if the users wants to use a Light theme.

This is related to #10639; if GetConsoleScreenBufferEx() returned the actual current color table, then at least an app could check whether the current theme is Light or Dark, and automatically pick colors with appropriate contrast.

Trying 2m was a desperate attempt on my part to find some way to control contrast reliably. What's really needed is for GetConsoleScreenBufferEx() to return the actual color table. Or to have some way (any way) to programmatically detect Light themes versus Dark themes in the terminal. There is currently no way to do that. And so there's no way for a program to automatically achieve appropriate contrast; the user has to do it manually (which is extra challenging if a user has a Light theme in some terminal profiles, but a Dark theme in others). Several users have complained to me about color contrast in my app in Light themes, because they don't want to have to configure anything other than the terminal color theme itself. But it's currently out of my hands; it's a limitation in Windows Terminal and/or the Linux "status quo".

Expected Behavior

No response

Actual Behavior

image

The 2m Faint text is always the darkest of the three in each line of output from the sample file. Which against a White background makes it effectively "intense/bold" instead of "faint". In a Light them, the actual effect is opposite from what the name of the attribute claims.

Expected Behavior: The 2m Faint text should be fainter than the other text, so instead of blending with Black, it should blend with White (i.e. the current background; or an argument could be made for it to blend with the default background).

j4james commented 8 months ago

FWIW, I have a work-in-progress refactoring of the attribute handling which is intended to bring our implementation closer to the DEC standard, and one of the changes is a fix for dim black, which is supposed to get brighter when it's dimmed (since it obviously can't get any darker).

I know this isn't exactly what you want, but it should at least improve the case where you have black text on a light background. For other colors, though, I believe our implementation is a reasonably close to what it's expected to be.

chrisant996 commented 8 months ago

For other colors, though, I believe our implementation is a reasonably close to what it's expected to be.

Zooming out, what I'm trying to shine a spotlight on is the fact that Light themes are just totally broken when using ANSI codes. At least in Windows Terminal. Maybe also Linux in general, I don't know.

If Normal Default Foreground is significantly fainter than Faint Default Foreground, then there's a problem with at least one of the following:

  1. The implementation.
  2. The spec /guidance.
  3. The concept of using Light themes.

πŸ™ƒ

When the VT specifications were developed, there was no such thing as a white background on a computer screen. My view is that since a seemingly large set of people seem to want to use light-background themes, it makes sense to evolve the industry and update the spec so it's possible for programs' output to still have reasonable contrast for readability even in Light themes.

j4james commented 8 months ago

The concept of using Light themes.

That's the one! Light themes are always going to be broken, because there's a general expectation that the 16 standard palette colors will be visible on a black background, with the top 8 being brighter, but they're also expected to work in the same way on the default background color. That's simply not possible with a light theme.

The only practical way to deal with this situation is for applications to adjust their palette and color usage to compensate for the weird background. And as you say, that requires we have a way to query the current palette (#3718, #10639), so that's definitely something we need to fix. Messing with the standard dim behavior isn't really going to help.

Although if all you care about is using the default foreground color with the default background color, then perhaps it would make sense to have a separate palette entry for the dim version of the default foreground, in the same way that some terminals let you set the bright version (#11939).

chrisant996 commented 8 months ago

Still. Back to the original topic:

Making Faint blend between current foreground and current background seems an accurate way to implement Faint.

j4james commented 8 months ago

That means the faint colors would vary depending on what background they were used with, which is definitely not expected behavior.

Say for example you're using one of those fancy prompts that have a pattern extending over multiple cells - something similar to this: β—’β—€. If the foreground were using a dim attribute, and background on the first cell is different from the second cell, then the diagonal stripe would no longer be a single color.

It's also worth mentioning that these attributes can apply to foreground or background, and potentially even both at the same time.

chrisant996 commented 8 months ago

Say for example you're using one of those fancy prompts that have a pattern extending over multiple cells - something similar to this: β—’β—€. If the foreground were using a dim attribute, and background on the first cell is different from the second cell, then the diagonal stripe would no longer be a single color.

Fair. So blend between current foreground and default background.

Windows Terminal currently implements Faint as blending between current foreground and Black. Since making default background = White in a theme implies that the user's intent is for the background to generally be White, it seems appropriate to blend between current foreground and default background, instead of between current foreground and Black.

It's also worth mentioning that these attributes can apply to foreground or background, and potentially even both at the same time.

How can 1m or 2m be applied to both the foreground and background at the same time? They're defined as applying to the foreground color. When 7m is applied for reverse video, then since foreground and background get swapped then 1m and 2m apply to the background instead of the foreground, but not to both at the same time.

Or are you talking about something else? Or are you saying that Windows Terminal has implemented 1m and 2m incorrectly?

Sample file: sample_127.txt

Running type sample_127.txt prints the following, and appears to demonstrate that 1m and 2m cannot apply to both the foreground and background at the same time. If I've missed something, please clarify.

image

j4james commented 8 months ago

How can 1m or 2m be applied to both the foreground and background at the same time?

There's a mode that controls whether the brightness attributes apply to just the foreground or both foreground and background. It affects bold, faint, and blink (which essentially just toggles the faint attribute). The current version of Windows Terminal doesn't support it yet, but it's implemented in my local fork.

chrisant996 commented 8 months ago

How can 1m or 2m be applied to both the foreground and background at the same time?

There's a mode that controls whether the brightness attributes apply to just the foreground or both foreground and background. It affects bold, faint, and blink (which essentially just toggles the faint attribute). The current version of Windows Terminal doesn't support it yet, but it's implemented in my local fork.

Ah ok. I've never noticed documentation about a CSI code for that mode. I'll look in your fork to figure out what code you're talking about.

j4james commented 8 months ago

I'll look in your fork to figure out what code you're talking about.

I'm sorry if I gave you the wrong impression, but you won't find anything in my public fork - that's just used for PR submissions. My development fork is private.

But if you want to know more about that mode, you can search online for a copy of the DEC VT520/VT525 Programmer Information manual, and then look for the DECBBSM mode.

chrisant996 commented 8 months ago

Ah, I see, thanks for the reference. πŸ‘

It looks like it's also possible to apply Bold, Blink, Faint to only the background. E.g. " CSI ?116h CSI 2m CSI ?116l CSI 22m background is faint but foreground is normal "

j4james commented 8 months ago

It looks like it's also possible to apply Bold, Blink, Faint to only the background. E.g. " CSI ?116h CSI 2m CSI ?116l CSI 22m background is faint but foreground is normal "

Unless I've misunderstood you, that's not going to work the way you think. It's a render mode, so it doesn't change what's written to the buffer - it only affects the way the buffer content is rendered.

In that example, your text is going to be stored in the buffer with default attributes, because you've just turned faint on and off again before writing anything. And even if you had written any faint text, it would only be applied to the foreground color when rendered, because that's the last state that you've left the DECBBSM mode.