Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix UIA and marks regressions due to cooked read #16105

Merged
merged 5 commits into from
Oct 24, 2023
Merged
Show file tree
Hide file tree
Changes from 4 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions .github/actions/spelling/expect/expect.txt
Original file line number Diff line number Diff line change
Expand Up @@ -1775,6 +1775,7 @@ stdafx
STDAPI
stdc
stdcpp
STDEXT
STDMETHODCALLTYPE
STDMETHODIMP
STGM
Expand Down
82 changes: 82 additions & 0 deletions src/buffer/out/textBuffer.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

any chance we accidentally step over end? (like, should we have a it>end assert or bail or something)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

len is either 1 or 2 depending on whether the current code point is a surrogate pair. A few lines above, len is only set to 2 if it + 1 != end which means that it += 2 is safe.

{
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).
Expand Down
1 change: 1 addition & 0 deletions src/buffer/out/textBuffer.hpp
Original file line number Diff line number Diff line change
Expand Up @@ -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;

Expand Down
4 changes: 2 additions & 2 deletions src/cascadia/UnitTests_TerminalCore/ConptyRoundtripTests.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -3243,7 +3243,7 @@ void ConptyRoundtripTests::WrapNewLineAtBottom()
}
else if (writingMethod == PrintWithWriteCharsLegacy)
{
WriteCharsLegacy(si, str, false, nullptr);
WriteCharsLegacy(si, str, nullptr);
}
};

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

Expand Down
2 changes: 1 addition & 1 deletion src/common.build.pre.props
Original file line number Diff line number Diff line change
Expand Up @@ -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>
Expand Down
112 changes: 33 additions & 79 deletions src/host/_stream.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down Expand Up @@ -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)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

note to self: this branch was removed. That seems good

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
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this branch is the same

{
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);
}
}

Expand All @@ -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);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this was removed, but was only ever used for

  • COOKED_READ_DATA::_writeChars
  • COOKED_READ_DATA::_handlePostCharInputLoop

}
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();
Expand Down Expand Up @@ -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();
Expand All @@ -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;
}

Expand All @@ -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;
}

Expand All @@ -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();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

note: interesting, we didn't beep if we were interactive before. Interesting?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

same with the null above, obv.

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:
Expand All @@ -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);
}
}
}
Expand Down Expand Up @@ -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
{
Expand Down
2 changes: 1 addition & 1 deletion src/host/_stream.h
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down
2 changes: 1 addition & 1 deletion src/host/ft_fuzzer/fuzzmain.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -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;
}
Loading
Loading