CUU and CUD should not move "across" margins.

src/host/getset.cpp

@@ -1417,18 +1417,45 @@ void DoSrvPrivateAllowCursorBlinking(SCREEN_INFORMATION& screenInfo, const bool
  // Make sure the cursor doesn't move outside the viewport.
  screenInfo.GetViewport().Clamp(clampedPos);
 
- // Make sure the cursor stays inside the margins, but only if it started there
- if (screenInfo.AreMarginsSet() && screenInfo.IsCursorInMargins(cursor.GetPosition()))
+ // Make sure the cursor stays inside the margins
+ if (screenInfo.AreMarginsSet())
  {
- try
+ const auto margins = screenInfo.GetAbsoluteScrollMargins().ToInclusive();
+
+ const auto newY = clampedPos.Y;
+ const auto cursorY = cursor.GetPosition().Y;
+
+ const auto lo = margins.Top;
+ const auto hi = margins.Bottom;
+
+ // See microsoft/terminal#2929 - If the cursor is _below_ the top
+ // margin, it should stay below the top margin. If it's _above_ the
+ // bottom, it should stay above the bottom. Cursor movements that stay
+ // outside the margins shouldn't necessarily be affected. For example,
+ // moving up while below the bottom margin shouldn't just jump straight
+ // to the bottom margin. See
+ // ScreenBufferTests::CursorUpDownOutsideMargins for a test of that
+ // behavior.
+ const bool cursorBelowTop = cursorY > lo;
+ const bool cursorAboveBottom = cursorY < hi;
+
+ if (cursorBelowTop)
+ {
+ try
+ {
+ clampedPos.Y = std::max(newY, lo);
+ }
+ CATCH_RETURN();
+ }
+
+ if (cursorAboveBottom)
  {
- const auto margins = screenInfo.GetAbsoluteScrollMargins().ToInclusive();
- const auto v = clampedPos.Y;
- const auto lo = margins.Top;
- const auto hi = margins.Bottom;
- clampedPos.Y = std::clamp(v, lo, hi);
+ try
+ {
+  clampedPos.Y = std::min(newY, hi);
+ }
+ CATCH_RETURN();
  }
- CATCH_RETURN();
  }
  cursor.SetPosition(clampedPos);
 

src/host/ut_host/ScreenBufferTests.cpp

@@ -184,6 +184,9 @@ class ScreenBufferTests
  TEST_METHOD(ClearAlternateBuffer);
 
  TEST_METHOD(InitializeTabStopsInVTMode);
+
+ TEST_METHOD(CursorUpDownAcrossMargins);
+ TEST_METHOD(CursorUpDownOutsideMargins);
 };
 
 void ScreenBufferTests::SingleAlternateBufferCreationTest()
@@ -4480,3 +4483,99 @@ void ScreenBufferTests::InitializeTabStopsInVTMode()
 
  VERIFY_IS_TRUE(gci.GetActiveOutputBuffer().AreTabsSet());
 }
+
+void ScreenBufferTests::CursorUpDownAcrossMargins()
+{
+ // Test inspired by: https:/microsoft/terminal/issues/2929
+ // echo -e "\e[6;19r\e[24H\e[99AX\e[1H\e[99BY\e[r"
+ // This does the following:
+ // * sets the top and bottom DECSTBM margins to 6 and 19
+ // * moves to line 24 (i.e. below the bottom margin)
+ // * executes the CUU sequence with a count of 99, to move up 99 lines
+ // * writes out X
+ // * moves to line 1 (i.e. above the top margin)
+ // * executes the CUD sequence with a count of 99, to move down 99 lines
+ // * writes out Y
+
+ auto& gci = ServiceLocator::LocateGlobals().getConsoleInformation();
+ auto& si = gci.GetActiveOutputBuffer();
+ auto& tbi = si.GetTextBuffer();
+ auto& stateMachine = si.GetStateMachine();
+ auto& cursor = si.GetTextBuffer().GetCursor();
+
+ VERIFY_IS_TRUE(si.GetViewport().BottomInclusive() > 24);
+
+ // Set some scrolling margins
+ stateMachine.ProcessString(L"\x1b[6;19r");
+ stateMachine.ProcessString(L"\x1b[24H");
+ VERIFY_ARE_EQUAL(23, cursor.GetPosition().Y);
+
+ stateMachine.ProcessString(L"\x1b[99A");
+ VERIFY_ARE_EQUAL(5, cursor.GetPosition().Y);
+ stateMachine.ProcessString(L"X");
+ {
+ auto iter = tbi.GetCellDataAt({ 0, 5 });
+ VERIFY_ARE_EQUAL(L"X", iter->Chars());
+ }
+ stateMachine.ProcessString(L"\x1b[1H");
+ VERIFY_ARE_EQUAL(0, cursor.GetPosition().Y);
+
+ stateMachine.ProcessString(L"\x1b[99B");
+ VERIFY_ARE_EQUAL(18, cursor.GetPosition().Y);
+ stateMachine.ProcessString(L"Y");
+ {
+ auto iter = tbi.GetCellDataAt({ 0, 18 });
+ VERIFY_ARE_EQUAL(L"Y", iter->Chars());
+ }
+ stateMachine.ProcessString(L"\x1b[r");
+}
+
+void ScreenBufferTests::CursorUpDownOutsideMargins()
+{
+ // Test inspired by the CursorUpDownAcrossMargins test.
+ // echo -e "\e[6;19r\e[24H\e[1AX\e[1H\e[1BY\e[r"
+ // This does the following:
+ // * sets the top and bottom DECSTBM margins to 6 and 19
+ // * moves to line 24 (i.e. below the bottom margin)
+ // * executes the CUU sequence with a count of 1, to move up 1 lines (still below margins)
+ // * writes out X
+ // * moves to line 1 (i.e. above the top margin)
+ // * executes the CUD sequence with a count of 1, to move down 1 lines (still above margins)
+ // * writes out Y
+
+ // This test is different becasue the end location of the vertical movement
+ // should not be within the margins at all. WE should not clamp this
+ // movement to be within the margins.
+
+ auto& gci = ServiceLocator::LocateGlobals().getConsoleInformation();
+ auto& si = gci.GetActiveOutputBuffer();
+ auto& tbi = si.GetTextBuffer();
+ auto& stateMachine = si.GetStateMachine();
+ auto& cursor = si.GetTextBuffer().GetCursor();
+
+ VERIFY_IS_TRUE(si.GetViewport().BottomInclusive() > 24);
+
+ // Set some scrolling margins
+ stateMachine.ProcessString(L"\x1b[6;19r");
+ stateMachine.ProcessString(L"\x1b[24H");
+ VERIFY_ARE_EQUAL(23, cursor.GetPosition().Y);
+
+ stateMachine.ProcessString(L"\x1b[1A");
+ VERIFY_ARE_EQUAL(22, cursor.GetPosition().Y);
+ stateMachine.ProcessString(L"X");
+ {
+ auto iter = tbi.GetCellDataAt({ 0, 22 });
+ VERIFY_ARE_EQUAL(L"X", iter->Chars());
+ }
+ stateMachine.ProcessString(L"\x1b[1H");
+ VERIFY_ARE_EQUAL(0, cursor.GetPosition().Y);
+
+ stateMachine.ProcessString(L"\x1b[1B");
+ VERIFY_ARE_EQUAL(1, cursor.GetPosition().Y);
+ stateMachine.ProcessString(L"Y");
+ {
+ auto iter = tbi.GetCellDataAt({ 0, 1 });
+ VERIFY_ARE_EQUAL(L"Y", iter->Chars());
+ }
+ stateMachine.ProcessString(L"\x1b[r");
+}