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

ANSI/VT bleeds through empty spaces on right when resizing console with wrap on resize enabled #2607

Closed
PhMajerus opened this issue Oct 26, 2017 · 4 comments

Comments

@PhMajerus
Copy link

Concerns Redstone 3 RTW / Windows 10 ver. 1709, tested on build 16299.19. Affects all WSL distros and more broadly, all Win32 CUI apps using VT.

VT text properties let us control the colors and other text attributes such as underline in-band.
When "Wrap on resize" is enabled, conhost automatically modifies the text buffer when the window is resized to reflow text, and, if necessary, padding lines with spaces until the '\n' is encountered.

The problem is when it pads with spaces, it applies the attributes of the last non-whitespace character of the line to them, instead of the attributes in effect after the last character.
This has the effect of extending colors, underline,... to the end of the line.
It should instead apply the attributes as per the state of the VT parser at that insertion point, which is often different as many scripts output "closing" VT sequences just before a \n or a whitespace.

This screenshot shows the issue using bash, both sets of lines should look exactly the same, but the window has been resized between the two commands, making the attributes of the "Line1" bleed through to the end of the line in the first set.
Note the "\e[m" explicitly stops the dark red background and underline, so applying it to padding spaces isn't up to interpretation, these attributes stop after the character '1'.
wraponresize bug

Even adding a whitespace after "\e[m" exhibits the same issue, so conhost seems to be grabbing the text attributes of the last non-whitepace character of the line when processing the wrap-on-resize.

@zadjii-msft
Copy link
Member

see also microsoft/terminal#32

We've know about this since we first added VT support. It's something I try my hand at fixing at least once a release, but boy it's a tricky one.

I'm sure there are other issues this one can dupe to

@PhMajerus
Copy link
Author

@zadjii-msft Any particular reason you select the last non-whitespace character to copy attributes from?
I don't pretend to know better, but here are some ideas... sometimes thinking about it from another perspective can help.

I would leave any existing space as they are and copy attributes from the last character of the line regardless of the character it is. After all if there are already spaces, they either came from the original string and had their attributes set properly when processing VT input, or they came from a previous resize and got the attributes as correctly as possible from existing ones. I think it would yield the expected result more often than basing it on the last non-whitespace.

Another idea: You seem to keep track of \n to avoid joining back explicit separate lines.
I'm not sure how you store the extra VT attributes, but I guess you do not keep the VT sequence and store it in the console text buffer. If you extended the internal equivalent of CHAR_INFO, maybe storing the VT attributes in effect after the string as the attributes of the \n char can help.
You already skip them when rendering, but could look for them when you need to insert extra characters on resize to know which attributes to copy to the new characters inserted before them.

@zadjii-msft
Copy link
Member

@PhMajerus No, there's no particular reason. The code that's responsible for that behavior is from the time immemorial, and I'm not sure any of us feel comfortable changing it without understanding what other possible repercussions it might have.

As for the rest of your ideas, they sound pretty similar to some other ideas we've discussed amongst the team before. Hopefully we'll be able to get a proper solution out sometime. We're pretty sure of the "right" way to fix this and a bunch of other bugs, but it'll require more than a just a few lines of code change.

@bitcrazed
Copy link
Contributor

This issue was moved to microsoft/terminal#75

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants