Repository: microsoft/terminal (language: cpp)

Issue:
Certain SGR attributes are not preserved after a resize sequence
# Environment

```none
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](https://user-images.githubusercontent.com/4181424/63697684-4c363480-c815-11e9-87c8-d652a0ca6b89.png)

# Cause

The resize escape sequence is handled by the [`DispatchCommon::s_ResizeWindow`](https://github.com/microsoft/terminal/blob/9b92986b49bed8cc41fde4d6ef080921c41e6d9e/src/terminal/adapter/DispatchCommon.cpp#L20-L68) 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`](https://github.com/microsoft/terminal/blob/594a7e4501d1108267388197ec187ca77f82756b/src/terminal/adapter/adaptDispatch.cpp#L1078-L1095) 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.


Discussion / hints:
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.
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.
Just as a note to ourselves, this is better, but not fixed:

![image](https://user-images.githubusercontent.com/18356694/145857118-ec4127f4-daca-49fa-9aca-f59bb2b79612.png)

(this is 22519)

<hr> 

_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. 

```sh
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. 

Fix patch:
diff --git a/src/host/outputStream.cpp b/src/host/outputStream.cpp
index 2bf21f7dbfa..84ffcb7be98 100644
--- a/src/host/outputStream.cpp
+++ b/src/host/outputStream.cpp
@@ -340,6 +340,14 @@ bool ConhostInternalGetSet::ResizeWindow(const til::CoordType sColumns, const ti
     auto api = ServiceLocator::LocateGlobals().api;
     auto& screenInfo = _io.GetActiveOutputBuffer();
 
+    // We need to save the attributes separately, since the wAttributes field in
+    // CONSOLE_SCREEN_BUFFER_INFOEX is not capable of representing the extended
+    // attribute values, and can end up corrupting that data when restored.
+    const auto attributes = screenInfo.GetTextBuffer().GetCurrentAttributes();
+    const auto restoreAttributes = wil::scope_exit([&] {
+        screenInfo.GetTextBuffer().SetCurrentAttributes(attributes);
+    });
+
     CONSOLE_SCREEN_BUFFER_INFOEX csbiex = { 0 };
     csbiex.cbSize = sizeof(CONSOLE_SCREEN_BUFFER_INFOEX);
     api->GetConsoleScreenBufferInfoExImpl(screenInfo, csbiex);
diff --git a/src/host/ut_host/ScreenBufferTests.cpp b/src/host/ut_host/ScreenBufferTests.cpp
index 48d4e277190..87b7dd57a91 100644
--- a/src/host/ut_host/ScreenBufferTests.cpp
+++ b/src/host/ut_host/ScreenBufferTests.cpp
@@ -123,6 +123,7 @@ class ScreenBufferTests
     TEST_METHOD(VtResize);
     TEST_METHOD(VtResizeComprehensive);
     TEST_METHOD(VtResizeDECCOLM);
+    TEST_METHOD(VtResizePreservingAttributes);
 
     TEST_METHOD(VtSoftResetCursorPosition);
 
@@ -1395,6 +1396,52 @@ void ScreenBufferTests::VtResizeDECCOLM()
     VERIFY_ARE_EQUAL(80, newViewWidth);
 }
 
+void ScreenBufferTests::VtResizePreservingAttributes()
+{
+    // Run this test in isolation - for one reason or another, this breaks other tests.
+    BEGIN_TEST_METHOD_PROPERTIES()
+        TEST_METHOD_PROPERTY(L"IsolationLevel", L"Method")
+        TEST_METHOD_PROPERTY(L"Data:resizeType", L"{0, 1}")
+    END_TEST_METHOD_PROPERTIES()
+
+    unsigned resizeType;
+    VERIFY_SUCCEEDED(TestData::TryGetValue(L"resizeType", resizeType));
+
+    auto& gci = ServiceLocator::LocateGlobals().getConsoleInformation();
+    auto& si = gci.GetActiveOutputBuffer().GetActiveBuffer();
+    auto& stateMachine = si.GetStateMachine();
+    WI_SetFlag(si.OutputMode, ENABLE_VIRTUAL_TERMINAL_PROCESSING);
+
+    // Set the attributes to something not supported by the legacy console.
+    auto testAttr = TextAttribute{ RGB(12, 34, 56), RGB(78, 90, 12) };
+    testAttr.SetCrossedOut(true);
+    testAttr.SetDoublyUnderlined(true);
+    testAttr.SetItalic(true);
+    si.GetTextBuffer().SetCurrentAttributes(testAttr);
+
+    switch (resizeType)
+    {
+    case 0:
+        Log::Comment(L"Resize the screen with CSI 8 t");
+        stateMachine.ProcessString(L"\x1b[8;24;132t"); // Set width to 132
+        VERIFY_ARE_EQUAL(132, si.GetTextBuffer().GetSize().Width());
+        stateMachine.ProcessString(L"\x1b[8;24;80t"); // Set width to 80
+        VERIFY_ARE_EQUAL(80, si.GetTextBuffer().GetSize().Width());
+        break;
+    case 1:
+        Log::Comment(L"Resize the screen with DECCOLM");
+        stateMachine.ProcessString(L"\x1b[?40h"); // Allow DECCOLM
+        stateMachine.ProcessString(L"\x1b[?3h"); // Set width to 132
+        VERIFY_ARE_EQUAL(132, si.GetTextBuffer().GetSize().Width());
+        stateMachine.ProcessString(L"\x1b[?3l"); // Set width to 80
+        VERIFY_ARE_EQUAL(80, si.GetTextBuffer().GetSize().Width());
+        break;
+    }
+
+    Log::Comment(L"Attributes should be preserved after resize");
+    VERIFY_ARE_EQUAL(testAttr, si.GetTextBuffer().GetCurrentAttributes());
+}
+
 void ScreenBufferTests::VtSoftResetCursorPosition()
 {
     auto& gci = ServiceLocator::LocateGlobals().getConsoleInformation();


Test patch: