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

"brightWhite" color in solarized light theme is the same as background #6390

Closed
gordonmessmer opened this issue Jun 7, 2020 · 8 comments
Closed
Labels
Resolution-Duplicate There's another issue on the tracker that's pretty much the same thing.

Comments

@gordonmessmer
Copy link

Environment

Windows build number: 10.0.18363.836
Windows Terminal version (if applicable): 1.0.1401.0

Steps to reproduce

Configure a WSL terminal with colorScheme: Solarized Light
Open a WSL terminal and run "top"

The values in the summary at the top of the display are invisible with this color scheme. The "brightWhite" color used in the Solarized Light theme is the same as "background."

In a terminal that supports bold fonts, I would suggest using the same value as "foreground", but for now, I think the value for brightBlack is probably best. I've fixed this by creating a scheme identical to Solarized Light, except for:

"brightWhite": "#002B36"

Expected behavior

brightWhite text should be foreground color, but bold.

Actual behavior

brightWhite text is invisible.

@ghost ghost 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 Jun 7, 2020
@skyline75489
Copy link
Collaborator

I suppose this is the same as #5682

@gordonmessmer
Copy link
Author

I don't think so. The issue here is simply that the color scheme Solarized Light uses the same color value for a foreground color as it does for the background color. This was probably an error made while importing the color scheme, because in any other terminal application, the "brightWhite" value in the Solarized Light color scheme is the same as the foreground color (but bold), not the background color.

@j4james
Copy link
Collaborator

j4james commented Jun 9, 2020

The official Solarized repo has bright white as the same color as the background. You can see an image of the colors here: https://github.com/altercation/solarized/tree/master/putty-colors-solarized

And if you look at the iTerm2 definition, this is bright white:
https://github.com/altercation/solarized/blob/master/iterm2-colors-solarized/Solarized%20Light.itermcolors#L68-L76
And this is the background color:
https://github.com/altercation/solarized/blob/master/iterm2-colors-solarized/Solarized%20Light.itermcolors#L149-L157

And again for the Xfce definition, this bright white:
https://github.com/altercation/solarized/blob/master/xfce4-terminal/light/terminalrc#L20
And this is the background color:
https://github.com/altercation/solarized/blob/master/xfce4-terminal/light/terminalrc#L4

The dark version has a similar problem in that it makes bright black invisible. This is a well known problem with the Solarized schemes (see altercation/solarized#220 and altercation/solarized#376).

According to the author, this choice was "very intentional", and a "considered compromise". I'm not sure what the compromise was, but I'm guessing some unspeakable horror would be unleashed if common color combinations were allowed to be visible. So if you like visible text, it's probably best you use something else.

That said, if we were to get color correction like the proposal in issue #2638, that could potentially help with this problem.

@DHowett
Copy link
Member

DHowett commented Jun 9, 2020

DAYS SINCE DUSTIN FELT REGRET SHIPPING
          SOLARIZED COLORS

                  ###
                 #   #
                #     #
                #     #
                #     #
                 #   #
                  ###

@gordonmessmer
Copy link
Author

You are correct, and I apologize. I agree that Solarized is bad in this respect.

However, I think there are two issues in Terminal, and I'd like to close this issue and open two new ones.

The first is the real underlying problem with this issue, and that is that Terminal interprets the uncolored "bold" ANSI sequence as a request for "briteWhite", and that is only a reasonable thing to do if the color scheme is light text on a dark background. For One Half Light, Tango Light, and Solarized Light, text that should be displayed in bold is either invisible or very difficult to see. Lacking support for bold text, Terminal should simply use the foreground color for bold text.

The second issue would resolve the first, if you are willing to consider it, but I am guessing that is less likely. In src/renderer/vt/VtSequences.cpp, a comment indicates, "in fact most terminals display the bright color when displaying bolded text" and I don't believe that's actually the case. Some do. Some make this an optional behavior. I've seen this behavior described as a "hack" for terminals that don't support aixterm ANSI sequences (90-97). Ideally Terminal should support both bold text and bright text individually, and not substitute one for the other. If bold can't be supported, it would be better, in my opinion, to simply ignore the bold sequence and use the standard palette for 30-37, and the bright palette for 90-97. If a survey of other terminal emulators would sway your mind, I'd be happy to determine which ones exhibit which behavior.

In any case, this bug doesn't actually have anything to do with Solarized (which admittedly does have a glaring flaw), which may alleviate your regret. :)

Please let me know if I should go ahead with separating this into two new issues.

@j4james
Copy link
Collaborator

j4james commented Jun 9, 2020

The first is the real underlying problem with this issue, and that is that Terminal interprets the uncolored "bold" ANSI sequence as a request for "briteWhite"

That is a genuine bug (issue #2661).

In src/renderer/vt/VtSequences.cpp, a comment indicates, "in fact most terminals display the bright color when displaying bolded text" and I don't believe that's actually the case. Some do. Some make this an optional behavior.

I know of only one terminal that doesn't support bold applying brightness to the SGR 30-37 colors, although as you say some do make it an optional behavior. I don't know if we have a dedicated issue for that option, but it's discussed in issue #109.

@zadjii-msft
Copy link
Member

Alright, so, this sounds like it's a combo /dupe of #2661/#5682/#4047/#2622/#4818/solarized-is-bad, and I'm gonna close it as such. I think we all regret shipping Solarized as a default Terminal theme, because it's simply bad, and we'll likely be removing it in the near future (see #6617).

@ghost
Copy link

ghost commented Jun 23, 2020

Hi! We've identified this issue as a duplicate of another one that already exists on this Issue Tracker. This specific instance is being closed in favor of tracking the concern over on the referenced thread. Thanks for your report!

@ghost ghost closed this as completed Jun 23, 2020
@ghost ghost added Resolution-Duplicate There's another issue on the tracker that's pretty much the same thing. 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 Jun 23, 2020
This issue was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Resolution-Duplicate There's another issue on the tracker that's pretty much the same thing.
Projects
None yet
Development

No branches or pull requests

5 participants