Prevent the virtual viewport bottom being updated incorrectly (#12972)

The "virtual bottom" marks the last line of the mutable viewport area,
which is the part of the buffer that VT sequences can write to. This
region should typically only move downwards as new lines are added to
the buffer, but there were a number of cases where it was incorrectly
being moved up, or moved down further than necessary. This PR attempts
to fix that.

There was an earlier, unsuccessful attempt to fix this in PR #9770 which
was later reverted (issue #9872 was the reason it had to be reverted).
PRs #2666, #2705, and #5317 were fixes for related virtual viewport
problems, some of which have either been extended or superseded by this
PR.

`SetConsoleCursorPositionImpl` is one of the cases that actually does
need to move the virtual viewport upwards sometimes, in particular when
the cmd shell resets the buffer with a `CLS` command. But when this
operation "snaps" the viewport to the location of the cursor, it needs
to use the virtual viewport as the frame of reference. This was
partially addressed by PR #2705, but that only applied in
terminal-scrolling mode, so I've now applied that fix regardless of the
mode.

`SetViewportOrigin` takes a flag which determines whether it will also
move the virtual bottom to match the visible viewport. In some case this
is appropriate (`SetConsoleCursorPositionImpl` being one example), but
in other cases (e.g. when panning the viewport downwards in the
`AdjustCursorPosition` function), it should only be allowed to move
downwards. We can't just not set the update flag in those cases, because
that also determines whether or not the viewport would be clamped, and
we don't want change that. So what I've done is limit
`SetViewportOrigin` to only move the virtual bottom downwards, and added
an explicit `UpdateBottom` call in those places that may also require
upward movement.

`ResizeWindow` in the `ConhostInternalGetSet` class has a similar
problem to `SetConsoleCursorPositionImpl`, in that it's updating the
viewport to account for the new size, but if that visible viewport is
scrolled back or forward, it would end up placing the virtual viewport
in the wrong place. So again the solution here was to use the virtual
viewport as the frame of reference for the position. However, if the
viewport is being shrunk, this can still result in the cursor falling
below the bottom, so we need an additional check to adjust for that.
This can't be applied in pty mode, though, because that would break the
conpty resizing operation.

`_InternalSetViewportSize` comes into play when resizing the window
manually, and again the viewport after the resize can end up truncating
the virtual bottom if not handled correctly. This was partially
addressed in the original code by clamping the new viewport above the
virtual bottom under certain conditions, and only in terminal scrolling
mode. I've replaced that with a new algorithm which links the virtual
bottom to the visible viewport bottom if the two intersect, but
otherwise leaves it unchanged. This applies regardless of the scrolling
mode.

`ResizeWithReflow` is another sizing operation that can affect the
virtual bottom. This occurs when a change of the window width requires
the buffer to be reflowed, and we need to reposition the viewport in the
newly generated buffer. Previously we were just setting the virtual
bottom to align with the new visible viewport, but that could easily
result in the buffer truncation if the visible viewport was scrolled
back at the time. We now set the virtual bottom to the last non-space
row, or the cursor row (whichever is larger). There'll be edge cases
where this is probably not ideal, but it should still work reasonably
well.

`MakeCursorVisible` was another case where the virtual bottom was being
updated (when requested with a flag) via a `SetViewportOrigin` call.
When I checked all the places it was used, though, none of them actually
required that behavior, and doing so could result in the virtual bottom
being incorrectly positioned, even after `SetViewportOrigin` was limited
to moving the virtual bottom downwards. So I've now made it so that
`MakeCursorVisible` never updates the virtual bottom.

`SelectAll` in the `Selection` class was a similar case. It was calling
`SetViewportOrigin` with the `updateBottom` flag set when that really
wasn't necessary and could result in the virtual bottom being
incorrectly set. I've changed the flag to false now.

## Validation Steps Performed

I've manually confirmed that the test cases in issue #9754 are working
now, except for the one involving margins, which is bigger problem with
`AdjustCursorPosition` which will need to be addressed separately.

I've also double checked the test cases from several other virtual
bottom issues (#1206, #1222, #5302, and #9872), and confirmed that
they're still working correctly with these changes.

And I've added a few screen buffer tests in which I've tried to cover as
many of the problematic code paths as possible.

Closes #9754

src/host/getset.cpp

@@ -688,8 +688,6 @@ void ApiRoutines::GetLargestConsoleWindowSizeImpl(const SCREEN_INFORMATION& cont
         LockConsole();
         auto Unlock = wil::scope_exit([&] { UnlockConsole(); });
 
-        auto& gci = ServiceLocator::LocateGlobals().getConsoleInformation();
-
         auto& buffer = context.GetActiveBuffer();
 
         const auto coordScreenBufferSize = buffer.GetBufferSize().Dimensions();
@@ -707,12 +705,9 @@ void ApiRoutines::GetLargestConsoleWindowSizeImpl(const SCREEN_INFORMATION& cont
         // Attempt to "snap" the viewport to the cursor position. If the cursor
         // is not in the current viewport, we'll try and move the viewport so
         // that the cursor is visible.
-        // microsoft/terminal#1222 - Use the "virtual" viewport here, so that
-        // when the console is in terminal-scrolling mode, the viewport snaps
-        // back to the virtual viewport's location.
-        const auto currentViewport = gci.IsTerminalScrolling() ?
-                                         buffer.GetVirtualViewport().ToInclusive() :
-                                         buffer.GetViewport().ToInclusive();
+        // GH#1222 and GH#9754 - Use the "virtual" viewport here, so that the
+        // viewport snaps back to the virtual viewport's location.
+        const auto currentViewport = buffer.GetVirtualViewport().ToInclusive();
         COORD delta{ 0 };
         {
             // When evaluating the X offset, we must convert the buffer position to
@@ -746,6 +741,12 @@ void ApiRoutines::GetLargestConsoleWindowSizeImpl(const SCREEN_INFORMATION& cont
         // buffer for us.
         RETURN_IF_NTSTATUS_FAILED(buffer.SetViewportOrigin(true, newWindowOrigin, true));
 
+        // SetViewportOrigin will only move the virtual bottom down, but in
+        // this particular case we also need to allow the virtual bottom to
+        // be moved up, so we have to call UpdateBottom explicitly. This is
+        // how the cmd shell's CLS command resets the buffer.
+        buffer.UpdateBottom();
+
         return S_OK;
     }
     CATCH_RETURN();

src/host/outputStream.cpp

@@ -317,8 +317,8 @@ bool ConhostInternalGetSet::ResizeWindow(const size_t width, const size_t height
     csbiex.cbSize = sizeof(CONSOLE_SCREEN_BUFFER_INFOEX);
     api->GetConsoleScreenBufferInfoExImpl(screenInfo, csbiex);
 
-    const auto oldViewport = Viewport::FromInclusive(csbiex.srWindow);
-    const auto newViewport = Viewport::FromDimensions(oldViewport.Origin(), sColumns, sRows);
+    const auto oldViewport = screenInfo.GetVirtualViewport();
+    auto newViewport = Viewport::FromDimensions(oldViewport.Origin(), sColumns, sRows);
     // Always resize the width of the console
     csbiex.dwSize.X = sColumns;
     // Only set the screen buffer's height if it's currently less than
@@ -328,6 +328,16 @@ bool ConhostInternalGetSet::ResizeWindow(const size_t width, const size_t height
         csbiex.dwSize.Y = sRows;
     }
 
+    // If the cursor row is now past the bottom of the viewport, we'll have to
+    // move the viewport down to bring it back into view. However, we don't want
+    // to do this in pty mode, because the conpty resize operation is dependent
+    // on the viewport *not* being adjusted.
+    const short cursorOverflow = csbiex.dwCursorPosition.Y - newViewport.BottomInclusive();
+    if (cursorOverflow > 0 && !IsConsolePty())
+    {
+        newViewport = Viewport::Offset(newViewport, { 0, cursorOverflow });
+    }
+
     // SetWindowInfo expect inclusive rects
     const auto sri = newViewport.ToInclusive();
 

src/host/screenInfo.cpp

@@ -818,7 +818,10 @@ void SCREEN_INFORMATION::SetViewportSize(const COORD* const pcoordSize)
     // Update our internal virtual bottom tracker if requested. This helps keep
     //      the viewport's logical position consistent from the perspective of a
     //      VT client application, even if the user scrolls the viewport with the mouse.
-    if (updateBottom)
+    //      We typically only want to this to move the virtual bottom down, though,
+    //      otherwise it can end up "truncating" the buffer if the user is viewing
+    //      the scrollback at the time the viewport origin is updated.
+    if (updateBottom && _virtualBottom < _viewport.BottomInclusive())
     {
         UpdateBottom();
     }
@@ -1239,23 +1242,21 @@ void SCREEN_INFORMATION::_InternalSetViewportSize(const COORD* const pcoordSize,
         srNewViewport.Top = std::max<SHORT>(0, srNewViewport.Top - offBottomDelta);
     }
 
-    // See MSFT:19917443
-    // If we're in terminal scrolling mode, and we've changed the height of the
-    //      viewport, the new viewport's bottom to the _virtualBottom.
-    // GH#1206 - Only do this if the viewport is _growing_ in height. This can
-    // cause unexpected behavior if we try to anchor the _virtualBottom to a
-    // position that will be greater than the height of the buffer.
-    const auto& gci = ServiceLocator::LocateGlobals().getConsoleInformation();
-    auto newViewport = Viewport::FromInclusive(srNewViewport);
-    if (gci.IsTerminalScrolling() && newViewport.Height() >= _viewport.Height())
+    // In general we want to avoid moving the virtual bottom unless it's aligned with
+    // the visible viewport, so we check whether the changes we're making would cause
+    // the bottom of the visible viewport to intersect the virtual bottom at any point.
+    // If so, we update the virtual bottom to match. We also update the virtual bottom
+    // if it's less than the new viewport height minus 1, because that would otherwise
+    // leave the virtual viewport extended past the top of the buffer.
+    const auto newViewport = Viewport::FromInclusive(srNewViewport);
+    if ((_virtualBottom >= _viewport.BottomInclusive() && _virtualBottom < newViewport.BottomInclusive()) ||
+        (_virtualBottom <= _viewport.BottomInclusive() && _virtualBottom > newViewport.BottomInclusive()) ||
+        _virtualBottom < newViewport.Height() - 1)
     {
-        const auto newTop = static_cast<short>(std::max(0, _virtualBottom - (newViewport.Height() - 1)));
-
-        newViewport = Viewport::FromDimensions(COORD({ newViewport.Left(), newTop }), newViewport.Dimensions());
+        _virtualBottom = srNewViewport.Bottom;
     }
 
     _viewport = newViewport;
-    UpdateBottom();
     Tracing::s_TraceWindowViewport(_viewport);
 
     // In Conpty mode, call TriggerScroll here without params. By not providing
@@ -1267,6 +1268,7 @@ void SCREEN_INFORMATION::_InternalSetViewportSize(const COORD* const pcoordSize,
     // till the start of the next frame. If any other text gets output before
     // that frame starts, there's a very real chance that it'll cause errors as
     // the engine tries to invalidate those regions.
+    const auto& gci = ServiceLocator::LocateGlobals().getConsoleInformation();
     if (gci.IsInVtIoMode() && ServiceLocator::LocateGlobals().pRender)
     {
         ServiceLocator::LocateGlobals().pRender->TriggerScroll();
@@ -1462,12 +1464,17 @@ bool SCREEN_INFORMATION::IsMaximizedY() const
 
     if (SUCCEEDED(hr))
     {
-        auto& newCursor = newTextBuffer->GetCursor();
+        // Make sure the new virtual bottom is far enough down to include both
+        // the cursor row and the last non-space row.
+        const auto cursorRow = newTextBuffer->GetCursor().GetPosition().Y;
+        const auto lastNonSpaceRow = newTextBuffer->GetLastNonSpaceCharacter().Y;
+        _virtualBottom = std::max({ cursorRow, lastNonSpaceRow });
+
         // Adjust the viewport so the cursor doesn't wildly fly off up or down.
-        const auto sCursorHeightInViewportAfter = newCursor.GetPosition().Y - _viewport.Top();
+        const auto sCursorHeightInViewportAfter = cursorRow - _viewport.Top();
         COORD coordCursorHeightDiff = { 0 };
         coordCursorHeightDiff.Y = gsl::narrow_cast<short>(sCursorHeightInViewportAfter - sCursorHeightInViewportBefore);
-        LOG_IF_FAILED(SetViewportOrigin(false, coordCursorHeightDiff, true));
+        LOG_IF_FAILED(SetViewportOrigin(false, coordCursorHeightDiff, false));
 
         newTextBuffer->SetCurrentAttributes(oldPrimaryAttributes);
 
@@ -1749,7 +1756,7 @@ void SCREEN_INFORMATION::SetCursorDBMode(const bool DoubleCursor)
     return STATUS_SUCCESS;
 }
 
-void SCREEN_INFORMATION::MakeCursorVisible(const COORD CursorPosition, const bool updateBottom)
+void SCREEN_INFORMATION::MakeCursorVisible(const COORD CursorPosition)
 {
     COORD WindowOrigin;
 
@@ -1781,7 +1788,7 @@ void SCREEN_INFORMATION::MakeCursorVisible(const COORD CursorPosition, const boo
 
     if (WindowOrigin.X != 0 || WindowOrigin.Y != 0)
     {
-        LOG_IF_FAILED(SetViewportOrigin(false, WindowOrigin, updateBottom));
+        LOG_IF_FAILED(SetViewportOrigin(false, WindowOrigin, false));
     }
 }
 
@@ -2308,6 +2315,10 @@ void SCREEN_INFORMATION::SetViewport(const Viewport& newViewport,
     const COORD coordNewOrigin = { 0, sNewTop };
     RETURN_IF_FAILED(SetViewportOrigin(true, coordNewOrigin, true));
 
+    // SetViewportOrigin will only move the virtual bottom down, but in this
+    // case we need to reset it to the top, so we have to update it explicitly.
+    UpdateBottom();
+
     // Place the cursor at the same x coord, on the row that's now the top
     RETURN_IF_FAILED(SetCursorPosition(COORD{ oldCursorPos.X, sNewTop }, false));
 

src/host/screenInfo.hpp

@@ -191,7 +191,7 @@ class SCREEN_INFORMATION : public ConsoleObjectHeader, public Microsoft::Console
     void SetCursorDBMode(const bool DoubleCursor);
     [[nodiscard]] NTSTATUS SetCursorPosition(const COORD Position, const bool TurnOn);
 
-    void MakeCursorVisible(const COORD CursorPosition, const bool updateBottom = true);
+    void MakeCursorVisible(const COORD CursorPosition);
 
     Microsoft::Console::Types::Viewport GetRelativeScrollMargins() const;
     Microsoft::Console::Types::Viewport GetAbsoluteScrollMargins() const;

src/host/selection.cpp

@@ -213,7 +213,7 @@ void Selection::ExtendSelection(_In_ COORD coordBufferPos)
         }
 
         // scroll if necessary to make cursor visible.
-        screenInfo.MakeCursorVisible(coordBufferPos, false);
+        screenInfo.MakeCursorVisible(coordBufferPos);
 
         _dwSelectionFlags |= CONSOLE_SELECTION_NOT_EMPTY;
         _srSelectionRect.Left = _srSelectionRect.Right = _coordSelectionAnchor.X;
@@ -224,7 +224,7 @@ void Selection::ExtendSelection(_In_ COORD coordBufferPos)
     else
     {
         // scroll if necessary to make cursor visible.
-        screenInfo.MakeCursorVisible(coordBufferPos, false);
+        screenInfo.MakeCursorVisible(coordBufferPos);
     }
 
     // remember previous selection rect
@@ -609,5 +609,5 @@ void Selection::SelectAll()
     SelectNewRegion(coordNewSelStart, coordNewSelEnd);
 
     // restore the old window position
-    LOG_IF_FAILED(screenInfo.SetViewportOrigin(true, coordWindowOrigin, true));
+    LOG_IF_FAILED(screenInfo.SetViewportOrigin(true, coordWindowOrigin, false));
 }

src/host/selectionInput.cpp

@@ -927,7 +927,7 @@ bool Selection::_HandleMarkModeSelectionNav(const INPUT_KEY_INFO* const pInputKe
 
             cursor.SetHasMoved(true);
             _coordSelectionAnchor = textBuffer.GetCursor().GetPosition();
-            ScreenInfo.MakeCursorVisible(_coordSelectionAnchor, false);
+            ScreenInfo.MakeCursorVisible(_coordSelectionAnchor);
             _srSelectionRect.Left = _srSelectionRect.Right = _coordSelectionAnchor.X;
             _srSelectionRect.Top = _srSelectionRect.Bottom = _coordSelectionAnchor.Y;
         }