Fix UIA and marks regressions due to cooked read (microsoft#16105)

The initial cooked read (= conhost readline) rewrite had two flaws:
* Using viewport scrolls under ConPTY to avoid emitting newlines
resulted in various bugs around marks, coloring, etc. It's still
somewhat unclear why this happened, but the next issue is related and
much worse.
* Rewriting the input line every time causes problems with accessibility
tools, as they'll re-announce unchanged parts again and again.

The solution to these is to simply stop writing the unchanged parts of
the prompt. To do this, code was added to measure the size of text
without actually inserting them into the buffer. Since this meant that
the "interactive" mode of `WriteCharsLegacy` would need to be duplicated
for the new code, I instead moved those parts into `COOKED_READ_DATA`.
That way we can now have the interactive transform of the prompt (=
Ctrl+C -> ^C) and the two text functions (measure text & actually write
text) are now agnostic to this transformation.

Closes microsoft#16034
Closes microsoft#16044

## Validation Steps Performed
* A vision impaired user checked it out and it seemed fine ✅

Author: lhecker

.github/actions/spelling/expect/expect.txt

@@ -1776,6 +1776,7 @@ stdafx
 STDAPI
 stdc
 stdcpp
+STDEXT
 STDMETHODCALLTYPE
 STDMETHODIMP
 STGM

src/buffer/out/textBuffer.cpp

@@ -416,6 +416,88 @@ size_t TextBuffer::GraphemePrev(const std::wstring_view& chars, size_t position)
     return til::utf16_iterate_prev(chars, position);
 }
 
+// Ever wondered how much space a piece of text needs before inserting it? This function will tell you!
+// It fundamentally behaves identical to the various ROW functions around `RowWriteState`.
+//
+// Set `columnLimit` to the amount of space that's available (e.g. `buffer_width - cursor_position.x`)
+// and it'll return the amount of characters that fit into this space. The out parameter `columns`
+// will contain the amount of columns this piece of text has actually used.
+//
+// Just like with `RowWriteState` one special case is when not all text fits into the given space:
+// In that case, this function always returns exactly `columnLimit`. This distinction is important when "inserting"
+// a wide glyph but there's only 1 column left. That 1 remaining column is supposed to be padded with whitespace.
+size_t TextBuffer::FitTextIntoColumns(const std::wstring_view& chars, til::CoordType columnLimit, til::CoordType& columns) noexcept
+{
+    columnLimit = std::max(0, columnLimit);
+
+    const auto beg = chars.begin();
+    const auto end = chars.end();
+    auto it = beg;
+    const auto asciiEnd = beg + std::min(chars.size(), gsl::narrow_cast<size_t>(columnLimit));
+
+    // ASCII fast-path: 1 char always corresponds to 1 column.
+    for (; it != asciiEnd && *it < 0x80; ++it)
+    {
+    }
+
+    const auto dist = gsl::narrow_cast<size_t>(it - beg);
+    auto col = gsl::narrow_cast<til::CoordType>(dist);
+
+    if (it == asciiEnd) [[likely]]
+    {
+        columns = col;
+        return dist;
+    }
+
+    // Unicode slow-path where we need to count text and columns separately.
+    for (;;)
+    {
+        auto ptr = &*it;
+        const auto wch = *ptr;
+        size_t len = 1;
+
+        col++;
+
+        // Even in our slow-path we can avoid calling IsGlyphFullWidth if the current character is ASCII.
+        // It also allows us to skip the surrogate pair decoding at the same time.
+        if (wch >= 0x80)
+        {
+            if (til::is_surrogate(wch))
+            {
+                const auto it2 = it + 1;
+                if (til::is_leading_surrogate(wch) && it2 != end && til::is_trailing_surrogate(*it2))
+                {
+                    len = 2;
+                }
+                else
+                {
+                    ptr = &UNICODE_REPLACEMENT;
+                }
+            }
+
+            col += IsGlyphFullWidth({ ptr, len });
+        }
+
+        // If we ran out of columns, we need to always return `columnLimit` and not `cols`,
+        // because if we tried inserting a wide glyph into just 1 remaining column it will
+        // fail to fit, but that remaining column still has been used up. When the caller sees
+        // `columns == columnLimit` they will line-wrap and continue inserting into the next row.
+        if (col > columnLimit)
+        {
+            columns = columnLimit;
+            return gsl::narrow_cast<size_t>(it - beg);
+        }
+
+        // But if we simply ran out of text we just need to return the actual number of columns.
+        it += len;
+        if (it == end)
+        {
+            columns = col;
+            return chars.size();
+        }
+    }
+}
+
 // Pretend as if `position` is a regular cursor in the TextBuffer.
 // This function will then pretend as if you pressed the left/right arrow
 // keys `distance` amount of times (negative = left, positive = right).

src/buffer/out/textBuffer.hpp

@@ -137,6 +137,7 @@ class TextBuffer final
 
     static size_t GraphemeNext(const std::wstring_view& chars, size_t position) noexcept;
     static size_t GraphemePrev(const std::wstring_view& chars, size_t position) noexcept;
+    static size_t FitTextIntoColumns(const std::wstring_view& chars, til::CoordType columnLimit, til::CoordType& columns) noexcept;
 
     til::point NavigateCursor(til::point position, til::CoordType distance) const;
 

src/cascadia/UnitTests_TerminalCore/ConptyRoundtripTests.cpp

@@ -3243,7 +3243,7 @@ void ConptyRoundtripTests::WrapNewLineAtBottom()
         }
         else if (writingMethod == PrintWithWriteCharsLegacy)
         {
-            WriteCharsLegacy(si, str, false, nullptr);
+            WriteCharsLegacy(si, str, nullptr);
         }
     };
 
@@ -3401,7 +3401,7 @@ void ConptyRoundtripTests::WrapNewLineAtBottomLikeMSYS()
         }
         else if (writingMethod == PrintWithWriteCharsLegacy)
         {
-            WriteCharsLegacy(si, str, false, nullptr);
+            WriteCharsLegacy(si, str, nullptr);
         }
     };
 

src/common.build.pre.props

@@ -133,7 +133,7 @@
           We didn't and it breaks WRL and large parts of conhost code.
       -->
       <DisableSpecificWarnings>4201;4312;4467;5105;26434;26445;26456;%(DisableSpecificWarnings)</DisableSpecificWarnings>
-      <PreprocessorDefinitions>_WINDOWS;EXTERNAL_BUILD;%(PreprocessorDefinitions)</PreprocessorDefinitions>
+      <PreprocessorDefinitions>_WINDOWS;EXTERNAL_BUILD;_SILENCE_STDEXT_ARR_ITERS_DEPRECATION_WARNING;%(PreprocessorDefinitions)</PreprocessorDefinitions>
       <SDLCheck>true</SDLCheck>
       <PrecompiledHeaderFile>precomp.h</PrecompiledHeaderFile>
       <DebugInformationFormat>ProgramDatabase</DebugInformationFormat>

src/host/_stream.cpp

@@ -39,7 +39,7 @@ using Microsoft::Console::VirtualTerminal::StateMachine;
 // - coordCursor - New location of cursor.
 // - fKeepCursorVisible - TRUE if changing window origin desirable when hit right edge
 // Return Value:
-static void AdjustCursorPosition(SCREEN_INFORMATION& screenInfo, _In_ til::point coordCursor, const bool interactive, _Inout_opt_ til::CoordType* psScrollY)
+static void AdjustCursorPosition(SCREEN_INFORMATION& screenInfo, _In_ til::point coordCursor, _Inout_opt_ til::CoordType* psScrollY)
 {
     const auto bufferSize = screenInfo.GetBufferSize().Dimensions();
     if (coordCursor.x < 0)
@@ -70,42 +70,19 @@ static void AdjustCursorPosition(SCREEN_INFORMATION& screenInfo, _In_ til::point
 
     if (coordCursor.y >= bufferSize.height)
     {
-        const auto vtIo = ServiceLocator::LocateGlobals().getConsoleInformation().GetVtIo();
-        const auto renderer = ServiceLocator::LocateGlobals().pRender;
-        const auto needsConPTYWorkaround = interactive && vtIo->IsUsingVt();
         auto& buffer = screenInfo.GetTextBuffer();
-        const auto isActiveBuffer = buffer.IsActiveBuffer();
+        buffer.IncrementCircularBuffer(buffer.GetCurrentAttributes());
 
-        // ConPTY translates scrolling into newlines. We don't want that during cooked reads (= "cmd.exe prompts")
-        // because the entire prompt is supposed to fit into the VT viewport, with no scrollback. If we didn't do that,
-        // any prompt larger than the viewport will cause >1 lines to be added to scrollback, even if typing backspaces.
-        // You can test this by removing this branch, launch Windows Terminal, and fill the entire viewport with text in cmd.exe.
-        if (needsConPTYWorkaround)
+        if (buffer.IsActiveBuffer())
         {
-            buffer.SetAsActiveBuffer(false);
-            buffer.IncrementCircularBuffer(buffer.GetCurrentAttributes());
-            buffer.SetAsActiveBuffer(isActiveBuffer);
-
-            if (isActiveBuffer && renderer)
+            if (const auto notifier = ServiceLocator::LocateAccessibilityNotifier())
             {
-                renderer->TriggerRedrawAll();
+                notifier->NotifyConsoleUpdateScrollEvent(0, -1);
             }
-        }
-        else
-        {
-            buffer.IncrementCircularBuffer(buffer.GetCurrentAttributes());
-
-            if (isActiveBuffer)
+            if (const auto renderer = ServiceLocator::LocateGlobals().pRender)
             {
-                if (const auto notifier = ServiceLocator::LocateAccessibilityNotifier())
-                {
-                    notifier->NotifyConsoleUpdateScrollEvent(0, -1);
-                }
-                if (renderer)
-                {
-                    static constexpr til::point delta{ 0, -1 };
-                    renderer->TriggerScroll(&delta);
-                }
+                static constexpr til::point delta{ 0, -1 };
+                renderer->TriggerScroll(&delta);
             }
         }
 
@@ -128,15 +105,11 @@ static void AdjustCursorPosition(SCREEN_INFORMATION& screenInfo, _In_ til::point
         LOG_IF_FAILED(screenInfo.SetViewportOrigin(false, WindowOrigin, true));
     }
 
-    if (interactive)
-    {
-        screenInfo.MakeCursorVisible(coordCursor);
-    }
-    LOG_IF_FAILED(screenInfo.SetCursorPosition(coordCursor, interactive));
+    LOG_IF_FAILED(screenInfo.SetCursorPosition(coordCursor, false));
 }
 
 // As the name implies, this writes text without processing its control characters.
-static void _writeCharsLegacyUnprocessed(SCREEN_INFORMATION& screenInfo, const std::wstring_view& text, const bool interactive, til::CoordType* psScrollY)
+void _writeCharsLegacyUnprocessed(SCREEN_INFORMATION& screenInfo, const std::wstring_view& text, til::CoordType* psScrollY)
 {
     const auto wrapAtEOL = WI_IsFlagSet(screenInfo.OutputMode, ENABLE_WRAP_AT_EOL_OUTPUT);
     const auto hasAccessibilityEventing = screenInfo.HasAccessibilityEventing();
@@ -165,18 +138,19 @@ static void _writeCharsLegacyUnprocessed(SCREEN_INFORMATION& screenInfo, const s
             screenInfo.NotifyAccessibilityEventing(state.columnBegin, cursorPosition.y, state.columnEnd - 1, cursorPosition.y);
         }
 
-        AdjustCursorPosition(screenInfo, cursorPosition, interactive, psScrollY);
+        AdjustCursorPosition(screenInfo, cursorPosition, psScrollY);
     }
 }
 
 // This routine writes a string to the screen while handling control characters.
 // `interactive` exists for COOKED_READ_DATA which uses it to transform control characters into visible text like "^X".
 // Similarly, `psScrollY` is also used by it to track whether the underlying buffer circled. It requires this information to know where the input line moved to.
-void WriteCharsLegacy(SCREEN_INFORMATION& screenInfo, const std::wstring_view& text, const bool interactive, til::CoordType* psScrollY)
+void WriteCharsLegacy(SCREEN_INFORMATION& screenInfo, const std::wstring_view& text, til::CoordType* psScrollY)
 {
     static constexpr wchar_t tabSpaces[8]{ L' ', L' ', L' ', L' ', L' ', L' ', L' ', L' ' };
 
     auto& textBuffer = screenInfo.GetTextBuffer();
+    const auto width = textBuffer.GetSize().Width();
     auto& cursor = textBuffer.GetCursor();
     const auto wrapAtEOL = WI_IsFlagSet(screenInfo.OutputMode, ENABLE_WRAP_AT_EOL_OUTPUT);
     auto it = text.begin();
@@ -197,15 +171,15 @@ void WriteCharsLegacy(SCREEN_INFORMATION& screenInfo, const std::wstring_view& t
         {
             pos.x = 0;
             pos.y++;
-            AdjustCursorPosition(screenInfo, pos, interactive, psScrollY);
+            AdjustCursorPosition(screenInfo, pos, psScrollY);
         }
     }
 
     // If ENABLE_PROCESSED_OUTPUT is set we search for C0 control characters and handle them like backspace, tab, etc.
-    // If it's not set, we can just straight up give everything to _writeCharsLegacyUnprocessed.
+    // If it's not set, we can just straight up give everything to WriteCharsLegacyUnprocessed.
     if (WI_IsFlagClear(screenInfo.OutputMode, ENABLE_PROCESSED_OUTPUT))
     {
-        _writeCharsLegacyUnprocessed(screenInfo, { it, end }, interactive, psScrollY);
+        _writeCharsLegacyUnprocessed(screenInfo, { it, end }, psScrollY);
         it = end;
     }
 
@@ -214,7 +188,7 @@ void WriteCharsLegacy(SCREEN_INFORMATION& screenInfo, const std::wstring_view& t
         const auto nextControlChar = std::find_if(it, end, [](const auto& wch) { return !IS_GLYPH_CHAR(wch); });
         if (nextControlChar != it)
         {
-            _writeCharsLegacyUnprocessed(screenInfo, { it, nextControlChar }, interactive, psScrollY);
+            _writeCharsLegacyUnprocessed(screenInfo, { it, nextControlChar }, psScrollY);
             it = nextControlChar;
         }
 
@@ -223,35 +197,24 @@ void WriteCharsLegacy(SCREEN_INFORMATION& screenInfo, const std::wstring_view& t
             switch (*it)
             {
             case UNICODE_NULL:
-                if (interactive)
-                {
-                    break;
-                }
-                _writeCharsLegacyUnprocessed(screenInfo, { &tabSpaces[0], 1 }, interactive, psScrollY);
+                _writeCharsLegacyUnprocessed(screenInfo, { &tabSpaces[0], 1 }, psScrollY);
                 continue;
             case UNICODE_BELL:
-                if (interactive)
-                {
-                    break;
-                }
                 std::ignore = screenInfo.SendNotifyBeep();
                 continue;
             case UNICODE_BACKSPACE:
             {
-                // Backspace handling for interactive mode should happen in COOKED_READ_DATA
-                // where it has full control over the text and can delete it directly.
-                // Otherwise handling backspacing tabs/whitespace can turn up complex and bug-prone.
-                assert(!interactive);
                 auto pos = cursor.GetPosition();
                 pos.x = textBuffer.GetRowByOffset(pos.y).NavigateToPrevious(pos.x);
-                AdjustCursorPosition(screenInfo, pos, interactive, psScrollY);
+                AdjustCursorPosition(screenInfo, pos, psScrollY);
                 continue;
             }
             case UNICODE_TAB:
             {
                 const auto pos = cursor.GetPosition();
-                const auto tabCount = gsl::narrow_cast<size_t>(8 - (pos.x & 7));
-                _writeCharsLegacyUnprocessed(screenInfo, { &tabSpaces[0], tabCount }, interactive, psScrollY);
+                const auto remaining = width - pos.x;
+                const auto tabCount = gsl::narrow_cast<size_t>(std::min(remaining, 8 - (pos.x & 7)));
+                _writeCharsLegacyUnprocessed(screenInfo, { &tabSpaces[0], tabCount }, psScrollY);
                 continue;
             }
             case UNICODE_LINEFEED:
@@ -264,38 +227,29 @@ void WriteCharsLegacy(SCREEN_INFORMATION& screenInfo, const std::wstring_view& t
 
                 textBuffer.GetMutableRowByOffset(pos.y).SetWrapForced(false);
                 pos.y = pos.y + 1;
-                AdjustCursorPosition(screenInfo, pos, interactive, psScrollY);
+                AdjustCursorPosition(screenInfo, pos, psScrollY);
                 continue;
             }
             case UNICODE_CARRIAGERETURN:
             {
                 auto pos = cursor.GetPosition();
                 pos.x = 0;
-                AdjustCursorPosition(screenInfo, pos, interactive, psScrollY);
+                AdjustCursorPosition(screenInfo, pos, psScrollY);
                 continue;
             }
             default:
                 break;
             }
 
-            // In the interactive mode we replace C0 control characters (0x00-0x1f) with ASCII representations like ^C (= 0x03).
-            if (interactive && *it < L' ')
+            // As a special favor to incompetent apps that attempt to display control chars,
+            // convert to corresponding OEM Glyph Chars
+            const auto cp = ServiceLocator::LocateGlobals().getConsoleInformation().OutputCP;
+            const auto ch = gsl::narrow_cast<char>(*it);
+            wchar_t wch = 0;
+            const auto result = MultiByteToWideChar(cp, MB_USEGLYPHCHARS, &ch, 1, &wch, 1);
+            if (result == 1)
             {
-                const wchar_t wchs[2]{ L'^', static_cast<wchar_t>(*it + L'@') };
-                _writeCharsLegacyUnprocessed(screenInfo, { &wchs[0], 2 }, interactive, psScrollY);
-            }
-            else
-            {
-                // As a special favor to incompetent apps that attempt to display control chars,
-                // convert to corresponding OEM Glyph Chars
-                const auto cp = ServiceLocator::LocateGlobals().getConsoleInformation().OutputCP;
-                const auto ch = gsl::narrow_cast<char>(*it);
-                wchar_t wch = 0;
-                const auto result = MultiByteToWideChar(cp, MB_USEGLYPHCHARS, &ch, 1, &wch, 1);
-                if (result == 1)
-                {
-                    _writeCharsLegacyUnprocessed(screenInfo, { &wch, 1 }, interactive, psScrollY);
-                }
+                _writeCharsLegacyUnprocessed(screenInfo, { &wch, 1 }, psScrollY);
             }
         }
     }
@@ -358,7 +312,7 @@ try
 
     if (WI_IsAnyFlagClear(screenInfo.OutputMode, ENABLE_VIRTUAL_TERMINAL_PROCESSING | ENABLE_PROCESSED_OUTPUT))
     {
-        WriteCharsLegacy(screenInfo, str, false, nullptr);
+        WriteCharsLegacy(screenInfo, str, nullptr);
     }
     else
     {

src/host/_stream.h

@@ -19,7 +19,7 @@ Revision History:
 
 #include "writeData.hpp"
 
-void WriteCharsLegacy(SCREEN_INFORMATION& screenInfo, const std::wstring_view& str, bool interactive, til::CoordType* psScrollY);
+void WriteCharsLegacy(SCREEN_INFORMATION& screenInfo, const std::wstring_view& str, til::CoordType* psScrollY);
 
 // NOTE: console lock must be held when calling this routine
 // String has been translated to unicode at this point.

src/host/ft_fuzzer/fuzzmain.cpp

@@ -132,6 +132,6 @@ extern "C" __declspec(dllexport) int LLVMFuzzerTestOneInput(const uint8_t* data,
     til::CoordType scrollY{};
     gci.LockConsole();
     auto u = wil::scope_exit([&]() { gci.UnlockConsole(); });
-    WriteCharsLegacy(gci.GetActiveOutputBuffer(), u16String, true, &scrollY);
+    WriteCharsLegacy(gci.GetActiveOutputBuffer(), u16String, &scrollY);
     return 0;
 }