Skip to content

Commit

Permalink
Use UIA notifications for text output (microsoft#12358)
Browse files Browse the repository at this point in the history
## Summary of the Pull Request
This change makes Windows Terminal raise a `RaiseNotificationEvent()` ([docs](https://docs.microsoft.com/en-us/uwp/api/windows.ui.xaml.automation.peers.automationpeer.raisenotificationevent?view=winrt-22000)) for new text output to the buffer.

This is intended to help Narrator identify what new output appears and reduce the workload of diffing the buffer when a `TextChanged` event occurs.

## Detailed Description of the Pull Request / Additional comments
The flow of the event occurs as follows:
- `Terminal::_WriteBuffer()`
   - New text is output to the text buffer. Notify the renderer that we have new text (and what that text is).
- `Renderer::TriggerNewTextNotification()`
   - Cycle through all the rendering engines and tell them to notify handle the new text output.
   - None of the rendering engines _except_ `UiaEngine` has it implemented, so really we're just notifying UIA.
- `UiaEngine::NotifyNewText()`
   - Concatenate any new output into a string.
   - When we're done painting, tell the notification system to actually notify of new events occurring and clear any stored output text. That way, we're ready for the next renderer frame.
- `InteractivityAutomationPeer::NotifyNewOutput()` --> `TermControlAutomationPeer::NotifyNewOutput`
   - NOTE: these are split because of the in-proc and out-of-proc separation of the buffer.
   - Actually `RaiseNotificationEvent()` for the new text output.

Additionally, we had to handle the "local echo" problem: when a key is pressed, the character is said twice (once for the keyboard event, and again for the character being written to the buffer). To accomplish this, we did the following:
- `TermControl`:
   - here, we already handle keyboard events, so I added a line saying "if we have an automation peer attached, record the keyboard event in the automation peer".
- `TermControlAutomationPeer`:
   - just before the notification is dispatched, check if the string of recent keyboard events match the beginning of the string of new output. If that's the case, we can assume that the common prefix was the "local echo". 

This is a fairly naive heuristic, but it's been working.

Closes the following ADO bugs:
- https://dev.azure.com/microsoft/OS/_workitems/edit/36506838
- (Probably) https://dev.azure.com/microsoft/OS/_workitems/edit/38011453

## Test cases
- [x] Base case: "echo hello"
- [x] Partial line change
- [x] Scrolling (should be unaffected)
- [x] Large output
- [x] "local echo": keyboard events read input character twice
  • Loading branch information
carlos-zamora authored Mar 11, 2022
1 parent 7fdcd6f commit f9be172
Show file tree
Hide file tree
Showing 23 changed files with 232 additions and 21 deletions.
1 change: 1 addition & 0 deletions .github/actions/spelling/expect/expect.txt
Original file line number Diff line number Diff line change
Expand Up @@ -2084,6 +2084,7 @@ rxvt
safearray
SAFECAST
safemath
sapi
sba
SBCS
SBCSDBCS
Expand Down
5 changes: 5 additions & 0 deletions src/cascadia/TerminalControl/InteractivityAutomationPeer.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -93,6 +93,11 @@ namespace winrt::Microsoft::Terminal::Control::implementation
_CursorChangedHandlers(*this, nullptr);
}

void InteractivityAutomationPeer::NotifyNewOutput(std::wstring_view newOutput)
{
_NewOutputHandlers(*this, hstring{ newOutput });
}

#pragma region ITextProvider
com_array<XamlAutomation::ITextRangeProvider> InteractivityAutomationPeer::GetSelection()
{
Expand Down
2 changes: 2 additions & 0 deletions src/cascadia/TerminalControl/InteractivityAutomationPeer.h
Original file line number Diff line number Diff line change
Expand Up @@ -49,6 +49,7 @@ namespace winrt::Microsoft::Terminal::Control::implementation
void SignalSelectionChanged() override;
void SignalTextChanged() override;
void SignalCursorChanged() override;
void NotifyNewOutput(std::wstring_view newOutput) override;
#pragma endregion

#pragma region ITextProvider Pattern
Expand All @@ -73,6 +74,7 @@ namespace winrt::Microsoft::Terminal::Control::implementation
TYPED_EVENT(SelectionChanged, IInspectable, IInspectable);
TYPED_EVENT(TextChanged, IInspectable, IInspectable);
TYPED_EVENT(CursorChanged, IInspectable, IInspectable);
TYPED_EVENT(NewOutput, IInspectable, hstring);

private:
Windows::UI::Xaml::Automation::Provider::ITextRangeProvider _CreateXamlUiaTextRange(::ITextRangeProvider* returnVal) const;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -14,5 +14,6 @@ namespace Microsoft.Terminal.Control
event Windows.Foundation.TypedEventHandler<Object, Object> SelectionChanged;
event Windows.Foundation.TypedEventHandler<Object, Object> TextChanged;
event Windows.Foundation.TypedEventHandler<Object, Object> CursorChanged;
event Windows.Foundation.TypedEventHandler<Object, String> NewOutput;
}
}
5 changes: 5 additions & 0 deletions src/cascadia/TerminalControl/TermControl.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -1097,6 +1097,11 @@ namespace winrt::Microsoft::Terminal::Control::implementation
keyDown) :
true;

if (vkey && keyDown && _automationPeer)
{
get_self<TermControlAutomationPeer>(_automationPeer)->RecordKeyEvent(vkey);
}

if (_cursorTimer)
{
// Manually show the cursor when a key is pressed. Restarting
Expand Down
107 changes: 107 additions & 0 deletions src/cascadia/TerminalControl/TermControlAutomationPeer.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,43 @@ namespace XamlAutomation
using winrt::Windows::UI::Xaml::Automation::Provider::ITextRangeProvider;
}

static constexpr wchar_t UNICODE_NEWLINE{ L'\n' };

// Method Description:
// - creates a copy of the provided text with all of the control characters removed
// Arguments:
// - text: the string we're sanitizing
// Return Value:
// - a copy of "sanitized" with all of the control characters removed
static std::wstring Sanitize(std::wstring_view text)
{
std::wstring sanitized{ text };
sanitized.erase(std::remove_if(sanitized.begin(), sanitized.end(), [](wchar_t c) {
return (c < UNICODE_SPACE && c != UNICODE_NEWLINE) || c == 0x7F /*DEL*/;
}),
sanitized.end());
return sanitized;
}

// Method Description:
// - verifies if a given string has text that would be read by a screen reader.
// - a string of control characters, for example, would not be read.
// Arguments:
// - text: the string we're validating
// Return Value:
// - true, if the text is readable. false, otherwise.
static constexpr bool IsReadable(std::wstring_view text)
{
for (const auto c : text)
{
if (c > UNICODE_SPACE)
{
return true;
}
}
return false;
}

namespace winrt::Microsoft::Terminal::Control::implementation
{
TermControlAutomationPeer::TermControlAutomationPeer(TermControl* owner,
Expand All @@ -45,6 +82,7 @@ namespace winrt::Microsoft::Terminal::Control::implementation
_contentAutomationPeer.SelectionChanged([this](auto&&, auto&&) { SignalSelectionChanged(); });
_contentAutomationPeer.TextChanged([this](auto&&, auto&&) { SignalTextChanged(); });
_contentAutomationPeer.CursorChanged([this](auto&&, auto&&) { SignalCursorChanged(); });
_contentAutomationPeer.NewOutput([this](auto&&, hstring newOutput) { NotifyNewOutput(newOutput); });
_contentAutomationPeer.ParentProvider(*this);
};

Expand All @@ -68,6 +106,17 @@ namespace winrt::Microsoft::Terminal::Control::implementation
_contentAutomationPeer.SetControlPadding(padding);
}

void TermControlAutomationPeer::RecordKeyEvent(const WORD vkey)
{
if (const auto charCode{ MapVirtualKey(vkey, MAPVK_VK_TO_CHAR) })
{
if (const auto keyEventChar{ gsl::narrow_cast<wchar_t>(charCode) }; IsReadable({ &keyEventChar, 1 }))
{
_keyEvents.emplace_back(keyEventChar);
}
}
}

// Method Description:
// - Signals the ui automation client that the terminal's selection has changed and should be updated
// Arguments:
Expand Down Expand Up @@ -142,8 +191,66 @@ namespace winrt::Microsoft::Terminal::Control::implementation
});
}

void TermControlAutomationPeer::NotifyNewOutput(std::wstring_view newOutput)
{
// Try to suppress any events (or event data)
// that is just the keypress the user made
auto sanitized{ Sanitize(newOutput) };
while (!_keyEvents.empty() && IsReadable(sanitized))
{
if (til::toupper_ascii(sanitized.front()) == _keyEvents.front())
{
// the key event's character (i.e. the "A" key) matches
// the output character (i.e. "a" or "A" text).
// We can assume that the output character resulted from
// the pressed key, so we can ignore it.
sanitized = sanitized.substr(1);
_keyEvents.pop_front();
}
else
{
// The output doesn't match,
// so clear the input stack and
// move on to fire the event.
_keyEvents.clear();
break;
}
}

// Suppress event if the remaining text is not readable
if (!IsReadable(sanitized))
{
return;
}

auto dispatcher{ Dispatcher() };
if (!dispatcher)
{
return;
}

// IMPORTANT:
// [1] make sure the scope returns a copy of "sanitized" so that it isn't accidentally deleted
// [2] AutomationNotificationProcessing::All --> ensures it can be interrupted by keyboard events
// [3] Do not "RunAsync(...).get()". For whatever reason, this causes NVDA to just not receive "SignalTextChanged()"'s events.
dispatcher.RunAsync(Windows::UI::Core::CoreDispatcherPriority::Normal, [weakThis{ get_weak() }, sanitizedCopy{ hstring{ sanitized } }]() {
if (auto strongThis{ weakThis.get() })
{
try
{
strongThis->RaiseNotificationEvent(AutomationNotificationKind::ActionCompleted,
AutomationNotificationProcessing::All,
sanitizedCopy,
L"TerminalTextOutput");
}
CATCH_LOG();
}
});
}

hstring TermControlAutomationPeer::GetClassNameCore() const
{
// IMPORTANT: Do NOT change the name. Screen readers like JAWS may be dependent on this being "TermControl".
return L"TermControl";
}

Expand Down
3 changes: 3 additions & 0 deletions src/cascadia/TerminalControl/TermControlAutomationPeer.h
Original file line number Diff line number Diff line change
Expand Up @@ -48,6 +48,7 @@ namespace winrt::Microsoft::Terminal::Control::implementation

void UpdateControlBounds();
void SetControlPadding(const Core::Padding padding);
void RecordKeyEvent(const WORD vkey);

#pragma region FrameworkElementAutomationPeer
hstring GetClassNameCore() const;
Expand All @@ -64,6 +65,7 @@ namespace winrt::Microsoft::Terminal::Control::implementation
void SignalSelectionChanged() override;
void SignalTextChanged() override;
void SignalCursorChanged() override;
void NotifyNewOutput(std::wstring_view newOutput) override;
#pragma endregion

#pragma region ITextProvider Pattern
Expand All @@ -78,5 +80,6 @@ namespace winrt::Microsoft::Terminal::Control::implementation
private:
winrt::Microsoft::Terminal::Control::implementation::TermControl* _termControl;
Control::InteractivityAutomationPeer _contentAutomationPeer;
std::deque<wchar_t> _keyEvents;
};
}
5 changes: 5 additions & 0 deletions src/cascadia/TerminalCore/Terminal.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -1002,6 +1002,11 @@ void Terminal::_WriteBuffer(const std::wstring_view& stringView)
_AdjustCursorPosition(proposedCursorPosition);
}

// Notify UIA of new text.
// It's important to do this here instead of in TextBuffer, because here you have access to the entire line of text,
// whereas TextBuffer writes it one character at a time via the OutputCellIterator.
_buffer->GetRenderTarget().TriggerNewTextNotification(stringView);

cursor.EndDeferDrawing();
}

Expand Down
1 change: 1 addition & 0 deletions src/cascadia/UnitTests_TerminalCore/ScrollTest.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -53,6 +53,7 @@ namespace
};
virtual void TriggerCircling(){};
void TriggerTitleChange(){};
void TriggerNewTextNotification(const std::wstring_view){};

private:
std::optional<COORD> _triggerScrollDelta;
Expand Down
10 changes: 10 additions & 0 deletions src/host/ScreenBufferRenderTarget.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -110,3 +110,13 @@ void ScreenBufferRenderTarget::TriggerTitleChange()
pRenderer->TriggerTitleChange();
}
}

void ScreenBufferRenderTarget::TriggerNewTextNotification(const std::wstring_view newText)
{
auto* pRenderer = ServiceLocator::LocateGlobals().pRender;
const auto* pActive = &ServiceLocator::LocateGlobals().getConsoleInformation().GetActiveOutputBuffer().GetActiveBuffer();
if (pRenderer != nullptr && pActive == &_owner)
{
pRenderer->TriggerNewTextNotification(newText);
}
}
1 change: 1 addition & 0 deletions src/host/ScreenBufferRenderTarget.hpp
Original file line number Diff line number Diff line change
Expand Up @@ -39,6 +39,7 @@ class ScreenBufferRenderTarget final : public Microsoft::Console::Render::IRende
void TriggerScroll(const COORD* const pcoordDelta) override;
void TriggerCircling() override;
void TriggerTitleChange() override;
void TriggerNewTextNotification(const std::wstring_view newText) override;

private:
SCREEN_INFORMATION& _owner;
Expand Down
5 changes: 5 additions & 0 deletions src/renderer/atlas/AtlasEngine.api.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -143,6 +143,11 @@ constexpr HRESULT vec2_narrow(U x, U y, AtlasEngine::vec2<T>& out) noexcept
return S_OK;
}

[[nodiscard]] HRESULT AtlasEngine::NotifyNewText(const std::wstring_view newText) noexcept
{
return S_OK;
}

[[nodiscard]] HRESULT AtlasEngine::UpdateFont(const FontInfoDesired& fontInfoDesired, _Out_ FontInfo& fontInfo) noexcept
{
return UpdateFont(fontInfoDesired, fontInfo, {}, {});
Expand Down
1 change: 1 addition & 0 deletions src/renderer/atlas/AtlasEngine.h
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,7 @@ namespace Microsoft::Console::Render
[[nodiscard]] HRESULT InvalidateAll() noexcept override;
[[nodiscard]] HRESULT InvalidateCircling(_Out_ bool* pForcePaint) noexcept override;
[[nodiscard]] HRESULT InvalidateTitle(std::wstring_view proposedTitle) noexcept override;
[[nodiscard]] HRESULT NotifyNewText(const std::wstring_view newText) noexcept override;
[[nodiscard]] HRESULT PrepareRenderInfo(const RenderFrameInfo& info) noexcept override;
[[nodiscard]] HRESULT ResetLineTransform() noexcept override;
[[nodiscard]] HRESULT PrepareLineTransform(LineRendition lineRendition, size_t targetRow, size_t viewportLeft) noexcept override;
Expand Down
5 changes: 5 additions & 0 deletions src/renderer/base/RenderEngineBase.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,11 @@ HRESULT RenderEngineBase::UpdateTitle(const std::wstring_view newTitle) noexcept
return hr;
}

HRESULT RenderEngineBase::NotifyNewText(const std::wstring_view /*newText*/) noexcept
{
return S_FALSE;
}

HRESULT RenderEngineBase::UpdateSoftFont(const gsl::span<const uint16_t> /*bitPattern*/,
const SIZE /*cellSize*/,
const size_t /*centeringHint*/) noexcept
Expand Down
8 changes: 8 additions & 0 deletions src/renderer/base/renderer.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -493,6 +493,14 @@ void Renderer::TriggerTitleChange()
NotifyPaintFrame();
}

void Renderer::TriggerNewTextNotification(const std::wstring_view newText)
{
FOREACH_ENGINE(pEngine)
{
LOG_IF_FAILED(pEngine->NotifyNewText(newText));
}
}

// Routine Description:
// - Update the title for a particular engine.
// Arguments:
Expand Down
2 changes: 2 additions & 0 deletions src/renderer/base/renderer.hpp
Original file line number Diff line number Diff line change
Expand Up @@ -54,6 +54,8 @@ namespace Microsoft::Console::Render
void TriggerCircling() override;
void TriggerTitleChange() override;

void TriggerNewTextNotification(const std::wstring_view newText) override;

void TriggerFontChange(const int iDpi,
const FontInfoDesired& FontInfoDesired,
_Out_ FontInfo& FontInfo);
Expand Down
1 change: 1 addition & 0 deletions src/renderer/inc/DummyRenderTarget.hpp
Original file line number Diff line number Diff line change
Expand Up @@ -31,4 +31,5 @@ class DummyRenderTarget final : public Microsoft::Console::Render::IRenderTarget
void TriggerScroll(const COORD* const /*pcoordDelta*/) override {}
void TriggerCircling() override {}
void TriggerTitleChange() override {}
void TriggerNewTextNotification(const std::wstring_view) override {}
};
1 change: 1 addition & 0 deletions src/renderer/inc/IRenderEngine.hpp
Original file line number Diff line number Diff line change
Expand Up @@ -69,6 +69,7 @@ namespace Microsoft::Console::Render
[[nodiscard]] virtual HRESULT InvalidateAll() noexcept = 0;
[[nodiscard]] virtual HRESULT InvalidateCircling(_Out_ bool* pForcePaint) noexcept = 0;
[[nodiscard]] virtual HRESULT InvalidateTitle(std::wstring_view proposedTitle) noexcept = 0;
[[nodiscard]] virtual HRESULT NotifyNewText(const std::wstring_view newText) noexcept = 0;
[[nodiscard]] virtual HRESULT PrepareRenderInfo(const RenderFrameInfo& info) noexcept = 0;
[[nodiscard]] virtual HRESULT ResetLineTransform() noexcept = 0;
[[nodiscard]] virtual HRESULT PrepareLineTransform(LineRendition lineRendition, size_t targetRow, size_t viewportLeft) noexcept = 0;
Expand Down
2 changes: 2 additions & 0 deletions src/renderer/inc/IRenderTarget.hpp
Original file line number Diff line number Diff line change
Expand Up @@ -45,6 +45,8 @@ namespace Microsoft::Console::Render
virtual void TriggerScroll(const COORD* const pcoordDelta) = 0;
virtual void TriggerCircling() = 0;
virtual void TriggerTitleChange() = 0;

virtual void TriggerNewTextNotification(const std::wstring_view newText) = 0;
};

inline Microsoft::Console::Render::IRenderTarget::~IRenderTarget() {}
Expand Down
2 changes: 2 additions & 0 deletions src/renderer/inc/RenderEngineBase.hpp
Original file line number Diff line number Diff line change
Expand Up @@ -38,6 +38,8 @@ namespace Microsoft::Console::Render

[[nodiscard]] HRESULT UpdateTitle(const std::wstring_view newTitle) noexcept override;

[[nodiscard]] HRESULT NotifyNewText(const std::wstring_view newText) noexcept override;

[[nodiscard]] HRESULT UpdateSoftFont(const gsl::span<const uint16_t> bitPattern,
const SIZE cellSize,
const size_t centeringHint) noexcept override;
Expand Down
Loading

0 comments on commit f9be172

Please sign in to comment.