Skip accessibility notifier and all event calculations when we're in PTY mode #10569

miniksa · 2021-07-06T22:31:13Z

Change accessibility notifier creation so we do not create one when we're in PTY mode. (Guard all call sites to skip math/event work when the notifier is null.) MSAA events are legacy events that are registered for globally and used by some screen readers to find content in the conhost window. The PTY mode is not responsible for hosting the display content or input window, so it makes sense for it to not broadcast these events and delegate the accessibility requirement to the connected terminal.

References

Don't notify a11y event when in ConPTY mode #10537

PR Checklist

Closes Make no-op IAccessibilityNotifier for ConPTY mode #10568
I work here
Manual test launches passed.

miniksa · 2021-07-06T22:31:32Z

cc: @skyline75489

skyline75489

Nice comments about why this is needed :)

src/interactivity/base/ServiceLocator.cpp

DHowett · 2021-07-07T16:18:28Z

src/host/srvinit.cpp

+    {
+        // If we're in ConPTY mode... set the no-op accessibility notifier
+        // to prevent sending expensive legacy MSAA events when we won't even be
+        // responsible for the terminal user interface.


We may want to audit the places where IAccessibilityNotifier is used, in addition to having the no-op interface.

I say this because ... the one that notifies about buffer changes reads the buffer first, before calling out to IAN. It's a waste of time if IAN is a no-op, which still increases our output time burden.

meh, you've got a good point about SCREEN_INFORMATION::NotifyAccessibilityEventing. If we're really trying to be as optimal as possible, then yea we shouldn't do that.

I'm wondering why SCREEN_INFORMATION::NotifyAccessibilityEventing is the only thing showed up in the WPR trace. Maybe this indicates that other places that calls IAccessibilityNotifier are not as expensive?

DHowett · 2021-07-07T16:19:21Z

src/host/srvinit.cpp

+        // to prevent sending expensive legacy MSAA events when we won't even be
+        // responsible for the terminal user interface.
+        std::unique_ptr<IAccessibilityNotifier> noOpNotifier = std::make_unique<Microsoft::Console::Interactivity::NoOpAccessibilityNotifier>();
+        RETURN_IF_NTSTATUS_FAILED(ServiceLocator::SetAccessibilityNotifier(std::move(noOpNotifier)));


I'm surprised this doesn't cause us to blow up later... because isn't somebody else going to set it?

Ah, it creates it if it was not already created. Clever.

miniksa · 2021-07-08T21:04:56Z

Alternative strategy proposal....

Instead of adding a no-op renderer... I'll change it so

all call sites will check if the notifier is null before calling it (or doing any extra work that would be required to call it)
startup explicitly creates an appropriate (win32 or onecore or none for PTY) notifier instead of the implicit create-on-first-use behavior
I'll undo the check is-in-PTY-mode inside the SCREEN_INFORMATION::NotifyAccessibilityEventing as it will no longer be needed and would be a waste of time.

Sound OK?

DHowett · 2021-07-08T21:09:49Z

S_OK for me

…all callsites and avoid parameter math if it's null to save more time.

miniksa · 2021-07-08T22:28:32Z

Cleared @lhecker approval for plan B.

lhecker

I like.

src/interactivity/base/ServiceLocator.cpp

DHowett

I was only going to have nits, but I think I had enough of them as to start asking questions. 😄

DHowett · 2021-07-09T15:41:00Z

src/host/_stream.cpp

-            screenInfo.NotifyAccessibilityEventing(CursorPosition.X, CursorPosition.Y, CursorPosition.X + gsl::narrow<SHORT>(i - 1), CursorPosition.Y);
+            if (screenInfo.HasAccessibilityEventing())
+            {
+                screenInfo.NotifyAccessibilityEventing(CursorPosition.X, CursorPosition.Y, CursorPosition.X + gsl::narrow<SHORT>(i - 1), CursorPosition.Y);
+            }


I get it (consistency!), but this also seems like a decision SCREEN_INFO can make for itself.

I wanted to avoid the math on the 3rd param too... and below...
That and the consistency argument together are what made me guard it.

DHowett · 2021-07-09T15:41:05Z

src/host/conimeinfo.cpp

+    if (screenInfo.HasAccessibilityEventing())
+    {
+        screenInfo.NotifyAccessibilityEventing(insertionPos.X, insertionPos.Y, gsl::narrow<SHORT>(insertionPos.X + lineVec.size() - 1), insertionPos.Y);
+    }


src/host/screenInfo.cpp

src/host/srvinit.cpp

ghost · 2021-07-09T18:45:41Z

Hello @DHowett!

Because this pull request has the AutoMerge label, I will be glad to assist with helping to merge this pull request once all check-in policies pass.

p.s. you can customize the way I help with merging this pull request, such as holding this pull request until a specific person approves. Simply @mention me (`@msftbot`) and give me an instruction to get started! Learn more here.

…PTY mode (#10569) Change accessibility notifier creation so we do not create one when we're in PTY mode. (Guard all call sites to skip math/event work when the notifier is null.) MSAA events are legacy events that are registered for globally and used by some screen readers to find content in the conhost window. The PTY mode is not responsible for hosting the display content or input window, so it makes sense for it to not broadcast these events and delegate the accessibility requirement to the connected terminal. ## References - #10537 ## PR Checklist * [x] Closes #10568 * [x] I work here * [x] Manual test launches passed. (cherry picked from commit 91b454a)

ghost · 2021-07-14T17:01:59Z

🎉Windows Terminal v1.9.1942.0 has been released which incorporates this pull request.:tada:

Handy links:

ghost · 2021-07-14T17:06:17Z

🎉Windows Terminal Preview v1.10.1933.0 has been released which incorporates this pull request.:tada:

Handy links:

Add no-op notifier when we're in PTY mode #10568

13cfbac

ghost added Area-Accessibility Issues related to accessibility Area-Performance Performance-related issue Issue-Task It's a feature request, but it doesn't really need a major design. Product-Conpty For console issues specifically related to conpty labels Jul 6, 2021

skyline75489 approved these changes Jul 7, 2021

View reviewed changes

lhecker approved these changes Jul 7, 2021

View reviewed changes

src/interactivity/base/ServiceLocator.cpp Outdated Show resolved Hide resolved

DHowett reviewed Jul 7, 2021

View reviewed changes

Plan B. Explicitly create the notifier unless PTY. Check for null at …

2a0367e

…all callsites and avoid parameter math if it's null to save more time.

miniksa requested a review from lhecker July 8, 2021 22:26

miniksa changed the title ~~Add no-op accessibility notifier when we're in PTY mode~~ Skip accessibility notifier and all event calculations when we're in PTY mode Jul 8, 2021

lhecker approved these changes Jul 8, 2021

View reviewed changes

src/interactivity/base/ServiceLocator.cpp Outdated Show resolved Hide resolved

DHowett requested changes Jul 9, 2021

View reviewed changes

ghost added Needs-Author-Feedback The original author of the issue/PR needs to come back and respond to something and removed Needs-Author-Feedback The original author of the issue/PR needs to come back and respond to something labels Jul 9, 2021

don't pass param to accessibility notifier creation

37a76e6

DHowett approved these changes Jul 9, 2021

View reviewed changes

DHowett added the AutoMerge Marked for automatic merge by the bot when requirements are met label Jul 9, 2021

ghost merged commit 91b454a into main Jul 9, 2021

ghost deleted the dev/miniksa/acc_nooop branch July 9, 2021 18:45

DHowett added the zPreview-Service-Consider label Jul 12, 2021

ghost mentioned this pull request Jul 14, 2021

Make no-op IAccessibilityNotifier for ConPTY mode #10568

Closed

skyline75489 mentioned this pull request Jul 16, 2021

Extremely slow performance when processing virtual terminal sequences #10462

Open

DHowett removed the zPreview-Service-Consider label Aug 25, 2021

This pull request was closed.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Skip accessibility notifier and all event calculations when we're in PTY mode #10569

Skip accessibility notifier and all event calculations when we're in PTY mode #10569

miniksa commented Jul 6, 2021 •

edited

Loading

miniksa commented Jul 6, 2021

skyline75489 left a comment

DHowett Jul 7, 2021

zadjii-msft Jul 7, 2021

skyline75489 Jul 8, 2021

DHowett Jul 7, 2021

DHowett Jul 7, 2021

miniksa commented Jul 8, 2021

DHowett commented Jul 8, 2021

miniksa commented Jul 8, 2021

lhecker left a comment

DHowett left a comment

DHowett Jul 9, 2021

miniksa Jul 9, 2021

DHowett Jul 9, 2021

ghost commented Jul 9, 2021

ghost commented Jul 14, 2021

ghost commented Jul 14, 2021

Skip accessibility notifier and all event calculations when we're in PTY mode #10569

Skip accessibility notifier and all event calculations when we're in PTY mode #10569

Conversation

miniksa commented Jul 6, 2021 • edited Loading

References

PR Checklist

miniksa commented Jul 6, 2021

skyline75489 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

miniksa commented Jul 8, 2021

DHowett commented Jul 8, 2021

miniksa commented Jul 8, 2021

lhecker left a comment

Choose a reason for hiding this comment

DHowett left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ghost commented Jul 9, 2021

p.s. you can customize the way I help with merging this pull request, such as holding this pull request until a specific person approves. Simply @mention me (@msftbot) and give me an instruction to get started! Learn more here.

ghost commented Jul 14, 2021

ghost commented Jul 14, 2021

miniksa commented Jul 6, 2021 •

edited

Loading

p.s. you can customize the way I help with merging this pull request, such as holding this pull request until a specific person approves. Simply @mention me (`@msftbot`) and give me an instruction to get started! Learn more here.