Rewrite the MSAA/UIA integration into conhost #19344
Conversation
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
This comment has been minimized.
This comment has been minimized.
| @@ -253,12 +253,13 @@ static FillConsoleResult FillConsoleImpl(SCREEN_INFORMATION& screenInfo, FillCon | |||
| ImageSlice::EraseCells(screenInfo.GetTextBuffer(), startingCoordinate, result.cellsModified); | |||
| } | |||
|
|
|||
| if (screenBuffer.HasAccessibilityEventing()) | |||
There was a problem hiding this comment.
I think we did this check to explicitly avoid doing any extra work in the ConPTY case
There was a problem hiding this comment.
It still skips the work, but inside the class. I think that putting the work for checks like this into the caller's responsibility is bad. It's repetitive, you can accidentally forget it and doesn't even meaningfully improve performance over doing the check inside the callsite.
IMO good performance improvements address "big picture" issues. This then frees us up to do "cleaner" code like this which is technically wasteful but not practically so.
| const auto& bufferBefore = gci.GetActiveOutputBuffer(); | ||
| const auto cursorBefore = bufferBefore.GetTextBuffer().GetCursor().GetPosition(); | ||
|
|
||
| auto raise = wil::scope_exit([&bufferBefore, cursorBefore] { |
There was a problem hiding this comment.
hit this with AVRF please. I'm worried about capturing this reference by reference.
can we just capture the pointer itself, since all we do is compare it?
There was a problem hiding this comment.
AFAIK lambda captures behave like auto so bufferBefore would capture a copy, not a reference. But I agree that capturing a pointer would be easier to read and understand.
| notifier->NotifyConsoleUpdateScrollEvent(0, -1); | ||
| } | ||
| auto& an = ServiceLocator::LocateGlobals().accessibilityNotifier; | ||
| an.ScrollBuffer(-1); |
There was a problem hiding this comment.
this used to be skipped in conpty - note to self to verify it still is
There was a problem hiding this comment.
(a11y eventing is disabled in ConPTY because the Window class doesn't get instantiated.)
| // Flags is expected to be 2, 1, or 0. 2 in selecting (whether or not visible), 1 if just visible, 0 if invisible/noselect. | ||
| if (WI_IsFlagSet(gci.Flags, CONSOLE_SELECTING)) | ||
| { | ||
| flags = IAccessibilityNotifier::ConsoleCaretEventFlags::CaretSelection; |
There was a problem hiding this comment.
📝 watch where Selection/Invisible/Visible go
| @@ -47,6 +47,7 @@ | |||
| <ClCompile Include="..\writeData.cpp" /> | |||
| <ClCompile Include="..\_output.cpp" /> | |||
| <ClCompile Include="..\_stream.cpp" /> | |||
| <ClCompile Include="..\AccessibilityNotifier.cpp" /> | |||
| void RemoteConsoleControl::Control(ControlType, PVOID, DWORD) noexcept | ||
| { | ||
| WI_ASSERT_FAIL(); | ||
| } | ||
|
|
||
| void RemoteConsoleControl::NotifyWinEvent(DWORD, HWND, LONG, LONG) noexcept | ||
| { | ||
| WI_ASSERT_FAIL(); | ||
| } |
There was a problem hiding this comment.
you need to test this out as a handoff recipient to verify these asserts never hit
There was a problem hiding this comment.
Will do. Theoretically nothing will happen because AccessibilityNotifier only exists if the Window class has been loaded.
| @@ -16,7 +16,6 @@ | |||
| </ClCompile> | |||
| </ItemDefinitionGroup> | |||
| <ItemGroup> | |||
| <ClCompile Include="..\AccessibilityNotifier.cpp" /> | |||
Goal: Remove
CursorBlinker.Problem: Spooky action at a distance via
Cursor::HasMoved.Solution: Moved all the a11y event raising into
_stream.cppand pray for the best.Goal: Prevent node.js from tanking conhost performance via MSAA (WHY).
Problem:
ServiceLocator.Solution: Unserviced the locator. Debounced event raising. Performance increased by >10x.
Problem 2: Lots of files changed.
This PR is a prerequisite for #19330
Validation Steps Performed
Ran NVDA with and without UIA enabled and with different delays. ✅