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

color "brightWhite" and its usage in WSL man pages #2864

Closed
neumannd opened this issue Sep 24, 2019 · 17 comments
Closed

color "brightWhite" and its usage in WSL man pages #2864

neumannd opened this issue Sep 24, 2019 · 17 comments
Labels
Needs-Triage It's a new issue that the core contributor team needs to triage at the next triage meeting Resolution-Duplicate There's another issue on the tracker that's pretty much the same thing.

Comments

@neumannd
Copy link

Possibly this issue is not an issue of Windows Terminal but of WSL or of one specific distribution within WSL.

Environment

Microsoft Windows [Version 10.0.18362.356]
Windows Terminal (Preview) [Version 0.4.2382.0]
WSL 1 with Ubuntu 18.04 (not sure how to find out the build number)

Steps to reproduce

  • start Windows Terminal
  • open a tab with WSL Ubuntu 18.04 (maybe also with other distributions)
  • choose a light theme (like One Half Light) in the profiles.json file
  • view a man page (e.g. man grep)
  • play with values for brightWhite in the choosen color schema

Expected behavior

General text should be colored in generic colors such as foreground or explicit colors like brightWhite should change their color.

Actual behavior

The WSL man page seems to use brightWhite for highlighted text in man pages. brightWhite remains white in the light themes. The remaining text has the color foreground.

grep manpage with default color schema One Half Light:

bug_brightWhite_is_white

grep manpage with modified color schema One Half Light (brightWhite set to #FFFFFF):

bug_brightWhite_is_black

@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 Sep 24, 2019
@zadjii-msft
Copy link
Member

This sounds a lot like #2638 to me

@zadjii-msft zadjii-msft added the Resolution-Duplicate There's another issue on the tracker that's pretty much the same thing. label Sep 24, 2019
@ghost ghost removed the Needs-Tag-Fix Doesn't match tag requirements label Sep 24, 2019
@neumannd
Copy link
Author

@zadjii-msft Thanks for linking. Looks similar.

brightWhite might have the same value than the background color. Does the algorithm work for equal colors?

@j4james
Copy link
Collaborator

j4james commented Sep 25, 2019

#2638 may mitigate the issue, but it doesn't solve the underlying problem. And that is that the bold/bright attribute doesn't work correctly when the default foreground color isn't "white" (more specifically, index color 7).

In the One Half Light color scheme, the default colors are #383A42 on #FAFAFA . So selecting black on white, and then the bold attribute, like this:

echo -e "\e[30;47m NORMAL \e[1m BOLD \e[m"

should be the equivalent of selecting the default colors, and then the bold attribute, like this:

echo -e "\e[m NORMAL \e[1m BOLD \e[m"

But in the latter sequence, the "bold" text becomes bright white instead of bright black. I'm a fairly certain that is not what we want.

image

@j4james
Copy link
Collaborator

j4james commented Sep 25, 2019

On further investigation, I believe the fault may be in the way the Windows Terminal applies the color scheme. If you set the palette using escape sequences, then it works correctly.

For example, this is the equivalent set of escape sequences to initialize the palette with the One Half Light color scheme:

echo -e "\e]4;0;rgb:38/3A/42\e\\"
echo -e "\e]4;1;rgb:E4/56/49\e\\"
echo -e "\e]4;2;rgb:50/A1/4F\e\\"
echo -e "\e]4;3;rgb:C1/83/01\e\\"
echo -e "\e]4;4;rgb:01/84/BC\e\\"
echo -e "\e]4;5;rgb:A6/26/A4\e\\"
echo -e "\e]4;6;rgb:09/97/B3\e\\"
echo -e "\e]4;7;rgb:FA/FA/FA\e\\"
echo -e "\e]4;8;rgb:4F/52/5D\e\\"
echo -e "\e]4;9;rgb:DF/6C/75\e\\"
echo -e "\e]4;10;rgb:98/C3/79\e\\"
echo -e "\e]4;11;rgb:E4/C0/7A\e\\"
echo -e "\e]4;12;rgb:61/AF/EF\e\\"
echo -e "\e]4;13;rgb:C5/77/DD\e\\"
echo -e "\e]4;14;rgb:56/B5/C1\e\\"
echo -e "\e]4;15;rgb:FF/FF/FF\e\\"
echo -e "\e]10;rgb:38/3A/42\e\\"
echo -e "\e]11;rgb:FA/FA/FA\e\\"

If you run man grep again after executing those sequences, you should find that it displays the "bold" text correctly.

@neumannd
Copy link
Author

@j4james Thanks for investigating the issues.

The bold text in the man pages is properly colored after executing those lines.

As workaround, I could copy your code into my .bashrc ... ;-) .

What exactly does \e]... and \e[... do? It is a bit hard to search for these terms.

@j4james
Copy link
Collaborator

j4james commented Sep 25, 2019

What exactly does \e]... and \e[... do? It is a bit hard to search for these terms.

\e[ is known as CSI, the Control Sequence Introducer, and marks the start of most standard escape sequences. You can read more about them on Wikipedia.

\e] is known as OSC, for Operating System Command, and is typically used for private escape sequences that might differ from one terminal emulator to the next. The ones used here - OSC 4, 10, and 11 - are color palette commands that were originally defined by XTerm. You can read more about them here.

@ghost
Copy link

ghost commented Sep 26, 2019

This issue has been marked as duplicate and has not had any activity for 1 day. It will be closed for housekeeping purposes.

@ghost ghost closed this as completed Sep 26, 2019
@j4james
Copy link
Collaborator

j4james commented Sep 26, 2019

@zadjii-msft I'm fairly certain this isn't a duplicate, or at least not a duplicate of #2638.

@DHowett-MSFT
Copy link
Contributor

I'm worried about this issue. I know that we have a duplicate somewhere, but I just can't find it. This is actually partially related to #293 (we transform 39 into 37, and technically we transform 1;39 into 97) and partially related to the fact that we don't support a separate "boldForeground" which should be used when the foreground color is bold. Where terminals usually have 19-color palettes (indexed 16 plus fg/bg/bold), we have 18 narrowed in certain instances to 16.

Ref #2661, plus the bug report I just can't find for "foreground but bold".

@DHowett-MSFT
Copy link
Contributor

(or, equally likely, we transform 37 into 39, but we don't transform 97 into 1;39 or 99. There's a bit of nuance. :))

@DHowett-MSFT DHowett-MSFT reopened this Sep 27, 2019
@j4james
Copy link
Collaborator

j4james commented Sep 27, 2019

I don't think it's a narrowing bug, or anything along those lines, because why then would it work when the palette is set via escape sequences? It's the exact same colors being used, so surely they should fail in the same way if that were the case?

My theory (which I haven't had a chance to investigate) is that the Windows Terminal might be trying to handle the palette mapping exclusively on the terminal side, and not passing that info through to conhost. So conhost still thinks it's working with the default palette (or whatever settings it has), and when you've got a derived color (from the bold attribute), there's no longer a direct link back to the correct color on the Terminal side.

Btw, I can accept this might still be a dup. I just didn't want it to get lost if the dup closure was based on #2638, which definitely isn't the issue here.

@DHowett-MSFT
Copy link
Contributor

DHowett-MSFT commented Sep 27, 2019

handle the palette mapping exclusively

It definitely is, and it should be. #2661 is one of the things that's caused by conhost caring too much.

Setting the color palette actually leverages the issue in 2661 to fix this one. If you do this:

`e]4;6;rgb:FA/AF/AA`e/`e]10;rgb:FA/AF/AA`e/

(powershell syntax, sorry)

and then just use \e[1m, you actually get back \e[96m from ConPTY. Because the default matches one of the indexed colors, it gets narrowed to the index.

If we stop querying the color table in conhost when we need to figure out which index to use to set the color we think we want, we'll start passing through color indices unharmed.

You're right that the derived color is being matched against the wrong index, but really we shouldn't be using color index derivations at all. 😄

@DHowett-MSFT
Copy link
Contributor

(sorry, i realized i just said the same thing three times there at the end.)

@j4james
Copy link
Collaborator

j4james commented Sep 27, 2019

OK, I think I get it now. So if the color was simply passed through from conhost as default + bold, without trying to map it to an index or COLORREF, then the Terminal side would actually have been able to map that to the correct palette values? In that I'm assuming #2661 might fix this.

@DHowett-MSFT
Copy link
Contributor

would have been able

Indeed. 😄

@ghost
Copy link

ghost commented Sep 29, 2019

This issue has been marked as duplicate and has not had any activity for 1 day. It will be closed for housekeeping purposes.

@ghost ghost closed this as completed Sep 29, 2019
@DHowett-MSFT
Copy link
Contributor

Just FYI -- confirmed that this is a case that #2661 will help with (plus/minus #293) 😄

image

Once we stop color mapping, that 1 will stop coming through as 97 (circled in blue)

This issue was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Needs-Triage It's a new issue that the core contributor team needs to triage at the next triage meeting 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

4 participants