Improve the VT cursor movement implementation (#3628)

## Summary of the Pull Request

Originally there were 3 different methods for implementing VT cursor movement, and between them they still couldn't handle some of the operations correctly. This PR unifies those operations into a single method that can handle every type of cursor movement, and which fixes some of the issues with the existing implementations. In particular it fixes the `CNL` and `CPL` operations, so they're now correctly constrained by the `DECSTBM` margins.

## References

If this PR is accepted, the method added here should make it trivial to implement the `VPR` and `HPR` commands in issue #3428.

## PR Checklist
* [x] Closes #2926
* [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 discussed this with core contributors already. If not checked, I'm ready to accept this work might be rejected in favor of a different grand plan. Issue number where discussion took place: #xxx

## Detailed Description of the Pull Request / Additional comments

The new [`AdaptDispatch::_CursorMovePosition`](d6c4f35cf6/src/terminal/adapter/adaptDispatch.cpp (L169)) method is based on the proposal I made in issue #3428 for the `VPR` and `HPR` comands. It takes three arguments: a row offset (which can be absolute or relative), a column offset (ditto), and a flag specifying whether the position should be constrained by the `DECSTBM` margins.

To make the code more readable, I've implemented the offsets using [a `struct` with some `constexpr` helper functions for the construction](d6c4f35cf6/src/terminal/adapter/adaptDispatch.hpp (L116-L125)). This lets you specify the parameters with expressions like `Offset::Absolute(col)` or `Offset::Forward(distance)` which I think makes the calling code a little easier to understand.

While implementing this new method, I noticed a couple of issues in the existing movement implementations which I thought would be good to fix at the same time.

1. When cursor movement is constrained horizontally, it should be constrained by the buffer width, and not the horizontal viewport boundaries. This is an issue I've previously corrected in other parts of the codebase, and I think the cursor movement was one of the last areas where it was still a problem.

2. A number of the commands had range and overflow checks for their parameters that were either unnecessary (testing for a condition that could never occur) or incorrect (if an operation overflows, the correct behavior is to clamp it, and not just fail). The new implementation handles legitimate overflows correctly, but doesn't check for impossible ranges.

Because of the change of behavior in point 1, I also had to update the implementations of [the `DECSC` and `CPR` commands](9cf7a9b577) to account for the column offset now being relative to the buffer and not the viewport, otherwise those operations would no longer work correctly.

## Validation Steps Performed

Because of the two changes in behavior mentioned above, there were a number of adapter tests that stopped working and needed to be updated. First off there were those that expected the column offset to be relative to the left viewport position and constrained by the viewport width. These now had to be updated to [use the full buffer width](49887a3589) as the allowed horizontal extent.

Then there were all the overflow and out-of-range tests that were testing conditions that could never occur in practice, or where the expected behavior that was tested was actually incorrect. I did spend some time trying to see if there was value in updating these tests somehow, but in the end I decided it was best to just [drop them](6e80d0de19) altogether.

For the `CNL` and `CPL` operations, there didn't appear to be any existing tests, so I added some [new screen buffer tests](d6c4f35cf6) to check that those operations now work correctly, both with and without margins.

(cherry picked from commit 2fec1787a0)

src/buffer/out/textBuffer.cpp

@@ -1243,26 +1243,6 @@ std::string TextBuffer::GenHTML(const TextAndColor& rows, const int fontHeightPo
 
             for (size_t col = 0; col < rows.text.at(row).length(); col++)
             {
-                // do not include \r nor \n as they don't have attributes
-                // and are not HTML friendly. For line break use '<BR>' instead.
-                const bool isLastCharInRow =
-                    col == rows.text.at(row).length() - 1 ||
-                    rows.text.at(row).at(col + 1) == '\r' ||
-                    rows.text.at(row).at(col + 1) == '\n';
-
-                bool colorChanged = false;
-                if (!fgColor.has_value() || rows.FgAttr.at(row).at(col) != fgColor.value())
-                {
-                    fgColor = rows.FgAttr.at(row).at(col);
-                    colorChanged = true;
-                }
-
-                if (!bkColor.has_value() || rows.BkAttr.at(row).at(col) != bkColor.value())
-                {
-                    bkColor = rows.BkAttr.at(row).at(col);
-                    colorChanged = true;
-                }
-
                 const auto writeAccumulatedChars = [&](bool includeCurrent) {
                     if (col >= startOffset)
                     {
@@ -1289,6 +1269,27 @@ std::string TextBuffer::GenHTML(const TextAndColor& rows, const int fontHeightPo
                     }
                 };
 
+                if (rows.text.at(row).at(col) == '\r' || rows.text.at(row).at(col) == '\n')
+                {
+                    // do not include \r nor \n as they don't have color attributes
+                    // and are not HTML friendly. For line break use '<BR>' instead.
+                    writeAccumulatedChars(false);
+                    break;
+                }
+
+                bool colorChanged = false;
+                if (!fgColor.has_value() || rows.FgAttr.at(row).at(col) != fgColor.value())
+                {
+                    fgColor = rows.FgAttr.at(row).at(col);
+                    colorChanged = true;
+                }
+
+                if (!bkColor.has_value() || rows.BkAttr.at(row).at(col) != bkColor.value())
+                {
+                    bkColor = rows.BkAttr.at(row).at(col);
+                    colorChanged = true;
+                }
+
                 if (colorChanged)
                 {
                     writeAccumulatedChars(false);
@@ -1310,10 +1311,10 @@ std::string TextBuffer::GenHTML(const TextAndColor& rows, const int fontHeightPo
 
                 hasWrittenAnyText = true;
 
-                if (isLastCharInRow)
+                // if this is the last character in the row, flush the whole row
+                if (col == rows.text.at(row).length() - 1)
                 {
                     writeAccumulatedChars(true);
-                    break;
                 }
             }
         }
@@ -1430,24 +1431,6 @@ std::string TextBuffer::GenRTF(const TextAndColor& rows, const int fontHeightPoi
 
             for (size_t col = 0; col < rows.text.at(row).length(); ++col)
             {
-                const bool isLastCharInRow =
-                    col == rows.text.at(row).length() - 1 ||
-                    rows.text.at(row).at(col + 1) == '\r' ||
-                    rows.text.at(row).at(col + 1) == '\n';
-
-                bool colorChanged = false;
-                if (!fgColor.has_value() || rows.FgAttr.at(row).at(col) != fgColor.value())
-                {
-                    fgColor = rows.FgAttr.at(row).at(col);
-                    colorChanged = true;
-                }
-
-                if (!bkColor.has_value() || rows.BkAttr.at(row).at(col) != bkColor.value())
-                {
-                    bkColor = rows.BkAttr.at(row).at(col);
-                    colorChanged = true;
-                }
-
                 const auto writeAccumulatedChars = [&](bool includeCurrent) {
                     if (col >= startOffset)
                     {
@@ -1470,6 +1453,27 @@ std::string TextBuffer::GenRTF(const TextAndColor& rows, const int fontHeightPoi
                     }
                 };
 
+                if (rows.text.at(row).at(col) == '\r' || rows.text.at(row).at(col) == '\n')
+                {
+                    // do not include \r nor \n as they don't have color attributes.
+                    // For line break use \line instead.
+                    writeAccumulatedChars(false);
+                    break;
+                }
+
+                bool colorChanged = false;
+                if (!fgColor.has_value() || rows.FgAttr.at(row).at(col) != fgColor.value())
+                {
+                    fgColor = rows.FgAttr.at(row).at(col);
+                    colorChanged = true;
+                }
+
+                if (!bkColor.has_value() || rows.BkAttr.at(row).at(col) != bkColor.value())
+                {
+                    bkColor = rows.BkAttr.at(row).at(col);
+                    colorChanged = true;
+                }
+
                 if (colorChanged)
                 {
                     writeAccumulatedChars(false);
@@ -1513,10 +1517,10 @@ std::string TextBuffer::GenRTF(const TextAndColor& rows, const int fontHeightPoi
                                    << " ";
                 }
 
-                if (isLastCharInRow)
+                // if this is the last character in the row, flush the whole row
+                if (col == rows.text.at(row).length() - 1)
                 {
                     writeAccumulatedChars(true);
-                    break;
                 }
             }
         }

src/cascadia/TerminalApp/AppLogic.cpp

@@ -27,10 +27,12 @@ namespace winrt
 // !!! IMPORTANT !!!
 // Make sure that these keys are in the same order as the
 // SettingsLoadWarnings/Errors enum is!
-static const std::array<std::wstring_view, 3> settingsLoadWarningsLabels {
+static const std::array<std::wstring_view, 5> settingsLoadWarningsLabels {
     USES_RESOURCE(L"MissingDefaultProfileText"),
     USES_RESOURCE(L"DuplicateProfileText"),
-    USES_RESOURCE(L"UnknownColorSchemeText")
+    USES_RESOURCE(L"UnknownColorSchemeText"),
+    USES_RESOURCE(L"InvalidBackgroundImage"),
+    USES_RESOURCE(L"InvalidIcon")
 };
 static const std::array<std::wstring_view, 2> settingsLoadErrorsLabels {
     USES_RESOURCE(L"NoProfilesText"),

src/cascadia/TerminalApp/CascadiaSettings.cpp

@@ -198,6 +198,10 @@ void CascadiaSettings::_ValidateSettings()
     // just use the hardcoded defaults
     _ValidateAllSchemesExist();
 
+    // Ensure all profile's with specified images resources have valid file path.
+    // This validates icons and background images.
+    _ValidateMediaResources();
+
     // TODO:GH#2548 ensure there's at least one key bound. Display a warning if
     // there's _NO_ keys bound to any actions. That's highly irregular, and
     // likely an indication of an error somehow.
@@ -424,6 +428,64 @@ void CascadiaSettings::_ValidateAllSchemesExist()
     }
 }
 
+// Method Description:
+// - Ensures that all specified images resources (icons and background images) are valid URIs.
+//   This does not verify that the icon or background image files are encoded as an image.
+// Arguments:
+// - <none>
+// Return Value:
+// - <none>
+// - Appends a SettingsLoadWarnings::InvalidBackgroundImage to our list of warnings if
+//   we find any invalid background images.
+// - Appends a SettingsLoadWarnings::InvalidIconImage to our list of warnings if
+//   we find any invalid icon images.
+void CascadiaSettings::_ValidateMediaResources()
+{
+    bool invalidBackground{ false };
+    bool invalidIcon{ false };
+
+    for (auto& profile : _profiles)
+    {
+        if (profile.HasBackgroundImage())
+        {
+            // Attempt to convert the path to a URI, the ctor will throw if it's invalid/unparseable.
+            // This covers file paths on the machine, app data, URLs, and other resource paths.
+            try
+            {
+                winrt::Windows::Foundation::Uri imagePath{ profile.GetExpandedBackgroundImagePath() };
+            }
+            catch (...)
+            {
+                profile.ResetBackgroundImagePath();
+                invalidBackground = true;
+            }
+        }
+
+        if (profile.HasIcon())
+        {
+            try
+            {
+                winrt::Windows::Foundation::Uri imagePath{ profile.GetExpandedIconPath() };
+            }
+            catch (...)
+            {
+                profile.ResetIconPath();
+                invalidIcon = true;
+            }
+        }
+    }
+
+    if (invalidBackground)
+    {
+        _warnings.push_back(::TerminalApp::SettingsLoadWarnings::InvalidBackgroundImage);
+    }
+
+    if (invalidIcon)
+    {
+        _warnings.push_back(::TerminalApp::SettingsLoadWarnings::InvalidIcon);
+    }
+}
+
 // Method Description:
 // - Create a TerminalSettings object for the provided newTerminalArgs. We'll
 //   use the newTerminalArgs to look up the profile that should be used to

src/cascadia/TerminalApp/CascadiaSettings.h

@@ -114,6 +114,7 @@ private:
     void _ReorderProfilesToMatchUserSettingsOrder();
     void _RemoveHiddenProfiles();
     void _ValidateAllSchemesExist();
+    void _ValidateMediaResources();
 
     friend class TerminalAppLocalTests::SettingsTests;
     friend class TerminalAppLocalTests::ProfileTests;

src/cascadia/TerminalApp/Pane.cpp

@@ -358,6 +358,25 @@ void Pane::Close()
     _ClosedHandlers(nullptr, nullptr);
 }
 
+// Method Description:
+// - Prepare this pane to be removed from the UI hierarchy by closing all controls
+//   and connections beneath it.
+void Pane::Shutdown()
+{
+    // Lock the create/close lock so that another operation won't concurrently
+    // modify our tree
+    std::unique_lock lock{ _createCloseLock };
+    if (_IsLeaf())
+    {
+        _control.Close();
+    }
+    else
+    {
+        _firstChild->Shutdown();
+        _secondChild->Shutdown();
+    }
+}
+
 // Method Description:
 // - Get the root UIElement of this pane. There may be a single TermControl as a
 //   child, or an entire tree of grids and panes as children of this element.

src/cascadia/TerminalApp/Pane.h

@@ -64,6 +64,7 @@ public:
                                                                   const winrt::Microsoft::Terminal::TerminalControl::TermControl& control);
     float CalcSnappedDimension(const bool widthOrHeight, const float dimension) const;
 
+    void Shutdown();
     void Close();
 
     WINRT_CALLBACK(Closed, winrt::Windows::Foundation::EventHandler<winrt::Windows::Foundation::IInspectable>);

src/cascadia/TerminalApp/Profile.cpp

@@ -847,6 +847,14 @@ void Profile::SetIconPath(std::wstring_view path)
     _icon.emplace(path);
 }
 
+// Method Description:
+// - Resets the std::optional holding the icon file path string.
+//   HasIcon() will return false after the execution of this function.
+void Profile::ResetIconPath()
+{
+    _icon.reset();
+}
+
 // Method Description:
 // - Returns this profile's icon path, if one is set. Otherwise returns the
 //   empty string. This method will expand any environment variables in the
@@ -880,6 +888,14 @@ winrt::hstring Profile::GetExpandedBackgroundImagePath() const
     return result;
 }
 
+// Method Description:
+// - Resets the std::optional holding the background image file path string.
+//   HasBackgroundImage() will return false after the execution of this function.
+void Profile::ResetBackgroundImagePath()
+{
+    _backgroundImage.reset();
+}
+
 // Method Description:
 // - Returns the name of this profile.
 // Arguments:

src/cascadia/TerminalApp/Profile.h

@@ -89,9 +89,11 @@ public:
     bool HasIcon() const noexcept;
     winrt::hstring GetExpandedIconPath() const;
     void SetIconPath(std::wstring_view path);
+    void ResetIconPath();
 
     bool HasBackgroundImage() const noexcept;
     winrt::hstring GetExpandedBackgroundImagePath() const;
+    void ResetBackgroundImagePath();
 
     CloseOnExitMode GetCloseOnExitMode() const noexcept;
     bool GetSuppressApplicationTitle() const noexcept;

src/cascadia/TerminalApp/Resources/en-US/Resources.resw

@@ -210,4 +210,10 @@ Temporarily using the Windows Terminal default settings.
   <data name="CloseWindowWarningTitle" xml:space="preserve">
     <value>Do you want to close all tabs?</value>
   </data>
+  <data name="InvalidBackgroundImage" xml:space="preserve">
+    <value>Found a profile with an invalid "backgroundImage". Defaulting that profile to have no background image. Make sure that when setting a "backgroundImage", the value is a valid file path to an image.</value>
+  </data>
+  <data name="InvalidIcon" xml:space="preserve">
+    <value>Found a profile with an invalid "icon". Defaulting that profile to have no icon. Make sure that when setting an "icon", the value is a valid file path to an image.</value>
+  </data>
 </root>

src/cascadia/TerminalApp/Tab.cpp

@@ -297,6 +297,13 @@ void Tab::NavigateFocus(const winrt::TerminalApp::Direction& direction)
     _rootPane->NavigateFocus(direction);
 }
 
+// Method Description:
+// - Prepares this tab for being removed from the UI hierarchy by shutting down all active connections.
+void Tab::Shutdown()
+{
+    _rootPane->Shutdown();
+}
+
 // Method Description:
 // - Closes the currently focused pane in this tab. If it's the last pane in
 //   this tab, our Closed event will be fired (at a later time) for anyone

src/cascadia/TerminalApp/Tab.h

@@ -37,6 +37,7 @@ public:
     winrt::hstring GetActiveTitle() const;
     void SetTabText(const winrt::hstring& text);
 
+    void Shutdown();
     void ClosePane();
 
     WINRT_CALLBACK(Closed, winrt::Windows::Foundation::EventHandler<winrt::Windows::Foundation::IInspectable>);

src/cascadia/TerminalApp/TerminalPage.cpp

@@ -757,16 +757,20 @@ namespace winrt::TerminalApp::implementation
     // - tabIndex: the index of the tab to be removed
     void TerminalPage::_RemoveTabViewItemByIndex(uint32_t tabIndex)
     {
+        // Removing the tab from the collection should destroy its control and disconnect its connection,
+        // but it doesn't always do so. The UI tree may still be holding the control and preventing its destruction.
+        auto iterator = _tabs.begin() + tabIndex;
+        (*iterator)->Shutdown();
+
+        _tabs.erase(iterator);
+        _tabView.TabItems().RemoveAt(tabIndex);
+
         // To close the window here, we need to close the hosting window.
-        if (_tabs.size() == 1)
+        if (_tabs.size() == 0)
         {
             _lastTabClosedHandlers(*this, nullptr);
         }
 
-        // Removing the tab from the collection will destroy its control and disconnect its connection.
-        _tabs.erase(_tabs.begin() + tabIndex);
-        _tabView.TabItems().RemoveAt(tabIndex);
-
         auto focusedTabIndex = _GetFocusedTabIndex();
         if (gsl::narrow_cast<int>(tabIndex) == focusedTabIndex)
         {

src/cascadia/TerminalApp/TerminalWarnings.h

@@ -23,7 +23,9 @@ namespace TerminalApp
     {
         MissingDefaultProfile = 0,
         DuplicateProfile = 1,
-        UnknownColorScheme = 2
+        UnknownColorScheme = 2,
+        InvalidBackgroundImage = 3,
+        InvalidIcon = 4
     };
 
     // SettingsLoadWarnings are scenarios where the settings had invalid state

src/cascadia/TerminalControl/TSFInputControl.cpp

@@ -59,23 +59,32 @@ namespace winrt::Microsoft::Terminal::TerminalControl::implementation
         // set the input scope to Text because this control is for any text.
         _editContext.InputScope(Core::CoreTextInputScope::Text);
 
-        _editContext.TextRequested({ this, &TSFInputControl::_textRequestedHandler });
+        _textRequestedRevoker = _editContext.TextRequested(winrt::auto_revoke, { this, &TSFInputControl::_textRequestedHandler });
 
-        _editContext.SelectionRequested({ this, &TSFInputControl::_selectionRequestedHandler });
+        _selectionRequestedRevoker = _editContext.SelectionRequested(winrt::auto_revoke, { this, &TSFInputControl::_selectionRequestedHandler });
 
-        _editContext.FocusRemoved({ this, &TSFInputControl::_focusRemovedHandler });
+        _focusRemovedRevoker = _editContext.FocusRemoved(winrt::auto_revoke, { this, &TSFInputControl::_focusRemovedHandler });
 
-        _editContext.TextUpdating({ this, &TSFInputControl::_textUpdatingHandler });
+        _textUpdatingRevoker = _editContext.TextUpdating(winrt::auto_revoke, { this, &TSFInputControl::_textUpdatingHandler });
 
-        _editContext.SelectionUpdating({ this, &TSFInputControl::_selectionUpdatingHandler });
+        _selectionUpdatingRevoker = _editContext.SelectionUpdating(winrt::auto_revoke, { this, &TSFInputControl::_selectionUpdatingHandler });
 
-        _editContext.FormatUpdating({ this, &TSFInputControl::_formatUpdatingHandler });
+        _formatUpdatingRevoker = _editContext.FormatUpdating(winrt::auto_revoke, { this, &TSFInputControl::_formatUpdatingHandler });
 
-        _editContext.LayoutRequested({ this, &TSFInputControl::_layoutRequestedHandler });
+        _layoutRequestedRevoker = _editContext.LayoutRequested(winrt::auto_revoke, { this, &TSFInputControl::_layoutRequestedHandler });
 
-        _editContext.CompositionStarted({ this, &TSFInputControl::_compositionStartedHandler });
+        _compositionStartedRevoker = _editContext.CompositionStarted(winrt::auto_revoke, { this, &TSFInputControl::_compositionStartedHandler });
 
-        _editContext.CompositionCompleted({ this, &TSFInputControl::_compositionCompletedHandler });
+        _compositionCompletedRevoker = _editContext.CompositionCompleted(winrt::auto_revoke, { this, &TSFInputControl::_compositionCompletedHandler });
+    }
+
+    // Method Description:
+    // - Prepares this TSFInputControl to be removed from the UI hierarchy.
+    void TSFInputControl::Close()
+    {
+        // Explicitly disconnect the LayoutRequested handler -- it can cause problems during application teardown.
+        // See GH#4159 for more info.
+        _layoutRequestedRevoker.revoke();
     }
 
     // Method Description:

src/cascadia/TerminalControl/TSFInputControl.h

@@ -37,6 +37,8 @@ namespace winrt::Microsoft::Terminal::TerminalControl::implementation
         void NotifyFocusEnter();
         void NotifyFocusLeave();
 
+        void Close();
+
         static void OnCompositionChanged(Windows::UI::Xaml::DependencyObject const&, Windows::UI::Xaml::DependencyPropertyChangedEventArgs const&);
 
         // -------------------------------- WinRT Events ---------------------------------
@@ -55,6 +57,16 @@ namespace winrt::Microsoft::Terminal::TerminalControl::implementation
         void _textUpdatingHandler(winrt::Windows::UI::Text::Core::CoreTextEditContext sender, winrt::Windows::UI::Text::Core::CoreTextTextUpdatingEventArgs const& args);
         void _formatUpdatingHandler(winrt::Windows::UI::Text::Core::CoreTextEditContext sender, winrt::Windows::UI::Text::Core::CoreTextFormatUpdatingEventArgs const& args);
 
+        winrt::Windows::UI::Text::Core::CoreTextEditContext::TextRequested_revoker _textRequestedRevoker;
+        winrt::Windows::UI::Text::Core::CoreTextEditContext::SelectionRequested_revoker _selectionRequestedRevoker;
+        winrt::Windows::UI::Text::Core::CoreTextEditContext::FocusRemoved_revoker _focusRemovedRevoker;
+        winrt::Windows::UI::Text::Core::CoreTextEditContext::TextUpdating_revoker _textUpdatingRevoker;
+        winrt::Windows::UI::Text::Core::CoreTextEditContext::SelectionUpdating_revoker _selectionUpdatingRevoker;
+        winrt::Windows::UI::Text::Core::CoreTextEditContext::FormatUpdating_revoker _formatUpdatingRevoker;
+        winrt::Windows::UI::Text::Core::CoreTextEditContext::LayoutRequested_revoker _layoutRequestedRevoker;
+        winrt::Windows::UI::Text::Core::CoreTextEditContext::CompositionStarted_revoker _compositionStartedRevoker;
+        winrt::Windows::UI::Text::Core::CoreTextEditContext::CompositionCompleted_revoker _compositionCompletedRevoker;
+
         Windows::UI::Xaml::Controls::Canvas _canvas;
         Windows::UI::Xaml::Controls::TextBlock _textBlock;
 

src/cascadia/TerminalControl/TSFInputControl.idl

@@ -27,5 +27,7 @@ namespace Microsoft.Terminal.TerminalControl
 
         void NotifyFocusEnter();
         void NotifyFocusLeave();
+
+        void Close();
     }
 }

src/cascadia/TerminalControl/TermControl.cpp

@@ -1725,6 +1725,8 @@ namespace winrt::Microsoft::Terminal::TerminalControl::implementation
             _connection.TerminalOutput(_connectionOutputEventToken);
             _connectionStateChangedRevoker.revoke();
 
+            _tsfInputControl.Close(); // Disconnect the TSF input control so it doesn't receive EditContext events.
+
             if (auto localConnection{ std::exchange(_connection, nullptr) })
             {
                 localConnection.Close();

src/cascadia/TerminalCore/Terminal.cpp

@@ -456,20 +456,33 @@ void Terminal::_WriteBuffer(const std::wstring_view& stringView)
         {
             // TODO: MSFT 21006766
             // This is not great but I need it demoable. Fix by making a buffer stream writer.
-            if (wch >= 0xD800 && wch <= 0xDFFF)
+            //
+            // If wch is a surrogate character we need to read 2 code units
+            // from the stringView to form a single code point.
+            const auto isSurrogate = wch >= 0xD800 && wch <= 0xDFFF;
+            const auto view = stringView.substr(i, isSurrogate ? 2 : 1);
+            const OutputCellIterator it{ view, _buffer->GetCurrentAttributes() };
+            const auto end = _buffer->Write(it);
+            const auto cellDistance = end.GetCellDistance(it);
+            const auto inputDistance = end.GetInputDistance(it);
+
+            if (inputDistance > 0)
             {
-                const OutputCellIterator it{ stringView.substr(i, 2), _buffer->GetCurrentAttributes() };
+                // If "wch" was a surrogate character, we just consumed 2 code units above.
-                const auto end = _buffer->Write(it);
+                // -> Increment "i" by 1 in that case and thus by 2 in total in this iteration.
-                const auto cellDistance = end.GetCellDistance(it);
-                i += cellDistance - 1;
                 proposedCursorPosition.X += gsl::narrow<SHORT>(cellDistance);
+                i += inputDistance - 1;
             }
             else
             {
-                const OutputCellIterator it{ stringView.substr(i, 1), _buffer->GetCurrentAttributes() };
+                // If _WriteBuffer() is called with a consecutive string longer than the viewport/buffer width
-                const auto end = _buffer->Write(it);
+                // the call to _buffer->Write() will refuse to write anything on the current line.
-                const auto cellDistance = end.GetCellDistance(it);
+                // GetInputDistance() thus returns 0, which would in turn cause i to be
-                proposedCursorPosition.X += gsl::narrow<SHORT>(cellDistance);
+                // decremented by 1 below and force the outer loop to loop forever.
+                // This if() basically behaves as if "\r\n" had been encountered above and retries the write.
+                // With well behaving shells during normal operation this safeguard should normally not be encountered.
+                proposedCursorPosition.X = 0;
+                proposedCursorPosition.Y++;
             }
         }
 

src/cascadia/UnitTests_TerminalCore/TerminalApiTest.cpp

@@ -12,6 +12,9 @@
 using namespace winrt::Microsoft::Terminal::Settings;
 using namespace Microsoft::Terminal::Core;
 
+using namespace WEX::Logging;
+using namespace WEX::TestExecution;
+
 namespace TerminalCoreUnitTests
 {
 #define WCS(x) WCSHELPER(x)
@@ -37,5 +40,74 @@ namespace TerminalCoreUnitTests
             VERIFY_IS_FALSE(term.SetColorTableEntry(256, 100));
             VERIFY_IS_FALSE(term.SetColorTableEntry(512, 100));
         }
+
+        // Terminal::_WriteBuffer used to enter infinite loops under certain conditions.
+        // This test ensures that Terminal::_WriteBuffer doesn't get stuck when
+        // PrintString() is called with more code units than the buffer width.
+        TEST_METHOD(PrintStringOfSurrogatePairs)
+        {
+            DummyRenderTarget renderTarget;
+            Terminal term;
+            term.Create({ 100, 100 }, 3, renderTarget);
+
+            std::wstring text;
+            text.reserve(600);
+
+            for (size_t i = 0; i < 100; ++i)
+            {
+                text.append(L"𐐌𐐜𐐬");
+            }
+
+            struct Baton
+            {
+                HANDLE done;
+                std::wstring text;
+                Terminal* pTerm;
+            } baton{
+                CreateEventW(nullptr, TRUE, FALSE, L"done signal"),
+                text,
+                &term,
+            };
+
+            Log::Comment(L"Launching thread to write data.");
+            const auto thread = CreateThread(
+                nullptr,
+                0,
+                [](LPVOID data) -> DWORD {
+                    const Baton& baton = *reinterpret_cast<Baton*>(data);
+                    Log::Comment(L"Writing data.");
+                    baton.pTerm->PrintString(baton.text);
+                    Log::Comment(L"Setting event.");
+                    SetEvent(baton.done);
+                    return 0;
+                },
+                (LPVOID)&baton,
+                0,
+                nullptr);
+
+            Log::Comment(L"Waiting for the write.");
+            switch (WaitForSingleObject(baton.done, 2000))
+            {
+            case WAIT_OBJECT_0:
+                Log::Comment(L"Didn't get stuck. Success.");
+                break;
+            case WAIT_TIMEOUT:
+                Log::Comment(L"Wait timed out. It got stuck.");
+                Log::Result(WEX::Logging::TestResults::Failed);
+                break;
+            case WAIT_FAILED:
+                Log::Comment(L"Wait failed for some reason. We didn't expect this.");
+                Log::Result(WEX::Logging::TestResults::Failed);
+                break;
+            default:
+                Log::Comment(L"Wait return code that no one expected. Fail.");
+                Log::Result(WEX::Logging::TestResults::Failed);
+                break;
+            }
+
+            TerminateThread(thread, 0);
+            CloseHandle(baton.done);
+            return;
+        }
     };
 }

src/host/getset.cpp

@@ -1407,67 +1407,6 @@ void DoSrvPrivateAllowCursorBlinking(SCREEN_INFORMATION& screenInfo, const bool
     return Status;
 }
 
-// Routine Description:
-// - A private API call for moving the cursor vertically in the buffer. This is
-//      because the vertical cursor movements in VT are constrained by the
-//      scroll margins, while the absolute positioning is not.
-// Parameters:
-// - screenInfo - a reference to the screen buffer we should move the cursor for
-// - lines - The number of lines to move the cursor. Up is negative, down positive.
-// Return value:
-// - S_OK if handled successfully. Otherwise an appropriate HRESULT for failing to clamp.
-[[nodiscard]] HRESULT DoSrvMoveCursorVertically(SCREEN_INFORMATION& screenInfo, const short lines)
-{
-    auto& cursor = screenInfo.GetActiveBuffer().GetTextBuffer().GetCursor();
-    COORD clampedPos = { cursor.GetPosition().X, cursor.GetPosition().Y + lines };
-
-    // Make sure the cursor doesn't move outside the viewport.
-    screenInfo.GetViewport().Clamp(clampedPos);
-
-    // Make sure the cursor stays inside the margins
-    if (screenInfo.AreMarginsSet())
-    {
-        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)
-        {
-            try
-            {
-                clampedPos.Y = std::min(clampedPos.Y, hi);
-            }
-            CATCH_RETURN();
-        }
-    }
-    cursor.SetPosition(clampedPos);
-
-    return S_OK;
-}
-
 // Routine Description:
 // - A private API call for swapping to the alternate screen buffer. In virtual terminals, there exists both a "main"
 //     screen buffer and an alternate. ASBSET creates a new alternate, and switches to it. If there is an already

src/host/getset.h

@@ -34,7 +34,6 @@ void DoSrvPrivateAllowCursorBlinking(SCREEN_INFORMATION& screenInfo, const bool
 
 [[nodiscard]] NTSTATUS DoSrvPrivateSetScrollingRegion(SCREEN_INFORMATION& screenInfo, const SMALL_RECT& scrollMargins);
 [[nodiscard]] NTSTATUS DoSrvPrivateReverseLineFeed(SCREEN_INFORMATION& screenInfo);
-[[nodiscard]] HRESULT DoSrvMoveCursorVertically(SCREEN_INFORMATION& screenInfo, const short lines);
 
 [[nodiscard]] NTSTATUS DoSrvPrivateUseAlternateScreenBuffer(SCREEN_INFORMATION& screenInfo);
 void DoSrvPrivateUseMainScreenBuffer(SCREEN_INFORMATION& screenInfo);

src/host/outputStream.cpp

@@ -397,23 +397,6 @@ bool ConhostInternalGetSet::PrivateReverseLineFeed()
     return NT_SUCCESS(DoSrvPrivateReverseLineFeed(_io.GetActiveOutputBuffer()));
 }
 
-// Routine Description:
-// - Connects the MoveCursorVertically call directly into our Driver Message servicing call inside Conhost.exe
-//   MoveCursorVertically is an internal-only "API" call that the vt commands can execute,
-//     but it is not represented as a function call on out public API surface.
-// Return Value:
-// - true if successful (see DoSrvMoveCursorVertically). false otherwise.
-bool ConhostInternalGetSet::MoveCursorVertically(const ptrdiff_t lines)
-{
-    SHORT l;
-    if (FAILED(PtrdiffTToShort(lines, &l)))
-    {
-        return false;
-    }
-
-    return SUCCEEDED(DoSrvMoveCursorVertically(_io.GetActiveOutputBuffer(), l));
-}
-
 // Routine Description:
 // - Connects the SetConsoleTitleW API call directly into our Driver Message servicing call inside Conhost.exe
 // Arguments:

src/host/outputStream.hpp

@@ -100,8 +100,6 @@ public:
 
     bool PrivateReverseLineFeed() override;
 
-    bool MoveCursorVertically(const ptrdiff_t lines) override;
-
     bool SetConsoleTitleW(const std::wstring_view title) override;
 
     bool PrivateUseAlternateScreenBuffer() override;

src/host/ut_host/ScreenBufferTests.cpp

@@ -199,6 +199,8 @@ class ScreenBufferTests
     TEST_METHOD(CursorUpDownOutsideMargins);
     TEST_METHOD(CursorUpDownExactlyAtMargins);
 
+    TEST_METHOD(CursorNextPreviousLine);
+
     TEST_METHOD(CursorSaveRestore);
 
     TEST_METHOD(ScreenAlignmentPattern);
@@ -5295,6 +5297,70 @@ void ScreenBufferTests::CursorUpDownExactlyAtMargins()
     stateMachine.ProcessString(L"\x1b[r");
 }
 
+void ScreenBufferTests::CursorNextPreviousLine()
+{
+    auto& gci = ServiceLocator::LocateGlobals().getConsoleInformation();
+    auto& si = gci.GetActiveOutputBuffer();
+    auto& stateMachine = si.GetStateMachine();
+    auto& cursor = si.GetTextBuffer().GetCursor();
+
+    Log::Comment(L"Make sure the viewport is at 0,0");
+    VERIFY_SUCCEEDED(si.SetViewportOrigin(true, COORD({ 0, 0 }), true));
+
+    Log::Comment(L"CNL without margins");
+    // Starting from column 20 of line 10.
+    cursor.SetPosition(COORD{ 20, 10 });
+    // Move down 5 lines (CNL).
+    stateMachine.ProcessString(L"\x1b[5E");
+    // We should end up in column 0 of line 15.
+    VERIFY_ARE_EQUAL(COORD({ 0, 15 }), cursor.GetPosition());
+
+    Log::Comment(L"CPL without margins");
+    // Starting from column 20 of line 10.
+    cursor.SetPosition(COORD{ 20, 10 });
+    // Move up 5 lines (CPL).
+    stateMachine.ProcessString(L"\x1b[5F");
+    // We should end up in column 0 of line 5.
+    VERIFY_ARE_EQUAL(COORD({ 0, 5 }), cursor.GetPosition());
+
+    // Set the margins to 8:12 (9:13 in VT coordinates).
+    stateMachine.ProcessString(L"\x1b[9;13r");
+    // 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"CNL inside margins");
+    // Starting from column 20 of line 10.
+    cursor.SetPosition(COORD{ 20, 10 });
+    // Move down 5 lines (CNL).
+    stateMachine.ProcessString(L"\x1b[5E");
+    // We should stop on line 12, the bottom margin.
+    VERIFY_ARE_EQUAL(COORD({ 0, 12 }), cursor.GetPosition());
+
+    Log::Comment(L"CPL inside margins");
+    // Starting from column 20 of line 10.
+    cursor.SetPosition(COORD{ 20, 10 });
+    // Move up 5 lines (CPL).
+    stateMachine.ProcessString(L"\x1b[5F");
+    // We should stop on line 8, the top margin.
+    VERIFY_ARE_EQUAL(COORD({ 0, 8 }), cursor.GetPosition());
+
+    Log::Comment(L"CNL below bottom");
+    // Starting from column 20 of line 13 (1 below bottom margin).
+    cursor.SetPosition(COORD{ 20, 13 });
+    // Move down 5 lines (CNL).
+    stateMachine.ProcessString(L"\x1b[5E");
+    // We should end up in column 0 of line 18.
+    VERIFY_ARE_EQUAL(COORD({ 0, 18 }), cursor.GetPosition());
+
+    Log::Comment(L"CPL above top margin");
+    // Starting from column 20 of line 7 (1 above top margin).
+    cursor.SetPosition(COORD{ 20, 7 });
+    // Move up 5 lines (CPL).
+    stateMachine.ProcessString(L"\x1b[5F");
+    // We should end up in column 0 of line 2.
+    VERIFY_ARE_EQUAL(COORD({ 0, 2 }), cursor.GetPosition());
+}
+
 void ScreenBufferTests::CursorSaveRestore()
 {
     auto& gci = ServiceLocator::LocateGlobals().getConsoleInformation();

src/renderer/dx/CustomTextLayout.cpp

@@ -404,8 +404,30 @@ CustomTextLayout::CustomTextLayout(gsl::not_null<IDWriteFactory1*> const factory
             // Offsets is how far to move the origin (in pixels) from where it is
             auto& offset = _glyphOffsets.at(i);
 
-            // Get how many columns we expected the glyph to have and mutiply into pixels.
+            // Get how many columns we expected the glyph to have and multiply into pixels.
-            const auto columns = _textClusterColumns.at(i);
+            UINT16 columns = 0;
+            {
+                // Because of typographic features such as ligatures, it is well possible for a glyph to represent
+                //  multiple code points. Previous calls to IDWriteTextAnalyzer::GetGlyphs stores the mapping
+                //  information between code points and glyphs in _glyphClusters.
+                // To properly allocate the columns for such glyphs, we need to find all characters that this glyph
+                //  is representing and add column counts for all the characters together.
+
+                // Find the range for current glyph run in _glyphClusters.
+                const auto runStartIterator = _glyphClusters.begin() + run.textStart;
+                const auto runEndIterator = _glyphClusters.begin() + run.textStart + run.textLength;
+
+                // Find the range of characters that the current glyph is representing.
+                const auto firstIterator = std::find(runStartIterator, runEndIterator, i - run.glyphStart);
+                const auto lastIterator = std::find(runStartIterator, runEndIterator, i - run.glyphStart + 1);
+
+                // Add all allocated column counts together.
+                for (auto j = firstIterator; j < lastIterator; j++)
+                {
+                    const auto charIndex = std::distance(_glyphClusters.begin(), j);
+                    columns += _textClusterColumns.at(charIndex);
+                }
+            }
             const auto advanceExpected = static_cast<float>(columns * _width);
 
             // If what we expect is bigger than what we have... pad it out.

src/terminal/adapter/adaptDispatch.cpp

@@ -81,137 +81,6 @@ void AdaptDispatch::PrintString(const std::wstring_view string)
     CATCH_LOG();
 }
 
-// Routine Description:
-// - Generalizes cursor movement for up/down/left/right and next/previous line.
-// Arguments:
-// - dir - Specific direction to move
-// - distance - Magnitude of the move
-// Return Value:
-// - True if handled successfully. False otherwise.
-bool AdaptDispatch::_CursorMovement(const CursorDirection dir, const size_t distance) const
-{
-    // First retrieve some information about the buffer
-    CONSOLE_SCREEN_BUFFER_INFOEX csbiex = { 0 };
-    csbiex.cbSize = sizeof(CONSOLE_SCREEN_BUFFER_INFOEX);
-    // Make sure to reset the viewport (with MoveToBottom )to where it was
-    //      before the user scrolled the console output
-    bool success = (_pConApi->MoveToBottom() && _pConApi->GetConsoleScreenBufferInfoEx(csbiex));
-
-    if (success)
-    {
-        COORD cursor = csbiex.dwCursorPosition;
-
-        // For next/previous line, we unconditionally need to move the X position to the left edge of the viewport.
-        switch (dir)
-        {
-        case CursorDirection::NextLine:
-        case CursorDirection::PrevLine:
-            cursor.X = csbiex.srWindow.Left;
-            break;
-        }
-
-        // Safely convert the UINT magnitude of the move we were given into a short (which is the size the console deals with)
-        SHORT delta = 0;
-        success = SUCCEEDED(SizeTToShort(distance, &delta));
-
-        if (success)
-        {
-            // Prepare our variables for math. All operations are some variation on these two parameters
-            SHORT* pModify = nullptr; // The coordinate X or Y gets modified
-            SHORT boundary = 0; // There is a particular edge of the viewport that is our boundary condition as we approach it.
-
-            // Up and Down modify the Y coordinate. Left and Right modify the X.
-            switch (dir)
-            {
-            case CursorDirection::Up:
-            case CursorDirection::Down:
-            case CursorDirection::NextLine:
-            case CursorDirection::PrevLine:
-                pModify = &cursor.Y;
-                break;
-            case CursorDirection::Left:
-            case CursorDirection::Right:
-                pModify = &cursor.X;
-                break;
-            default:
-                success = false;
-                break;
-            }
-
-            // Moving upward is bounded by top, etc.
-            switch (dir)
-            {
-            case CursorDirection::Up:
-            case CursorDirection::PrevLine:
-                boundary = csbiex.srWindow.Top;
-                break;
-            case CursorDirection::Down:
-            case CursorDirection::NextLine:
-                boundary = csbiex.srWindow.Bottom;
-                break;
-            case CursorDirection::Left:
-                boundary = csbiex.srWindow.Left;
-                break;
-            case CursorDirection::Right:
-                boundary = csbiex.srWindow.Right;
-                break;
-            default:
-                success = false;
-                break;
-            }
-
-            if (success && pModify)
-            {
-                // For up and left, we need to subtract the magnitude of the vector to get the new spot. Right/down = add.
-                // Use safe short subtraction to prevent under/overflow.
-                switch (dir)
-                {
-                case CursorDirection::Up:
-                case CursorDirection::Left:
-                case CursorDirection::PrevLine:
-                    success = SUCCEEDED(ShortSub(*pModify, delta, pModify));
-                    break;
-                case CursorDirection::Down:
-                case CursorDirection::Right:
-                case CursorDirection::NextLine:
-                    success = SUCCEEDED(ShortAdd(*pModify, delta, pModify));
-                    break;
-                }
-
-                if (success)
-                {
-                    // Now apply the boundary condition. Up, Left can't be smaller than their boundary. Top, Right can't be larger.
-                    switch (dir)
-                    {
-                    case CursorDirection::Up:
-                    case CursorDirection::Left:
-                    case CursorDirection::PrevLine:
-                        *pModify = std::max(*pModify, boundary);
-                        break;
-                    case CursorDirection::Down:
-                    case CursorDirection::Right:
-                    case CursorDirection::NextLine:
-                        // For the bottom and right edges, the viewport value is stated to be one outside the rectangle.
-                        *pModify = std::min(*pModify, gsl::narrow<SHORT>(boundary - 1));
-                        break;
-                    default:
-                        success = false;
-                        break;
-                    }
-
-                    if (success)
-                    {
-                        // Finally, attempt to set the adjusted cursor position back into the console.
-                        success = _pConApi->SetConsoleCursorPosition(cursor);
-                    }
-                }
-            }
-        }
-    }
-
-    return success;
-}
-
 // Routine Description:
 // - CUU - Handles cursor upward movement by given distance.
 // CUU and CUD are handled seperately from other CUP sequences, because they are
@@ -225,12 +94,7 @@ bool AdaptDispatch::_CursorMovement(const CursorDirection dir, const size_t dist
 // - True if handled successfully. False otherwise.
 bool AdaptDispatch::CursorUp(const size_t distance)
 {
-    SHORT sDelta = 0;
+    return _CursorMovePosition(Offset::Backward(distance), Offset::Unchanged(), true);
-    if (SUCCEEDED(SizeTToShort(distance, &sDelta)))
-    {
-        return _pConApi->MoveCursorVertically(-sDelta);
-    }
-    return false;
 }
 
 // Routine Description:
@@ -246,12 +110,7 @@ bool AdaptDispatch::CursorUp(const size_t distance)
 // - True if handled successfully. False otherwise.
 bool AdaptDispatch::CursorDown(const size_t distance)
 {
-    SHORT sDelta = 0;
+    return _CursorMovePosition(Offset::Forward(distance), Offset::Unchanged(), true);
-    if (SUCCEEDED(SizeTToShort(distance, &sDelta)))
-    {
-        return _pConApi->MoveCursorVertically(sDelta);
-    }
-    return false;
 }
 
 // Routine Description:
@@ -262,7 +121,7 @@ bool AdaptDispatch::CursorDown(const size_t distance)
 // - True if handled successfully. False otherwise.
 bool AdaptDispatch::CursorForward(const size_t distance)
 {
-    return _CursorMovement(CursorDirection::Right, distance);
+    return _CursorMovePosition(Offset::Unchanged(), Offset::Forward(distance), true);
 }
 
 // Routine Description:
@@ -273,7 +132,7 @@ bool AdaptDispatch::CursorForward(const size_t distance)
 // - True if handled successfully. False otherwise.
 bool AdaptDispatch::CursorBackward(const size_t distance)
 {
-    return _CursorMovement(CursorDirection::Left, distance);
+    return _CursorMovePosition(Offset::Unchanged(), Offset::Backward(distance), true);
 }
 
 // Routine Description:
@@ -285,7 +144,7 @@ bool AdaptDispatch::CursorBackward(const size_t distance)
 // - True if handled successfully. False otherwise.
 bool AdaptDispatch::CursorNextLine(const size_t distance)
 {
-    return _CursorMovement(CursorDirection::NextLine, distance);
+    return _CursorMovePosition(Offset::Forward(distance), Offset::Absolute(1), true);
 }
 
 // Routine Description:
@@ -297,18 +156,18 @@ bool AdaptDispatch::CursorNextLine(const size_t distance)
 // - True if handled successfully. False otherwise.
 bool AdaptDispatch::CursorPrevLine(const size_t distance)
 {
-    return _CursorMovement(CursorDirection::PrevLine, distance);
+    return _CursorMovePosition(Offset::Backward(distance), Offset::Absolute(1), true);
 }
 
 // Routine Description:
-// - Generalizes cursor movement to a specific coordinate position
+// - Generalizes cursor movement to a specific position, which can be absolute or relative.
-// - If a parameter is left blank, we will maintain the existing position in that dimension.
 // Arguments:
-// - row - Optional row to move to
+// - rowOffset - The row to move to
-// - column - Optional column to move to
+// - colOffset - The column to move to
+// - clampInMargins - Should the position be clamped within the scrolling margins
 // Return Value:
 // - True if handled successfully. False otherwise.
-bool AdaptDispatch::_CursorMovePosition(const std::optional<size_t> row, const std::optional<size_t> column) const
+bool AdaptDispatch::_CursorMovePosition(const Offset rowOffset, const Offset colOffset, const bool clampInMargins) const
 {
     bool success = true;
 
@@ -321,81 +180,67 @@ bool AdaptDispatch::_CursorMovePosition(const std::optional<size_t> row, const s
 
     if (success)
     {
-        // handle optional parameters. If not specified, keep same cursor position from what we just loaded.
+        // Calculate the viewport boundaries as inclusive values.
-        size_t rowActual = 0;
+        // srWindow is exclusive so we need to subtract 1 from the bottom.
-        size_t columnActual = 0;
+        const int viewportTop = csbiex.srWindow.Top;
+        const int viewportBottom = csbiex.srWindow.Bottom - 1;
 
-        if (row)
+        // Calculate the absolute margins of the scrolling area.
-        {
+        const int topMargin = viewportTop + _scrollMargins.Top;
-            if (row.value() != 0)
+        const int bottomMargin = viewportTop + _scrollMargins.Bottom;
-            {
+        const bool marginsSet = topMargin < bottomMargin;
-                rowActual = row.value() - 1; // In VT, the origin is 1,1. For our array, it's 0,0. So subtract 1.
 
-                // If the origin mode is relative, and the scrolling region is set (the bottom is non-zero),
+        // For relative movement, the given offsets will be relative to
-                // line numbers start at the top margin of the scrolling region, and cannot move below the bottom.
+        // the current cursor position.
-                if (_isOriginModeRelative && _scrollMargins.Bottom != 0)
+        int row = csbiex.dwCursorPosition.Y;
-                {
+        int col = csbiex.dwCursorPosition.X;
-                    rowActual += _scrollMargins.Top;
+
-                    rowActual = std::min<decltype(rowActual)>(rowActual, _scrollMargins.Bottom);
+        // But if the row is absolute, it will be relative to the top of the
-                }
+        // viewport, or the top margin, depending on the origin mode.
-            }
+        if (rowOffset.IsAbsolute)
-            else
-            {
-                success = false; // The parser should never return 0 (0 maps to 1), so this is a failure condition.
-            }
-        }
-        else
         {
-            // remember, in VT speak, this is relative to the viewport. not absolute.
+            row = _isOriginModeRelative ? topMargin : viewportTop;
-            SHORT diff = 0;
-            success = SUCCEEDED(ShortSub(csbiex.dwCursorPosition.Y, csbiex.srWindow.Top, &diff)) && SUCCEEDED(ShortToSizeT(diff, &rowActual));
         }
 
-        if (success)
+        // And if the column is absolute, it'll be relative to column 0.
+        // Horizontal positions are not affected by the viewport.
+        if (colOffset.IsAbsolute)
         {
-            if (column)
+            col = 0;
+        }
+
+        // Adjust the base position by the given offsets and clamp the results.
+        // The row is constrained within the viewport's vertical boundaries,
+        // while the column is constrained by the buffer width.
+        row = std::clamp(row + rowOffset.Value, viewportTop, viewportBottom);
+        col = std::clamp(col + colOffset.Value, 0, csbiex.dwSize.X - 1);
+
+        // If the operation needs to be clamped inside the margins, or the origin
+        // mode is relative (which always requires margin clamping), then the row
+        // may need to be adjusted further.
+        if (marginsSet && (clampInMargins || _isOriginModeRelative))
+        {
+            // 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.
+            if (csbiex.dwCursorPosition.Y >= topMargin)
             {
-                if (column.value() != 0)
+                row = std::max(row, topMargin);
-                {
-                    columnActual = column.value() - 1; // In VT, the origin is 1,1. For our array, it's 0,0. So subtract 1.
-                }
-                else
-                {
-                    success = false; // The parser should never return 0 (0 maps to 1), so this is a failure condition.
-                }
             }
-            else
+            if (csbiex.dwCursorPosition.Y <= bottomMargin)
             {
-                // remember, in VT speak, this is relative to the viewport. not absolute.
+                row = std::min(row, bottomMargin);
-                SHORT diff = 0;
-                success = SUCCEEDED(ShortSub(csbiex.dwCursorPosition.X, csbiex.srWindow.Left, &diff)) && SUCCEEDED(ShortToSizeT(diff, &columnActual));
             }
         }
 
-        if (success)
+        // Finally, attempt to set the adjusted cursor position back into the console.
-        {
+        const COORD newPos = { gsl::narrow_cast<SHORT>(col), gsl::narrow_cast<SHORT>(row) };
-            COORD cursor = csbiex.dwCursorPosition;
+        success = _pConApi->SetConsoleCursorPosition(newPos);
-
-            // Safely convert the size_t positions we were given into shorts (which is the size the console deals with)
-            success = SUCCEEDED(SizeTToShort(rowActual, &cursor.Y)) && SUCCEEDED(SizeTToShort(columnActual, &cursor.X));
-
-            if (success)
-            {
-                // Set the line and column values as offsets from the viewport edge. Use safe math to prevent overflow.
-                success = SUCCEEDED(ShortAdd(cursor.Y, csbiex.srWindow.Top, &cursor.Y)) &&
-                          SUCCEEDED(ShortAdd(cursor.X, csbiex.srWindow.Left, &cursor.X));
-
-                if (success)
-                {
-                    // Apply boundary tests to ensure the cursor isn't outside the viewport rectangle.
-                    cursor.Y = std::clamp(cursor.Y, csbiex.srWindow.Top, gsl::narrow<SHORT>(csbiex.srWindow.Bottom - 1));
-                    cursor.X = std::clamp(cursor.X, csbiex.srWindow.Left, gsl::narrow<SHORT>(csbiex.srWindow.Right - 1));
-
-                    // Finally, attempt to set the adjusted cursor position back into the console.
-                    success = _pConApi->SetConsoleCursorPosition(cursor);
-                }
-            }
-        }
     }
 
     return success;
@@ -409,7 +254,7 @@ bool AdaptDispatch::_CursorMovePosition(const std::optional<size_t> row, const s
 // - True if handled successfully. False otherwise.
 bool AdaptDispatch::CursorHorizontalPositionAbsolute(const size_t column)
 {
-    return _CursorMovePosition(std::nullopt, column);
+    return _CursorMovePosition(Offset::Unchanged(), Offset::Absolute(column), false);
 }
 
 // Routine Description:
@@ -420,7 +265,7 @@ bool AdaptDispatch::CursorHorizontalPositionAbsolute(const size_t column)
 // - True if handled successfully. False otherwise.
 bool AdaptDispatch::VerticalLinePositionAbsolute(const size_t line)
 {
-    return _CursorMovePosition(line, std::nullopt);
+    return _CursorMovePosition(Offset::Absolute(line), Offset::Unchanged(), false);
 }
 
 // Routine Description:
@@ -432,7 +277,7 @@ bool AdaptDispatch::VerticalLinePositionAbsolute(const size_t line)
 // - True if handled successfully. False otherwise.
 bool AdaptDispatch::CursorPosition(const size_t line, const size_t column)
 {
-    return _CursorMovePosition(line, column);
+    return _CursorMovePosition(Offset::Absolute(line), Offset::Absolute(column), false);
 }
 
 // Routine Description:
@@ -458,15 +303,14 @@ bool AdaptDispatch::CursorSaveState()
     if (success)
     {
         // The cursor is given to us by the API as relative to the whole buffer.
-        // But in VT speak, the cursor should be relative to the current viewport. Adjust.
+        // But in VT speak, the cursor row should be relative to the current viewport top.
-        COORD const coordCursor = csbiex.dwCursorPosition;
+        COORD coordCursor = csbiex.dwCursorPosition;
-
+        coordCursor.Y -= csbiex.srWindow.Top;
-        SMALL_RECT const srViewport = csbiex.srWindow;
 
         // VT is also 1 based, not 0 based, so correct by 1.
         auto& savedCursorState = _savedCursorState.at(_usingAltBuffer);
-        savedCursorState.Column = coordCursor.X - srViewport.Left + 1;
+        savedCursorState.Column = coordCursor.X + 1;
-        savedCursorState.Row = coordCursor.Y - srViewport.Top + 1;
+        savedCursorState.Row = coordCursor.Y + 1;
         savedCursorState.IsOriginModeRelative = _isOriginModeRelative;
         savedCursorState.Attributes = attributes;
         savedCursorState.TermOutput = _termOutput;
@@ -488,7 +332,7 @@ bool AdaptDispatch::CursorRestoreState()
     auto& savedCursorState = _savedCursorState.at(_usingAltBuffer);
 
     auto row = savedCursorState.Row;
-    auto col = savedCursorState.Column;
+    const auto col = savedCursorState.Column;
 
     // If the origin mode is relative, and the scrolling region is set (the bottom is non-zero),
     // we need to make sure the restored position is clamped within the margins.
@@ -500,7 +344,7 @@ bool AdaptDispatch::CursorRestoreState()
 
     // The saved coordinates are always absolute, so we need reset the origin mode temporarily.
     _isOriginModeRelative = false;
-    bool success = _CursorMovePosition(row, col);
+    bool success = CursorPosition(row, col);
 
     // Once the cursor position is restored, we can then restore the actual origin mode.
     _isOriginModeRelative = savedCursorState.IsOriginModeRelative;
@@ -851,8 +695,7 @@ bool AdaptDispatch::_CursorPositionReport() const
         // First pull the cursor position relative to the entire buffer out of the console.
         COORD coordCursorPos = csbiex.dwCursorPosition;
 
-        // Now adjust it for its position in respect to the current viewport.
+        // Now adjust it for its position in respect to the current viewport top.
-        coordCursorPos.X -= csbiex.srWindow.Left;
         coordCursorPos.Y -= csbiex.srWindow.Top;
 
         // NOTE: 1,1 is the top-left corner of the viewport in VT-speak, so add 1.

src/terminal/adapter/adaptDispatch.hpp

@@ -100,15 +100,6 @@ namespace Microsoft::Console::VirtualTerminal
                                 const std::basic_string_view<size_t> parameters) override; // DTTERM_WindowManipulation
 
     private:
-        enum class CursorDirection
-        {
-            Up,
-            Down,
-            Left,
-            Right,
-            NextLine,
-            PrevLine
-        };
         enum class ScrollDirection
         {
             Up,
@@ -122,9 +113,18 @@ namespace Microsoft::Console::VirtualTerminal
             TextAttribute Attributes = {};
             TerminalOutput TermOutput = {};
         };
+        struct Offset
+        {
+            int Value;
+            bool IsAbsolute;
+            // VT origin is at 1,1 so we need to subtract 1 from absolute positions.
+            static constexpr Offset Absolute(const size_t value) { return { gsl::narrow_cast<int>(value) - 1, true }; };
+            static constexpr Offset Forward(const size_t value) { return { gsl::narrow_cast<int>(value), false }; };
+            static constexpr Offset Backward(const size_t value) { return { -gsl::narrow_cast<int>(value), false }; };
+            static constexpr Offset Unchanged() { return Forward(0); };
+        };
 
-        bool _CursorMovement(const CursorDirection dir, const size_t distance) const;
+        bool _CursorMovePosition(const Offset rowOffset, const Offset colOffset, const bool clampInMargins) const;
-        bool _CursorMovePosition(const std::optional<size_t> row, const std::optional<size_t> column) const;
         bool _EraseSingleLineHelper(const CONSOLE_SCREEN_BUFFER_INFOEX& csbiex,
                                     const DispatchTypes::EraseType eraseType,
                                     const size_t lineId) const;

src/terminal/adapter/conGetSet.hpp

@@ -91,8 +91,6 @@ namespace Microsoft::Console::VirtualTerminal
         virtual bool PrivateSuppressResizeRepaint() = 0;
         virtual bool IsConsolePty(bool& isPty) const = 0;
 
-        virtual bool MoveCursorVertically(const ptrdiff_t lines) = 0;
-
         virtual bool DeleteLines(const size_t count) = 0;
         virtual bool InsertLines(const size_t count) = 0;
 

src/terminal/adapter/ut_adapter/adapterTest.cpp

@@ -376,19 +376,6 @@ public:
         return TRUE;
     }
 
-    bool MoveCursorVertically(const ptrdiff_t lines) override
-    {
-        Log::Comment(L"MoveCursorVertically MOCK called...");
-        short l;
-        VERIFY_SUCCEEDED(PtrdiffTToShort(lines, &l));
-        if (_moveCursorVerticallyResult)
-        {
-            VERIFY_ARE_EQUAL(_expectedLines, l);
-            _cursorPos = { _cursorPos.X, _cursorPos.Y + l };
-        }
-        return !!_moveCursorVerticallyResult;
-    }
-
     bool SetConsoleTitleW(const std::wstring_view title)
     {
         Log::Comment(L"SetConsoleTitleW MOCK called...");
@@ -745,8 +732,6 @@ public:
         // Attribute default is gray on black.
         _attribute = FOREGROUND_BLUE | FOREGROUND_GREEN | FOREGROUND_RED;
         _expectedAttribute = _attribute;
-
-        _expectedLines = 0;
     }
 
     void PrepCursor(CursorX xact, CursorY yact)
@@ -756,16 +741,16 @@ public:
         switch (xact)
         {
         case CursorX::LEFT:
-            Log::Comment(L"Cursor set to left edge of viewport.");
+            Log::Comment(L"Cursor set to left edge of buffer.");
-            _cursorPos.X = _viewport.Left;
+            _cursorPos.X = 0;
             break;
         case CursorX::RIGHT:
-            Log::Comment(L"Cursor set to right edge of viewport.");
+            Log::Comment(L"Cursor set to right edge of buffer.");
-            _cursorPos.X = _viewport.Right - 1;
+            _cursorPos.X = _bufferSize.X - 1;
             break;
         case CursorX::XCENTER:
-            Log::Comment(L"Cursor set to centered X of viewport.");
+            Log::Comment(L"Cursor set to centered X of buffer.");
-            _cursorPos.X = _viewport.Left + ((_viewport.Right - _viewport.Left) / 2);
+            _cursorPos.X = _bufferSize.X / 2;
             break;
         }
 
@@ -862,7 +847,6 @@ public:
     bool _expectedMeta = false;
     unsigned int _expectedOutputCP = 0;
     bool _isPty = false;
-    short _expectedLines = 0;
     bool _privateBoldTextResult = false;
     bool _expectedIsBold = false;
     bool _isBold = false;
@@ -921,7 +905,6 @@ public:
     COLORREF _expectedCursorColor = 0;
     bool _getConsoleOutputCPResult = false;
     bool _isConsolePtyResult = false;
-    bool _moveCursorVerticallyResult = false;
     bool _privateSetDefaultAttributesResult = false;
     bool _moveToBottomResult = false;
 
@@ -1041,24 +1024,6 @@ public:
         Log::Comment(L"Test 1: Cursor doesn't move when placed in corner of viewport.");
         _testGetSet->PrepData(direction);
 
-        switch (direction)
-        {
-        case CursorDirection::UP:
-            Log::Comment(L"Testing up direction.");
-            _testGetSet->_expectedLines = -1;
-            _testGetSet->_moveCursorVerticallyResult = true;
-            break;
-        case CursorDirection::DOWN:
-            Log::Comment(L"Testing down direction.");
-            _testGetSet->_expectedLines = 1;
-            _testGetSet->_moveCursorVerticallyResult = true;
-            break;
-        default:
-            _testGetSet->_expectedLines = 0;
-            _testGetSet->_moveCursorVerticallyResult = false;
-            break;
-        }
-
         VERIFY_IS_TRUE((_pDispatch.get()->*(moveFunc))(1));
 
         Log::Comment(L"Test 1b: Cursor moves to left of line with next/prev line command when cursor can't move higher/lower.");
@@ -1079,7 +1044,7 @@ public:
 
         if (fDoTest1b)
         {
-            _testGetSet->_expectedCursorPos.X = _testGetSet->_viewport.Left;
+            _testGetSet->_expectedCursorPos.X = 0;
             VERIFY_IS_TRUE((_pDispatch.get()->*(moveFunc))(1));
         }
         else
@@ -1095,13 +1060,9 @@ public:
         {
         case CursorDirection::UP:
             _testGetSet->_expectedCursorPos.Y--;
-            _testGetSet->_expectedLines = -1;
-            _testGetSet->_moveCursorVerticallyResult = true;
             break;
         case CursorDirection::DOWN:
             _testGetSet->_expectedCursorPos.Y++;
-            _testGetSet->_expectedLines = 1;
-            _testGetSet->_moveCursorVerticallyResult = true;
             break;
         case CursorDirection::RIGHT:
             _testGetSet->_expectedCursorPos.X++;
@@ -1111,11 +1072,11 @@ public:
             break;
         case CursorDirection::NEXTLINE:
             _testGetSet->_expectedCursorPos.Y++;
-            _testGetSet->_expectedCursorPos.X = _testGetSet->_viewport.Left;
+            _testGetSet->_expectedCursorPos.X = 0;
             break;
         case CursorDirection::PREVLINE:
             _testGetSet->_expectedCursorPos.Y--;
-            _testGetSet->_expectedCursorPos.X = _testGetSet->_viewport.Left;
+            _testGetSet->_expectedCursorPos.X = 0;
             break;
         }
 
@@ -1131,26 +1092,22 @@ public:
         {
         case CursorDirection::UP:
             _testGetSet->_expectedCursorPos.Y = _testGetSet->_viewport.Top;
-            _testGetSet->_expectedLines = -100;
-            _testGetSet->_moveCursorVerticallyResult = true;
             break;
         case CursorDirection::DOWN:
             _testGetSet->_expectedCursorPos.Y = _testGetSet->_viewport.Bottom - 1;
-            _testGetSet->_expectedLines = 100;
-            _testGetSet->_moveCursorVerticallyResult = true;
             break;
         case CursorDirection::RIGHT:
-            _testGetSet->_expectedCursorPos.X = _testGetSet->_viewport.Right - 1;
+            _testGetSet->_expectedCursorPos.X = _testGetSet->_bufferSize.X - 1;
             break;
         case CursorDirection::LEFT:
-            _testGetSet->_expectedCursorPos.X = _testGetSet->_viewport.Left;
+            _testGetSet->_expectedCursorPos.X = 0;
             break;
         case CursorDirection::NEXTLINE:
-            _testGetSet->_expectedCursorPos.X = _testGetSet->_viewport.Left;
+            _testGetSet->_expectedCursorPos.X = 0;
             _testGetSet->_expectedCursorPos.Y = _testGetSet->_viewport.Bottom - 1;
             break;
         case CursorDirection::PREVLINE:
-            _testGetSet->_expectedCursorPos.X = _testGetSet->_viewport.Left;
+            _testGetSet->_expectedCursorPos.X = 0;
             _testGetSet->_expectedCursorPos.Y = _testGetSet->_viewport.Top;
             break;
         }
@@ -1158,64 +1115,19 @@ public:
         VERIFY_IS_TRUE((_pDispatch.get()->*(moveFunc))(100));
 
         // error cases
-        // give too large an up distance, cursor move should fail, cursor should stay the same.
-        Log::Comment(L"Test 4: When given invalid (massive) move distance that doesn't fit in a short, call fails and cursor doesn't move.");
-        _testGetSet->PrepData(CursorX::XCENTER, CursorY::YCENTER);
-
-        VERIFY_IS_FALSE((_pDispatch.get()->*(moveFunc))(UINT_MAX));
-        VERIFY_ARE_EQUAL(_testGetSet->_expectedCursorPos, _testGetSet->_cursorPos);
-
-        // cause short underflow. cursor move should fail. cursor should stay the same.
-        Log::Comment(L"Test 5: When an over/underflow occurs in cursor math, call fails and cursor doesn't move.");
-        _testGetSet->PrepData(direction);
-
-        switch (direction)
-        {
-        case CursorDirection::UP:
-        case CursorDirection::PREVLINE:
-            _testGetSet->_cursorPos.Y = -10;
-            break;
-        case CursorDirection::DOWN:
-        case CursorDirection::NEXTLINE:
-            _testGetSet->_cursorPos.Y = 10;
-            break;
-        case CursorDirection::RIGHT:
-            _testGetSet->_cursorPos.X = 10;
-            break;
-        case CursorDirection::LEFT:
-            _testGetSet->_cursorPos.X = -10;
-            break;
-        }
-
-        _testGetSet->_expectedCursorPos = _testGetSet->_cursorPos;
-
-        VERIFY_IS_FALSE((_pDispatch.get()->*(moveFunc))(SHRT_MAX + 1));
-        VERIFY_ARE_EQUAL(_testGetSet->_expectedCursorPos, _testGetSet->_cursorPos);
-
         // SetConsoleCursorPosition throws failure. Parameters are otherwise normal.
-        Log::Comment(L"Test 6: When SetConsoleCursorPosition throws a failure, call fails and cursor doesn't move.");
+        Log::Comment(L"Test 4: When SetConsoleCursorPosition throws a failure, call fails and cursor doesn't move.");
         _testGetSet->PrepData(direction);
         _testGetSet->_setConsoleCursorPositionResult = FALSE;
-        _testGetSet->_moveCursorVerticallyResult = false;
 
         VERIFY_IS_FALSE((_pDispatch.get()->*(moveFunc))(0));
         VERIFY_ARE_EQUAL(_testGetSet->_expectedCursorPos, _testGetSet->_cursorPos);
 
         // GetConsoleScreenBufferInfo throws failure. Parameters are otherwise normal.
-        Log::Comment(L"Test 7: When GetConsoleScreenBufferInfo throws a failure, call fails and cursor doesn't move.");
+        Log::Comment(L"Test 5: When GetConsoleScreenBufferInfo throws a failure, call fails and cursor doesn't move.");
         _testGetSet->PrepData(CursorX::LEFT, CursorY::TOP);
         _testGetSet->_getConsoleScreenBufferInfoExResult = FALSE;
-        _testGetSet->_moveCursorVerticallyResult = true;
+        VERIFY_IS_FALSE((_pDispatch.get()->*(moveFunc))(0));
-        Log::Comment(NoThrowString().Format(
-            L"Cursor Up and Down don't need GetConsoleScreenBufferInfoEx, so they will succeed"));
-        if (direction == CursorDirection::UP || direction == CursorDirection::DOWN)
-        {
-            VERIFY_IS_TRUE((_pDispatch.get()->*(moveFunc))(0));
-        }
-        else
-        {
-            VERIFY_IS_FALSE((_pDispatch.get()->*(moveFunc))(0));
-        }
         VERIFY_ARE_EQUAL(_testGetSet->_expectedCursorPos, _testGetSet->_cursorPos);
     }
 
@@ -1229,7 +1141,8 @@ public:
         short sCol = (_testGetSet->_viewport.Right - _testGetSet->_viewport.Left) / 2;
         short sRow = (_testGetSet->_viewport.Bottom - _testGetSet->_viewport.Top) / 2;
 
-        _testGetSet->_expectedCursorPos.X = _testGetSet->_viewport.Left + (sCol - 1);
+        // The X coordinate is unaffected by the viewport.
+        _testGetSet->_expectedCursorPos.X = sCol - 1;
         _testGetSet->_expectedCursorPos.Y = _testGetSet->_viewport.Top + (sRow - 1);
 
         VERIFY_IS_TRUE(_pDispatch.get()->CursorPosition(sRow, sCol));
@@ -1237,7 +1150,8 @@ public:
         Log::Comment(L"Test 2: Move to 0, 0 (which is 1,1 in VT speak)");
         _testGetSet->PrepData(CursorX::RIGHT, CursorY::BOTTOM);
 
-        _testGetSet->_expectedCursorPos.X = _testGetSet->_viewport.Left;
+        // The X coordinate is unaffected by the viewport.
+        _testGetSet->_expectedCursorPos.X = 0;
         _testGetSet->_expectedCursorPos.Y = _testGetSet->_viewport.Top;
 
         VERIFY_IS_TRUE(_pDispatch.get()->CursorPosition(1, 1));
@@ -1245,45 +1159,27 @@ public:
         Log::Comment(L"Test 3: Move beyond rectangle (down/right too far). Should be bounded back in.");
         _testGetSet->PrepData(CursorX::LEFT, CursorY::TOP);
 
-        sCol = (_testGetSet->_viewport.Right - _testGetSet->_viewport.Left) * 2;
+        sCol = (_testGetSet->_bufferSize.X) * 2;
         sRow = (_testGetSet->_viewport.Bottom - _testGetSet->_viewport.Top) * 2;
 
-        _testGetSet->_expectedCursorPos.X = _testGetSet->_viewport.Right - 1;
+        _testGetSet->_expectedCursorPos.X = _testGetSet->_bufferSize.X - 1;
         _testGetSet->_expectedCursorPos.Y = _testGetSet->_viewport.Bottom - 1;
 
         VERIFY_IS_TRUE(_pDispatch.get()->CursorPosition(sRow, sCol));
 
-        Log::Comment(L"Test 4: Values too large for short. Cursor shouldn't move. Return false.");
+        Log::Comment(L"Test 4: GetConsoleInfo API returns false. No move, return false.");
-        _testGetSet->PrepData(CursorX::LEFT, CursorY::TOP);
-
-        VERIFY_IS_FALSE(_pDispatch.get()->CursorPosition(UINT_MAX, UINT_MAX));
-
-        Log::Comment(L"Test 5: Overflow during addition. Cursor shouldn't move. Return false.");
-        _testGetSet->PrepData(CursorX::LEFT, CursorY::TOP);
-
-        _testGetSet->_viewport.Left = SHRT_MAX;
-        _testGetSet->_viewport.Top = SHRT_MAX;
-
-        VERIFY_IS_FALSE(_pDispatch.get()->CursorPosition(5, 5));
-
-        Log::Comment(L"Test 6: GetConsoleInfo API returns false. No move, return false.");
         _testGetSet->PrepData(CursorX::LEFT, CursorY::TOP);
 
         _testGetSet->_getConsoleScreenBufferInfoExResult = FALSE;
 
         VERIFY_IS_FALSE(_pDispatch.get()->CursorPosition(1, 1));
 
-        Log::Comment(L"Test 7: SetCursor API returns false. No move, return false.");
+        Log::Comment(L"Test 5: SetCursor API returns false. No move, return false.");
         _testGetSet->PrepData(CursorX::LEFT, CursorY::TOP);
 
         _testGetSet->_setConsoleCursorPositionResult = FALSE;
 
         VERIFY_IS_FALSE(_pDispatch.get()->CursorPosition(1, 1));
-
-        Log::Comment(L"Test 8: Move to 0,0. Cursor shouldn't move. Return false. 1,1 is the top left corner in VT100 speak. 0,0 isn't a position. The parser will give 1 for a 0 input.");
-        _testGetSet->PrepData(CursorX::LEFT, CursorY::TOP);
-
-        VERIFY_IS_FALSE(_pDispatch.get()->CursorPosition(0, 0));
     }
 
     TEST_METHOD(CursorSingleDimensionMoveTest)
@@ -1297,8 +1193,8 @@ public:
         //// Used to switch between the various function options.
         typedef bool (AdaptDispatch::*CursorMoveFunc)(size_t);
         CursorMoveFunc moveFunc = nullptr;
-        SHORT* psViewportEnd = nullptr;
+        SHORT sRangeEnd = 0;
-        SHORT* psViewportStart = nullptr;
+        SHORT sRangeStart = 0;
         SHORT* psCursorExpected = nullptr;
 
         // Modify variables based on directionality of this test
@@ -1306,26 +1202,27 @@ public:
         size_t dir;
         VERIFY_SUCCEEDED_RETURN(TestData::TryGetValue(L"uiDirection", dir));
         direction = (AbsolutePosition)dir;
+        _testGetSet->PrepData();
 
         switch (direction)
         {
         case AbsolutePosition::CursorHorizontal:
             Log::Comment(L"Testing cursor horizontal movement.");
-            psViewportEnd = &_testGetSet->_viewport.Right;
+            sRangeEnd = _testGetSet->_bufferSize.X;
-            psViewportStart = &_testGetSet->_viewport.Left;
+            sRangeStart = 0;
             psCursorExpected = &_testGetSet->_expectedCursorPos.X;
             moveFunc = &AdaptDispatch::CursorHorizontalPositionAbsolute;
             break;
         case AbsolutePosition::VerticalLine:
             Log::Comment(L"Testing vertical line movement.");
-            psViewportEnd = &_testGetSet->_viewport.Bottom;
+            sRangeEnd = _testGetSet->_viewport.Bottom;
-            psViewportStart = &_testGetSet->_viewport.Top;
+            sRangeStart = _testGetSet->_viewport.Top;
             psCursorExpected = &_testGetSet->_expectedCursorPos.Y;
             moveFunc = &AdaptDispatch::VerticalLinePositionAbsolute;
             break;
         }
 
-        if (moveFunc == nullptr || psViewportEnd == nullptr || psViewportStart == nullptr || psCursorExpected == nullptr)
+        if (moveFunc == nullptr || psCursorExpected == nullptr)
         {
             VERIFY_FAIL();
             return;
@@ -1334,16 +1231,16 @@ public:
         Log::Comment(L"Test 1: Place cursor within the viewport. Start from top left, move to middle.");
         _testGetSet->PrepData(CursorX::LEFT, CursorY::TOP);
 
-        short sVal = (*psViewportEnd - *psViewportStart) / 2;
+        short sVal = (sRangeEnd - sRangeStart) / 2;
 
-        *psCursorExpected = *psViewportStart + (sVal - 1);
+        *psCursorExpected = sRangeStart + (sVal - 1);
 
         VERIFY_IS_TRUE((_pDispatch.get()->*(moveFunc))(sVal));
 
         Log::Comment(L"Test 2: Move to 0 (which is 1 in VT speak)");
         _testGetSet->PrepData(CursorX::RIGHT, CursorY::BOTTOM);
 
-        *psCursorExpected = *psViewportStart;
+        *psCursorExpected = sRangeStart;
         sVal = 1;
 
         VERIFY_IS_TRUE((_pDispatch.get()->*(moveFunc))(sVal));
@@ -1351,29 +1248,13 @@ public:
         Log::Comment(L"Test 3: Move beyond rectangle (down/right too far). Should be bounded back in.");
         _testGetSet->PrepData(CursorX::LEFT, CursorY::TOP);
 
-        sVal = (*psViewportEnd - *psViewportStart) * 2;
+        sVal = (sRangeEnd - sRangeStart) * 2;
 
-        *psCursorExpected = *psViewportEnd - 1;
+        *psCursorExpected = sRangeEnd - 1;
 
         VERIFY_IS_TRUE((_pDispatch.get()->*(moveFunc))(sVal));
 
-        Log::Comment(L"Test 4: Values too large for short. Cursor shouldn't move. Return false.");
+        Log::Comment(L"Test 4: GetConsoleInfo API returns false. No move, return false.");
-        _testGetSet->PrepData(CursorX::LEFT, CursorY::TOP);
-
-        sVal = SHORT_MAX;
-
-        VERIFY_IS_FALSE((_pDispatch.get()->*(moveFunc))(sVal));
-
-        Log::Comment(L"Test 5: Overflow during addition. Cursor shouldn't move. Return false.");
-        _testGetSet->PrepData(CursorX::LEFT, CursorY::TOP);
-
-        _testGetSet->_viewport.Left = SHRT_MAX;
-
-        sVal = 5;
-
-        VERIFY_IS_FALSE((_pDispatch.get()->*(moveFunc))(sVal));
-
-        Log::Comment(L"Test 6: GetConsoleInfo API returns false. No move, return false.");
         _testGetSet->PrepData(CursorX::LEFT, CursorY::TOP);
 
         _testGetSet->_getConsoleScreenBufferInfoExResult = FALSE;
@@ -1382,7 +1263,7 @@ public:
 
         VERIFY_IS_FALSE((_pDispatch.get()->*(moveFunc))(sVal));
 
-        Log::Comment(L"Test 7: SetCursor API returns false. No move, return false.");
+        Log::Comment(L"Test 5: SetCursor API returns false. No move, return false.");
         _testGetSet->PrepData(CursorX::LEFT, CursorY::TOP);
 
         _testGetSet->_setConsoleCursorPositionResult = FALSE;
@@ -1390,13 +1271,6 @@ public:
         sVal = 1;
 
         VERIFY_IS_FALSE((_pDispatch.get()->*(moveFunc))(sVal));
-
-        Log::Comment(L"Test 8: Move to 0. Cursor shouldn't move. Return false. 1 is the left edge in VT100 speak. 0 isn't a position. The parser will give 1 for a 0 input.");
-        _testGetSet->PrepData(CursorX::LEFT, CursorY::TOP);
-
-        sVal = 0;
-
-        VERIFY_IS_FALSE((_pDispatch.get()->*(moveFunc))(sVal));
     }
 
     TEST_METHOD(CursorSaveRestoreTest)
@@ -1921,8 +1795,7 @@ public:
         // start with the cursor position in the buffer.
         COORD coordCursorExpected = _testGetSet->_cursorPos;
 
-        // to get to VT, we have to adjust it to its position relative to the viewport.
+        // to get to VT, we have to adjust it to its position relative to the viewport top.
-        coordCursorExpected.X -= _testGetSet->_viewport.Left;
         coordCursorExpected.Y -= _testGetSet->_viewport.Top;
 
         // Then note that VT is 1,1 based for the top left, so add 1. (The rest of the console uses 0,0 for array index bases.)
@@ -2020,6 +1893,7 @@ public:
         Log::Comment(L"Starting test...");
 
         SMALL_RECT srTestMargins = { 0 };
+        _testGetSet->_bufferSize = { 100, 600 };
         _testGetSet->_viewport.Right = 8;
         _testGetSet->_viewport.Bottom = 8;
         _testGetSet->_getConsoleScreenBufferInfoExResult = TRUE;

src/terminal/parser/InputStateMachineEngine.cpp

@@ -204,7 +204,7 @@ bool InputStateMachineEngine::_DoControlCharacter(const wchar_t wch, const bool
     {
         // This is a C0 Control Character.
         // This should be translated as Ctrl+(wch+x40)
-        const wchar_t actualChar = wch;
+        wchar_t actualChar = wch;
         bool writeCtrl = true;
 
         short vkey = 0;
@@ -213,7 +213,9 @@ bool InputStateMachineEngine::_DoControlCharacter(const wchar_t wch, const bool
         switch (wch)
         {
         case L'\b':
-            success = _GenerateKeyFromChar(wch + 0x40, vkey, modifierState);
+            // Process Ctrl+Bksp to delete whole words
+            actualChar = '\x7f';
+            success = _GenerateKeyFromChar(actualChar, vkey, modifierState);
             modifierState = 0;
             break;
         case L'\r':

src/terminal/parser/stateMachine.cpp

@@ -1269,13 +1269,17 @@ void StateMachine::ProcessString(const std::wstring_view string)
         {
             if (_isActionableFromGround(string.at(current))) // If the current char is the start of an escape sequence, or should be executed in ground state...
             {
-                // Because the _run above is composed INCLUDING current, we must
+                if (!_run.empty())
-                // trim it off here since we just determined it's actionable
+                {
-                // and only pass through everything before it.
+                    // Because the _run above is composed INCLUDING current, we must
-                const auto allLeadingUpTo = _run.substr(0, _run.size() - 1);
+                    // trim it off here since we just determined it's actionable
+                    // and only pass through everything before it.
+                    const auto allLeadingUpTo = _run.substr(0, _run.size() - 1);
+
+                    _engine->ActionPrintString(allLeadingUpTo); // ... print all the chars leading up to it as part of the run...
+                    _trace.DispatchPrintRunTrace(allLeadingUpTo);
+                }
 
-                _engine->ActionPrintString(allLeadingUpTo); // ... print all the chars leading up to it as part of the run...
-                _trace.DispatchPrintRunTrace(allLeadingUpTo);
                 _processingIndividually = true; // begin processing future characters individually...
                 start = current;
                 continue;
@@ -1290,9 +1294,7 @@ void StateMachine::ProcessString(const std::wstring_view string)
     // When we leave the loop, current has been advanced to the length of the string itself
     // (or one past the array index to the final char) so this `substr` operation doesn't +1
     // to include the final character (unlike the one inside the top of the loop above.)
-    // NOTE: std::basic_string_view will auto-trim excessively large sizes down to the valid length
+    _run = start < string.size() ? string.substr(start) : std::wstring_view{};
-    //       so passing something too large in the second parameter WILL NOT FAIL.
-    _run = string.substr(start, current - start);
 
     // If we're at the end of the string and have remaining un-printed characters,
     if (!_processingIndividually && !_run.empty())

src/terminal/parser/ut_parser/InputEngineTest.cpp

@@ -349,6 +349,10 @@ void InputEngineTest::C0Test()
         case L'\t': // Tab
             writeCtrl = false;
             break;
+        case L'\b': // backspace
+            wch = '\x7f';
+            expectedWch = '\x7f';
+            break;
         }
 
         short keyscan = VkKeyScanW(expectedWch);

src/winconpty/winconpty.cpp

@@ -82,7 +82,8 @@ HRESULT _CreatePseudoConsole(const HANDLE hToken,
     RETURN_IF_WIN32_BOOL_FALSE(CreatePipe(signalPipeConhostSide.addressof(), signalPipeOurSide.addressof(), &sa, 0));
     RETURN_IF_WIN32_BOOL_FALSE(SetHandleInformation(signalPipeConhostSide.get(), HANDLE_FLAG_INHERIT, HANDLE_FLAG_INHERIT));
 
-    const wchar_t* pwszFormat = L"%s --headless %s--width %hu --height %hu --signal 0x%x --server 0x%x";
+    // GH4061: Ensure that the path to executable in the format is escaped so C:\Program.exe cannot collide with C:\Program Files
+    const wchar_t* pwszFormat = L"\"%s\" --headless %s--width %hu --height %hu --signal 0x%x --server 0x%x";
     // This is plenty of space to hold the formatted string
     wchar_t cmd[MAX_PATH]{};
     const BOOL bInheritCursor = (dwFlags & PSEUDOCONSOLE_INHERIT_CURSOR) == PSEUDOCONSOLE_INHERIT_CURSOR;
@@ -149,7 +150,7 @@ HRESULT _CreatePseudoConsole(const HANDLE hToken,
         if (hToken == INVALID_HANDLE_VALUE || hToken == nullptr)
         {
             // Call create process
-            RETURN_IF_WIN32_BOOL_FALSE(CreateProcessW(nullptr,
+            RETURN_IF_WIN32_BOOL_FALSE(CreateProcessW(_ConsoleHostPath(),
                                                       cmd,
                                                       nullptr,
                                                       nullptr,
@@ -164,7 +165,7 @@ HRESULT _CreatePseudoConsole(const HANDLE hToken,
         {
             // Call create process
             RETURN_IF_WIN32_BOOL_FALSE(CreateProcessAsUserW(hToken,
-                                                            nullptr,
+                                                            _ConsoleHostPath(),
                                                             cmd,
                                                             nullptr,
                                                             nullptr,