Repository: microsoft/terminal (language: cpp)

Issue:
Terminal can omit backgrounds, links and underlines on empty cells
**Original Title**: Hyperlink on spaces over-optimization breaks rendering and usability

### Windows Terminal version

1.14.1452.0

### Windows build number

10.0.22000.708

### Other Software

My ActiveScript Shell, just to use JScript to easily script the console, but problem is with VT optimization.


### Steps to reproduce

When a hyperlink contains trailing spaces, they are shown or trimmed depending on the number of trailing spaces.
Starting at 9 spaces or more, they get used as spacing, but are not considered part of the hyperlink, while 8 or less are properly rendered and clickable as part of the hyperlink.

### Expected Behavior

Hyperlinks extending on space characters can be a design decision for consistency and usability, the Terminal should not decide to shorten the number of columns the hyperlink is effectively available on.
Note non-preview build 1.12.10983.0 does not trim the spaces, so this is a new bug introduced in the Preview version 1.14.1452.0.

### Actual Behavior

Current Preview build seems over-agressive in optimizing the spaces, and swallows spaces without abiding by the original VT output of apps.

![image](https://user-images.githubusercontent.com/25664275/172058559-5cc604aa-5779-458b-99b6-9cd922b16287.png)
See how the spaces above the red underlines are different between 8 and 9 spaces.



Discussion / hints:

Fix patch:
diff --git a/src/buffer/out/TextAttribute.cpp b/src/buffer/out/TextAttribute.cpp
index 203242c7f8a..54bb9d9123b 100644
--- a/src/buffer/out/TextAttribute.cpp
+++ b/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
diff --git a/src/buffer/out/TextAttribute.hpp b/src/buffer/out/TextAttribute.hpp
index b85417f2def..f43d5846169 100644
--- a/src/buffer/out/TextAttribute.hpp
+++ b/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;
diff --git a/src/cascadia/UnitTests_TerminalCore/ConptyRoundtripTests.cpp b/src/cascadia/UnitTests_TerminalCore/ConptyRoundtripTests.cpp
index a497a826597..07f246b68ff 100644
--- a/src/cascadia/UnitTests_TerminalCore/ConptyRoundtripTests.cpp
+++ b/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);
+}
diff --git a/src/renderer/vt/paint.cpp b/src/renderer/vt/paint.cpp
index fade9487eb8..42a7a2496d1 100644
--- a/src/renderer/vt/paint.cpp
+++ b/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


Test patch: