Repository: microsoft/terminal (language: cpp)

Issue:
Wrapping over multiple lines in a single write fails in conhost
### Windows Terminal version

Commit 4a243f044572146e18e0051badb1b5b3f3c28ac8

### Windows build number

10.0.19045.4412

### Other Software

_No response_

### Steps to reproduce

1. Open a bash shell in a recent build of OpenConsole.
2. Make sure you have some scrollback enabled.
3. Set the width to 80 columns
4. Execute the following statement:
   ```
   printf "\e[2J\e[999;70HFirst line Second line                                                                     Third line\e[H"
   ```


### Expected Behavior

That should output a string a text wrapping over three lines, starting at the bottom of the page, so the viewport scrolls up by two rows. It should look something like this:

![image](https://github.com/microsoft/terminal/assets/4181424/fdc0b8c1-e386-4485-8259-e8600b544a44)


### Actual Behavior

The third line doesn't scroll the viewport, but instead wraps up onto the previous line (i.e. the first line), ending up like this:

![image](https://github.com/microsoft/terminal/assets/4181424/b4b341b7-5fbb-4792-9b6b-8143ffb02f5a)

This was a regression caused by PR #16615. The problem is that we're reusing the same `Page` object in multiple `_DoLineFeed` calls, but when a scroll is triggered (specifically one that pans the viewport), the viewport coordinates stored in the `Page` object are no longer correct. I'm working on a PR to fix this.

Note that this doesn't appear to affect Windows Terminal, because it doesn't have any scrollback in conhost, so the viewport doesn't move.


Discussion / hints:

Fix patch:
diff --git a/src/host/ut_host/ScreenBufferTests.cpp b/src/host/ut_host/ScreenBufferTests.cpp
index 7e34d2f31e6..54b48a3e967 100644
--- a/src/host/ut_host/ScreenBufferTests.cpp
+++ b/src/host/ut_host/ScreenBufferTests.cpp
@@ -262,6 +262,7 @@ class ScreenBufferTests
     TEST_METHOD(CopyDoubleWidthRectangularArea);
 
     TEST_METHOD(DelayedWrapReset);
+    TEST_METHOD(MultilineWrap);
 
     TEST_METHOD(EraseColorMode);
 
@@ -8323,6 +8324,39 @@ void ScreenBufferTests::DelayedWrapReset()
     }
 }
 
+void ScreenBufferTests::MultilineWrap()
+{
+    auto& gci = ServiceLocator::LocateGlobals().getConsoleInformation();
+    auto& si = gci.GetActiveOutputBuffer().GetActiveBuffer();
+    auto& stateMachine = si.GetStateMachine();
+    const auto bufferAttr = si.GetTextBuffer().GetCurrentAttributes();
+    const auto width = si.GetTextBuffer().GetSize().Width();
+    const auto bottomRow = si.GetViewport().BottomInclusive();
+
+    // Starting on the bottom row.
+    si.GetTextBuffer().GetCursor().SetPosition({ 0, bottomRow });
+
+    // Write out enough text to wrap over four lines.
+    auto fourLines = std::wstring{};
+    fourLines += L"1";
+    fourLines += std::wstring(width - 1, L' ');
+    fourLines += L"2";
+    fourLines += std::wstring(width - 1, L' ');
+    fourLines += L"3";
+    fourLines += std::wstring(width - 1, L' ');
+    fourLines += L"4";
+    stateMachine.ProcessString(fourLines);
+
+    Log::Comment(L"Cursor should have moved down three rows");
+    VERIFY_ARE_EQUAL(bottomRow + 3, si.GetTextBuffer().GetCursor().GetPosition().y);
+
+    Log::Comment(L"Bottom four rows should have the content 1, 2, 3, and 4");
+    VERIFY_IS_TRUE(_ValidateLineContains(bottomRow + 0, L"1", bufferAttr));
+    VERIFY_IS_TRUE(_ValidateLineContains(bottomRow + 1, L"2", bufferAttr));
+    VERIFY_IS_TRUE(_ValidateLineContains(bottomRow + 2, L"3", bufferAttr));
+    VERIFY_IS_TRUE(_ValidateLineContains(bottomRow + 3, L"4", bufferAttr));
+}
+
 void ScreenBufferTests::EraseColorMode()
 {
     BEGIN_TEST_METHOD_PROPERTIES()
diff --git a/src/terminal/adapter/PageManager.cpp b/src/terminal/adapter/PageManager.cpp
index 5408b249738..76b03faf779 100644
--- a/src/terminal/adapter/PageManager.cpp
+++ b/src/terminal/adapter/PageManager.cpp
@@ -93,6 +93,12 @@ til::CoordType Page::YPanOffset() const noexcept
     return 0; // Vertical panning is not yet supported
 }
 
+void Page::MoveViewportDown() noexcept
+{
+    _viewport.top++;
+    _viewport.bottom++;
+}
+
 PageManager::PageManager(ITerminalApi& api, Renderer& renderer) noexcept :
     _api{ api },
     _renderer{ renderer }
diff --git a/src/terminal/adapter/PageManager.hpp b/src/terminal/adapter/PageManager.hpp
index ab913902609..e625d24d142 100644
--- a/src/terminal/adapter/PageManager.hpp
+++ b/src/terminal/adapter/PageManager.hpp
@@ -33,6 +33,7 @@ namespace Microsoft::Console::VirtualTerminal
         til::CoordType BufferHeight() const noexcept;
         til::CoordType XPanOffset() const noexcept;
         til::CoordType YPanOffset() const noexcept;
+        void MoveViewportDown() noexcept;
 
     private:
         TextBuffer& _buffer;
diff --git a/src/terminal/adapter/adaptDispatch.cpp b/src/terminal/adapter/adaptDispatch.cpp
index 4135cac88d6..9205e798d04 100644
--- a/src/terminal/adapter/adaptDispatch.cpp
+++ b/src/terminal/adapter/adaptDispatch.cpp
@@ -73,14 +73,14 @@ void AdaptDispatch::PrintString(const std::wstring_view string)
 
 void AdaptDispatch::_WriteToBuffer(const std::wstring_view string)
 {
-    const auto page = _pages.ActivePage();
+    auto page = _pages.ActivePage();
     auto& textBuffer = page.Buffer();
     auto& cursor = page.Cursor();
     auto cursorPosition = cursor.GetPosition();
     const auto wrapAtEOL = _api.GetSystemMode(ITerminalApi::Mode::AutoWrap);
     const auto& attributes = page.Attributes();
 
-    const auto [topMargin, bottomMargin] = _GetVerticalMargins(page, true);
+    auto [topMargin, bottomMargin] = _GetVerticalMargins(page, true);
     const auto [leftMargin, rightMargin] = _GetHorizontalMargins(page.Width());
 
     auto lineWidth = textBuffer.GetLineWidth(cursorPosition.y);
@@ -107,7 +107,14 @@ void AdaptDispatch::_WriteToBuffer(const std::wstring_view string)
             // different position from where the EOL was marked.
             if (delayedCursorPosition == cursorPosition)
             {
-                _DoLineFeed(page, true, true);
+                if (_DoLineFeed(page, true, true))
+                {
+                    // If the line feed caused the viewport to move down, we
+                    // need to adjust the page viewport and margins to match.
+                    page.MoveViewportDown();
+                    std::tie(topMargin, bottomMargin) = _GetVerticalMargins(page, true);
+                }
+
                 cursorPosition = cursor.GetPosition();
                 // We need to recalculate the width when moving to a new line.
                 lineWidth = textBuffer.GetLineWidth(cursorPosition.y);
@@ -2505,14 +2512,15 @@ bool AdaptDispatch::CarriageReturn()
 // - withReturn - Set to true if a carriage return should be performed as well.
 // - wrapForced - Set to true is the line feed was the result of the line wrapping.
 // Return Value:
-// - <none>
-void AdaptDispatch::_DoLineFeed(const Page& page, const bool withReturn, const bool wrapForced)
+// - True if the viewport panned down. False if not.
+bool AdaptDispatch::_DoLineFeed(const Page& page, const bool withReturn, const bool wrapForced)
 {
     auto& textBuffer = page.Buffer();
     const auto pageWidth = page.Width();
     const auto bufferHeight = page.BufferHeight();
     const auto [topMargin, bottomMargin] = _GetVerticalMargins(page, true);
     const auto [leftMargin, rightMargin] = _GetHorizontalMargins(pageWidth);
+    auto viewportMoved = false;
 
     auto& cursor = page.Cursor();
     const auto currentPosition = cursor.GetPosition();
@@ -2555,6 +2563,7 @@ void AdaptDispatch::_DoLineFeed(const Page& page, const bool withReturn, const b
         // the end of the buffer.
         _api.SetViewportPosition({ page.XPanOffset(), page.Top() + 1 });
         newPosition.y++;
+        viewportMoved = true;
 
         // And if the bottom margin didn't cover the full page, we copy the
         // lower part of the page down so it remains static. But for a full
@@ -2594,6 +2603,7 @@ void AdaptDispatch::_DoLineFeed(const Page& page, const bool withReturn, const b
 
     cursor.SetPosition(newPosition);
     _ApplyCursorMovementFlags(cursor);
+    return viewportMoved;
 }
 
 // Routine Description:
diff --git a/src/terminal/adapter/adaptDispatch.hpp b/src/terminal/adapter/adaptDispatch.hpp
index dbea2fcd887..fac54bcdc23 100644
--- a/src/terminal/adapter/adaptDispatch.hpp
+++ b/src/terminal/adapter/adaptDispatch.hpp
@@ -249,7 +249,7 @@ namespace Microsoft::Console::VirtualTerminal
                                              const VTInt rightMargin,
                                              const bool homeCursor = false);
 
-        void _DoLineFeed(const Page& page, const bool withReturn, const bool wrapForced);
+        bool _DoLineFeed(const Page& page, const bool withReturn, const bool wrapForced);
 
         void _DeviceStatusReport(const wchar_t* parameters) const;
         void _CursorPositionReport(const bool extendedReport);


Test patch: