Improve the legacy color conversions (#6358)

This PR provides a faster algorithm for converting 8-bit and 24-bit
colors into the 4-bit legacy values that are required by the Win32
console APIs. It also fixes areas of the code that were incorrectly
using a simple 16-color conversion that didn't handle 8-bit and 24-bit
values.

The faster conversion algorithm should be an improvement for issues #783
and #3950.

One of the main points of this PR was to fix the
`ReadConsoleOutputAttribute` API, which was using a simplified legacy
color conversion (the original `TextAttribute:GetLegacyAttributes`
method), which could only handle values from the 16-color table. RGB
values, and colors from the 256-color table, would be mapped to
completely nonsensical values. This API has now been updated to use the
more correct `Settings::GenerateLegacyAttributes` method.

But there were also a couple of other places in the code that were using
`GetLegacyAttributes` when they really had no reason to be working with
legacy attributes at all. This could result in colors being downgraded
to 4-bit values (often badly, as explained above), when the code was
already perfectly capable of displaying the full 24-bits.

This included the fill colors in the IME composer (in `ConsoleImeInfo`),
and the construction of the highlighting colors in the color
search/selection handler (`Selection::_HandleColorSelection`). I also
got rid of some legacy attribute code in the `Popup` class, which was
originally intended to update colors below the popup when the settings
changed, but actually caused more problems than it solved.

The other major goal of this PR was to improve the performance of the
`GenerateLegacyAttributes` method, since the existing implementation
could be quite slow when dealing with RGB values.

The simple cases are handled much the same as they were before. For an
`IsDefault` color, we get the default index from the
`Settings::_wFillAttribute` field. For an `IsIndex16` color, the index
can just be returned as is.

For an `IsRgb` color, the RGB components are compressed down to 8 bits
(3 red, 3 green, 2 blue), simply by dropping the least significant bits.
This 8-bit value is then used to lookup a representative 16-color value
from a hard-coded table. An `IsIndex256` color is also converted with a
lookup table, just using the existing 8-bit index.

The RGB mapping table was calculated by taking each compressed 8-bit
color, and picking a entry from the _Campbell_ palette that best
approximated that color. This was done by looking at a range of 24-bit
colors that mapped to the 8-bit value, finding the best _Campbell_ match
for each of them (using a [CIEDE2000] color difference calculation), and
then the most common match became the index that the 8-bit value would
map to.

The 256-color table was just a simpler version of this process. For each
entry in the table, we take the default RGB palette value, and find it's
closest match in the _Campbell_ palette.

Because these tables are hard-coded, the results won't adjust to changes
in the palette. However, they should still produce reasonable results
for palettes that follow the standard ANSI color range. And since
they're only a very loose approximation of the colors anyway, the exact
value really isn't that important.

That said, I have tried to make sure that if you take an RGB value for a
particular index in a reasonable color scheme, then the legacy color
mapped from that value should ideally match the same index. This will
never be possible for all color schemes, but I have tweaked a few of the
table entries to improve the results for some of the common schemes.

One other point worth making regarding the hard-coded tables: even if we
wanted to take the active palette into account, that wouldn't actually
be possible over a conpty connection, because we can't easily know what
color scheme the client application is using. At least this way the
results in conhost are guaranteed to be the same as in the Windows
Terminal.

[CIEDE2000]: https://en.wikipedia.org/wiki/Color_difference#CIEDE2000

## Validation Steps Performed

This code still passes the `TextAttributeTests` that check the basic
`GetLegacyAttribute` behaviour and verify the all legacy attributes
roundtrip correctly. However, some of the values in the `RgbColorTests`
had to be updated, since we're now intentionally returning different
values as a result of the changes to the RGB conversion algorithm.

I haven't added additional unit tests, but I have done a lot of manual
testing to see how well the new algorithm works with a range of colors
and a variety of different color schemes. It's not perfect in every
situation, but I think it works well enough for the purpose it serves.

I've also confirmed that the issues reported in #5940 and #6247 are now
fixed by these changes. 

Closes #5940 
Closes #6247

.github/actions/spell-check/expect/expect.txt

@@ -1976,6 +1976,7 @@ robomac
 roundtrip
 ROWSTOSCROLL
 RRF
+RRRGGGBB
 rtcore
 RTEXT
 rtf

src/buffer/out/TextAttribute.cpp

@@ -5,14 +5,24 @@
 #include "TextAttribute.hpp"
 #include "../../inc/conattrs.hpp"
 
-bool TextAttribute::IsLegacy() const noexcept
+// Routine Description:
+// - Returns a WORD with legacy-style attributes for this textattribute.
+// Parameters:
+// - defaultAttributes: the attribute values to be used for default colors.
+// Return value:
+// - a WORD with legacy-style attributes for this textattribute.
+WORD TextAttribute::GetLegacyAttributes(const WORD defaultAttributes) const noexcept
 {
- return _foreground.IsLegacy() && _background.IsLegacy();
+ const BYTE fgIndex = _foreground.GetLegacyIndex(defaultAttributes & FG_ATTRS);
+ const BYTE bgIndex = _background.GetLegacyIndex((defaultAttributes & BG_ATTRS) >> 4);
+ const WORD metaAttrs = _wAttrLegacy & META_ATTRS;
+ const bool brighten = _foreground.IsIndex16() && IsBold();
+ return fgIndex | (bgIndex << 4) | metaAttrs | (brighten ? FOREGROUND_INTENSITY : 0);
 }
 
-bool TextAttribute::IsHighColor() const noexcept
+bool TextAttribute::IsLegacy() const noexcept
 {
- return _foreground.IsHighColor() || _background.IsHighColor();
+ return _foreground.IsLegacy() && _background.IsLegacy();
 }
 
 // Arguments:
@@ -241,21 +251,6 @@ void TextAttribute::SetDefaultBackground() noexcept
  _background = TextColor();
 }
 
-// Method Description:
-// - Returns true if this attribute indicates its foreground is the "default"
-// foreground. Its _rgbForeground will contain the actual value of the
-// default foreground. If the default colors are ever changed, this method
-// should be used to identify attributes with the default fg value, and
-// update them accordingly.
-// Arguments:
-// - <none>
-// Return Value:
-// - true iff this attribute indicates it's the "default" foreground color.
-bool TextAttribute::ForegroundIsDefault() const noexcept
-{
- return _foreground.IsDefault();
-}
-
 // Method Description:
 // - Returns true if this attribute indicates its background is the "default"
 // background. Its _rgbBackground will contain the actual value of the

src/buffer/out/TextAttribute.hpp

@@ -59,38 +59,7 @@ class TextAttribute final
  {
  }
 
- WORD GetLegacyAttributes() const noexcept
- {
- const BYTE fg = (_foreground.GetIndex() & FG_ATTRS);
- const BYTE bg = (_background.GetIndex() << 4) & BG_ATTRS;
- const WORD meta = (_wAttrLegacy & META_ATTRS);
- const bool brighten = _foreground.IsIndex16() && IsBold();
- return (fg | bg | meta) | (brighten ? FOREGROUND_INTENSITY : 0);
- }
-
- // Method Description:
- // - Returns a WORD with legacy-style attributes for this textattribute.
- // If either the foreground or background of this textattribute is not
- // a legacy attribute, then instead use the provided default index as
- // the value for that component.
- // Arguments:
- // - defaultFgIndex: the BYTE to use as the index for the foreground, should
- // the foreground not be a legacy style attribute.
- // - defaultBgIndex: the BYTE to use as the index for the background, should
- // the background not be a legacy style attribute.
- // Return Value:
- // - a WORD with legacy-style attributes for this textattribute.
- WORD GetLegacyAttributes(const BYTE defaultFgIndex,
- const BYTE defaultBgIndex) const noexcept
- {
- const BYTE fgIndex = _foreground.IsLegacy() ? _foreground.GetIndex() : defaultFgIndex;
- const BYTE bgIndex = _background.IsLegacy() ? _background.GetIndex() : defaultBgIndex;
- const BYTE fg = (fgIndex & FG_ATTRS);
- const BYTE bg = (bgIndex << 4) & BG_ATTRS;
- const WORD meta = (_wAttrLegacy & META_ATTRS);
- const bool brighten = _foreground.IsIndex16() && IsBold();
- return (fg | bg | meta) | (brighten ? FOREGROUND_INTENSITY : 0);
- }
+ WORD GetLegacyAttributes(const WORD defaultAttributes = 0x07) const noexcept;
 
  COLORREF CalculateRgbForeground(std::basic_string_view<COLORREF> colorTable,
  COLORREF defaultFgColor,
@@ -119,7 +88,6 @@ class TextAttribute final
  friend constexpr bool operator!=(const WORD& legacyAttr, const TextAttribute& attr) noexcept;
 
  bool IsLegacy() const noexcept;
- bool IsHighColor() const noexcept;
  bool IsBold() const noexcept;
  bool IsItalic() const noexcept;
  bool IsBlinking() const noexcept;
@@ -149,7 +117,6 @@ class TextAttribute final
  void SetDefaultForeground() noexcept;
  void SetDefaultBackground() noexcept;
 
- bool ForegroundIsDefault() const noexcept;
  bool BackgroundIsDefault() const noexcept;
 
  void SetStandardErase() noexcept;

src/buffer/out/TextColor.cpp

@@ -4,16 +4,57 @@
 #include "precomp.h"
 #include "TextColor.h"
 
+// clang-format off
+
+// A table mapping 8-bit RGB colors, in the form RRRGGGBB,
+// down to one of the 16 colors in the legacy palette.
+constexpr std::array<BYTE, 256> CompressedRgbToIndex16 = {
+ 0, 1, 1, 9, 0, 0, 1, 1, 2, 1, 1, 1, 2, 8, 1, 9,
+ 2, 2, 3, 3, 2, 2, 11, 3, 10, 10, 11, 11, 10, 10, 10, 11,
+ 0, 5, 1, 1, 0, 0, 1, 1, 8, 1, 1, 1, 2, 8, 1, 9,
+ 2, 2, 3, 3, 2, 2, 11, 3, 10, 10, 10, 11, 10, 10, 10, 11,
+ 5, 5, 5, 1, 4, 5, 1, 1, 8, 8, 1, 9, 2, 8, 9, 9,
+ 2, 2, 3, 3, 2, 2, 11, 3, 10, 10, 11, 11, 10, 10, 10, 11,
+ 4, 5, 5, 1, 4, 5, 5, 1, 8, 5, 5, 1, 8, 8, 9, 9,
+ 2, 2, 8, 9, 10, 2, 11, 3, 10, 10, 11, 11, 10, 10, 10, 11,
+ 4, 13, 5, 5, 4, 13, 5, 5, 4, 13, 13, 13, 6, 8, 13, 9,
+ 6, 8, 8, 9, 10, 10, 11, 3, 10, 10, 11, 11, 10, 10, 10, 11,
+ 4, 13, 13, 13, 4, 13, 13, 13, 4, 12, 13, 13, 6, 12, 13, 13,
+ 6, 6, 8, 9, 6, 6, 7, 7, 10, 14, 14, 7, 10, 10, 14, 11,
+ 4, 12, 13, 13, 4, 12, 13, 13, 4, 12, 13, 13, 6, 12, 12, 13,
+ 6, 6, 12, 7, 6, 6, 7, 7, 6, 14, 14, 7, 14, 14, 14, 15,
+ 12, 12, 13, 13, 12, 12, 13, 13, 12, 12, 12, 13, 12, 12, 12, 13,
+ 6, 12, 12, 7, 6, 6, 7, 7, 6, 14, 14, 7, 14, 14, 14, 15
+};
+
+// A table mapping indexed colors from the 256-color palette,
+// down to one of the 16 colors in the legacy palette.
+constexpr std::array<BYTE, 256> Index256ToIndex16 = {
+ 0, 1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13, 14, 15,
+ 0, 1, 1, 1, 9, 9, 2, 1, 1, 1, 1, 1, 2, 2, 3, 3,
+ 3, 3, 2, 2, 11, 11, 3, 3, 10, 10, 11, 11, 11, 11, 10, 10,
+ 10, 10, 11, 11, 5, 5, 5, 5, 1, 1, 8, 8, 1, 1, 9, 9,
+ 2, 2, 3, 3, 3, 3, 2, 2, 11, 11, 3, 3, 10, 10, 11, 11,
+ 11, 11, 10, 10, 10, 10, 11, 11, 4, 13, 5, 5, 5, 5, 4, 13,
+ 13, 13, 13, 13, 6, 8, 8, 8, 9, 9, 10, 10, 11, 11, 3, 3,
+ 10, 10, 11, 11, 11, 11, 10, 10, 10, 10, 11, 11, 4, 13, 13, 13,
+ 13, 13, 4, 12, 13, 13, 13, 13, 6, 6, 8, 8, 9, 9, 6, 6,
+ 7, 7, 7, 7, 10, 14, 14, 14, 7, 7, 10, 10, 14, 14, 11, 11,
+ 4, 12, 13, 13, 13, 13, 4, 12, 13, 13, 13, 13, 6, 6, 12, 12,
+ 7, 7, 6, 6, 7, 7, 7, 7, 6, 14, 14, 14, 7, 7, 14, 14,
+ 14, 14, 15, 15, 12, 12, 13, 13, 13, 13, 12, 12, 12, 12, 13, 13,
+ 6, 12, 12, 12, 7, 7, 6, 6, 7, 7, 7, 7, 6, 14, 14, 14,
+ 7, 7, 14, 14, 14, 14, 15, 15, 0, 0, 0, 0, 0, 0, 8, 8,
+ 8, 8, 8, 8, 8, 8, 8, 8, 7, 7, 7, 7, 7, 7, 15, 15
+};
+
+// clang-format on
+
 bool TextColor::IsLegacy() const noexcept
 {
  return IsIndex16() || (IsIndex256() && _index < 16);
 }
 
-bool TextColor::IsHighColor() const noexcept
-{
- return IsRgb() || (IsIndex256() && _index >= 16);
-}
-
 bool TextColor::IsIndex16() const noexcept
 {
  return _meta == ColorType::IsIndex16;
@@ -135,6 +176,37 @@ COLORREF TextColor::GetColor(std::basic_string_view<COLORREF> colorTable,
  }
 }
 
+// Method Description:
+// - Return a legacy index value that best approximates this color.
+// Arguments:
+// - defaultIndex: The index to use for a default color.
+// Return Value:
+// - an index into the 16-color table
+BYTE TextColor::GetLegacyIndex(const BYTE defaultIndex) const noexcept
+{
+ if (IsDefault())
+ {
+ return defaultIndex;
+ }
+ else if (IsIndex16())
+ {
+ return GetIndex();
+ }
+ else if (IsIndex256())
+ {
+ return Index256ToIndex16.at(GetIndex());
+ }
+ else
+ {
+ // We compress the RGB down to an 8-bit value and use that to
+ // lookup a representative 16-color index from a hard-coded table.
+ const BYTE compressedRgb = (_red & 0b11100000) +
+ ((_green >> 3) & 0b00011100) +
+ ((_blue >> 6) & 0b00000011);
+ return CompressedRgbToIndex16.at(compressedRgb);
+ }
+}
+
 // Method Description:
 // - Return a COLORREF containing our stored value. Will return garbage if this
 //attribute is not a RGB attribute.

src/buffer/out/TextColor.h

@@ -78,7 +78,6 @@ struct TextColor
  friend constexpr bool operator!=(const TextColor& a, const TextColor& b) noexcept;
 
  bool IsLegacy() const noexcept;
- bool IsHighColor() const noexcept;
  bool IsIndex16() const noexcept;
  bool IsIndex256() const noexcept;
  bool IsDefault() const noexcept;
@@ -92,6 +91,8 @@ struct TextColor
  const COLORREF defaultColor,
  const bool brighten) const noexcept;
 
+ BYTE GetLegacyIndex(const BYTE defaultIndex) const noexcept;
+
  constexpr BYTE GetIndex() const noexcept
  {
  return _index;

src/host/cmdline.cpp

@@ -26,23 +26,6 @@
 
 #pragma hdrstop
 using Microsoft::Console::Interactivity::ServiceLocator;
-// Routine Description:
-// - This routine is called when the user changes the screen/popup colors.
-// - It goes through the popup structures and changes the saved contents to reflect the new screen/popup colors.
-void CommandLine::UpdatePopups(const TextAttribute& NewAttributes,
- const TextAttribute& NewPopupAttributes,
- const TextAttribute& OldAttributes,
- const TextAttribute& OldPopupAttributes)
-{
- for (auto& popup : _popups)
- {
- try
- {
- popup->UpdateStoredColors(NewAttributes, NewPopupAttributes, OldAttributes, OldPopupAttributes);
- }
- CATCH_LOG();
- }
-}
 
 // Routine Description:
 // - This routine validates a string buffer and returns the pointers of where the strings start within the buffer.

src/host/cmdline.h

@@ -84,11 +84,6 @@ class CommandLine
  bool HasPopup() const noexcept;
  Popup& GetPopup();
 
- void UpdatePopups(const TextAttribute& NewAttributes,
- const TextAttribute& NewPopupAttributes,
- const TextAttribute& OldAttributes,
- const TextAttribute& OldPopupAttributes);
-
  void EndCurrentPopup();
  void EndAllPopups();
 

src/host/conareainfo.cpp

@@ -24,8 +24,8 @@ ConversionAreaBufferInfo::ConversionAreaBufferInfo(const COORD coordBufferSize)
 
 ConversionAreaInfo::ConversionAreaInfo(const COORD bufferSize,
  const COORD windowSize,
- const CHAR_INFO fill,
- const CHAR_INFO popupFill,
+ const TextAttribute& fill,
+ const TextAttribute& popupFill,
  const FontInfo fontInfo) :
  _caInfo{ bufferSize },
  _isHidden{ true },
@@ -37,8 +37,8 @@ ConversionAreaInfo::ConversionAreaInfo(const COORD bufferSize,
  THROW_IF_NTSTATUS_FAILED(SCREEN_INFORMATION::CreateInstance(windowSize,
  fontInfo,
  bufferSize,
- TextAttribute{ fill.Attributes },
- TextAttribute{ popupFill.Attributes },
+ fill,
+ popupFill,
  0,
  &pNewScreen));
 

src/host/conareainfo.h

@@ -42,8 +42,8 @@ class ConversionAreaInfo final
 public:
  ConversionAreaInfo(const COORD bufferSize,
  const COORD windowSize,
- const CHAR_INFO fill,
- const CHAR_INFO popupFill,
+ const TextAttribute& fill,
+ const TextAttribute& popupFill,
  const FontInfo fontInfo);
  ~ConversionAreaInfo() = default;
  ConversionAreaInfo(const ConversionAreaInfo&) = delete;

src/host/conimeinfo.cpp

@@ -146,11 +146,9 @@ void ConsoleImeInfo::ClearAllAreas()
 
  const COORD windowSize = gci.GetActiveOutputBuffer().GetViewport().Dimensions();
 
- CHAR_INFO fill;
- fill.Attributes = gci.GetActiveOutputBuffer().GetAttributes().GetLegacyAttributes();
+ const TextAttribute fill = gci.GetActiveOutputBuffer().GetAttributes();
 
- CHAR_INFO popupFill;
- popupFill.Attributes = gci.GetActiveOutputBuffer().GetPopupAttributes()->GetLegacyAttributes();
+ const TextAttribute popupFill = gci.GetActiveOutputBuffer().GetPopupAttributes();
 
  const FontInfo& fontInfo = gci.GetActiveOutputBuffer().GetCurrentFont();
 

src/host/ft_host/API_RgbColorTests.cpp

@@ -320,21 +320,21 @@ BOOL Validate256GridToLegacy(COORD cursorPosInitial)
 
  // Verify some other locations in the table, that will be RGB->Legacy conversions.
  VERIFY_ARE_EQUAL(GetGridAttrs(1, 1, rOutputBuffer, actualWriteSize), MakeAttribute(0x1, 0x1));
- VERIFY_ARE_EQUAL(GetGridAttrs(2, 1, rOutputBuffer, actualWriteSize), MakeAttribute(0xB, 0xB));
+ VERIFY_ARE_EQUAL(GetGridAttrs(3, 3, rOutputBuffer, actualWriteSize), MakeAttribute(0xB, 0xB));
  VERIFY_ARE_EQUAL(GetGridAttrs(2, 2, rOutputBuffer, actualWriteSize), MakeAttribute(0x2, 0x2));
- VERIFY_ARE_EQUAL(GetGridAttrs(2, 3, rOutputBuffer, actualWriteSize), MakeAttribute(0x3, 0x3));
- VERIFY_ARE_EQUAL(GetGridAttrs(3, 4, rOutputBuffer, actualWriteSize), MakeAttribute(0x4, 0x4));
+ VERIFY_ARE_EQUAL(GetGridAttrs(2, 1, rOutputBuffer, actualWriteSize), MakeAttribute(0x3, 0x3));
+ VERIFY_ARE_EQUAL(GetGridAttrs(5, 8, rOutputBuffer, actualWriteSize), MakeAttribute(0x4, 0x4));
  VERIFY_ARE_EQUAL(GetGridAttrs(3, 5, rOutputBuffer, actualWriteSize), MakeAttribute(0x5, 0x5));
- VERIFY_ARE_EQUAL(GetGridAttrs(4, 5, rOutputBuffer, actualWriteSize), MakeAttribute(0x9, 0x9));
- VERIFY_ARE_EQUAL(GetGridAttrs(4, 6, rOutputBuffer, actualWriteSize), MakeAttribute(0x6, 0x6));
- VERIFY_ARE_EQUAL(GetGridAttrs(4, 7, rOutputBuffer, actualWriteSize), MakeAttribute(0x7, 0x7));
+ VERIFY_ARE_EQUAL(GetGridAttrs(6, 8, rOutputBuffer, actualWriteSize), MakeAttribute(0x9, 0x9));
+ VERIFY_ARE_EQUAL(GetGridAttrs(8, 8, rOutputBuffer, actualWriteSize), MakeAttribute(0x6, 0x6));
+ VERIFY_ARE_EQUAL(GetGridAttrs(9, 8, rOutputBuffer, actualWriteSize), MakeAttribute(0x7, 0x7));
  VERIFY_ARE_EQUAL(GetGridAttrs(3, 11, rOutputBuffer, actualWriteSize), MakeAttribute(0x8, 0x8));
  VERIFY_ARE_EQUAL(GetGridAttrs(3, 12, rOutputBuffer, actualWriteSize), MakeAttribute(0x1, 0x1));
  VERIFY_ARE_EQUAL(GetGridAttrs(4, 12, rOutputBuffer, actualWriteSize), MakeAttribute(0xA, 0xA));
- VERIFY_ARE_EQUAL(GetGridAttrs(5, 12, rOutputBuffer, actualWriteSize), MakeAttribute(0xD, 0xD));
- VERIFY_ARE_EQUAL(GetGridAttrs(10, 12, rOutputBuffer, actualWriteSize), MakeAttribute(0xE, 0xE));
- VERIFY_ARE_EQUAL(GetGridAttrs(10, 13, rOutputBuffer, actualWriteSize), MakeAttribute(0xC, 0xC));
- VERIFY_ARE_EQUAL(GetGridAttrs(11, 13, rOutputBuffer, actualWriteSize), MakeAttribute(0xF, 0xF));
+ VERIFY_ARE_EQUAL(GetGridAttrs(8, 1, rOutputBuffer, actualWriteSize), MakeAttribute(0xD, 0xD));
+ VERIFY_ARE_EQUAL(GetGridAttrs(12, 0, rOutputBuffer, actualWriteSize), MakeAttribute(0xE, 0xE));
+ VERIFY_ARE_EQUAL(GetGridAttrs(12, 4, rOutputBuffer, actualWriteSize), MakeAttribute(0xC, 0xC));
+ VERIFY_ARE_EQUAL(GetGridAttrs(12, 3, rOutputBuffer, actualWriteSize), MakeAttribute(0xF, 0xF));
 
  // Greyscale ramp
  VERIFY_ARE_EQUAL(GetGridAttrs(14, 8, rOutputBuffer, actualWriteSize), MakeAttribute(0x0, 0x0));

src/host/output.cpp

@@ -148,16 +148,19 @@ 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());
+
  // 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.
  if ((amountRead == 0 && it->DbcsAttr().IsTrailing()) ||
  (amountRead == (amountToRead - 1) && it->DbcsAttr().IsLeading()))
  {
- retVal.push_back(it->TextAttr().GetLegacyAttributes());
+ retVal.push_back(legacyAttributes);
  }
  else
  {
- retVal.push_back(it->TextAttr().GetLegacyAttributes() | it->DbcsAttr().GeneratePublicApiAttributeFormat());
+ retVal.push_back(legacyAttributes | it->DbcsAttr().GeneratePublicApiAttributeFormat());
  }
 
  amountRead++;
@@ -384,7 +387,7 @@ void ScrollRegion(SCREEN_INFORMATION& screenInfo,
 
  // However, if the character is null and we were given a null attribute (represented as legacy 0),
  // then we'll just fill with spaces and whatever the buffer's default colors are.
- if (fillCharGiven == UNICODE_NULL && fillAttrsGiven.IsLegacy() && fillAttrsGiven.GetLegacyAttributes() == 0)
+ if (fillCharGiven == UNICODE_NULL && fillAttrsGiven == TextAttribute{ 0 })
  {
  fillData = OutputCellIterator(UNICODE_SPACE, screenInfo.GetAttributes());
  }