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

Certain SGR attributes are not preserved after a resize sequence #2540

Closed
j4james opened this issue Aug 26, 2019 · 3 comments · Fixed by #15269
Closed

Certain SGR attributes are not preserved after a resize sequence #2540

j4james opened this issue Aug 26, 2019 · 3 comments · Fixed by #15269
Labels
Area-Output Related to output processing (inserting text into buffer, retrieving buffer text, etc.) Area-VT Virtual Terminal sequence support Help Wanted We encourage anyone to jump in on these. In-PR This issue has a related PR Issue-Bug It either shouldn't be doing this or needs an investigation. Priority-1 A description (P1) Product-Conhost For issues in the Console codebase
Milestone

Comments

@j4james
Copy link
Collaborator

j4james commented Aug 26, 2019

Environment

Windows build number: Version 10.0.18362.239

Steps to reproduce

Open a WSL conhost shell, and execute the following two commands:

printf "\e[30;48;5;214m ORANGE \n\e[8;24;80t ORANGE \e[m\n"
printf "\e[7;38;5;15;48;5;4m REVERSE \n\e[8;24;80t REVERSE \e[m\n"

The first command uses an SGR escape sequence to set the background color to index 214 (a shade of orange), writes out the text "ORANGE", then uses the XTerm window manipulation sequence to resize the screen, and writes out "ORANGE" again.

The second command does the same sort of thing but with different attributes. It sets the colors to bright white (index 15) on blue (index 4), and also enable the reverse video attribute (making it blue on white).

Expected behavior

The screen resize should have no effect on the active SGR attributes, so in each case I'd expect to see two matching lines of text with identical colors.

Actual behavior

In the first test, the second "ORANGE" line is a shade of yellow rather than orange (the exact color will likely depend on your palette).

In the second test, the second "REVERSE" line is not actually reversed - it's white on blue instead of blue on white.

image

Cause

The resize escape sequence is handled by the DispatchCommon::s_ResizeWindow method, which does so by reading the screen buffer info with GetConsoleScreenBufferInfoEx, modifiying just the dwSize and srWindow fields (i.e the buffer and viewport sizes), and then writing the updated info back with SetConsoleScreenBufferInfoEx. The problem is that the CONSOLE_SCREEN_BUFFER_INFOEX structure can't reliable represent the text attributes in the wAttributes field, so they can become corrupted in the process.

The DECCOLM escape sequence, which is handled by the AdaptDispatch::SetColumns method, has essentially the same problem.

The reverse video case could probably be fixed with improvements to the GenerateLegacyAttributes method, but it's never going to be possible to accurately handle every attribute value with a legacy equivalent. I think the real solution is to avoid using the SetConsoleScreenBufferInfoEx method, and try and update the window and buffer size more directly, via SetConsoleWindowInfo and SetConsoleScreenBufferSize. I haven't actually tried that, though, so maybe it's not as simple as I think.

@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 Aug 26, 2019
@DHowett-MSFT DHowett-MSFT added Area-VT Virtual Terminal sequence support Help Wanted We encourage anyone to jump in on these. Issue-Bug It either shouldn't be doing this or needs an investigation. Product-Conhost For issues in the Console codebase and removed Needs-Triage It's a new issue that the core contributor team needs to triage at the next triage meeting labels Aug 26, 2019
@ghost ghost removed the Needs-Tag-Fix Doesn't match tag requirements label Aug 26, 2019
@DHowett-MSFT
Copy link
Contributor

Great analysis. This stems from the fact that we tried, in VT dispatch, to use the "public" console API (or whatever local simulacrum we could get our hands on)... we probably need a private resize for VT to hit.

@DHowett-MSFT DHowett-MSFT added the Area-Output Related to output processing (inserting text into buffer, retrieving buffer text, etc.) label Aug 26, 2019
@DHowett-MSFT DHowett-MSFT added this to the Console Backlog milestone Aug 26, 2019
@zadjii-msft
Copy link
Member

This is also kinda related to #1765/#1795, The DispatchCommon::s_ResizeWindow is a house of horrors. I've wanted to replace it's implementation with a new ConGetSet::PrivateResizeTerminalWindow API for a while now, which would remove the need for us to double resize with SetConsoleScreenBufferInfoEx/SetConsoleWindowSize. It's a mess.

@zadjii-msft
Copy link
Member

zadjii-msft commented Dec 13, 2021

Just as a note to ourselves, this is better, but not fixed:

image

(this is 22519)


On 22590 and main (after the fix for #32), April 2022

The reverse one is fixed, it's the orange one that's broken still.

printf "\e[30;48;5;214m ORANGE \n\e[8;24;80t ORANGE \e[m\n"

Guess: On the resize, we're resetting the ACTIVE attributes to the closest one on the 16 color table. That's resulting in the 256-color orange being emitted with a 16-color color instead.

@zadjii-msft zadjii-msft added the Priority-1 A description (P1) label Dec 13, 2021
@zadjii-msft zadjii-msft modified the milestones: Console Backlog, Backlog Jan 4, 2022
@microsoft-github-policy-service microsoft-github-policy-service bot added the In-PR This issue has a related PR label Apr 30, 2023
microsoft-github-policy-service bot pushed a commit that referenced this issue May 2, 2023
## Summary of the Pull Request

When the screen is resized in ConHost via a VT escape sequence, the
active text attributes could end up being corrupted if they were set to
something that the legacy console APIs didn't support (e.g. RGB colors).
This PR fixes that issue.

## Detailed Description of the Pull Request / Additional comments

The way a resize is implemented is by retrieving the buffer information
with `GetConsoleScreenBufferInfoEx`, updating the size fields, and then
writing the data back out again with `SetConsoleScreenBufferInfoEx`.
However, this also results in the active attributes being updated via
the `wAttributes` field, and that's only capable of representing legacy
console attributes.

We address this by saving the full `TextAttribute` value before it gets
corrupted in the `SetConsoleScreenBufferInfoEx` call, and then restore
it again afterwards.

## Validation Steps Performed

I've added a unit test to verify the attributes are correctly preserved
for all VT resize operations, and I've also manually confirmed the test
case in #2540 is now working as expected.

## PR Checklist
- [x] Closes #2540
- [x] Tests added/passed
- [ ] Documentation updated
- [ ] Schema updated (if necessary)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area-Output Related to output processing (inserting text into buffer, retrieving buffer text, etc.) Area-VT Virtual Terminal sequence support Help Wanted We encourage anyone to jump in on these. In-PR This issue has a related PR Issue-Bug It either shouldn't be doing this or needs an investigation. Priority-1 A description (P1) Product-Conhost For issues in the Console codebase
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants