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 #75

Closed
bitcrazed opened this issue Feb 16, 2018 · 4 comments
Labels
Product-Conhost For issues in the Console codebase Resolution-Duplicate There's another issue on the tracker that's pretty much the same thing. Work-Item It's being tracked by an actual work item internally. (to be removed soon)

Comments

@bitcrazed
Copy link
Contributor

From @PhMajerus on October 26, 2017 15:29

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.

Copied from original issue: microsoft/WSL#2607

@bitcrazed bitcrazed added Work-Item It's being tracked by an actual work item internally. (to be removed soon) console Resolution-Duplicate There's another issue on the tracker that's pretty much the same thing. labels Feb 16, 2018
@bitcrazed
Copy link
Contributor Author

From @zadjii-msft on October 27, 2017 17:45

see also #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

@bitcrazed
Copy link
Contributor Author

From @PhMajerus on October 28, 2017 1:12

@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.

@bitcrazed
Copy link
Contributor Author

From @zadjii-msft on October 30, 2017 15:32

@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.

@DHowett-MSFT
Copy link
Contributor

Closing as a dupe of #32.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Product-Conhost For issues in the Console codebase Resolution-Duplicate There's another issue on the tracker that's pretty much the same thing. Work-Item It's being tracked by an actual work item internally. (to be removed soon)
Projects
None yet
Development

No branches or pull requests

3 participants