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.

Sign up for GitHub

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

Jump to bottom

Underline not rendered after some SGR 22/39/49 #3076

Closed
egmontkob opened this issue Oct 5, 2019 · 7 comments · Fixed by #6506
Closed

Underline not rendered after some SGR 22/39/49 #3076

egmontkob opened this issue Oct 5, 2019 · 7 comments · Fixed by #6506
Labels
Area-VT Virtual Terminal sequence support Issue-Bug It either shouldn't be doing this or needs an investigation. Needs-Tag-Fix Doesn't match tag requirements Priority-2 A description (P2) Product-Conpty For console issues specifically related to conpty Resolution-Fix-Committed Fix is checked in, but it might be 3-4 weeks until a release.
Milestone
Windows vNext

Comments

@egmontkob
Copy link

egmontkob commented Oct 5, 2019 •
edited
Loading

Environment

Windows build number: Win32NT 10.0.18362.0
Windows Terminal version (if applicable): 0.5.2762.0

Steps to reproduce

echo -e '\e[4m underlined \e[33m yellow \e[39m should still be underlined \e[m'

Expected behavior

The entire line should be underlined.

Actual behavior

The "should still be underlined" bits aren't underlined.

Note that SGR codes 22, 27, 39 and 49 seem to cause this effect, but only if they're predeced by a 1, 7, 30-37 or 40-47, resp., that is, only if they do actually turn off some other attribute.

Also note that if the "should still be underlined" text is made longer so that it overflows to the next line (or even multiple new lines), it's underlined in the last one of these lines. This makes it perhaps a reincarnation of #47.

Furthermore, changing the window width and thus causing a reflow of the text removes the underlining from the last line.
@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 Oct 5, 2019
@DHowett-MSFT DHowett-MSFT added Product-Conhost For issues in the Console codebase Area-VT Virtual Terminal sequence support Issue-Bug It either shouldn't be doing this or needs an investigation. labels Oct 6, 2019
@ghost ghost removed the Needs-Tag-Fix Doesn't match tag requirements label Oct 6, 2019
@DHowett-MSFT DHowett-MSFT added Product-Conpty For console issues specifically related to conpty and removed Product-Conhost For issues in the Console codebase labels Oct 6, 2019
@DHowett-MSFT
Copy link
Contributor

Investigation yields that the attributes are read & stored correctly; next step is looking at VtEngine (ConPTY out).

@DHowett-MSFT
Copy link
Contributor

VtEngine::_RgbUpdateDrawingBrushes doesn't factor in any attributes other than bold & color (actual RGB value) when it decides to emit \e[m. This is something that'll probably be addressed by #2661, which suggests that we should defer converting attributes into RGB values (and other unpacked flags) as late as possible.

@DHowett-MSFT
Copy link
Contributor

I'm keeping this one open as a linked workitem. Thanks for finding this, @egmontkob!

@DHowett-MSFT DHowett-MSFT mentioned this issue Oct 6, 2019
Closed
1 task
@aeiplatform
Copy link

Since echo -e is not POSIX compliant, you should rather combine printf and tput for text formatting.
In POSIX echo acts like echo -e by default but not in bash, because of arguments expansion.

I guess this command for my current bash works as expected.
(GNU bash, version 4.3.48(1)-release (x86_64-pc-linux-gnu)).
image

In POSIX this would be equivalent for yours example:
printf "%s underline %s yellow %s should still be underlined %s\n" "$(tput smul)" "$(tput setaf 3)" "$(tput setaf 7)" "$(tput sgr0)"

@egmontkob
Copy link
Author

Re echo -e vs. printf: I know, using echo -e is a bad habit of mine, I know I should use printf. :)

I disagree with tput, though (and echo vs printf doesn't really matter either). I'm not writing portable software here, I'm writing a reproducer test case for a bug in the terminal emulator.

Using tput instead of raw escape sequence introduces a level of indirection, makes the behavior depend on $TERM as well as potentially the terminfo version where my intent is not to, and is IMO harder to read. The terminal emulator cares about numbers like 37 or 39, and not terminfo capabilities such as setaf.

Also, by far not all escape sequences can be generated by tput, let alone deliberately broken ones, which some of my other bugreports are about.

E.g. there's no terminfo capability I'm aware of that would generate \e[39m that my example uses. You mistakenly changed that to \e[37m, or, well, its tput equivalent, modifying the test case, which could be the reason for you not hitting the bug.

@zadjii-msft zadjii-msft modified the milestones: 21H1, 20H1 Oct 24, 2019
@zadjii-msft zadjii-msft added the Priority-2 A description (P2) label Oct 24, 2019
@DHowett-MSFT DHowett-MSFT modified the milestones: 20H1, 21H1 Oct 24, 2019
@DHowett-MSFT DHowett-MSFT removed the Needs-Triage It's a new issue that the core contributor team needs to triage at the next triage meeting label Oct 29, 2019
@DHowett-MSFT
Copy link
Contributor

Lol- I just hit this myself, forgot we had a bug on it and re-investigated it.

Apparently what I said months ago is still true. Weird how that works.

@j4james j4james mentioned this issue May 14, 2020
Closed
This was referenced Jun 13, 2020
Closed
Merged
@ghost ghost added the In-PR This issue has a related PR label Jun 15, 2020
DHowett pushed a commit that referenced this issue Jul 1, 2020
@j4james
Improve the propagation of color attributes over ConPTY (#6506)
ddbe370 
This PR reimplements the VT rendering engines to do a better job of
preserving the original color types when propagating attributes over
ConPTY. For the 16-color renderers it provides better support for
default colors and improves the efficiency of the color narrowing
conversions. It also fixes problems with the ordering of character
renditions that could result in attributes being dropped.

Originally the base renderer would calculate the RGB color values and
legacy/extended attributes up front, passing that data on to the active
engine's `UpdateDrawingBrushes` method. With this new implementation,
the renderer now just passes through the original `TextAttribute` along
with an `IRenderData` interface, and leaves it to the engines to extract
the information they need.

The GDI and DirectX engines now have to lookup the RGB colors themselves
(via simple `IRenderData` calls), but have no need for the other
attributes. The VT engines extract the information that they need from
the `TextAttribute`, instead of having to reverse engineer it from
`COLORREF`s.

The process for the 256-color Xterm engine starts with a check for
default colors. If both foreground and background are default, it
outputs a SGR 0 reset, and clears the `_lastTextAttribute` completely to
make sure any reset state is reapplied. With that out the way, the
foreground and background are updated (if changed) in one of 4 ways.
They can either be a default value (SGR 39 and 49), a 16-color index
(using ANSI or AIX sequences), a 256-color index, or a 24-bit RGB value
(both using SGR 38 and 48 sequences).

Then once the colors are accounted for, there is a separate step that
handles the character rendition attributes (bold, italics, underline,
etc.) This step must come _after_ the color sequences, in case a SGR
reset is required, which would otherwise have cleared any character
rendition attributes if it came last (which is what happened in the
original implementation).

The process for the 16-color engines is a little different. The target
client in this case (Windows telnet) is incapable of setting default
colors individually, so we need to output an SGR 0 reset if _either_
color has changed to default. With that out the way, we use the
`TextColor::GetLegacyIndex` method to obtain an approximate 16-color
index for each color, and apply the bold attribute by brightening the
foreground index (setting bit 8) if the color type permits that.

However, since Windows telnet only supports the 8 basic ANSI colors, the
best we can do for bright colors is to output an SGR 1 attribute to get
a bright foreground. There is nothing we can do about a bright
background, so after that we just have to drop the high bit from the
colors. If the resulting index values have changed from what they were
before, we then output ANSI 8-color SGR sequences to update them.

As with the 256-color engine, there is also a final step to handle the
character rendition attributes. But in this case, the only supported
attributes are underline and reversed video.

Since the VT engines no longer depend on the active color table and
default color values, there was quite a lot of code that could now be
removed. This included the `IDefaultColorProvider` interface and
implementations, the `Find(Nearest)TableIndex` functions, and also the
associated HLS conversion and difference calculations.

VALIDATION

Other than simple API parameter changes, the majority of updates
required in the unit tests were to correct assumptions about the way the
colors should be rendered, which were the source of the narrowing bugs
this PR was trying to fix. Like passing white on black to the
`UpdateDrawingBrushes` API, and expecting it to output the default `SGR
0` sequence, or passing an RGB color and expecting an indexed SGR
sequence.

In addition to that, I've added some VT renderer tests to make sure the
rendition attributes (bold, underline, etc) are correctly retained when
a default color update causes an `SGR 0` sequence to be generated (the
source of bug #3076). And I've extended the VT renderer color tests
(both 256-color and 16-color) to make sure we're covering all of the
different color types (default, RGB, and both forms of indexed colors).

I've also tried to manually verify that all of the test cases in the
linked bug reports (and their associated duplicates) are now fixed when
this PR is applied.

Closes #2661
Closes #3076
Closes #3717
Closes #5384
Closes #5864

This is only a partial fix for #293, but I suspect the remaining cases
are unfixable.
@ghost ghost added Needs-Tag-Fix Doesn't match tag requirements and removed In-PR This issue has a related PR labels Jul 1, 2020
@ghost ghost added the Resolution-Fix-Committed Fix is checked in, but it might be 3-4 weeks until a release. label Jul 1, 2020
@DHowett DHowett mentioned this issue Jul 20, 2020
Closed
@ghost
Copy link

ghost commented Jul 22, 2020

🎉This issue was addressed in #6506, which has now been successfully released as Windows Terminal Preview v1.2.2022.0.:tada:

Handy links:

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
Area-VT Virtual Terminal sequence support Issue-Bug It either shouldn't be doing this or needs an investigation. Needs-Tag-Fix Doesn't match tag requirements Priority-2 A description (P2) Product-Conpty For console issues specifically related to conpty Resolution-Fix-Committed Fix is checked in, but it might be 3-4 weeks until a release.
Projects
None yet
Milestone
Windows vNext
Development

Successfully merging a pull request may close this issue.

Improve the propagation of color attributes over ConPTY j4james/terminal
4 participants
@egmontkob @DHowett-MSFT @zadjii-msft @aeiplatform