Improve conpty rendering of default colors in legacy apps

src/buffer/out/TextAttribute.cpp

@@ -5,16 +5,31 @@
 #include "TextAttribute.hpp"
 #include "../../inc/conattrs.hpp"
 
+BYTE TextAttribute::s_legacyDefaultForeground = 7;
+BYTE TextAttribute::s_legacyDefaultBackground = 0;
+
 // Routine Description:
-// - Returns a WORD with legacy-style attributes for this textattribute.
+// - Sets the legacy attributes which map to and from the default colors.
 // Parameters:
 // - defaultAttributes: the attribute values to be used for default colors.
 // Return value:
+// - None
+void TextAttribute::SetLegacyDefaultAttributes(const WORD defaultAttributes) noexcept
+{
+    s_legacyDefaultForeground = defaultAttributes & FG_ATTRS;
+    s_legacyDefaultBackground = (defaultAttributes & BG_ATTRS) >> 4;
+}
+
+// Routine Description:
+// - Returns a WORD with legacy-style attributes for this textattribute.
+// Parameters:
+// - None
+// Return value:
 // - a WORD with legacy-style attributes for this textattribute.
-WORD TextAttribute::GetLegacyAttributes(const WORD defaultAttributes) const noexcept
+WORD TextAttribute::GetLegacyAttributes() const noexcept
 {
-    const BYTE fgIndex = _foreground.GetLegacyIndex(defaultAttributes & FG_ATTRS);
-    const BYTE bgIndex = _background.GetLegacyIndex((defaultAttributes & BG_ATTRS) >> 4);
+    const BYTE fgIndex = _foreground.GetLegacyIndex(s_legacyDefaultForeground);
+    const BYTE bgIndex = _background.GetLegacyIndex(s_legacyDefaultBackground);
     const WORD metaAttrs = _wAttrLegacy & META_ATTRS;
     const bool brighten = _foreground.IsIndex16() && IsBold();
     return fgIndex | (bgIndex << 4) | metaAttrs | (brighten ? FOREGROUND_INTENSITY : 0);

src/buffer/out/TextAttribute.hpp

@@ -42,8 +42,8 @@ class TextAttribute final
 
     explicit constexpr TextAttribute(const WORD wLegacyAttr) noexcept :
         _wAttrLegacy{ gsl::narrow_cast<WORD>(wLegacyAttr & META_ATTRS) },
-        _foreground{ gsl::narrow_cast<BYTE>(wLegacyAttr & FG_ATTRS), true },
-        _background{ gsl::narrow_cast<BYTE>((wLegacyAttr & BG_ATTRS) >> 4), true },
+        _foreground{ s_LegacyIndexOrDefault(wLegacyAttr & FG_ATTRS, s_legacyDefaultForeground) },
+        _background{ s_LegacyIndexOrDefault((wLegacyAttr & BG_ATTRS) >> 4, s_legacyDefaultBackground) },
         _extendedAttrs{ ExtendedAttributes::Normal }
     {
         // If we're given lead/trailing byte information with the legacy color, strip it.
@@ -59,7 +59,8 @@ class TextAttribute final
     {
     }
 
-    WORD GetLegacyAttributes(const WORD defaultAttributes = 0x07) const noexcept;
+    static void SetLegacyDefaultAttributes(const WORD defaultAttributes) noexcept;
+    WORD GetLegacyAttributes() const noexcept;
 
     COLORREF CalculateRgbForeground(std::basic_string_view<COLORREF> colorTable,
                                     COLORREF defaultFgColor,
@@ -151,6 +152,14 @@ class TextAttribute final
     COLORREF _GetRgbBackground(std::basic_string_view<COLORREF> colorTable,
                                COLORREF defaultColor) const noexcept;
 
+    static constexpr TextColor s_LegacyIndexOrDefault(const BYTE requestedIndex, const BYTE defaultIndex)
+    {
+        return requestedIndex == defaultIndex ? TextColor{} : TextColor{ requestedIndex, true };
+    }
+
+    static BYTE s_legacyDefaultForeground;
+    static BYTE s_legacyDefaultBackground;
+
     WORD _wAttrLegacy;
     TextColor _foreground;
     TextColor _background;

src/buffer/out/ut_textbuffer/TextAttributeTests.cpp

@@ -21,6 +21,7 @@ class TextAttributeTests
     TEST_METHOD(TestRoundtripExhaustive);
     TEST_METHOD(TestTextAttributeColorGetters);
     TEST_METHOD(TestReverseDefaultColors);
+    TEST_METHOD(TestRoundtripDefaultColors);
 
     static const int COLOR_TABLE_SIZE = 16;
     COLORREF _colorTable[COLOR_TABLE_SIZE];
@@ -117,13 +118,10 @@ void TextAttributeTests::TestRoundtripExhaustive()
 
         auto attr = TextAttribute(wLegacy);
 
-        bool isLegacy = attr.IsLegacy();
-        bool areEqual = (wLegacy == attr.GetLegacyAttributes());
-        if (!(isLegacy && areEqual))
+        if (wLegacy != attr.GetLegacyAttributes())
         {
             Log::Comment(NoThrowString().Format(
                 L"Failed on wLegacy=0x%x", wLegacy));
-            VERIFY_IS_TRUE(attr.IsLegacy());
             VERIFY_ARE_EQUAL(wLegacy, attr.GetLegacyAttributes());
         }
     }
@@ -205,3 +203,44 @@ void TextAttributeTests::TestReverseDefaultColors()
     VERIFY_ARE_EQUAL(green, attr._GetRgbBackground(view, _defaultBg));
     VERIFY_ARE_EQUAL(green, attr.CalculateRgbBackground(view, _defaultFg, _defaultBg));
 }
+
+void TextAttributeTests::TestRoundtripDefaultColors()
+{
+    // Set the legacy default colors to yellow on blue.
+    const BYTE fgLegacyDefault = FOREGROUND_RED;
+    const BYTE bgLegacyDefault = BACKGROUND_BLUE;
+    TextAttribute::SetLegacyDefaultAttributes(fgLegacyDefault | bgLegacyDefault);
+
+    WORD legacyAttribute;
+    TextAttribute textAttribute;
+
+    Log::Comment(L"Foreground legacy default index should map to default text color.");
+    legacyAttribute = fgLegacyDefault | BACKGROUND_GREEN;
+    textAttribute.SetDefaultForeground();
+    textAttribute.SetIndexedBackground256(BACKGROUND_GREEN >> 4);
+    VERIFY_ARE_EQUAL(textAttribute, TextAttribute{ legacyAttribute });
+
+    Log::Comment(L"Default foreground text color should map back to legacy default index.");
+    VERIFY_ARE_EQUAL(legacyAttribute, textAttribute.GetLegacyAttributes());
+
+    Log::Comment(L"Background legacy default index should map to default text color.");
+    legacyAttribute = FOREGROUND_GREEN | bgLegacyDefault;
+    textAttribute.SetIndexedForeground256(FOREGROUND_GREEN);
+    textAttribute.SetDefaultBackground();
+    VERIFY_ARE_EQUAL(textAttribute, TextAttribute{ legacyAttribute });
+
+    Log::Comment(L"Default background text color should map back to legacy default index.");
+    VERIFY_ARE_EQUAL(legacyAttribute, textAttribute.GetLegacyAttributes());
+
+    Log::Comment(L"Foreground and background legacy defaults should map to default text colors.");
+    legacyAttribute = fgLegacyDefault | bgLegacyDefault;
+    textAttribute.SetDefaultForeground();
+    textAttribute.SetDefaultBackground();
+    VERIFY_ARE_EQUAL(textAttribute, TextAttribute{ legacyAttribute });
+
+    Log::Comment(L"Default foreground and background text colors should map back to legacy defaults.");
+    VERIFY_ARE_EQUAL(legacyAttribute, textAttribute.GetLegacyAttributes());
+
+    // Reset the legacy default colors to white on black.
+    TextAttribute::SetLegacyDefaultAttributes(FOREGROUND_RED | FOREGROUND_GREEN | FOREGROUND_BLUE);
+}

src/host/consoleInformation.cpp

@@ -356,8 +356,8 @@ Microsoft::Console::CursorBlinker& CONSOLE_INFORMATION::GetCursorBlinker() noexc
 }
 
 // Method Description:
-// - Generates a CHAR_INFO for this output cell, using our
-//      GenerateLegacyAttributes method to generate the legacy style attributes.
+// - Generates a CHAR_INFO for this output cell, using the TextAttribute
+//      GetLegacyAttributes method to generate the legacy style attributes.
 // Arguments:
 // - cell: The cell to get the CHAR_INFO from
 // Return Value:
@@ -371,8 +371,7 @@ CHAR_INFO CONSOLE_INFORMATION::AsCharInfo(const OutputCellView& cell) const noex
     //    use gci to look up the correct legacy attributes to use
     //    (for mapping RGB values to the nearest table value)
     const auto& attr = cell.TextAttr();
-    ci.Attributes = GenerateLegacyAttributes(attr);
-    ;
+    ci.Attributes = attr.GetLegacyAttributes();
     ci.Attributes |= cell.DbcsAttr().GeneratePublicApiAttributeFormat();
     return ci;
 }

src/host/getset.cpp

@@ -881,7 +881,7 @@ void ApiRoutines::GetLargestConsoleWindowSizeImpl(const SCREEN_INFORMATION& cont
                                                       (target.Y == -currentBufferDimensions.Y);
             const bool noClipProvided = clip == std::nullopt;
             const bool fillIsBlank = (fillCharacter == UNICODE_SPACE) &&
-                                     (fillAttribute == gci.GenerateLegacyAttributes(buffer.GetAttributes()));
+                                     (fillAttribute == buffer.GetAttributes().GetLegacyAttributes());
 
             if (sourceIsWholeBuffer && targetIsNegativeBufferHeight && noClipProvided && fillIsBlank)
             {

src/host/output.cpp

@@ -41,7 +41,7 @@ using namespace Microsoft::Console::Interactivity;
     NTSTATUS Status = SCREEN_INFORMATION::CreateInstance(gci.GetWindowSize(),
                                                          fiFont,
                                                          gci.GetScreenBufferSize(),
-                                                         gci.GetDefaultAttributes(),
+                                                         TextAttribute{},
                                                          TextAttribute{ gci.GetPopupFillAttribute() },
                                                          gci.GetCursorSize(),
                                                          &gci.ScreenBuffers);
@@ -148,8 +148,7 @@ std::vector<WORD> ReadOutputAttributes(const SCREEN_INFORMATION& screenInfo,
     // While we haven't read enough cells yet and the iterator is still valid (hasn't reached end of buffer)
     while (amountRead < amountToRead && it)
     {
-        const auto& gci = ServiceLocator::LocateGlobals().getConsoleInformation();
-        const auto legacyAttributes = gci.GenerateLegacyAttributes(it->TextAttr());
+        const auto legacyAttributes = it->TextAttr().GetLegacyAttributes();
 
         // If the first thing we read is trailing, pad with a space.
         // OR If the last thing we read is leading, pad with a space.

src/host/screenInfo.cpp

@@ -356,8 +356,8 @@ void SCREEN_INFORMATION::GetScreenBufferInformation(_Out_ PCOORD pcoordSize,
 
     *psrWindow = _viewport.ToInclusive();
 
-    *pwAttributes = gci.GenerateLegacyAttributes(GetAttributes());
-    *pwPopupAttributes = gci.GenerateLegacyAttributes(_PopupAttributes);
+    *pwAttributes = GetAttributes().GetLegacyAttributes();
+    *pwPopupAttributes = _PopupAttributes.GetLegacyAttributes();
 
     // the copy length must be constant for now to keep OACR happy with buffer overruns.
     for (size_t i = 0; i < COLOR_TABLE_SIZE; i++)
@@ -572,8 +572,6 @@ void SCREEN_INFORMATION::NotifyAccessibilityEventing(const short sStartX,
                                                      const short sEndX,
                                                      const short sEndY)
 {
-    const CONSOLE_INFORMATION& gci = ServiceLocator::LocateGlobals().getConsoleInformation();
-
     // Fire off a winevent to let accessibility apps know what changed.
     if (IsActiveScreenBuffer())
     {
@@ -586,7 +584,7 @@ void SCREEN_INFORMATION::NotifyAccessibilityEventing(const short sStartX,
             {
                 const auto cellData = GetCellDataAt({ sStartX, sStartY });
                 const LONG charAndAttr = MAKELONG(Utf16ToUcs2(cellData->Chars()),
-                                                  gci.GenerateLegacyAttributes(cellData->TextAttr()));
+                                                  cellData->TextAttr().GetLegacyAttributes());
                 _pAccessibilityNotifier->NotifyConsoleUpdateSimpleEvent(MAKELONG(sStartX, sStartY),
                                                                         charAndAttr);
             }

src/host/settings.cpp

@@ -349,6 +349,10 @@ void Settings::Validate()
         }
     }
 
+    // At this point the default fill attributes are fully initialized
+    // so we can pass on the final colors to the TextAttribute class.
+    TextAttribute::SetLegacyDefaultAttributes(_wFillAttribute);
+
     FAIL_FAST_IF(!(_dwWindowSize.X > 0));
     FAIL_FAST_IF(!(_dwWindowSize.Y > 0));
     FAIL_FAST_IF(!(_dwScreenBufferSize.X > 0));
@@ -757,19 +761,6 @@ COLORREF Settings::GetColorTableEntry(const size_t index) const
     return _colorTable.at(index);
 }
 
-// Routine Description:
-// - Generates a legacy attribute from the given TextAttributes.
-//     This needs to be a method on the Settings because the generated index
-//     is dependent upon the default fill attributes.
-// Parameters:
-// - attributes - The TextAttributes to generate a legacy attribute for.
-// Return value:
-// - A WORD representing the legacy attributes that most closely represent the given fullcolor attributes.
-WORD Settings::GenerateLegacyAttributes(const TextAttribute attributes) const
-{
-    return attributes.GetLegacyAttributes(_wFillAttribute);
-}
-
 COLORREF Settings::GetCursorColor() const noexcept
 {
     return _CursorColor;
@@ -820,20 +811,6 @@ void Settings::SetDefaultBackgroundColor(const COLORREF defaultBackground) noexc
     _DefaultBackground = defaultBackground;
 }
 
-TextAttribute Settings::GetDefaultAttributes() const noexcept
-{
-    auto attrs = TextAttribute{ _wFillAttribute };
-    if (_DefaultForeground != INVALID_COLOR)
-    {
-        attrs.SetDefaultForeground();
-    }
-    if (_DefaultBackground != INVALID_COLOR)
-    {
-        attrs.SetDefaultBackground();
-    }
-    return attrs;
-}
-
 bool Settings::IsTerminalScrolling() const noexcept
 {
     return _TerminalScrolling;

src/host/settings.hpp

@@ -179,8 +179,6 @@ class Settings
     COLORREF GetDefaultBackgroundColor() const noexcept;
     void SetDefaultBackgroundColor(const COLORREF defaultBackground) noexcept;
 
-    TextAttribute GetDefaultAttributes() const noexcept;
-
     bool IsTerminalScrolling() const noexcept;
     void SetTerminalScrolling(const bool terminalScrollingEnabled) noexcept;
 
@@ -252,7 +250,4 @@ class Settings
     COLORREF _DefaultBackground;
     bool _TerminalScrolling;
     friend class RegistrySerialization;
-
-public:
-    WORD GenerateLegacyAttributes(const TextAttribute attributes) const;
 };

src/host/ut_host/ScreenBufferTests.cpp

@@ -1317,7 +1317,7 @@ void ScreenBufferTests::VtScrollMarginsNewlineColor()
     const COLORREF magenta = RGB(255, 0, 255);
     gci.SetDefaultForegroundColor(yellow);
     gci.SetDefaultBackgroundColor(magenta);
-    const TextAttribute defaultAttrs = gci.GetDefaultAttributes();
+    const TextAttribute defaultAttrs = {};
     si.SetAttributes(defaultAttrs);
 
     Log::Comment(NoThrowString().Format(L"Begin by clearing the screen."));
@@ -2071,7 +2071,7 @@ void ScreenBufferTests::TestAltBufferVtDispatching()
     WI_SetFlag(mainBuffer.OutputMode, ENABLE_VIRTUAL_TERMINAL_PROCESSING);
     // Make sure we're suing the default attributes at the start of the test,
     // Otherwise they could be polluted from a previous test.
-    mainBuffer.SetAttributes(gci.GetDefaultAttributes());
+    mainBuffer.SetAttributes({});
 
     VERIFY_IS_NULL(mainBuffer._psiAlternateBuffer);
     VERIFY_IS_NULL(mainBuffer._psiMainBuffer);
@@ -2121,7 +2121,7 @@ void ScreenBufferTests::TestAltBufferVtDispatching()
         // recall: vt coordinates are (row, column), 1-indexed
         VERIFY_ARE_EQUAL(COORD({ 5, 4 }), altCursor.GetPosition());
 
-        const TextAttribute expectedDefaults = gci.GetDefaultAttributes();
+        const TextAttribute expectedDefaults = {};
         TextAttribute expectedRgb = expectedDefaults;
         expectedRgb.SetBackground(RGB(255, 0, 255));
 
@@ -2194,7 +2194,7 @@ void ScreenBufferTests::SetDefaultsIndividuallyBothDefault()
 
     gci.SetDefaultForegroundColor(yellow);
     gci.SetDefaultBackgroundColor(magenta);
-    si.SetDefaultAttributes(gci.GetDefaultAttributes(), TextAttribute{ gci.GetPopupFillAttribute() });
+    si.SetDefaultAttributes({}, TextAttribute{ gci.GetPopupFillAttribute() });
 
     Log::Comment(NoThrowString().Format(L"Write 6 X's:"));
     Log::Comment(NoThrowString().Format(L"  The first in default-fg on default-bg (yellow on magenta)"));
@@ -2303,7 +2303,7 @@ void ScreenBufferTests::SetDefaultsTogether()
 
     gci.SetDefaultForegroundColor(yellow);
     gci.SetDefaultBackgroundColor(magenta);
-    si.SetDefaultAttributes(gci.GetDefaultAttributes(), TextAttribute{ gci.GetPopupFillAttribute() });
+    si.SetDefaultAttributes({}, TextAttribute{ gci.GetPopupFillAttribute() });
 
     Log::Comment(NoThrowString().Format(L"Write 6 X's:"));
     Log::Comment(NoThrowString().Format(L"  The first in default-fg on default-bg (yellow on magenta)"));
@@ -2378,7 +2378,7 @@ void ScreenBufferTests::ReverseResetWithDefaultBackground()
 
     gci.SetDefaultForegroundColor(INVALID_COLOR);
     gci.SetDefaultBackgroundColor(magenta);
-    si.SetDefaultAttributes(gci.GetDefaultAttributes(), TextAttribute{ gci.GetPopupFillAttribute() });
+    si.SetDefaultAttributes({}, TextAttribute{ gci.GetPopupFillAttribute() });
 
     Log::Comment(NoThrowString().Format(L"Write 3 X's:"));
     Log::Comment(NoThrowString().Format(L"  The first in default-attr on default color (magenta)"));
@@ -2446,7 +2446,7 @@ void ScreenBufferTests::BackspaceDefaultAttrs()
     COLORREF magenta = RGB(255, 0, 255);
 
     gci.SetDefaultBackgroundColor(magenta);
-    si.SetDefaultAttributes(gci.GetDefaultAttributes(), TextAttribute{ gci.GetPopupFillAttribute() });
+    si.SetDefaultAttributes({}, TextAttribute{ gci.GetPopupFillAttribute() });
 
     Log::Comment(NoThrowString().Format(L"Write 2 X's, then backspace one."));
 
@@ -2509,7 +2509,7 @@ void ScreenBufferTests::BackspaceDefaultAttrsWriteCharsLegacy()
     COLORREF magenta = RGB(255, 0, 255);
 
     gci.SetDefaultBackgroundColor(magenta);
-    si.SetDefaultAttributes(gci.GetDefaultAttributes(), TextAttribute{ gci.GetPopupFillAttribute() });
+    si.SetDefaultAttributes({}, TextAttribute{ gci.GetPopupFillAttribute() });
 
     Log::Comment(NoThrowString().Format(L"Write 2 X's, then backspace one."));
 
@@ -2577,7 +2577,7 @@ void ScreenBufferTests::BackspaceDefaultAttrsInPrompt()
     COLORREF magenta = RGB(255, 0, 255);
 
     gci.SetDefaultBackgroundColor(magenta);
-    si.SetDefaultAttributes(gci.GetDefaultAttributes(), TextAttribute{ gci.GetPopupFillAttribute() });
+    si.SetDefaultAttributes({}, TextAttribute{ gci.GetPopupFillAttribute() });
     TextAttribute expectedDefaults{};
 
     Log::Comment(NoThrowString().Format(L"Write 3 X's, move to the left, then delete-char the second."));

src/inc/test/CommonState.hpp

@@ -100,7 +100,7 @@ class CommonState
         THROW_IF_FAILED(SCREEN_INFORMATION::CreateInstance(coordWindowSize,
                                                            *m_pFontInfo,
                                                            coordScreenBufferSize,
-                                                           gci.GetDefaultAttributes(),
+                                                           TextAttribute{},
                                                            TextAttribute{ FOREGROUND_BLUE | FOREGROUND_INTENSITY | BACKGROUND_RED },
                                                            uiCursorSize,
                                                            &gci.pCurrentScreenBuffer));
@@ -157,7 +157,7 @@ class CommonState
 
         UINT uiCursorSize = 12;
 
-        auto initialAttributes = useDefaultAttributes ? gci.GetDefaultAttributes() :
+        auto initialAttributes = useDefaultAttributes ? TextAttribute{} :
                                                         TextAttribute{ FOREGROUND_BLUE | FOREGROUND_GREEN | BACKGROUND_RED | BACKGROUND_INTENSITY };
 
         m_backupTextBufferInfo.swap(gci.pCurrentScreenBuffer->_textBuffer);

src/interactivity/win32/menu.cpp

@@ -583,8 +583,11 @@ void Menu::s_PropertiesUpdate(PCONSOLE_STATE_INFO pStateInfo)
     gci.SetDefaultForegroundColor(pStateInfo->DefaultForeground);
     gci.SetDefaultBackgroundColor(pStateInfo->DefaultBackground);
 
+    // Make sure the updated fill attributes are passed on to the TextAttribute class.
+    TextAttribute::SetLegacyDefaultAttributes(pStateInfo->ScreenAttributes);
+
     // Set the screen info's default text attributes to defaults -
-    ScreenInfo.SetDefaultAttributes(gci.GetDefaultAttributes(), TextAttribute{ gci.GetPopupFillAttribute() });
+    ScreenInfo.SetDefaultAttributes({}, TextAttribute{ gci.GetPopupFillAttribute() });
 
     CommandHistory::s_ResizeAll(pStateInfo->HistoryBufferSize);
     gci.SetNumberOfHistoryBuffers(pStateInfo->NumberOfHistoryBuffers);