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

Combined SGR attributes 1 and 7, "Bold or increased intensity" and "Reverse video" do not render as expected #6471

Closed
rbeesley opened this issue Jun 12, 2020 · 6 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-Conpty For console issues specifically related to conpty Resolution-Duplicate There's another issue on the tracker that's pretty much the same thing.

Comments

@rbeesley
Copy link
Contributor

Environment

Platform ServicePack Version      VersionString
-------- ----------- -------      -------------
 Win32NT             10.0.19640.0 Microsoft Windows NT 10.0.19640.0
Windows Terminal Preview
Version: 1.1.1611.0

Steps to reproduce

See the attached full capture for more guidance in what the R1C1 reference addresses are indicating.

@ECHO OFF
SETLOCAL ENABLEDELAYEDEXPANSION

FOR /F "tokens=4 delims= " %%_ IN ('chcp') DO (SET CHCP=%%_)
IF DEFINED CHCP (
  chcp 65001>NUL
)

ECHO.
ECHO Baseline Foreground and Background:
ECHO.
ECHO ␛[33m           → �[33mHello, World^^!�[m :  R8C1
ECHO ␛[1;33m         → �[1;33mHello, World^^!�[m :  R9C1
ECHO ␛[30;43m        → �[30;43mHello, World^^!�[m :  R2C8
ECHO ␛[30;103m       → �[30;103mHello, World^^!�[m :  R2C9
ECHO.
ECHO.
ECHO Test Cases:
ECHO.
ECHO ␛[33m␛[43m      → �[33m�[43mHello, World^^!�[m :  R8C8
ECHO ␛[7;33m␛[43m    → �[7;33m�[43mHello, World^^!�[m : R40C8
ECHO.
ECHO ␛[33m␛[103m     → �[33m�[103mHello, World^^!�[m :  R8C9
ECHO ␛[7;33m␛[103m   → �[7;33m�[103mHello, World^^!�[m : R40C9
ECHO ␛[33;7m␛[103m   → �[33;7m�[103mHello, World^^!�[m :     -
ECHO ␛[103m␛[7;33m   → �[103m�[7;33mHello, World^^!�[m :     -
ECHO ␛[103m␛[33;7m   → �[103m�[33;7mHello, World^^!�[m :     -
ECHO.
ECHO ␛[1;33m␛[43m    → �[1;33m�[43mHello, World^^!�[m :  R9C8
ECHO ␛[1;7;33m␛[43m  → �[1;7;33m�[43mHello, World^^!�[m : R41C8
ECHO ␛[7;1;33m␛[43m  → �[7;1;33m�[43mHello, World^^!�[m :     -
ECHO ␛[43m␛[1;7;33m  → �[43m�[1;7;33mHello, World^^!�[m :     -
ECHO ␛[43m␛[7;1;33m  → �[43m�[7;1;33mHello, World^^!�[m :     -
ECHO.
ECHO ␛[1;33m␛[103m   → �[1;33m�[103mHello, World^^!�[m :  R9C9
ECHO ␛[1;7;33m␛[103m → �[1;7;33m�[103mHello, World^^!�[m : R41C9

IF DEFINED CHCP (
  chcp !CHCP!>NUL
)

ENDLOCAL

Expected behavior

With the string ␛[1;7;33m␛[43m R41C8␛[m, I believe that the foreground and background color output should be the same as ␛[33m␛[103m R8C9␛[m.

image

In this example, if ␛[33m␛[103m with a video reverse attribute ␛[7;33m␛[103m will invert like it is ␛[1;33m␛[43m, then the same should be true in the reverse. ␛[1;33m␛[43m with the video reverse attribute ␛[1;7;33m␛[43m should invert like it is ␛[33m␛[103m.

Actual behavior

Instead of switching the foreground and background colors, it looks like the intensity attribute is applied after switching the two colors.

image

This has the impact that you cannot see the dark text against the light background because the intensity attribute is applied after the background color is assigned the foreground color, and so when these are in the same color pairing it becomes light on light.

I chose this specific example because under Campbell there is enough contrast between the normal and the intensity colors to clearly demonstrate this problem, but as you can see in the full table the problem exists for all color pairings. You can also see that the reverse colors in a column, the foreground color maintains the same intensity for the entire set of rows, but it would be expected that they would alternate from light to dark between adjoining rows.

The additional test cases which don't have a reference in the table demonstrate that where in the order of attributes that the reverse video attribute is placed should not change the result. This is potentially an ambiguous case in the spec as it could be inferred that setting intensity before or after setting reverse video might have an impact, but I think it is more expressive and likely more how it was intended, that the reverse video attribute can be placed anywhere and reverses the foreground and background colors. An alternate consideration would be that it would actually invert the colors, not just flip the foreground for the background. The ECMA-048 standard actually defines it as "negative image" rather than invert, which is suggestive that flipping the foreground and background colors is the incorrect rendering anyway.

image

@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 12, 2020
@rbeesley
Copy link
Contributor Author

I explored this a little more and using SGR 1 to set the foreground intensity gives different results than using 90-97. This may be more of a problem with regard to not supporting bold. I'll provide more details when I can capture them and add them to this issue.

@j4james
Copy link
Collaborator

j4james commented Jun 13, 2020

Your original test case sounds like #3076, which is essentially a variant of the #2661 issue. I know it's fixed in my 2661 branch.

In general, if it works in conhost, but fails in Windows Terminal, then it's likely some variant of #2661.

@rbeesley
Copy link
Contributor Author

@j4james, spot on. It works in conhost but fails in Windows Terminal.

@zadjii-msft
Copy link
Member

For my own sanity:

expected actual

@DHowett DHowett 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-Conpty For console issues specifically related to conpty labels Jun 16, 2020
@ghost ghost removed the Needs-Tag-Fix Doesn't match tag requirements label Jun 16, 2020
@DHowett
Copy link
Member

DHowett commented Jun 16, 2020

Thanks for the first-line triage, and the excellent repros @rbeesley. I'm gonna call this one a /dupe of #2661 #3076 (even though the graphic rendition is different, they're all gated on this one thing.)

#6506 fixes this, as well. Please ignore the garish coloring -- it is intended to catch issues related to applications emitting legacy attributes.

image

@ghost
Copy link

ghost commented Jun 16, 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 16, 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 labels Jun 16, 2020
This issue was closed.
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-Conpty For console issues specifically related to conpty 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