Skip to content

Commit

Permalink
do not allow CUU and CUD to move "across" margins. (#2996)
Browse files Browse the repository at this point in the history 
If we're moving the cursor up, its vertical movement should be stopped
at the top margin. It should not magically jump up to the bottom margin.
Similarly, this applies to moving down and the bottom margin.
Furthermore, this constraint should always apply, not just when the
start position is within BOTH margins

Fixes #2929.
  • Loading branch information
@zadjii-msft @DHowett
zadjii-msft authored and DHowett committed Oct 1, 2019
1 parent 3336169 commit 0ce08af
Show file tree
Hide file tree
Showing 2 changed files with 206 additions and 9 deletions.
44 changes: 35 additions & 9 deletions src/host/getset.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1430,18 +1430,44 @@ 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 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(clampedPos.Y, 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(clampedPos.Y, hi);
}
CATCH_RETURN();
}
CATCH_RETURN();
}
cursor.SetPosition(clampedPos);

Expand Down
171 changes: 171 additions & 0 deletions src/host/ut_host/ScreenBufferTests.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -184,6 +184,10 @@ class ScreenBufferTests
TEST_METHOD(ClearAlternateBuffer);

TEST_METHOD(InitializeTabStopsInVTMode);

TEST_METHOD(CursorUpDownAcrossMargins);
TEST_METHOD(CursorUpDownOutsideMargins);
TEST_METHOD(CursorUpDownExactlyAtMargins);
};

void ScreenBufferTests::SingleAlternateBufferCreationTest()
Expand Down Expand Up @@ -4500,3 +4504,170 @@ void ScreenBufferTests::InitializeTabStopsInVTMode()

VERIFY_IS_TRUE(gci.GetActiveOutputBuffer().AreTabsSet());
}

void ScreenBufferTests::CursorUpDownAcrossMargins()
{
// Test inspired by: https://github.com/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");
}

void ScreenBufferTests::CursorUpDownExactlyAtMargins()
{
// Test inspired by the CursorUpDownAcrossMargins test.
// echo -e "\e[6;19r\e[19H\e[1B1\e[1A2\e[6H\e[1A3\e[1B4\e[r"
// This does the following:
// * sets the top and bottom DECSTBM margins to 6 and 19
// * moves to line 19 (i.e. on the bottom margin)
// * executes the CUD sequence with a count of 1, to move down 1 lines (still on the margin)
// * writes out 1
// * executes the CUU sequence with a count of 1, to move up 1 lines (now inside margins)
// * writes out 2
// * moves to line 6 (i.e. on the top margin)
// * executes the CUU sequence with a count of 1, to move up 1 lines (still on the margin)
// * writes out 3
// * executes the CUD sequence with a count of 1, to move down 1 lines (still above margins)
// * writes out 4

// This test is different becasue the starting location for these scroll
// operations is _exactly_ on 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[19;1H");
VERIFY_ARE_EQUAL(18, cursor.GetPosition().Y);
stateMachine.ProcessString(L"\x1b[1B");
VERIFY_ARE_EQUAL(18, cursor.GetPosition().Y);
stateMachine.ProcessString(L"1");
{
auto iter = tbi.GetCellDataAt({ 0, 18 });
VERIFY_ARE_EQUAL(L"1", iter->Chars());
}

stateMachine.ProcessString(L"\x1b[1A");
VERIFY_ARE_EQUAL(17, cursor.GetPosition().Y);
stateMachine.ProcessString(L"2");
{
auto iter = tbi.GetCellDataAt({ 1, 17 });
VERIFY_ARE_EQUAL(L"2", iter->Chars());
}

stateMachine.ProcessString(L"\x1b[6;1H");
VERIFY_ARE_EQUAL(5, cursor.GetPosition().Y);

stateMachine.ProcessString(L"\x1b[1A");
VERIFY_ARE_EQUAL(5, cursor.GetPosition().Y);
stateMachine.ProcessString(L"3");
{
auto iter = tbi.GetCellDataAt({ 0, 5 });
VERIFY_ARE_EQUAL(L"3", iter->Chars());
}

stateMachine.ProcessString(L"\x1b[1B");
VERIFY_ARE_EQUAL(6, cursor.GetPosition().Y);
stateMachine.ProcessString(L"4");
{
auto iter = tbi.GetCellDataAt({ 1, 6 });
VERIFY_ARE_EQUAL(L"4", iter->Chars());
}

stateMachine.ProcessString(L"\x1b[r");
}

0 comments on commit 0ce08af

Please sign in to comment.