Skip to content

Commit

Permalink
Ensure automation peer is created regardless of terminal initializati…
Browse files Browse the repository at this point in the history
…on (#10971)

## Summary of the Pull Request
The bug was that Narrator would still read the content of the old tab/pane although a new tab/pane was introduced. This is caused by the automation peer not being created when XAML requests it. Normally, we would prevent the automation peer from being created if the terminal was not fully initialized.

This change allows the automation peer to be created regardless of the terminal being fully initialized by...
- `TermControl`: `_InitializeTerminal` updates the padding (dependent on the `SwapChainPanel`) upon full initialization
- `ControlCore`: initialize the `_renderer` in the ctor so that we can attach the UIA Engine before `ControlCore::Initialize()` is called (dependent on `SwapChainPanel` loading)

As a bonus, this also fixes a locking issue where logging would attempt to get the text range's text and lock twice. The locking fix is very similar to #10937.

## PR Checklist
Closes [MSFT 33353327](https://microsoft.visualstudio.com/OS/_workitems/edit/33353327)

## Validation Steps Performed
- New pane from key binding is announced by Narrator
- New tab from key binding is announced by Narrator
  • Loading branch information
carlos-zamora authored Aug 18, 2021
1 parent 68294f8 commit 638c6d0
Show file tree
Hide file tree
Showing 4 changed files with 57 additions and 40 deletions.
55 changes: 29 additions & 26 deletions src/cascadia/TerminalControl/ControlCore.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -104,6 +104,32 @@ namespace winrt::Microsoft::Terminal::Control::implementation
auto pfnTerminalTaskbarProgressChanged = std::bind(&ControlCore::_terminalTaskbarProgressChanged, this);
_terminal->TaskbarProgressChangedCallback(pfnTerminalTaskbarProgressChanged);

// MSFT 33353327: Initialize the renderer in the ctor instead of Initialize().
// We need the renderer to be ready to accept new engines before the SwapChainPanel is ready to go.
// If we wait, a screen reader may try to get the AutomationPeer (aka the UIA Engine), and we won't be able to attach
// the UIA Engine to the renderer. This prevents us from signaling changes to the cursor or buffer.
{
// First create the render thread.
// Then stash a local pointer to the render thread so we can initialize it and enable it
// to paint itself *after* we hand off its ownership to the renderer.
// We split up construction and initialization of the render thread object this way
// because the renderer and render thread have circular references to each other.
auto renderThread = std::make_unique<::Microsoft::Console::Render::RenderThread>();
auto* const localPointerToThread = renderThread.get();

// Now create the renderer and initialize the render thread.
_renderer = std::make_unique<::Microsoft::Console::Render::Renderer>(_terminal.get(), nullptr, 0, std::move(renderThread));

_renderer->SetRendererEnteredErrorStateCallback([weakThis = get_weak()]() {
if (auto strongThis{ weakThis.get() })
{
strongThis->_RendererEnteredErrorStateHandlers(*strongThis, nullptr);
}
});

THROW_IF_FAILED(localPointerToThread->Initialize(_renderer.get()));
}

// Get our dispatcher. If we're hosted in-proc with XAML, this will get
// us the same dispatcher as TermControl::Dispatcher(). If we're out of
// proc, this'll return null. We'll need to instead make a new
Expand Down Expand Up @@ -196,27 +222,6 @@ namespace winrt::Microsoft::Terminal::Control::implementation
return false;
}

// First create the render thread.
// Then stash a local pointer to the render thread so we can initialize it and enable it
// to paint itself *after* we hand off its ownership to the renderer.
// We split up construction and initialization of the render thread object this way
// because the renderer and render thread have circular references to each other.
auto renderThread = std::make_unique<::Microsoft::Console::Render::RenderThread>();
auto* const localPointerToThread = renderThread.get();

// Now create the renderer and initialize the render thread.
_renderer = std::make_unique<::Microsoft::Console::Render::Renderer>(_terminal.get(), nullptr, 0, std::move(renderThread));
::Microsoft::Console::Render::IRenderTarget& renderTarget = *_renderer;

_renderer->SetRendererEnteredErrorStateCallback([weakThis = get_weak()]() {
if (auto strongThis{ weakThis.get() })
{
strongThis->_RendererEnteredErrorStateHandlers(*strongThis, nullptr);
}
});

THROW_IF_FAILED(localPointerToThread->Initialize(_renderer.get()));

// Set up the DX Engine
auto dxEngine = std::make_unique<::Microsoft::Console::Render::DxEngine>();
_renderer->AddRenderEngine(dxEngine.get());
Expand Down Expand Up @@ -248,7 +253,7 @@ namespace winrt::Microsoft::Terminal::Control::implementation
_settings.InitialCols(width);
_settings.InitialRows(height);

_terminal->CreateFromSettings(_settings, renderTarget);
_terminal->CreateFromSettings(_settings, *_renderer);

// IMPORTANT! Set this callback up sooner than later. If we do it
// after Enable, then it'll be possible to paint the frame once
Expand Down Expand Up @@ -1465,10 +1470,8 @@ namespace winrt::Microsoft::Terminal::Control::implementation

void ControlCore::AttachUiaEngine(::Microsoft::Console::Render::IRenderEngine* const pEngine)
{
if (_renderer)
{
_renderer->AddRenderEngine(pEngine);
}
// _renderer will always exist since it's introduced in the ctor
_renderer->AddRenderEngine(pEngine);
}

bool ControlCore::IsInReadOnlyMode() const
Expand Down
2 changes: 1 addition & 1 deletion src/cascadia/TerminalControl/ControlInteractivity.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -614,7 +614,7 @@ namespace winrt::Microsoft::Terminal::Control::implementation
Control::InteractivityAutomationPeer ControlInteractivity::OnCreateAutomationPeer()
try
{
auto autoPeer = winrt::make_self<implementation::InteractivityAutomationPeer>(this);
const auto autoPeer = winrt::make_self<implementation::InteractivityAutomationPeer>(this);

_uiaEngine = std::make_unique<::Microsoft::Console::Render::UiaEngine>(autoPeer.get());
_core->AttachUiaEngine(_uiaEngine.get());
Expand Down
30 changes: 22 additions & 8 deletions src/cascadia/TerminalControl/TermControl.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -377,7 +377,7 @@ namespace winrt::Microsoft::Terminal::Control::implementation
_changeBackgroundColor(bg);

// Apply padding as swapChainPanel's margin
auto newMargin = ParseThicknessFromPadding(newSettings.Padding());
const auto newMargin = ParseThicknessFromPadding(newSettings.Padding());
SwapChainPanel().Margin(newMargin);

TSFInputControl().Margin(newMargin);
Expand Down Expand Up @@ -511,18 +511,20 @@ namespace winrt::Microsoft::Terminal::Control::implementation
// - The automation peer for our control
Windows::UI::Xaml::Automation::Peers::AutomationPeer TermControl::OnCreateAutomationPeer()
{
if (_initializedTerminal && !_IsClosing()) // only set up the automation peer if we're ready to go live
// MSFT 33353327: We're purposefully not using _initializedTerminal to ensure we're fully initialized.
// Doing so makes us return nullptr when XAML requests an automation peer.
// Instead, we need to give XAML an automation peer, then fix it later.
if (!_IsClosing())
{
// create a custom automation peer with this code pattern:
// (https://docs.microsoft.com/en-us/windows/uwp/design/accessibility/custom-automation-peers)
if (const auto& interactivityAutoPeer{ _interactivity.OnCreateAutomationPeer() })
{
auto margins{ SwapChainPanel().Margin() };

Core::Padding padding{ margins.Left,
margins.Top,
margins.Right,
margins.Bottom };
const auto margins{ SwapChainPanel().Margin() };
const Core::Padding padding{ margins.Left,
margins.Top,
margins.Right,
margins.Bottom };
_automationPeer = winrt::make<implementation::TermControlAutomationPeer>(this, padding, interactivityAutoPeer);
return _automationPeer;
}
Expand Down Expand Up @@ -717,6 +719,18 @@ namespace winrt::Microsoft::Terminal::Control::implementation

_initializedTerminal = true;

// MSFT 33353327: If the AutomationPeer was created before we were done initializing,
// make sure it's properly set up now.
if (_automationPeer)
{
_automationPeer.UpdateControlBounds();
const auto margins{ GetPadding() };
_automationPeer.SetControlPadding(Core::Padding{ margins.Left,
margins.Top,
margins.Right,
margins.Bottom });
}

// Likewise, run the event handlers outside of lock (they could
// be reentrant)
_InitializedHandlers(*this, nullptr);
Expand Down
10 changes: 5 additions & 5 deletions src/types/UiaTextRangeBase.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -938,7 +938,12 @@ try
const auto maxLengthOpt = (maxLength == -1) ?
std::nullopt :
std::optional<unsigned int>{ maxLength };
_pData->LockConsole();
auto Unlock = wil::scope_exit([this]() noexcept {
_pData->UnlockConsole();
});
const auto text = _getTextValue(maxLengthOpt);
Unlock.reset();

*pRetVal = SysAllocString(text.c_str());
RETURN_HR_IF_NULL(E_OUTOFMEMORY, *pRetVal);
Expand All @@ -958,11 +963,6 @@ CATCH_RETURN();
#pragma warning(disable : 26447) // compiler isn't filtering throws inside the try/catch
std::wstring UiaTextRangeBase::_getTextValue(std::optional<unsigned int> maxLength) const
{
_pData->LockConsole();
auto Unlock = wil::scope_exit([&]() noexcept {
_pData->UnlockConsole();
});

std::wstring textData{};
if (!IsDegenerate())
{
Expand Down

0 comments on commit 638c6d0

Please sign in to comment.