Remove unwanted DECSTBM clipping (#2764)

The `DECSTBM` margins are meant to define the range of lines within which
certain vertical scrolling operations take place. However, we were applying
these margin restrictions in the `ScrollRegion` function, which is also used in
a number of places that shouldn't be affected by `DECSTBM`.

This includes the `ICH` and `DCH` escape sequences (which are only affected by
the horizontal margins, which we don't yet support), the
`ScrollConsoleScreenBuffer` API (which is public Console API, not meant to be
affected by the VT terminal emulation), and the `CSI 3 J` erase scrollback
extension (which isn't really scrolling as such, but uses the `ScrollRegion`
function to manipulate the scrollback buffer).

This commit moves the margin clipping out of the `ScrollRegion` function, so it
can be applied exclusively in the places that need it.

With the margin clipping removed from the `ScrollRegion` function, it now had
to be applied manually in the places it was actually required. This included:

* The `DoSrvPrivateReverseLineFeed` function (for the `RI` control): This was
* just a matter of updating the bottom of the scroll rect to the bottom margin
* (at least when the margins were actually set), since the top of the scroll
* rect would always be the top of the viewport.  The
* `DoSrvPrivateModifyLinesImpl` function (for the `IL` and `DL` commands):
* Again this was just a matter of updating the bottom of the scroll rect, since
* the cursor position would always determine the top of the scroll rect.  The
* `AdaptDispatch::_ScrollMovement` method (for the `SU` and `SD` commands):
* This required updating both the top and bottom coordinates of the scroll
* rect, and also a simpler destination Y coordinate (the way the `ScrollRegion`
* function worked before, the caller was expected to take the margins into
* account when determining the destination).

On the plus side, there was now no longer a need to override the margins when
calling `ScrollRegion` in the `AdjustCursorPosition` function. In the first
case, the margins had needed to be cleared (_stream.cpp 143-145), but that is
now the default behaviour. In the second case, there had been a more
complicated adjustment of the margins (_stream.cpp 196-209), but that code was
never actually used so could be removed completely (to get to that point either
_fScrollUp_ was true, so _scrollDownAtTop_ couldn't also be true, or
_fScrollDown_ was true, but in that case there is a check to make sure
_scrollDownAtTop_ is false).

While testing, I also noticed that one of the `ScrollRegion` calls in the
`AdjustCursorPosition` function was not setting the horizontal range correctly
- the scrolling should always affect the full buffer width rather than just the
  viewport width - so I've fixed that now as well.

## Validation Steps Performed

For commands like `RI`, `IL`, `DL`, etc. where we've changed the implementation
but not the behaviour, there were already unit tests that could confirm that
the new implementation was still producing the correct results.

Where there has been a change in behaviour - namely for the `ICH` and `DCH`
commands, and the `ScrollConsoleScreenBuffer` API - I've extended the existing
unit tests to check that they still function correctly even when the `DECSTBM`
margins are set (which would previously have caused them to fail).

I've also tested manually with the test cases in issues #2543 and #2659, and
confirmed that they now work as expected.

Closes #2543
Closes #2659

(cherry picked from commit 0c8a4df)

Author: j4james

src/host/_stream.cpp

@@ -140,15 +140,11 @@ using Microsoft::Console::VirtualTerminal::StateMachine;
         const COORD newPostMarginsOrigin = { 0, moveToYPosition };
         const COORD newViewOrigin = { 0, newViewTop };
 
-        // Unset the margins to scroll the content below the margins,
-        //      then restore them after.
-        screenInfo.SetScrollMargins(Viewport::FromInclusive({ 0 }));
         try
         {
             ScrollRegion(screenInfo, scrollRect, std::nullopt, newPostMarginsOrigin, UNICODE_SPACE, bufferAttributes);
         }
         CATCH_LOG();
-        screenInfo.SetScrollMargins(relativeMargins);
 
         // Move the viewport down
         auto hr = screenInfo.SetViewportOrigin(true, newViewOrigin, true);
@@ -186,39 +182,19 @@ using Microsoft::Console::VirtualTerminal::StateMachine;
         SMALL_RECT scrollRect = { 0 };
         scrollRect.Top = srMargins.Top;
         scrollRect.Bottom = srMargins.Bottom;
-        scrollRect.Left = screenInfo.GetViewport().Left(); // NOTE: Left/Right Scroll margins don't do anything currently.
-        scrollRect.Right = screenInfo.GetViewport().RightInclusive();
+        scrollRect.Left = 0; // NOTE: Left/Right Scroll margins don't do anything currently.
+        scrollRect.Right = bufferSize.X - 1; // -1, otherwise this would be an exclusive rect.
 
         COORD dest;
         dest.X = scrollRect.Left;
         dest.Y = scrollRect.Top - diff;
 
-        SMALL_RECT clipRect = scrollRect;
-        // Typically ScrollRegion() clips by the scroll margins. However, if
-        //      we're scrolling down at the top of the viewport, we'll need to
-        //      not clip at the margins, instead move the contents of the margins
-        //      up above the viewport. So we'll clear out the current margins, and
-        //      set them to the viewport+(#diff rows above the viewport).
-        if (scrollDownAtTop)
-        {
-            clipRect.Top -= diff;
-            auto fakeMargins = srMargins;
-            fakeMargins.Top -= diff;
-            auto fakeRelative = viewport.ConvertToOrigin(Viewport::FromInclusive(fakeMargins));
-            screenInfo.SetScrollMargins(fakeRelative);
-        }
-
         try
         {
-            ScrollRegion(screenInfo, scrollRect, clipRect, dest, UNICODE_SPACE, bufferAttributes);
+            ScrollRegion(screenInfo, scrollRect, scrollRect, dest, UNICODE_SPACE, bufferAttributes);
         }
         CATCH_LOG();
 
-        if (scrollDownAtTop)
-        {
-            // Undo the fake margins we set above
-            screenInfo.SetScrollMargins(relativeMargins);
-        }
         coordCursor.Y -= diff;
     }
 

src/host/getset.cpp

@@ -1368,6 +1368,11 @@ void DoSrvPrivateAllowCursorBlinking(SCREEN_INFORMATION& screenInfo, const bool
             srScroll.Right = SHORT_MAX;
             srScroll.Top = viewport.Top;
             srScroll.Bottom = viewport.Bottom;
+            // Clip to the DECSTBM margin boundary
+            if (screenInfo.AreMarginsSet())
+            {
+                srScroll.Bottom = screenInfo.GetAbsoluteScrollMargins().BottomInclusive();
+            }
             // Paste coordinate for cut text above
             COORD coordDestination;
             coordDestination.X = 0;
@@ -2044,6 +2049,11 @@ void DoSrvPrivateModifyLinesImpl(const unsigned int count, const bool insert)
         srScroll.Right = SHORT_MAX;
         srScroll.Top = cursorPosition.Y;
         srScroll.Bottom = screenInfo.GetViewport().BottomInclusive();
+        // Clip to the DECSTBM margin boundary
+        if (screenInfo.AreMarginsSet())
+        {
+            srScroll.Bottom = screenInfo.GetAbsoluteScrollMargins().BottomInclusive();
+        }
         // Paste coordinate for cut text above
         COORD coordDestination;
         coordDestination.X = 0;

src/host/output.cpp

@@ -361,19 +361,6 @@ void ScrollRegion(SCREEN_INFORMATION& screenInfo,
     // If there was no clip rect, we'll clip to the entire buffer size.
     auto clip = Viewport::FromInclusive(clipRectGiven.value_or(buffer.ToInclusive()));
 
-    // Account for the scroll margins set by DECSTBM
-    // DECSTBM command can sometimes apply a clipping behavior as well. Check if we have any
-    // margins defined by DECSTBM and further restrict the clipping area here.
-    if (screenInfo.AreMarginsSet())
-    {
-        const auto margin = screenInfo.GetScrollingRegion();
-
-        // Update the clip rectangle to only include the area that is also in the margin.
-        clip = Viewport::Intersect(clip, margin);
-
-        // We'll also need to update the source rectangle, but we need to do that later.
-    }
-
     // OK, make sure that the clip rectangle also fits inside the buffer
     clip = Viewport::Intersect(buffer, clip);
 
@@ -416,18 +403,6 @@ void ScrollRegion(SCREEN_INFORMATION& screenInfo,
         targetOrigin.Y += currentSourceOrigin.Y - originalSourceOrigin.Y;
     }
 
-    // See MSFT:20204600 - Update the source rectangle to only include the region
-    //      inside the scroll margins. We need to do this AFTER we calculate the
-    //      delta between the currentSourceOrigin and the originalSourceOrigin.
-    // Don't combine this with the above block, because if there are margins set
-    //      and the source rectangle was clipped by the buffer, we still want to
-    //      adjust the target origin point based on the clipping of the buffer.
-    if (screenInfo.AreMarginsSet())
-    {
-        const auto margin = screenInfo.GetScrollingRegion();
-        source = Viewport::Intersect(source, margin);
-    }
-
     // And now the target viewport is the same size as the source viewport but at the different position.
     auto target = Viewport::FromDimensions(targetOrigin, source.Dimensions());
 

src/host/ut_host/ApiRoutinesTests.cpp

@@ -594,9 +594,12 @@ class ApiRoutinesTests
     TEST_METHOD(ApiScrollConsoleScreenBufferW)
     {
         BEGIN_TEST_METHOD_PROPERTIES()
+            TEST_METHOD_PROPERTY(L"data:setMargins", L"{false, true}")
             TEST_METHOD_PROPERTY(L"data:checkClipped", L"{false, true}")
         END_TEST_METHOD_PROPERTIES();
 
+        bool setMargins;
+        VERIFY_SUCCEEDED(TestData::TryGetValue(L"setMargins", setMargins), L"Get whether or not we should set the DECSTBM margins.");
         bool checkClipped;
         VERIFY_SUCCEEDED(TestData::TryGetValue(L"checkClipped", checkClipped), L"Get whether or not we should check all the options using a clipping rectangle.");
 
@@ -605,6 +608,13 @@ class ApiRoutinesTests
 
         VERIFY_SUCCEEDED(si.GetTextBuffer().ResizeTraditional({ 5, 5 }), L"Make the buffer small so this doesn't take forever.");
 
+        // Tests are run both with and without the DECSTBM margins set. This should not alter
+        // the results, since ScrollConsoleScreenBuffer should not be affected by VT margins.
+        auto& stateMachine = si.GetStateMachine();
+        stateMachine.ProcessString(setMargins ? L"\x1b[2;4r" : L"\x1b[r");
+        // Make sure we clear the margins on exit so they can't break other tests.
+        auto clearMargins = wil::scope_exit([&] { stateMachine.ProcessString(L"\x1b[r"); });
+
         gci.LockConsole();
         auto Unlock = wil::scope_exit([&] { gci.UnlockConsole(); });
 

src/host/ut_host/ScreenBufferTests.cpp

@@ -3282,6 +3282,13 @@ void ScreenBufferTests::ScrollOperations()
 
 void ScreenBufferTests::InsertChars()
 {
+    BEGIN_TEST_METHOD_PROPERTIES()
+        TEST_METHOD_PROPERTY(L"Data:setMargins", L"{false, true}")
+    END_TEST_METHOD_PROPERTIES();
+
+    bool setMargins;
+    VERIFY_SUCCEEDED(TestData::TryGetValue(L"setMargins", setMargins));
+
     auto& gci = ServiceLocator::LocateGlobals().getConsoleInformation();
     auto& si = gci.GetActiveOutputBuffer().GetActiveBuffer();
     auto& stateMachine = si.GetStateMachine();
@@ -3295,6 +3302,12 @@ void ScreenBufferTests::InsertChars()
     VERIFY_SUCCEEDED(si.ResizeScreenBuffer({ bufferWidth, bufferHeight }, false));
     si.SetViewport(Viewport::FromExclusive({ viewportStart, 0, viewportEnd, 25 }), true);
 
+    // Tests are run both with and without the DECSTBM margins set. This should not alter
+    // the results, since the ICH operation is not affected by vertical margins.
+    stateMachine.ProcessString(setMargins ? L"\x1b[15;20r" : L"\x1b[r");
+    // Make sure we clear the margins on exit so they can't break other tests.
+    auto clearMargins = wil::scope_exit([&] { stateMachine.ProcessString(L"\x1b[r"); });
+
     Log::Comment(
         L"Test 1: Fill the line with Qs. Write some text within the viewport boundaries. "
         L"Then insert 5 spaces at the cursor. Watch spaces get inserted, text slides right "
@@ -3425,6 +3438,13 @@ void ScreenBufferTests::InsertChars()
 
 void ScreenBufferTests::DeleteChars()
 {
+    BEGIN_TEST_METHOD_PROPERTIES()
+        TEST_METHOD_PROPERTY(L"Data:setMargins", L"{false, true}")
+    END_TEST_METHOD_PROPERTIES();
+
+    bool setMargins;
+    VERIFY_SUCCEEDED(TestData::TryGetValue(L"setMargins", setMargins));
+
     auto& gci = ServiceLocator::LocateGlobals().getConsoleInformation();
     auto& si = gci.GetActiveOutputBuffer().GetActiveBuffer();
     auto& stateMachine = si.GetStateMachine();
@@ -3438,6 +3458,12 @@ void ScreenBufferTests::DeleteChars()
     VERIFY_SUCCEEDED(si.ResizeScreenBuffer({ bufferWidth, bufferHeight }, false));
     si.SetViewport(Viewport::FromExclusive({ viewportStart, 0, viewportEnd, 25 }), true);
 
+    // Tests are run both with and without the DECSTBM margins set. This should not alter
+    // the results, since the DCH operation is not affected by vertical margins.
+    stateMachine.ProcessString(setMargins ? L"\x1b[15;20r" : L"\x1b[r");
+    // Make sure we clear the margins on exit so they can't break other tests.
+    auto clearMargins = wil::scope_exit([&] { stateMachine.ProcessString(L"\x1b[r"); });
+
     Log::Comment(
         L"Test 1: Fill the line with Qs. Write some text within the viewport boundaries. "
         L"Then delete 5 characters at the cursor. Watch the rest of the line slide left, "

src/terminal/adapter/adaptDispatch.cpp

@@ -963,13 +963,17 @@ bool AdaptDispatch::_ScrollMovement(const ScrollDirection sdDirection, _In_ unsi
             srScreen.Right = SHORT_MAX;
             srScreen.Top = csbiex.srWindow.Top;
             srScreen.Bottom = csbiex.srWindow.Bottom - 1; // srWindow is exclusive, hence the - 1
+            // Clip to the DECSTBM margin boundaries
+            if (_srScrollMargins.Top < _srScrollMargins.Bottom)
+            {
+                srScreen.Top = csbiex.srWindow.Top + _srScrollMargins.Top;
+                srScreen.Bottom = csbiex.srWindow.Top + _srScrollMargins.Bottom;
+            }
 
             // Paste coordinate for cut text above
             COORD coordDestination;
             coordDestination.X = srScreen.Left;
-            // Scroll starting from the top of the scroll margins.
-            coordDestination.Y = (_srScrollMargins.Top + srScreen.Top) + sDistance * (sdDirection == ScrollDirection::Up ? -1 : 1);
-            // We don't need to worry about clipping the margins at all, ScrollRegion inside conhost will do that correctly for us
+            coordDestination.Y = srScreen.Top + sDistance * (sdDirection == ScrollDirection::Up ? -1 : 1);
 
             // Fill character for remaining space left behind by "cut" operation (or for fill if we "cut" the entire line)
             CHAR_INFO ciFill;