Don't remove spaces when printing a new bottom line with a background color

.github/actions/spell-check/whitelist/alphabet.txt

@@ -8,6 +8,7 @@ abcdefghijk
 abcdefghijklmnop
 ABCDEFGHIJKLMNOPQRST
 abcdefghijklmnopqrstuvwxyz
+BBBBBBBBBBBBBBDDDD
 QQQQQQQQQQABCDEFGHIJ
 QQQQQQQQQQABCDEFGHIJKLMNOPQRSTQQQQQQQQQ
 QQQQQQQQQQABCDEFGHIJKLMNOPQRSTQQQQQQQQQQ

src/cascadia/UnitTests_TerminalCore/ConptyRoundtripTests.cpp

@@ -198,6 +198,8 @@ class TerminalCoreUnitTests::ConptyRoundtripTests final
 
     TEST_METHOD(TestResizeWithCookedRead);
 
+    TEST_METHOD(NewLinesAtBottomWithBackground);
+
 private:
     bool _writeCallback(const char* const pch, size_t const cch);
     void _flushFirstFrame();
@@ -2571,7 +2573,9 @@ void ConptyRoundtripTests::TestResizeWithCookedRead()
     // * (0, 0)
     // the rest of the cases are added here for completeness.
 
+    // Don't let the cooked read pollute other tests
     BEGIN_TEST_METHOD_PROPERTIES()
+        TEST_METHOD_PROPERTY(L"IsolationLevel", L"Method")
         TEST_METHOD_PROPERTY(L"Data:dx", L"{-10, -1, 0, 1, -10}")
         TEST_METHOD_PROPERTY(L"Data:dy", L"{-10, -1, 0, 1, 10}")
     END_TEST_METHOD_PROPERTIES()
@@ -2612,3 +2616,128 @@ void ConptyRoundtripTests::TestResizeWithCookedRead()
 
     // By simply reaching the end of this test, we know that we didn't crash. Hooray!
 }
+
+void ConptyRoundtripTests::NewLinesAtBottomWithBackground()
+{
+    BEGIN_TEST_METHOD_PROPERTIES()
+        TEST_METHOD_PROPERTY(L"Data:paintEachNewline", L"{false, true}")
+        TEST_METHOD_PROPERTY(L"Data:spacesToPrint", L"{1, 7, 8, 9, 32}")
+    END_TEST_METHOD_PROPERTIES();
+
+    INIT_TEST_PROPERTY(bool, paintEachNewline, L"If true, call PaintFrame after each pair of lines.");
+    INIT_TEST_PROPERTY(int, spacesToPrint, L"Controls the number of spaces printed after the first '#'");
+
+    // See https://github.com/microsoft/terminal/issues/5502
+    Log::Comment(L"Attempts to emit text to a new bottom line with spaces with "
+                 L"a colored background. When that happens, we should make "
+                 L"sure to still print the spaces, because the information "
+                 L"about their background color is important.");
+    VERIFY_IS_NOT_NULL(_pVtRenderEngine.get());
+
+    auto& g = ServiceLocator::LocateGlobals();
+    auto& renderer = *g.pRender;
+    auto& gci = g.getConsoleInformation();
+    auto& si = gci.GetActiveOutputBuffer();
+    auto& sm = si.GetStateMachine();
+    auto* hostTb = &si.GetTextBuffer();
+    auto* termTb = term->_buffer.get();
+
+    _flushFirstFrame();
+
+    _checkConptyOutput = false;
+    _logConpty = true;
+
+    auto defaultAttrs = si.GetAttributes();
+    auto conhostBlueAttrs = defaultAttrs;
+
+    // Conhost and Terminal store attributes in different bits.
+    conhostBlueAttrs.SetIndexedAttributes({ static_cast<BYTE>(FOREGROUND_GREEN) },
+                                          { static_cast<BYTE>(FOREGROUND_BLUE) });
+    auto terminalBlueAttrs = TextAttribute();
+    terminalBlueAttrs.SetIndexedAttributes({ static_cast<BYTE>(XTERM_GREEN_ATTR) },
+                                           { static_cast<BYTE>(XTERM_BLUE_ATTR) });
+
+    const size_t width = static_cast<size_t>(TerminalViewWidth);
+
+    // We're going to print 4 more rows than the entire height of the viewport,
+    // causing the buffer to circle 4 times. This is 2 extra iterations of the
+    // two lines we're printing per iteration.
+    const auto circledRows = 4;
+    for (auto i = 0; i < (TerminalViewHeight + circledRows) / 2; i++)
+    {
+        // We're printing pairs of lines: ('_' is a space character)
+        //
+        // Line 1 chars: __________________ (break)
+        // Line 1 attrs: DDDDDDDDDDDDDDDDDD (all default attrs)
+        // Line 2 chars: ____#_________#___ (break)
+        // Line 2 attrs: BBBBBBBBBBBBBBDDDD (First spacesToPrint+5 are blue BG, then default attrs)
+        //                    [<----->]
+        //                      This number of spaces controled by spacesToPrint
+        if (i > 0)
+        {
+            sm.ProcessString(L"\r\n");
+        }
+
+        // In WSL:
+        // echo -e "\e[m\r\n\e[44;32m    #         \e[m#"
+
+        sm.ProcessString(L"\x1b[m");
+        sm.ProcessString(L"\r");
+        sm.ProcessString(L"\n");
+        sm.ProcessString(L"\x1b[44;32m");
+        sm.ProcessString(L"    #");
+        sm.ProcessString(std::wstring(spacesToPrint, L' '));
+        sm.ProcessString(L"\x1b[m");
+        sm.ProcessString(L"#");
+
+        if (paintEachNewline)
+        {
+            VERIFY_SUCCEEDED(renderer.PaintFrame());
+        }
+    }
+
+    auto verifyBuffer = [&](const TextBuffer& tb, const til::rectangle viewport) {
+        const auto width = viewport.width<short>();
+        const auto isTerminal = viewport.top() != 0;
+
+        // Conhost and Terminal store attributes in different bits.
+        const auto blueAttrs = isTerminal ? terminalBlueAttrs : conhostBlueAttrs;
+
+        for (short row = 0; row < viewport.bottom<short>() - 2; row++)
+        {
+            Log::Comment(NoThrowString().Format(L"Checking row %d", row));
+            VERIFY_IS_FALSE(tb.GetRowByOffset(row).GetCharRow().WasWrapForced());
+
+            const auto isBlank = (row % 2) == 0;
+            const auto rowCircled = row > (viewport.bottom<short>() - 1 - circledRows);
+            // When the buffer circles, new lines will be initialized using the
+            // current text attributes. Those will be the default-on-default
+            // attributes. All of the Terminal's buffer will use
+            // default-on-default.
+            const auto actualDefaultAttrs = rowCircled || isTerminal ? TextAttribute() : defaultAttrs;
+            Log::Comment(NoThrowString().Format(L"isBlank=%d, rowCircled=%d", isBlank, rowCircled));
+
+            if (isBlank)
+            {
+                TestUtils::VerifyLineContains(tb, { 0, row }, L' ', actualDefaultAttrs, viewport.width<size_t>());
+            }
+            else
+            {
+                auto iter = TestUtils::VerifyLineContains(tb, { 0, row }, L' ', blueAttrs, 4u);
+                TestUtils::VerifyLineContains(iter, L'#', blueAttrs, 1u);
+                TestUtils::VerifyLineContains(iter, L' ', blueAttrs, static_cast<size_t>(spacesToPrint));
+                TestUtils::VerifyLineContains(iter, L'#', TextAttribute(), 1u);
+                TestUtils::VerifyLineContains(iter, L' ', actualDefaultAttrs, static_cast<size_t>(width - 15));
+            }
+        }
+    };
+
+    Log::Comment(L"========== Checking the host buffer state ==========");
+    verifyBuffer(*hostTb, si.GetViewport().ToInclusive());
+
+    Log::Comment(L"Painting the frame");
+    VERIFY_SUCCEEDED(renderer.PaintFrame());
+
+    Log::Comment(L"========== Checking the terminal buffer state ==========");
+    verifyBuffer(*termTb, term->_mutableViewport.ToInclusive());
+}

src/cascadia/UnitTests_TerminalCore/TestUtils.h

@@ -139,4 +139,50 @@ class TerminalCoreUnitTests::TestUtils
         std::replace(escaped.begin(), escaped.end(), L'\x0D', L'\x240D'); // CR
         return escaped;
     }
+
+    template<class... T>
+    static bool VerifyLineContains(TextBufferCellIterator& actual, T&&... expectedContent)
+    {
+        auto expected = OutputCellIterator{ std::forward<T>(expectedContent)... };
+
+        WEX::TestExecution::SetVerifyOutput settings(WEX::TestExecution::VerifyOutputSettings::LogOnlyFailures);
+        auto charsProcessed = 0;
+
+        while (actual && expected)
+        {
+            auto actualChars = actual->Chars();
+            auto expectedChars = expected->Chars();
+            auto actualAttrs = actual->TextAttr();
+            auto expectedAttrs = expected->TextAttr();
+
+            auto mismatched = (actualChars != expectedChars || actualAttrs != expectedAttrs);
+            if (mismatched)
+            {
+                Log::Comment(NoThrowString().Format(
+                    L"Character or attribute at index %d was mismatched", charsProcessed));
+            }
+
+            VERIFY_ARE_EQUAL(expectedChars, actualChars);
+            VERIFY_ARE_EQUAL(expectedAttrs, actualAttrs);
+            if (mismatched)
+            {
+                return false;
+            }
+            ++actual;
+            ++expected;
+            ++charsProcessed;
+        }
+
+        WEX::Logging::Log::Comment(WEX::Common::NoThrowString().Format(
+            L"Successfully validated the chars and attrs of %d cells", charsProcessed));
+        return true;
+    };
+
+    template<class... T>
+    static TextBufferCellIterator VerifyLineContains(const TextBuffer& tb, COORD position, T&&... expectedContent)
+    {
+        auto actual = tb.GetCellLineDataAt(position);
+        VerifyLineContains(actual, std::forward<T>(expectedContent)...);
+        return actual;
+    }
 };

src/renderer/vt/XtermEngine.cpp

@@ -469,6 +469,16 @@ try
     const bool allInvalidated = _invalidMap.all();
     _newBottomLine = !allInvalidated;
 
+    // GH#5502 - keep track of the BG color we had when we emitted this new
+    // bottom line. If the color changes by the time we get to printing that
+    // line, we'll need to make sure that we don't do any optimizations like
+    // _removing spaces_, because the background color of the spaces will be
+    // important information to send to the connected Terminal.
+    if (_newBottomLine)
+    {
+        _newBottomLineBG = _LastBG;
+    }
+
     return S_OK;
 }
 CATCH_RETURN();

src/renderer/vt/paint.cpp

@@ -445,6 +445,12 @@ using namespace Microsoft::Console::Types;
                               (!_clearedAllThisFrame);
     const bool printingBottomLine = coord.Y == _lastViewport.BottomInclusive();
 
+    // GH#5502 - If the background color of the "new bottom line" is different
+    // than when we emitted the line, we can't optimize out the spaces from it.
+    // We'll still need to emit those spaces, so that the connected terminal
+    // will have the same background color on those blank cells.
+    const bool bgMatched = _newBottomLineBG.has_value() ? (_newBottomLineBG.value() == _LastBG) : true;
+
     // If we're not using erase char, but we did erase all at the start of the
     // frame, don't add spaces at the end.
     //
@@ -459,7 +465,7 @@ using namespace Microsoft::Console::Types;
     // on the same line.
     const bool removeSpaces = !lineWrapped && (useEraseChar ||
                                                _clearedAllThisFrame ||
-                                               (_newBottomLine && printingBottomLine));
+                                               (_newBottomLine && printingBottomLine && bgMatched));
     const size_t cchActual = removeSpaces ?
                                  (cchLine - numSpaces) :
                                  cchLine;
@@ -578,6 +584,7 @@ using namespace Microsoft::Console::Types;
     if (printingBottomLine)
     {
         _newBottomLine = false;
+        _newBottomLineBG = std::nullopt;
     }
 
     return S_OK;

src/renderer/vt/vtrenderer.hpp

@@ -153,6 +153,7 @@ namespace Microsoft::Console::Render
         bool _delayedEolWrap{ false };
 
         bool _resizeQuirk{ false };
+        std::optional<COLORREF> _newBottomLineBG{ std::nullopt };
 
         [[nodiscard]] HRESULT _Write(std::string_view const str) noexcept;
         [[nodiscard]] HRESULT _WriteFormattedString(const std::string* const pFormat, ...) noexcept;