Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Accessibility: Signaling Model #2447

Closed
1 of 5 tasks
carlos-zamora opened this issue Aug 15, 2019 · 11 comments · Fixed by #4826
Closed
1 of 5 tasks

Accessibility: Signaling Model #2447

carlos-zamora opened this issue Aug 15, 2019 · 11 comments · Fixed by #4826
Assignees
Labels
Area-Accessibility Issues related to accessibility Issue-Feature Complex enough to require an in depth planning process and actual budgeted, scheduled work. Priority-1 A description (P1) Product-Terminal The new Windows Terminal. Resolution-Fix-Committed Fix is checked in, but it might be 3-4 weeks until a release.
Milestone

Comments

@carlos-zamora
Copy link
Member

carlos-zamora commented Aug 15, 2019

Description of the new feature/enhancement

UIA has signal events (link). ConHost supported them. WindowUiaProvider was an integral part to that.

Windows Terminal currently doesn't support them. We have some architecture in place to do so though.

Proposed technical implementation details (optional)

WindowUiaProvider already has the groundwork for that. It might be that we don't even need a WindowUiaProvider and just have XAML do some of it.

For reference, the signaling model for conhost was as follows:

<ConHost Event (i.e.: new text)>
--> Signal WindowUiaProvider
--> Signal ScreenInfoUiaProvider
--> Raise UiaAutomationEvent to Client

The client then responds to these events with more actions (i.e.: recreate object, etc...)

EDIT: The functionality for signaling includes...

  • Selection Events (selection changed) PR
  • Text Buffer Events (new output)
  • Scroll Events

I may possible need...

  • Resize events
  • Cursor Events
@carlos-zamora carlos-zamora added the Issue-Feature Complex enough to require an in depth planning process and actual budgeted, scheduled work. label Aug 15, 2019
@ghost ghost added Needs-Triage It's a new issue that the core contributor team needs to triage at the next triage meeting Needs-Tag-Fix Doesn't match tag requirements labels Aug 15, 2019
@carlos-zamora carlos-zamora added Area-Accessibility Issues related to accessibility Product-Terminal The new Windows Terminal. and removed Needs-Tag-Fix Doesn't match tag requirements Needs-Triage It's a new issue that the core contributor team needs to triage at the next triage meeting labels Aug 15, 2019
@carlos-zamora carlos-zamora self-assigned this Aug 15, 2019
@DHowett-MSFT DHowett-MSFT added this to the Terminal v1.0 milestone Aug 16, 2019
@codeofdusk
Copy link
Contributor

Blocking nvaccess/nvda#10305.

@DHowett-MSFT DHowett-MSFT added this to the Terminal v1.0 milestone Feb 11, 2020
ghost pushed a commit that referenced this issue Feb 20, 2020
## Summary of the Pull Request
We used to return multiple text ranges to represent one selection. We only support one selection at a time, so we should only return one range.

Additionally, I moved all TriggerSelection() calls to the renderer from Terminal to TermControl for consistency. This ensures we only call it _once_ when we make a change to our selection state.

## References
#2447 - helps polish Signaling for Selection
#4465 - This is more apparent as the problem holding back Signaling for Selection

## PR Checklist
* [x] Closes #4452 

Tested using Accessibility Insights.
DHowett-MSFT pushed a commit that referenced this issue Feb 28, 2020
- When performing chunk selection, the expansion now occurs at the time
  of the selection, not the rendering of the selection
- `GetSelectionRects()` was moved to the `TextBuffer` and is now shared
  between ConHost and Windows Terminal
- Some of the selection variables were renamed for clarity
- Selection COORDs are now in the Text Buffer coordinate space
- Fixes an issue with Shift+Click after performing a Multi-Click
  Selection

## References
This also contributes to...
- #4509: UIA Box Selection
- #2447: UIA Signaling for Selection
- #1354: UIA support for Wide Glyphs

Now that the expansion occurs at before render-time, the selection
anchors are an accurate representation of what is selected. We just need
to move `GetText` to the `TextBuffer`. Then we can have those three
issues just rely on code from the text buffer. This also means ConHost
gets some of this stuff for free 😀

### TextBuffer
- `GetTextRects` is the abstracted form of `GetSelectionRects`
- `_ExpandTextRow` is still needed to handle wide glyphs properly

### Terminal
- Rename...
    - `_boxSelection` --> `_blockSelection` for consistency with ConHost
    - `_selectionAnchor` --> `_selectionStart` for consistency with UIA
    - `_endSelectionPosition` --> `_selectionEnd` for consistency with
      UIA
- Selection anchors are in Text Buffer coordinates now
- Really rely on `SetSelectionEnd` to accomplish appropriate chunk
  selection and shift+click actions

## Validation Steps Performed
- Shift+Click
- Multi-Click --> Shift+Click
- Chunk Selection at...
    - top of buffer
    - bottom of buffer
    - random region in scrollback

Closes #4465
Closes #4547
@carlos-zamora
Copy link
Member Author

@codeofdusk @michaelDCurran @LeonarddeR
CC @DHowett-MSFT

I've attached a release x64 build of dev/cazamor/acc/sig/output. I'll be working off of that branch to get automation events working for output. The changes are pretty naive but simple: whenever we need to render new content to the screen, fire a TextChanged automation event.

For something as simple as echo hello, I can see that some degenerate ranges are created at the end of the written command, then at the end of the prompt after the command has been executed. However, it seems like a range connecting both points is never actually created and read.

Are you expecting a different Automation Event? Do I need to configure my TermControlAutomationPeer any further, maybe? Let me know what's going on from your end :)

NOTE: you'll have to run WindowsTerminal.exe here, NOT OpenConsole.exe. ConHost has not been switched over to the new signaling model yet, whereas Windows Terminal has.

WT_signal_output.zip

@codeofdusk
Copy link
Contributor

@codeofdusk @michaelDCurran @LeonarddeR
CC @DHowett-MSFT

I've attached a release x64 build of dev/cazamor/acc/sig/output. I'll be working off of that branch to get automation events working for output. The changes are pretty naive but simple: whenever we need to render new content to the screen, fire a TextChanged automation event.

For something as simple as echo hello, I can see that some degenerate ranges are created at the end of the written command, then at the end of the prompt after the command has been executed. However, it seems like a range connecting both points is never actually created and read.

Are you expecting a different Automation Event? Do I need to configure my TermControlAutomationPeer any further, maybe? Let me know what's going on from your end :)

NOTE: you'll have to run WindowsTerminal.exe here, NOT OpenConsole.exe. ConHost has not been switched over to the new signaling model yet, whereas Windows Terminal has.

WT_signal_output.zip

Thanks @carlos-zamora This looks sufficient for implementing terminal support in NVDA. Things are mostly working (see nvaccess/nvda#10784), except we're getting doubled characters when typing (but I think I know where the issue is, and it's on our side). Are there plans to have this merged soon?

@carlos-zamora
Copy link
Member Author

@codeofdusk @michaelDCurran @LeonarddeR
CC @DHowett-MSFT
I've attached a release x64 build of dev/cazamor/acc/sig/output. I'll be working off of that branch to get automation events working for output. The changes are pretty naive but simple: whenever we need to render new content to the screen, fire a TextChanged automation event.
For something as simple as echo hello, I can see that some degenerate ranges are created at the end of the written command, then at the end of the prompt after the command has been executed. However, it seems like a range connecting both points is never actually created and read.
Are you expecting a different Automation Event? Do I need to configure my TermControlAutomationPeer any further, maybe? Let me know what's going on from your end :)
NOTE: you'll have to run WindowsTerminal.exe here, NOT OpenConsole.exe. ConHost has not been switched over to the new signaling model yet, whereas Windows Terminal has.
WT_signal_output.zip

Thanks @carlos-zamora This looks sufficient for implementing terminal support in NVDA. Things are mostly working (see nvaccess/nvda#10784), except we're getting doubled characters when typing (but I think I know where the issue is, and it's on our side). Are there plans to have this merged soon?

If I can get it working for Narrator too, that'd be ideal. But, I guess there's no harm in merging what I have then iterating on master. I'll submit a PR today then. Thanks!

@codeofdusk
Copy link
Contributor

@codeofdusk @michaelDCurran @LeonarddeR
CC @DHowett-MSFT
I've attached a release x64 build of dev/cazamor/acc/sig/output. I'll be working off of that branch to get automation events working for output. The changes are pretty naive but simple: whenever we need to render new content to the screen, fire a TextChanged automation event.
For something as simple as echo hello, I can see that some degenerate ranges are created at the end of the written command, then at the end of the prompt after the command has been executed. However, it seems like a range connecting both points is never actually created and read.
Are you expecting a different Automation Event? Do I need to configure my TermControlAutomationPeer any further, maybe? Let me know what's going on from your end :)
NOTE: you'll have to run WindowsTerminal.exe here, NOT OpenConsole.exe. ConHost has not been switched over to the new signaling model yet, whereas Windows Terminal has.
WT_signal_output.zip

Thanks @carlos-zamora This looks sufficient for implementing terminal support in NVDA. Things are mostly working (see nvaccess/nvda#10784), except we're getting doubled characters when typing (but I think I know where the issue is, and it's on our side). Are there plans to have this merged soon?

If I can get it working for Narrator too, that'd be ideal. But, I guess there's no harm in merging what I have then iterating on master. I'll submit a PR today then. Thanks!

I've tested with Narrator and observed, as you did, that we aren't getting new text announcements.

In NVDA, we have an NVDAObjects.behaviors.LiveText class that waits for text change events from various APIs (such as UIA), diffs the current text against the last-reported text, and speaks the changes. Does Narrator have a similar paraddigm for consoles?

@carlos-zamora
Copy link
Member Author

@codeofdusk @michaelDCurran @LeonarddeR
CC @DHowett-MSFT
I've attached a release x64 build of dev/cazamor/acc/sig/output. I'll be working off of that branch to get automation events working for output. The changes are pretty naive but simple: whenever we need to render new content to the screen, fire a TextChanged automation event.
For something as simple as echo hello, I can see that some degenerate ranges are created at the end of the written command, then at the end of the prompt after the command has been executed. However, it seems like a range connecting both points is never actually created and read.
Are you expecting a different Automation Event? Do I need to configure my TermControlAutomationPeer any further, maybe? Let me know what's going on from your end :)
NOTE: you'll have to run WindowsTerminal.exe here, NOT OpenConsole.exe. ConHost has not been switched over to the new signaling model yet, whereas Windows Terminal has.
WT_signal_output.zip

Thanks @carlos-zamora This looks sufficient for implementing terminal support in NVDA. Things are mostly working (see nvaccess/nvda#10784), except we're getting doubled characters when typing (but I think I know where the issue is, and it's on our side). Are there plans to have this merged soon?

If I can get it working for Narrator too, that'd be ideal. But, I guess there's no harm in merging what I have then iterating on master. I'll submit a PR today then. Thanks!

I've tested with Narrator and observed, as you did, that we aren't getting new text announcements.

In NVDA, we have an NVDAObjects.behaviors.LiveText class that waits for text change events from various APIs (such as UIA), diffs the current text against the last-reported text, and speaks the changes. Does Narrator have a similar paraddigm for consoles?

Not sure. Their code can be a bit complicated haha. I'm working with one of their devs to get a better look. I'm hoping that it can get fixed by next week because we're definitely VERY close.

@LeonarddeR
Copy link

If narrator doesn't use a diffing method like NVDA does, the only alternative I can think of is making the terminal a live region. Pretty sure it doesn't work like this, though, as it has several drawbacks.

@LeonarddeR
Copy link

Ah and in theory, UIA notification events could be used as well. If they are fired by conhost, NVDA would ignore them as NVDA only speaks notification events fire by the foreground application.

carlos-zamora added a commit that referenced this issue Mar 9, 2020
## Summary of the Pull Request
`GetTextForClipboard` already exists in the TextBuffer. It makes sense to use that for UIA as well. This changes the behavior or `GetText()` such that it does not remove leading/trailing whitespace anymore. That is more of an expected behavior.

## References
This also contributes to...
- #4509: UIA Box Selection
- #2447: UIA Signaling for Selection
- #1354: UIA support for Wide Glyphs
Now that the expansion occurs at before render-time, the selection anchors are an accurate representation of what is selected. We just need to move GetText to the TextBuffer. Then we can have those three issues just rely on code from the text buffer. This also means ConHost gets some of this stuff for free 😀

## Detailed Description of the Pull Request / Additional comments
- `TextBuffer::GetTextForClipboard()` --> `GetText()`
- `TextBuffer::GetText()` no longer requires GetForegroundColor/GetBackgroundColor. If either of these are not defined, we return a `TextAndColor` with only the `text` field populated.
- renamed a few parameters for copying text to the clipboard for clarity
- Updated `UiaTextRange::GetText()` to use `TextBuffer::GetText()`

## Validation Steps Performed
Manual tests for UIA using accessibility insights and Windows Terminal's copy action (w/ and w/out shift)

Added tests as well.
@codeofdusk
Copy link
Contributor

But, I guess there's no harm in merging what I have then iterating on master.

@carlos-zamora Do you still plan to do this?

@carlos-zamora
Copy link
Member Author

But, I guess there's no harm in merging what I have then iterating on master.

@carlos-zamora Do you still plan to do this?

Yup! Here it is: #4826

@ghost ghost added the In-PR This issue has a related PR label Mar 17, 2020
@ghost ghost closed this as completed in #4826 Mar 18, 2020
@ghost ghost added Resolution-Fix-Committed Fix is checked in, but it might be 3-4 weeks until a release. and removed In-PR This issue has a related PR labels Mar 18, 2020
ghost pushed a commit that referenced this issue Mar 18, 2020
## Summary of the Pull Request
This notifies automation clients (i.e.: NVDA, narrator, etc...) of new output being rendered to the screen.

## References
Close #2447 - Signaling for new output and cursor
Close #3791 - fixed by signaling cursor changes

## Detailed Description of the Pull Request / Additional comments
- Added tracing for UiaRenderer. This makes it easier to debug issues with notifying an automation client.
- Fire TextChanged automation events when new content is output to the screen.

## Validation Steps Performed
Verified with NVDA [1]

## Narrator
Narrator works _better_, but is unable to detect new output consistently. There is no harm for narrator when this change goes in.

[1] #2447 (comment)
DHowett-MSFT pushed a commit that referenced this issue Apr 14, 2020
## Summary of the Pull Request
This notifies automation clients (i.e.: NVDA, narrator, etc...) of new output being rendered to the screen.

## References
Close #2447 - Signaling for new output and cursor
Close #3791 - fixed by signaling cursor changes

## Detailed Description of the Pull Request / Additional comments
- Added tracing for UiaRenderer. This makes it easier to debug issues with notifying an automation client.
- Fire TextChanged automation events when new content is output to the screen.

## Validation Steps Performed
Verified with NVDA [1]

## Narrator
Narrator works _better_, but is unable to detect new output consistently. There is no harm for narrator when this change goes in.

[1] #2447 (comment)
@ghost
Copy link

ghost commented Apr 22, 2020

🎉This issue was addressed in #4826, which has now been successfully released as Windows Terminal Preview v0.11.1121.0.:tada:

Handy links:

donno2048 added a commit to donno2048/terminal that referenced this issue Sep 28, 2020
## Summary of the Pull Request
This notifies automation clients (i.e.: NVDA, narrator, etc...) of new output being rendered to the screen.

## References
Close #2447 - Signaling for new output and cursor
Close #3791 - fixed by signaling cursor changes

## Detailed Description of the Pull Request / Additional comments
- Added tracing for UiaRenderer. This makes it easier to debug issues with notifying an automation client.
- Fire TextChanged automation events when new content is output to the screen.

## Validation Steps Performed
Verified with NVDA [1]

## Narrator
Narrator works _better_, but is unable to detect new output consistently. There is no harm for narrator when this change goes in.

[1] microsoft/terminal#2447 (comment)
This issue was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area-Accessibility Issues related to accessibility Issue-Feature Complex enough to require an in depth planning process and actual budgeted, scheduled work. Priority-1 A description (P1) Product-Terminal The new Windows Terminal. Resolution-Fix-Committed Fix is checked in, but it might be 3-4 weeks until a release.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants