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.

Sign up for GitHub

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

Jump to bottom

Narrator scan mode *does not work* in 1.11+ #11488

Closed
DHowett opened this issue Oct 12, 2021 · 1 comment · Fixed by #11501
Closed

Narrator scan mode *does not work* in 1.11+ #11488

DHowett opened this issue Oct 12, 2021 · 1 comment · Fixed by #11501
Assignees
@carlos-zamora
Labels
Area-Accessibility Issues related to accessibility Issue-Bug It either shouldn't be doing this or needs an investigation. Priority-0 Bugs that we consider release-blocking/recall-class (P0) Product-Terminal The new Windows Terminal. Resolution-Fix-Committed Fix is checked in, but it might be 3-4 weeks until a release. Severity-Blocking We won't ship a release like this! No-siree.
Milestone
Terminal v1.12

Comments

@DHowett
Copy link
Member

DHowett commented Oct 12, 2021

Windows Terminal version (or Windows build number)

1.11+

Other Software

No response

Steps to reproduce

Enable scan mode, or any mode, and try to navigate the buffer using narrator.

Expected Behavior

No response

Actual Behavior

It reads one or two things, and then gets "stuck" or moves into the titlebar.
@DHowett DHowett added Issue-Bug It either shouldn't be doing this or needs an investigation. Area-Accessibility Issues related to accessibility Product-Terminal The new Windows Terminal. Severity-Blocking We won't ship a release like this! No-siree. Priority-0 Bugs that we consider release-blocking/recall-class (P0) labels Oct 12, 2021
@ghost ghost added the Needs-Triage It's a new issue that the core contributor team needs to triage at the next triage meeting label Oct 12, 2021
@DHowett
Copy link
Member Author

DHowett commented Oct 12, 2021

this is p0 and blocking for 1.11 moving to stable, 1.12 going to preview

@DHowett DHowett removed the Needs-Triage It's a new issue that the core contributor team needs to triage at the next triage meeting label Oct 12, 2021
@DHowett DHowett added this to the Terminal v1.12 milestone Oct 12, 2021
@carlos-zamora carlos-zamora mentioned this issue Oct 13, 2021
Merged
@ghost ghost added the In-PR This issue has a related PR label Oct 13, 2021
@ghost ghost closed this as completed in #11501 Oct 13, 2021
@ghost ghost removed the In-PR This issue has a related PR label Oct 13, 2021
ghost pushed a commit that referenced this issue Oct 13, 2021
@carlos-zamora
Properly initialize XamlUiaTextRange with ProviderFromPeer (#11501)
02ac246 
## Summary of the Pull Request
As a part of the Interactivity split, `TermControlAutomationPeer` had to be split into `TermControlAutomationPeer` (TCAP) and `InteractivityAutomationPeer` (IAP). Just about all of the functions in `InterativityAutomationPeer` operate by calling the non-XAML UIA Provider then wrapping the resulting `UIATextRange` into a XAML format (a `XamlUiaTextRange` [XUTR]). As a part of that XUTR constructor, we need a reference to the parent provider.

We generally get that via `ProviderFromPeer()`, but IAP's `ProviderFromPeer()` returned null (presumably because IAP isn't in the UI tree, whereas TCAP is directly registered as the automation peer for the `TermControl`).

It looks like some screen readers didn't care (like NVDA, though there may be a chance we just didn't encounter an issue just yet), but Narrator definitely did.

The fix was to provide XUTR constructors the `ProviderFromPeer` from TCAP, _not_ IAP. To accomplish this, IAP now holds a weak reference to TCAP, and provides the `ProviderFromPeer` when needed. We can't cache this result because there is no guarantee that it won't change.

Some miscellaneous changes include:
- `TermControl::OnCreateAutomationPeer` now returns the existing auto peer instead of always creating a new one
- `TCAP::WrapArrayOfTextRangeProviders` was removed as it was unused (normally, this would be directly affected by the main `ProviderFromPeer` change here)
- `XUTR::GetEnclosingElement` is now hooked up to trace logging for debugging purposes

## References
Introduced in #10051
Closes #11488 

## Validation Steps Performed
✅ Narrator scan mode now works (verified with character, word, and line navigation)
✅ NVDA movement still works (verified with word and line navigation)
@ghost ghost added the Resolution-Fix-Committed Fix is checked in, but it might be 3-4 weeks until a release. label Oct 13, 2021
PankajBhojwani pushed a commit that referenced this issue Oct 13, 2021
@carlos-zamora @PankajBhojwani
Properly initialize XamlUiaTextRange with ProviderFromPeer (#11501)
91b8b90 
## Summary of the Pull Request
As a part of the Interactivity split, `TermControlAutomationPeer` had to be split into `TermControlAutomationPeer` (TCAP) and `InteractivityAutomationPeer` (IAP). Just about all of the functions in `InterativityAutomationPeer` operate by calling the non-XAML UIA Provider then wrapping the resulting `UIATextRange` into a XAML format (a `XamlUiaTextRange` [XUTR]). As a part of that XUTR constructor, we need a reference to the parent provider.

We generally get that via `ProviderFromPeer()`, but IAP's `ProviderFromPeer()` returned null (presumably because IAP isn't in the UI tree, whereas TCAP is directly registered as the automation peer for the `TermControl`).

It looks like some screen readers didn't care (like NVDA, though there may be a chance we just didn't encounter an issue just yet), but Narrator definitely did.

The fix was to provide XUTR constructors the `ProviderFromPeer` from TCAP, _not_ IAP. To accomplish this, IAP now holds a weak reference to TCAP, and provides the `ProviderFromPeer` when needed. We can't cache this result because there is no guarantee that it won't change.

Some miscellaneous changes include:
- `TermControl::OnCreateAutomationPeer` now returns the existing auto peer instead of always creating a new one
- `TCAP::WrapArrayOfTextRangeProviders` was removed as it was unused (normally, this would be directly affected by the main `ProviderFromPeer` change here)
- `XUTR::GetEnclosingElement` is now hooked up to trace logging for debugging purposes

## References
Introduced in #10051
Closes #11488 

## Validation Steps Performed
✅ Narrator scan mode now works (verified with character, word, and line navigation)
✅ NVDA movement still works (verified with word and line navigation)
PankajBhojwani pushed a commit that referenced this issue Oct 15, 2021
@carlos-zamora @PankajBhojwani
Properly initialize XamlUiaTextRange with ProviderFromPeer (#11501)
f94b98b 
## Summary of the Pull Request
As a part of the Interactivity split, `TermControlAutomationPeer` had to be split into `TermControlAutomationPeer` (TCAP) and `InteractivityAutomationPeer` (IAP). Just about all of the functions in `InterativityAutomationPeer` operate by calling the non-XAML UIA Provider then wrapping the resulting `UIATextRange` into a XAML format (a `XamlUiaTextRange` [XUTR]). As a part of that XUTR constructor, we need a reference to the parent provider.

We generally get that via `ProviderFromPeer()`, but IAP's `ProviderFromPeer()` returned null (presumably because IAP isn't in the UI tree, whereas TCAP is directly registered as the automation peer for the `TermControl`).

It looks like some screen readers didn't care (like NVDA, though there may be a chance we just didn't encounter an issue just yet), but Narrator definitely did.

The fix was to provide XUTR constructors the `ProviderFromPeer` from TCAP, _not_ IAP. To accomplish this, IAP now holds a weak reference to TCAP, and provides the `ProviderFromPeer` when needed. We can't cache this result because there is no guarantee that it won't change.

Some miscellaneous changes include:
- `TermControl::OnCreateAutomationPeer` now returns the existing auto peer instead of always creating a new one
- `TCAP::WrapArrayOfTextRangeProviders` was removed as it was unused (normally, this would be directly affected by the main `ProviderFromPeer` change here)
- `XUTR::GetEnclosingElement` is now hooked up to trace logging for debugging purposes

## References
Introduced in #10051
Closes #11488

## Validation Steps Performed
✅ Narrator scan mode now works (verified with character, word, and line navigation)
✅ NVDA movement still works (verified with word and line navigation)

(cherry picked from commit 02ac246)
This issue was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@carlos-zamora carlos-zamora

Labels
Area-Accessibility Issues related to accessibility Issue-Bug It either shouldn't be doing this or needs an investigation. Priority-0 Bugs that we consider release-blocking/recall-class (P0) Product-Terminal The new Windows Terminal. Resolution-Fix-Committed Fix is checked in, but it might be 3-4 weeks until a release. Severity-Blocking We won't ship a release like this! No-siree.
Projects
None yet
Milestone
Terminal v1.12
Development

Successfully merging a pull request may close this issue.

Properly initialize XamlUiaTextRange with ProviderFromPeer microsoft/terminal
2 participants
@DHowett @carlos-zamora