Fix the ConPTY extended attributes optimization (#13661)

... which should have never worked in the first place


Quick filing a PR for review. This is the bulk of the actual code changes. Figured it was best to review the conpty changes sooner than later and I can add tests in the morning.

Test cases:
```
printf "\e[7m         test         \e[m\n"
```

```
printf "\e[7m"; printf ' %.0s' $(seq 1 $COLUMNS); printf "\e[m\n"
```

After:
![image](https://user-images.githubusercontent.com/18356694/182478185-6e65ab99-5c27-4772-af3b-2baa22387ec1.png)


Closes #13229


Definitely fixes:
* [x] #13643
* [x] PowerShell/PowerShell#17812

src/buffer/out/TextAttribute.cpp

@@ -357,11 +357,6 @@ void TextAttribute::SetReverseVideo(bool isReversed) noexcept
     WI_UpdateFlag(_wAttrLegacy, COMMON_LVB_REVERSE_VIDEO, isReversed);
 }
 
-ExtendedAttributes TextAttribute::GetExtendedAttributes() const noexcept
-{
-    return _extendedAttrs;
-}
-
 // Routine Description:
 // - swaps foreground and background color
 void TextAttribute::Invert() noexcept

src/buffer/out/TextAttribute.hpp

@@ -109,7 +109,10 @@ class TextAttribute final
     void SetOverlined(bool isOverlined) noexcept;
     void SetReverseVideo(bool isReversed) noexcept;
 
-    ExtendedAttributes GetExtendedAttributes() const noexcept;
+    constexpr ExtendedAttributes GetExtendedAttributes() const noexcept
+    {
+        return _extendedAttrs;
+    }
 
     bool IsHyperlink() const noexcept;
 
@@ -161,6 +164,13 @@ class TextAttribute final
     {
         return WI_IsAnyFlagSet(_wAttrLegacy, COMMON_LVB_GRID_HORIZONTAL | COMMON_LVB_GRID_LVERTICAL | COMMON_LVB_GRID_RVERTICAL | COMMON_LVB_UNDERSCORE);
     }
+    constexpr bool HasAnyExtendedAttributes() const noexcept
+    {
+        return GetExtendedAttributes() != ExtendedAttributes::Normal ||
+               IsAnyGridLineEnabled() ||
+               GetHyperlinkId() != 0 ||
+               IsReverseVideo();
+    }
 
 private:
     static std::array<TextColor, 16> s_legacyForegroundColorMap;

src/cascadia/UnitTests_TerminalCore/ConptyRoundtripTests.cpp

@@ -229,6 +229,9 @@ class TerminalCoreUnitTests::ConptyRoundtripTests final
 
     TEST_METHOD(AltBufferResizeCrash);
 
+    TEST_METHOD(TestNoExtendedAttrsOptimization);
+    TEST_METHOD(TestNoBackgroundAttrsOptimization);
+
 private:
     bool _writeCallback(const char* const pch, const size_t cch);
     void _flushFirstFrame();
@@ -4114,6 +4117,11 @@ void ConptyRoundtripTests::AltBufferResizeCrash()
                  L"particular combination of resizing could crash the terminal."
                  L" This test makes sure we don't.");
 
+    // Anything that resizes the buffer needs IsolationLevel:Method
+    BEGIN_TEST_METHOD_PROPERTIES()
+        TEST_METHOD_PROPERTY(L"IsolationLevel", L"Method")
+    END_TEST_METHOD_PROPERTIES()
+
     auto& g = ServiceLocator::LocateGlobals();
     auto& renderer = *g.pRender;
     auto& gci = g.getConsoleInformation();
@@ -4154,3 +4162,99 @@ void ConptyRoundtripTests::AltBufferResizeCrash()
     Log::Comment(L"Painting the frame");
     VERIFY_SUCCEEDED(renderer.PaintFrame());
 }
+
+void ConptyRoundtripTests::TestNoExtendedAttrsOptimization()
+{
+    Log::Comment(L"We don't want conpty to optimize out runs of spaces that DO "
+                 L"have extended attrs, because EL / ECH don't fill space with "
+                 L"those attributes");
+
+    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->_mainBuffer.get();
+
+    gci.LockConsole(); // Lock must be taken to manipulate alt/main buffer state.
+    auto unlock = wil::scope_exit([&] { gci.UnlockConsole(); });
+
+    _flushFirstFrame();
+
+    _checkConptyOutput = false;
+
+    TextAttribute reverseAttrs{};
+    reverseAttrs.SetReverseVideo(true);
+
+    auto verifyBuffer = [&](const TextBuffer& tb) {
+        auto iter0 = TestUtils::VerifyLineContains(tb, { 0, 0 }, L' ', reverseAttrs, 9u);
+        TestUtils::VerifyExpectedString(L"test", iter0);
+        TestUtils::VerifyLineContains(iter0, L' ', reverseAttrs, 9u);
+
+        TestUtils::VerifyLineContains(tb, { 0, 1 }, L' ', reverseAttrs, static_cast<uint32_t>(TerminalViewWidth));
+    };
+
+    Log::Comment(L"========== Fill test content ==========");
+    sm.ProcessString(L"\x1b[7m         test         \x1b[m\n");
+    sm.ProcessString(L"\x1b[7m");
+    sm.ProcessString(std::wstring(TerminalViewWidth, L' '));
+    sm.ProcessString(L"\x1b[m\n");
+
+    Log::Comment(L"========== Check host buffer ==========");
+    verifyBuffer(*hostTb);
+
+    Log::Comment(L"Painting the frame");
+    VERIFY_SUCCEEDED(renderer.PaintFrame());
+
+    Log::Comment(L"========== Check terminal buffer ==========");
+    verifyBuffer(*termTb);
+}
+
+void ConptyRoundtripTests::TestNoBackgroundAttrsOptimization()
+{
+    Log::Comment(L"Same as above, with BG attrs");
+
+    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->_mainBuffer.get();
+
+    gci.LockConsole(); // Lock must be taken to manipulate alt/main buffer state.
+    auto unlock = wil::scope_exit([&] { gci.UnlockConsole(); });
+
+    _flushFirstFrame();
+
+    _checkConptyOutput = false;
+
+    TextAttribute bgAttrs{};
+    bgAttrs.SetIndexedBackground(TextColor::DARK_WHITE);
+
+    auto verifyBuffer = [&](const TextBuffer& tb) {
+        auto iter0 = TestUtils::VerifyLineContains(tb, { 0, 0 }, L' ', bgAttrs, 9u);
+        TestUtils::VerifyExpectedString(L"test", iter0);
+        TestUtils::VerifyLineContains(iter0, L' ', bgAttrs, 9u);
+
+        TestUtils::VerifyLineContains(tb, { 0, 1 }, L' ', bgAttrs, static_cast<uint32_t>(TerminalViewWidth));
+    };
+
+    Log::Comment(L"========== Fill test content ==========");
+    sm.ProcessString(L"\x1b[47m         test         \x1b[m\n");
+    sm.ProcessString(L"\x1b[47m");
+    sm.ProcessString(std::wstring(TerminalViewWidth, L' '));
+    sm.ProcessString(L"\x1b[m\n");
+
+    Log::Comment(L"========== Check host buffer ==========");
+    verifyBuffer(*hostTb);
+
+    Log::Comment(L"Painting the frame");
+    VERIFY_SUCCEEDED(renderer.PaintFrame());
+
+    Log::Comment(L"========== Check terminal buffer ==========");
+    verifyBuffer(*termTb);
+}

src/renderer/vt/paint.cpp

@@ -424,10 +424,15 @@ using namespace Microsoft::Console::Types;
     // the inbox telnet client doesn't understand the Erase Character sequence,
     // and it uses xterm-ascii. This ensures that xterm and -256color consumers
     // get the enhancements, and telnet isn't broken.
+    //
+    // GH#13229: ECH and EL don't fill the space with "meta" attributes like
+    // underline, reverse video, hyperlinks, etc. If these spaces had those
+    // attrs, then don't try and optimize them out.
     const auto optimalToUseECH = numSpaces > ERASE_CHARACTER_STRING_LENGTH;
     const auto useEraseChar = (optimalToUseECH) &&
                               (!_newBottomLine) &&
-                              (!_clearedAllThisFrame);
+                              (!_clearedAllThisFrame) &&
+                              (!_lastTextAttributes.HasAnyExtendedAttributes());
     const auto printingBottomLine = coord.Y == _lastViewport.BottomInclusive();
 
     // GH#5502 - If the background color of the "new bottom line" is different