Use UIA notifications for text output #12358

carlos-zamora · 2022-02-03T23:26:12Z

Summary of the Pull Request

This change makes Windows Terminal raise a RaiseNotificationEvent() (docs) 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:

Test cases

Base case: "echo hello"
Partial line change
Scrolling (should be unaffected)
Large output
"local echo": keyboard events read input character twice

codeofdusk · 2022-02-04T04:52:33Z

This definitely opens some exciting possibilities, but will need to be handled with care on the client side!

How can/should UIA clients know that notifications are supported and there's no need to use the old strategy?

carlos-zamora · 2022-02-04T21:01:51Z

This definitely opens some exciting possibilities, but will need to be handled with care on the client side!

How can/should UIA clients know that notifications are supported and there's no need to use the old strategy?

Could you do something like "if we received a notification event with the ID 'TerminalTextOutput', this is the newest iteration"?

codeofdusk · 2022-02-04T22:34:48Z

Could you do something like "if we received a notification event with the ID 'TerminalTextOutput', this is the newest iteration"?

Maybe? @josephsl, @LeonarddeR, @michaelDCurran, @jcsteh is it possible to change the overlay class associated with an NVDA object in response to receiving a particular event? (I'm thinking not, but being proven wrong would ease this).

Other idea: maybe all terminals could inherit from LiveText, but we stopMonitoring and unregister for textChange once we get a UIA notification?

I think it'd probably be a lot simpler if they set the automationId on the terminal control to something unique (it's currently empty), then we can detect new/old overlay class as normal.

jcsteh · 2022-02-04T22:40:17Z

No. An overlay has to be determined at the time the object is created. That could be due to a notification event, but it could just as well be due to some other event; e.g. focus. That said, you could set a flag if you see such an event. That's not as clean, but life isn't clean. :)

LeonarddeR · 2022-02-05T07:38:47Z

I see that the terminal edit control currently doesn't have an automation ID. I guess one could be added at this point to distinguish the old from the new implementation.

lhecker · 2022-02-07T17:43:52Z

src/cascadia/TerminalControl/TermControlAutomationPeer.cpp

+        dispatcher.RunAsync(Windows::UI::Core::CoreDispatcherPriority::Normal, [weakThis{ get_weak() }, notificationText{ hstring(newOutput) }]() {
+            if (auto strongThis{ weakThis.get() })
+            {
+                strongThis->RaiseNotificationEvent(AutomationNotificationKind::ActionCompleted,
+                                                   AutomationNotificationProcessing::ImportantAll,
+                                                   notificationText,
+                                                   L"TerminalTextOutput");
+            }
+        });


Don't you need to react to back-pressure here? What if you're called really fast on a slow system? Won't the background jobs just pile up more and more?

We need the jobs to pile up. The screen reader wants to receive the entirety of the output.

Converting this thread to track synchronous notification dispatching. There's a bit of a concern here that the notification events being dispatched in an async way may be processed out of order.

We need the jobs to pile up.

This could be a misunderstanding, but I think "piling them up" is not a good idea and needs to be prevented in this PR for the reasons I mentioned before.

Windows Terminal can write the famous big.txt within half a second, but it has 128457 lines of text. Do we really want 128457 asynchronous jobs to be put into the job queue or some other queue? This is why I mentioned earlier we need to control for back pressure. Personally I can imagine two solutions:

A semaphore to let it run ahead by let's say one viewport height

Only write the next line to the screen when the previous one finished being narrated (a semaphore of size 1)

I can imagine that a user of a narrator might want to have it finish the execution as fast as possible, but I'm worried that creating 128457 async tasks isn't a viable solution either.

Would it be acceptable to make it read out something like "128,000 lines skipped"?

How would an out-of-band announcement like that distinguish itself from output text?

Good question... I'm just throwing ideas at the wall and seeing if anything sticks. Reading 128,000 lines seems intense.

src/buffer/out/textBuffer.cpp

src/cascadia/TerminalControl/TermControlAutomationPeer.cpp

carlos-zamora · 2022-02-09T23:19:58Z

src/renderer/uia/UiaRenderer.cpp

+// - <none>
+// Return Value:
+// - S_FALSE since we do nothing.
+[[nodiscard]] HRESULT UiaEngine::Present() noexcept


The diff is weird here. I basically just moved all of the contents of EndPaint() over to Present(). This way we're not doing this under lock. Leonard said that's the main difference between the two functions.

It's also necessary in this case, because RaiseNotificationEvent() calls back into us (UiaTextRangeBase::GetAttributeValue specifically) which locks the console. This is why we have to do this outside of the console lock inside Present().

carlos-zamora · 2022-02-09T23:55:43Z

I see that the terminal edit control currently doesn't have an automation ID. I guess one could be added at this point to distinguish the old from the new implementation.

@codeofdusk Our concern with the automation ID approach is that a user could have multiple terminals open within the same instance (via panes or tabs), so each pane's TermControl would need a different ID, which kind of defeats the point.

How does this sound:

Windows Terminal: update TermControl's ClassName from TermControl to TermControl 2
Conhost: possibly set/update that class name to Console 2 (once notification events are implemented there)

That way you also get two different versioning systems in one go.

carlos-zamora · 2022-02-10T00:04:27Z

Large output is acting a little weird in this latest build. Printing 5 lines of text works fine with...

for($i=0; $i -lt 5; $i++){echo $i}

But printing 10 lines of text with...

for($i=0; $i -lt 10; $i++){echo $i}

is inconsistent. Occasionally a random number is missing. More often, the new prompt also isn't read.

I'm still debugging through this issue, so if anybody has any ideas, let me know. Might have to do with dispatcher in NotifyNewOutput being null?

josephsl · 2022-02-11T23:11:36Z

Hi, which notification processing instruction is the event using? I can see that Narrator (and other screen readers) can cut off speech if it receives UIA notification with an “important” processing instruction. Thanks.

carlos-zamora · 2022-02-11T23:14:10Z

Hi, which notification processing instruction is the event using? I can see that Narrator (and other screen readers) can cut off speech if it receives UIA notification with an “important” processing instruction. Thanks.

@josephsl AutomationNotificationProcessing::All

src/cascadia/TerminalControl/TermControlAutomationPeer.cpp

josephsl · 2022-02-11T23:19:17Z

Hi, in this case, Narrator ought to announce everything serially (I know that NVDA cuts off current notification if notification processing instruction is “important” or “most recent”; I added notification event for NVDA in 2018 and folks added notification processing instruction handling after that).

…notification-event

DHowett

If the rest of the team is comfortable with this, let's ship it. It scares me, but it's better than what we have today...

Just confirming: none of this happens if there's no screen reader enabled, right?

DHowett · 2022-03-11T20:40:34Z

src/cascadia/TerminalCore/Terminal.cpp

+    // 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);


It feels weird to reach all the way down through the renderer to tell the TermControlAutomationPeer that there was text.

sequenceDiagram TerminalControl ->> TerminalCore: Text TerminalCore ->> Renderer: Text Renderer ->> DX Engine: Text Renderer ->> UIA Engine: Text UIA Engine ->> TCAP: Text TCAP ->> Narrator: Text

Loading

To be fair, the UIA Engine already operated this way. It just didn't know what text was written (only if text was written).

DHowett · 2022-03-11T20:41:13Z

src/renderer/atlas/AtlasEngine.api.cpp

@@ -141,6 +141,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;


you added a base implementation that returns S_FALSE; does atlas need an S_OK version?

AtlasEngine doesn't inherit from RenderEngineBase.
(I did this so I can control the implementation as much as possible.)

DHowett · 2022-03-11T20:41:25Z

src/host/ScreenBufferRenderTarget.cpp

@@ -110,3 +110,13 @@ void ScreenBufferRenderTarget::TriggerTitleChange()
        pRenderer->TriggerTitleChange();
    }
 }
+
+void ScreenBufferRenderTarget::TriggerNewTextNotification(const std::wstring_view newText)


Does conhost need this?

Not yet! Conhost doesn't even use a UIA engine, so this code never gets called. Bill has expressed interest that Conhost should switch over to this notification system because it would make screen readers' lives so much easier (it removes the need to diff). But it makes sense to test this out on Windows Terminal first, then update Conhost later.

DHowett

Note that @j4james is removing the IRenderTarget interface... which may complicate this for you.

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. 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 - [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 (cherry picked from commit f9be172)

## 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 (cherry picked from commit f9be172)

ghost · 2022-03-25T22:49:59Z

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

Handy links:

ghost · 2022-03-25T22:51:35Z

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

Handy links:

LeonarddeR · 2022-03-26T12:40:09Z

How are screen readers supposed to find out whether events are supported or not? See my suggestion in #12358 (comment)

@jcsteh pointed out in #12358 (comment) that we could do something with a flag as soon as an event is received, but I agree that this is far from clean. It would really help if we could find this out from the start. We can look at the app version to begin with, but that would bug with conhost as that has an entirely different versioning scheme.

DHowett · 2022-04-08T23:34:50Z

I believe we settled on giving the control in Terminal a name; we can revisit this once we port the changes to conhost :)

codeofdusk · 2022-04-09T00:05:01Z

I believe we settled on giving the control in Terminal a name

Did we? I thought JAWS had issues with that... CC @carlos-zamora? Did you implement the custom property?

…14048) microsoft/terminal#12358 Summary of the issue: A comment in NVDAObjects.UIA.FindOverlayClasses does not reflect reality. Description of how this pull request fixes the issue: Comment replaced with a more accurate one.

Related to microsoft/terminal#12358 Summary of the issue: For microsoft/terminal#12358, we discussed having a second `TermControl2` class name to distinguish terminals that do and don't support UIA notifications, but this was never implemented and there are no plans to do so. Description of how this pull request fixes the issue: Remove unneeded constant Testing strategy: Alpha testing Known issues with pull request: None known

[Prototype] Use UIA notifications for text output

91eeb59

codeofdusk mentioned this pull request Feb 3, 2022

UI Automation in Windows Console and Windows Terminal: block SV2M2 notification events for now and preserve accessibility after UIA class name change nvaccess/nvda#13261

Merged

5 tasks

carlos-zamora added 2 commits February 3, 2022 15:56

fix audit mode

2108d08

update other fake renderers for testing

320b1f8

lhecker reviewed Feb 7, 2022

View reviewed changes

make events synchronous; notify text line-by-line

a369772

carlos-zamora commented Feb 9, 2022

View reviewed changes

carlos-zamora added 2 commits February 11, 2022 10:58

update class name for 'versioning'

e2373d1

fix large output handling (for accevent)

aa7ecb4

This comment was marked as resolved.

Sign in to view

sapi doesn't exist

31762fc

This comment was marked as resolved.

Sign in to view

std::move

551d648

carlos-zamora commented Feb 11, 2022

View reviewed changes

src/cascadia/TerminalControl/TermControlAutomationPeer.cpp Outdated Show resolved Hide resolved

This comment was marked as resolved.

Sign in to view

josephsl mentioned this pull request Feb 11, 2022

NVDAObjects.UIA: handle UIA notification event if enabled by users nvaccess/nvda#11590

Closed

suppress 'local echo' from keyboard input

768dd7c

This comment was marked as resolved.

Sign in to view

add sapi to spellcheck

facc78c

Merge remote-tracking branch 'origin/main' into dev/cazamor/a11y/uia-…

e5e5831

…notification-event

DHowett requested changes Mar 11, 2022

View reviewed changes

ghost added the Needs-Author-Feedback The original author of the issue/PR needs to come back and respond to something label Mar 11, 2022

DHowett reviewed Mar 11, 2022

View reviewed changes

carlos-zamora assigned carlos-zamora and unassigned DHowett Mar 11, 2022

ghost removed the Needs-Author-Feedback The original author of the issue/PR needs to come back and respond to something label Mar 11, 2022

carlos-zamora assigned DHowett and unassigned carlos-zamora Mar 11, 2022

DHowett approved these changes Mar 11, 2022

View reviewed changes

ghost merged commit f9be172 into main Mar 11, 2022

ghost deleted the dev/cazamor/a11y/uia-notification-event branch March 11, 2022 23:50

DHowett mentioned this pull request Mar 12, 2022

Eliminate the IRenderTarget interface #12568

Merged

6 tasks

zadjii-msft mentioned this pull request Mar 17, 2022

Jaws screen reader #11730

Closed

jpospical mentioned this pull request Mar 26, 2022

Text is announced twice in the latest version of Windows Terminal (1.12.10732.0) nvaccess/nvda#13541

Closed

codeofdusk mentioned this pull request Jun 9, 2022

Windows Terminal: Leverage new UI Automation notifications support nvaccess/nvda#13781

Closed

zadjii-msft mentioned this pull request Jun 22, 2022

Crash while holding a key #13357

Closed

codeofdusk mentioned this pull request Jul 26, 2022

Add a means by which UIA clients can determine whether a build of Windows Terminal does or doesn't support notifications #13601

Open

codeofdusk mentioned this pull request Aug 22, 2022

Replace outdated comment in Windows Terminal overlay class selection nvaccess/nvda#14048

Merged

6 tasks

carlos-zamora mentioned this pull request Oct 12, 2023

Fix UIA and marks regressions due to cooked read #16105

Merged

codeofdusk mentioned this pull request Oct 9, 2024

Remove unneeded and unused TermControl2 class name nvaccess/nvda#17269

Merged

5 tasks

This pull request was closed.

Use UIA notifications for text output #12358

Use UIA notifications for text output #12358

Conversation

carlos-zamora commented Feb 3, 2022 • edited Loading

Summary of the Pull Request

Detailed Description of the Pull Request / Additional comments

Test cases

codeofdusk commented Feb 4, 2022

carlos-zamora commented Feb 4, 2022

codeofdusk commented Feb 4, 2022 • edited Loading

jcsteh commented Feb 4, 2022 via email

LeonarddeR commented Feb 5, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lhecker Feb 8, 2022 • edited Loading

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

lhecker Feb 9, 2022 • edited Loading

Choose a reason for hiding this comment

carlos-zamora commented Feb 9, 2022

carlos-zamora commented Feb 10, 2022

This comment was marked as resolved.

This comment was marked as resolved.

This comment was marked as resolved.

josephsl commented Feb 11, 2022 via email

carlos-zamora commented Feb 11, 2022

This comment was marked as resolved.

josephsl commented Feb 11, 2022 via email

This comment was marked as resolved.

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

lhecker Mar 11, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DHowett left a comment

Choose a reason for hiding this comment

ghost commented Mar 25, 2022

ghost commented Mar 25, 2022

LeonarddeR commented Mar 26, 2022 • edited Loading

DHowett commented Apr 8, 2022

codeofdusk commented Apr 9, 2022

carlos-zamora commented Feb 3, 2022 •

edited

Loading

codeofdusk commented Feb 4, 2022 •

edited

Loading

lhecker Feb 8, 2022 •

edited

Loading

lhecker Feb 9, 2022 •

edited

Loading

lhecker Mar 11, 2022 •

edited

Loading

LeonarddeR commented Mar 26, 2022 •

edited

Loading