Correct fill attributes when scrolling and erasing (#3100)

## Summary of the Pull Request

Operations that erase areas of the screen are typically meant to do so using the current color attributes, but with the rendition attributes reset (what we refer to as meta attributes). This also includes scroll operations that have to clear the area of the screen that has scrolled into view. The only exception is the _Erase Scrollback_ operation, which needs to reset the buffer with the default attributes. This PR updates all of these cases to apply the correct attributes when scrolling and erasing.

## PR Checklist
* [x] Closes #2553
* [x] CLA signed. If not, go over [here](https://cla.opensource.microsoft.com/microsoft/Terminal) and sign the CLA
* [x] Tests added/passed
* [ ] Requires documentation to be updated
* [ ] I've not really discussed this with core contributors. I'm ready to accept this work might be rejected in favor of a different grand plan. 

## Detailed Description of the Pull Request / Additional comments

My initial plan was to use a special case legacy attribute value to indicate the "standard erase attribute" which could safely be passed through the legacy APIs. But this wouldn't cover the cases that required default attributes to be used. And then with the changes in PR #2668 and #2987, it became clear that our requirements could be better achieved with a couple of new private APIs that wouldn't have to depend on legacy attribute hacks at all.

To that end, I've added the `PrivateFillRegion` and `PrivateScrollRegion` APIs to the `ConGetSet` interface. These are just thin wrappers around the existing `SCREEN_INFORMATION::Write` method and the `ScrollRegion` function respectively, but with a simple boolean parameter to choose between filling with default attributes or the standard erase attributes (i.e the current colors but with meta attributes reset).

With those new APIs in place, I could then update most scroll operations to use `PrivateScrollRegion`, and most erase operations to use `PrivateFillRegion`.

The functions affected by scrolling included:
* `DoSrvPrivateReverseLineFeed` (the RI command)
* `DoSrvPrivateModifyLinesImpl` (the IL and DL commands)
* `AdaptDispatch::_InsertDeleteHelper` (the ICH and DCH commands)
* `AdaptDispatch::_ScrollMovement` (the SU and SD commands)

The functions affected by erasing included:
* `AdaptDispatch::_EraseSingleLineHelper` (the EL command, and most ED variants)
* `AdaptDispatch::EraseCharacters` (the ECH command)

While updating these erase methods, I noticed that both of them also required boundary fixes similar to those in PR #2505 (i.e. the horizontal extent of the erase operation should apply to the full width of the buffer, and not just the current viewport width), so I've addressed that at the same time.

In addition to the changes above, there were also a few special cases, the first being the line feed handling, which required updating in a number of places to use the correct erase attributes:

* `SCREEN_INFORMATION::InitializeCursorRowAttributes` - this is used to initialise the rows that pan into view when the viewport is moved down the buffer.
* `TextBuffer::IncrementCircularBuffer` - this occurs when we scroll passed the very end of the buffer, and a recycled row now needs to be reinitialised.
* `AdjustCursorPosition` - when within margin boundaries, this relies on a couple of direct calls to `ScrollRegion` which needed to be passed the correct fill attributes.

The second special case was the full screen erase sequence (`ESC 2 J`), which is handled separately from the other ED sequences. This required updating the `SCREEN_INFORMATION::VtEraseAll` method to use the standard erase attributes, and also required changes to the horizontal extent of the filled area, since it should have been clearing the full buffer width (the same issue as the other erase operations mentioned above).

Finally, there was the `AdaptDispatch::_EraseScrollback` method, which uses both scroll and fill operations, which could now be handled by the new `PrivateScrollRegion` and `PrivateFillRegion` APIs. But in this case we needed to fill with the default attributes rather than the standard erase attributes. And again this implementation needed some changes to make sure the full width of the active area was retained after the erase, similar to the horizontal boundary issues with the other erase operations.

Once all these changes were made, there were a few areas of the code that could then be simplified quite a bit. The `FillConsoleOutputCharacterW`, `FillConsoleOutputAttribute`, and `ScrollConsoleScreenBufferW` were no longer needed in the `ConGetSet` interface, so all of that code could now be removed. The `_EraseSingleLineDistanceHelper` and `_EraseAreaHelper` methods in the `AdaptDispatch` class were also no longer required and could be removed.

Then there were the hacks to handle legacy default colors in the `FillConsoleOutputAttributeImpl` and `ScrollConsoleScreenBufferWImpl` implementations. Since those hacks were only needed for VT operations, and the VT code no longer calls those methods, there was no longer a need to retain that behaviour (in fact there are probably some edge cases where that behaviour might have been considered a bug when reached via the public console APIs). 

## Validation Steps Performed

For most of the scrolling operations there were already existing tests in place, and those could easily be extended to check that the meta attributes were correctly reset when filling the revealed lines of the scrolling region.

In the screen buffer tests, I made updates of that sort to  the `ScrollOperations` method (handling SU, SD, IL, DL, and RI), the `InsertChars` and `DeleteChars` methods (ICH and DCH), and the `VtNewlinePastViewport` method (LF). I also added a new `VtNewlinePastEndOfBuffer` test to check the case where the line feed causes the viewport to pan past the end of the buffer.

The erase operations, however, were being covered by adapter tests, and those aren't really suited for this kind of functionality (the same sort of issue came up in PR #2505). As a result I've had to reimplement those tests as screen buffer tests.

Most of the erase operations are covered by the `EraseTests` method, except the for the scrollback erase which has a dedicated `EraseScrollbackTests` method. I've also had to replace the `HardReset` adapter test, but that was already mostly covered by the `HardResetBuffer` screen buffer test, which I've now extended slightly (it could do with some more checks, but I think that can wait for a future PR when we're fixing other RIS issues).

Author: j4james

src/buffer/out/TextAttribute.cpp

@@ -266,3 +266,12 @@ bool TextAttribute::BackgroundIsDefault() const noexcept
 {
     return _background.IsDefault();
 }
+
+// Routine Description:
+// - Resets the meta and extended attributes, which is what the VT standard
+//      requires for most erasing and filling operations.
+void TextAttribute::SetStandardErase() noexcept
+{
+    SetExtendedAttributes(ExtendedAttributes::Normal);
+    SetMetaAttributes(0);
+}

src/buffer/out/TextAttribute.hpp

@@ -156,6 +156,8 @@ class TextAttribute final
     bool ForegroundIsDefault() const noexcept;
     bool BackgroundIsDefault() const noexcept;
 
+    void SetStandardErase() noexcept;
+
     constexpr bool IsRgb() const noexcept
     {
         return _foreground.IsRgb() || _background.IsRgb();

src/buffer/out/textBuffer.cpp

@@ -539,17 +539,24 @@ bool TextBuffer::NewlineCursor()
 //Routine Description:
 // - Increments the circular buffer by one. Circular buffer is represented by FirstRow variable.
 //Arguments:
-// - <none>
+// - inVtMode - set to true in VT mode, so standard erase attributes are used for the new row.
 //Return Value:
 // - true if we successfully incremented the buffer.
-bool TextBuffer::IncrementCircularBuffer()
+bool TextBuffer::IncrementCircularBuffer(const bool inVtMode)
 {
     // FirstRow is at any given point in time the array index in the circular buffer that corresponds
     // to the logical position 0 in the window (cursor coordinates and all other coordinates).
     _renderTarget.TriggerCircling();
 
     // First, clean out the old "first row" as it will become the "last row" of the buffer after the circle is performed.
-    const bool fSuccess = _storage.at(_firstRow).Reset(_currentAttributes);
+    auto fillAttributes = _currentAttributes;
+    if (inVtMode)
+    {
+        // The VT standard requires that the new row is initialized with
+        // the current background color, but with no meta attributes set.
+        fillAttributes.SetStandardErase();
+    }
+    const bool fSuccess = _storage.at(_firstRow).Reset(fillAttributes);
     if (fSuccess)
     {
         // Now proceed to increment.

src/buffer/out/textBuffer.hpp

@@ -101,7 +101,7 @@ class TextBuffer final
     bool NewlineCursor();
 
     // Scroll needs access to this to quickly rotate around the buffer.
-    bool IncrementCircularBuffer();
+    bool IncrementCircularBuffer(const bool inVtMode = false);
 
     COORD GetLastNonSpaceCharacter() const;
     COORD GetLastNonSpaceCharacter(const Microsoft::Console::Types::Viewport viewport) const;

src/host/_output.cpp

@@ -219,24 +219,6 @@ void WriteToScreen(SCREEN_INFORMATION& screenInfo, const Viewport& region)
     try
     {
         TextAttribute useThisAttr(attribute);
-
-        // Here we're being a little clever -
-        // Because RGB color can't roundtrip the API, certain VT sequences will forget the RGB color
-        // because their first call to GetScreenBufferInfo returned a legacy attr.
-        // If they're calling this with the default attrs, they likely wanted to use the RGB default attrs.
-        // This could create a scenario where someone emitted RGB with VT,
-        // THEN used the API to FillConsoleOutput with the default attrs, and DIDN'T want the RGB color
-        // they had set.
-        if (screenBuffer.InVTMode())
-        {
-            const auto& gci = ServiceLocator::LocateGlobals().getConsoleInformation();
-            auto bufferLegacy = gci.GenerateLegacyAttributes(screenBuffer.GetAttributes());
-            if (bufferLegacy == attribute)
-            {
-                useThisAttr = TextAttribute(screenBuffer.GetAttributes());
-            }
-        }
-
         const OutputCellIterator it(useThisAttr, lengthToWrite);
         const auto done = screenBuffer.Write(it, startingCoordinate);
 

src/host/_stream.cpp

@@ -74,7 +74,10 @@ constexpr unsigned int LOCAL_BUFFER_SIZE = 100;
         }
     }
 
-    const auto bufferAttributes = screenInfo.GetAttributes();
+    // The VT standard requires the lines revealed when scrolling are filled
+    // with the current background color, but with no meta attributes set.
+    auto fillAttributes = screenInfo.GetAttributes();
+    fillAttributes.SetStandardErase();
 
     const auto relativeMargins = screenInfo.GetRelativeScrollMargins();
     auto viewport = screenInfo.GetViewport();
@@ -144,7 +147,7 @@ constexpr unsigned int LOCAL_BUFFER_SIZE = 100;
 
         try
         {
-            ScrollRegion(screenInfo, scrollRect, std::nullopt, newPostMarginsOrigin, UNICODE_SPACE, bufferAttributes);
+            ScrollRegion(screenInfo, scrollRect, std::nullopt, newPostMarginsOrigin, UNICODE_SPACE, fillAttributes);
         }
         CATCH_LOG();
 
@@ -193,7 +196,7 @@ constexpr unsigned int LOCAL_BUFFER_SIZE = 100;
 
         try
         {
-            ScrollRegion(screenInfo, scrollRect, scrollRect, dest, UNICODE_SPACE, bufferAttributes);
+            ScrollRegion(screenInfo, scrollRect, scrollRect, dest, UNICODE_SPACE, fillAttributes);
         }
         CATCH_LOG();
 

src/host/getset.cpp

@@ -836,32 +836,6 @@ void ApiRoutines::GetLargestConsoleWindowSizeImpl(const SCREEN_INFORMATION& cont
         auto& buffer = context.GetActiveBuffer();
 
         TextAttribute useThisAttr(fillAttribute);
-
-        // Here we're being a little clever - similar to FillConsoleOutputAttributeImpl
-        // Because RGB/default color can't roundtrip the API, certain VT
-        //      sequences will forget the RGB color because their first call to
-        //      GetScreenBufferInfo returned a legacy attr.
-        // If they're calling this with the legacy attrs version of our current
-        //      attributes, they likely wanted to use the full version of
-        //      our current attributes, whether that be RGB or _default_ colored.
-        // This could create a scenario where someone emitted RGB with VT,
-        //      THEN used the API to ScrollConsoleOutput with the legacy attrs,
-        //      and DIDN'T want the RGB color. As in FillConsoleOutputAttribute,
-        //      this scenario is highly unlikely, and we can reasonably do this
-        //      on their behalf.
-        // see MSFT:19853701
-
-        if (buffer.InVTMode())
-        {
-            const auto& gci = ServiceLocator::LocateGlobals().getConsoleInformation();
-            const auto currentAttributes = buffer.GetAttributes();
-            const auto bufferLegacy = gci.GenerateLegacyAttributes(currentAttributes);
-            if (bufferLegacy == fillAttribute)
-            {
-                useThisAttr = currentAttributes;
-            }
-        }
-
         ScrollRegion(buffer, source, clip, target, fillCharacter, useThisAttr);
 
         return S_OK;
@@ -1422,25 +1396,12 @@ void DoSrvPrivateAllowCursorBlinking(SCREEN_INFORMATION& screenInfo, const bool
             coordDestination.X = 0;
             coordDestination.Y = viewport.Top + 1;
 
-            // Here we previously called to ScrollConsoleScreenBufferWImpl to
-            // perform the scrolling operation. However, that function only
-            // accepts a WORD for the fill attributes. That means we'd lose
-            // 256/RGB fidelity for fill attributes. So instead, we'll just call
-            // ScrollRegion ourselves, with the same params that
-            // ScrollConsoleScreenBufferWImpl would have.
-            // See microsoft/terminal#832, #2702 for more context.
-            try
-            {
-                LockConsole();
-                auto Unlock = wil::scope_exit([&] { UnlockConsole(); });
-                ScrollRegion(screenInfo,
-                             srScroll,
-                             srScroll,
-                             coordDestination,
-                             UNICODE_SPACE,
-                             screenInfo.GetAttributes());
-            }
-            CATCH_LOG();
+            // Note the revealed lines are filled with the standard erase attributes.
+            Status = NTSTATUS_FROM_HRESULT(DoSrvPrivateScrollRegion(screenInfo,
+                                                                    srScroll,
+                                                                    srScroll,
+                                                                    coordDestination,
+                                                                    true));
         }
     }
     return Status;
@@ -2145,25 +2106,12 @@ void DoSrvPrivateModifyLinesImpl(const unsigned int count, const bool insert)
             coordDestination.Y = (cursorPosition.Y) - gsl::narrow<short>(count);
         }
 
-        // Here we previously called to ScrollConsoleScreenBufferWImpl to
-        // perform the scrolling operation. However, that function only accepts
-        // a WORD for the fill attributes. That means we'd lose 256/RGB fidelity
-        // for fill attributes. So instead, we'll just call ScrollRegion
-        // ourselves, with the same params that ScrollConsoleScreenBufferWImpl
-        // would have.
-        // See microsoft/terminal#832 for more context.
-        try
-        {
-            LockConsole();
-            auto Unlock = wil::scope_exit([&] { UnlockConsole(); });
-            ScrollRegion(screenInfo,
-                         srScroll,
-                         srScroll,
-                         coordDestination,
-                         UNICODE_SPACE,
-                         screenInfo.GetAttributes());
-        }
-        CATCH_LOG();
+        // Note the revealed lines are filled with the standard erase attributes.
+        LOG_IF_FAILED(DoSrvPrivateScrollRegion(screenInfo,
+                                               srScroll,
+                                               srScroll,
+                                               coordDestination,
+                                               true));
 
         // The IL and DL controls are also expected to move the cursor to the left margin.
         // For now this is just column 0, since we don't yet support DECSLRM.
@@ -2287,3 +2235,99 @@ void DoSrvPrivateMoveToBottom(SCREEN_INFORMATION& screenInfo)
     }
     CATCH_RETURN();
 }
+
+// Routine Description:
+// - A private API call for filling a region of the screen buffer.
+// Arguments:
+// - screenInfo - Reference to screen buffer info.
+// - startPosition - The position to begin filling at.
+// - fillLength - The number of characters to fill.
+// - fillChar - Character to fill the target region with.
+// - standardFillAttrs - If true, fill with the standard erase attributes.
+//                       If false, fill with the default attributes.
+// Return value:
+// - S_OK or failure code from thrown exception
+[[nodiscard]] HRESULT DoSrvPrivateFillRegion(SCREEN_INFORMATION& screenInfo,
+                                             const COORD startPosition,
+                                             const size_t fillLength,
+                                             const wchar_t fillChar,
+                                             const bool standardFillAttrs) noexcept
+{
+    try
+    {
+        if (fillLength == 0)
+        {
+            return S_OK;
+        }
+
+        LockConsole();
+        auto Unlock = wil::scope_exit([&] { UnlockConsole(); });
+
+        // For most VT erasing operations, the standard requires that the
+        // erased area be filled with the current background color, but with
+        // no additional meta attributes set. For all other cases, we just
+        // fill with the default attributes.
+        auto fillAttrs = TextAttribute{};
+        if (standardFillAttrs)
+        {
+            fillAttrs = screenInfo.GetAttributes();
+            fillAttrs.SetStandardErase();
+        }
+
+        const auto fillData = OutputCellIterator{ fillChar, fillAttrs, fillLength };
+        screenInfo.Write(fillData, startPosition, false);
+
+        // Notify accessibility
+        auto endPosition = startPosition;
+        const auto bufferSize = screenInfo.GetBufferSize();
+        bufferSize.MoveInBounds(fillLength - 1, endPosition);
+        screenInfo.NotifyAccessibilityEventing(startPosition.X, startPosition.Y, endPosition.X, endPosition.Y);
+        return S_OK;
+    }
+    CATCH_RETURN();
+}
+
+// Routine Description:
+// - A private API call for moving a block of data in the screen buffer,
+//    optionally limiting the effects of the move to a clipping rectangle.
+// Arguments:
+// - screenInfo - Reference to screen buffer info.
+// - scrollRect - Region to copy/move (source and size).
+// - clipRect - Optional clip region to contain buffer change effects.
+// - destinationOrigin - Upper left corner of target region.
+// - standardFillAttrs - If true, fill with the standard erase attributes.
+//                       If false, fill with the default attributes.
+// Return value:
+// - S_OK or failure code from thrown exception
+[[nodiscard]] HRESULT DoSrvPrivateScrollRegion(SCREEN_INFORMATION& screenInfo,
+                                               const SMALL_RECT scrollRect,
+                                               const std::optional<SMALL_RECT> clipRect,
+                                               const COORD destinationOrigin,
+                                               const bool standardFillAttrs) noexcept
+{
+    try
+    {
+        LockConsole();
+        auto Unlock = wil::scope_exit([&] { UnlockConsole(); });
+
+        // For most VT scrolling operations, the standard requires that the
+        // erased area be filled with the current background color, but with
+        // no additional meta attributes set. For all other cases, we just
+        // fill with the default attributes.
+        auto fillAttrs = TextAttribute{};
+        if (standardFillAttrs)
+        {
+            fillAttrs = screenInfo.GetAttributes();
+            fillAttrs.SetStandardErase();
+        }
+
+        ScrollRegion(screenInfo,
+                     scrollRect,
+                     clipRect,
+                     destinationOrigin,
+                     UNICODE_SPACE,
+                     fillAttrs);
+        return S_OK;
+    }
+    CATCH_RETURN();
+}

src/host/getset.h

@@ -92,3 +92,15 @@ void DoSrvPrivateMoveToBottom(SCREEN_INFORMATION& screenInfo);
 [[nodiscard]] HRESULT DoSrvPrivateSetDefaultForegroundColor(const COLORREF value) noexcept;
 
 [[nodiscard]] HRESULT DoSrvPrivateSetDefaultBackgroundColor(const COLORREF value) noexcept;
+
+[[nodiscard]] HRESULT DoSrvPrivateFillRegion(SCREEN_INFORMATION& screenInfo,
+                                             const COORD startPosition,
+                                             const size_t fillLength,
+                                             const wchar_t fillChar,
+                                             const bool standardFillAttrs) noexcept;
+
+[[nodiscard]] HRESULT DoSrvPrivateScrollRegion(SCREEN_INFORMATION& screenInfo,
+                                               const SMALL_RECT scrollRect,
+                                               const std::optional<SMALL_RECT> clipRect,
+                                               const COORD destinationOrigin,
+                                               const bool standardFillAttrs) noexcept;

src/host/output.cpp

@@ -296,7 +296,8 @@ static void _ScrollScreen(SCREEN_INFORMATION& screenInfo, const Viewport& source
 bool StreamScrollRegion(SCREEN_INFORMATION& screenInfo)
 {
     // Rotate the circular buffer around and wipe out the previous final line.
-    bool fSuccess = screenInfo.GetTextBuffer().IncrementCircularBuffer();
+    const bool inVtMode = WI_IsFlagSet(screenInfo.OutputMode, ENABLE_VIRTUAL_TERMINAL_PROCESSING);
+    bool fSuccess = screenInfo.GetTextBuffer().IncrementCircularBuffer(inVtMode);
     if (fSuccess)
     {
         // Trigger a graphical update if we're active.