Accessibility: TermControl Automation Peer #2083
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
- Moved WindowUiaProvider to Types (Shared) - Hooked up WindowUiaProvider to BaseWindow (WT) UIATree is now accessible in WT
…g used. TODO: - replace IWindow with IConsoleWindow (for my own sanity) - turn SIUP into an abstract class where any use of ServiceLocator and Selection is abstracted out
- Made IslandWindow inherit IConsoleWindow - Moved some UIATree logic to IslandWindow - Fixed ConHost to work with UIATree (sorta) At the moment, ConHost is using a temp ScreenInfoUIAProvider. This will be fixed with next inheritance patch. TODO: - turn SIUP into an abstract class where any use of ServiceLocator and Selection is abstracted out
Added correct strings to WindowUiaProvider description fields
Created TermControlAP (AutomationPeer) Created XamlUiaTextRange to wrap UiaTextRange Need to fix... - GetSelection() - GetVisibleRanges() - Compare() (A few other minor ones too)
- Moved WindowUiaProvider to Types (Shared) - Hooked up WindowUiaProvider to BaseWindow (WT) UIATree is now accessible in WT
…g used. TODO: - replace IWindow with IConsoleWindow (for my own sanity) - turn SIUP into an abstract class where any use of ServiceLocator and Selection is abstracted out
- Made IslandWindow inherit IConsoleWindow - Moved some UIATree logic to IslandWindow - Fixed ConHost to work with UIATree (sorta) At the moment, ConHost is using a temp ScreenInfoUIAProvider. This will be fixed with next inheritance patch. TODO: - turn SIUP into an abstract class where any use of ServiceLocator and Selection is abstracted out
Added correct strings to WindowUiaProvider description fields
zadjii-msft
reviewed
Jul 25, 2019
|
So, there's a big set of "TODO CARLOS" in the changes I made. It's all one issue: hwnd is now optional. Should we ship with this and worry about it later? Edit: just threw them into separate work items. Need to do some refactoring anyways. |
miniksa
requested changes
Jul 25, 2019
ghost
added
Needs-Author-Feedback
Attached TODO CARLOS to actual github issue
ghost
added
Needs-Author-Feedback
miniksa
reviewed
Jul 29, 2019
Fixed ownership issue with SAFEARRAYs
# Conflicts: # src/cascadia/WindowsTerminal/WindowUiaProvider.cpp # src/host/tracing.cpp # src/host/tracing.hpp # src/interactivity/win32/WindowMetrics.cpp # src/interactivity/win32/window.cpp # src/types/IUiaWindow.h # src/types/ScreenInfoUiaProvider.cpp # src/types/ScreenInfoUiaProvider.h # src/types/UiaTextRange.cpp # src/types/WindowUiaProviderBase.cpp # src/types/WindowUiaProviderBase.hpp # src/types/lib/types.vcxproj.filters
zadjii-msft
reviewed
Jul 30, 2019
There was a problem hiding this comment.
I'm not sure I have anything worth blocking over, but I certainly have questions. Judging based on this PR, #2120 is going to help me feel a lot more comfortable with this change - I really didn't love having to check _pUiaParent all over the ScreenInfoUiaProvider, but if that's getting done in #2120 I'll just deal with it for now.
miniksa
requested changes
Jul 30, 2019
ghost
added
the
Needs-Author-Feedback
DHowett-MSFT
suggested changes
Jul 30, 2019
ghost
added
Needs-Author-Feedback
More PR changes
zadjii-msft
approved these changes
Jul 30, 2019
miniksa
approved these changes
Jul 30, 2019
DHowett-MSFT
suggested changes
Jul 30, 2019
ghost
added
the
Needs-Author-Feedback
and a minor bugfix on compare
ghost
removed
the
Needs-Author-Feedback
|
🎆 |
|
🎉 Handy links: |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Summary of the Pull Request
Adds accessibility patterns to the Terminal Control using ConHost's accessibility model.
References
Builds on the work of #1691 and #1915
Detailed Description of the Pull Request / Additional comments
Let's start with the easy change:
TermControl'scontrolRootwas removed.TermControlis aUserControlnow.Ok. Now we've got a story to tell here....
TermControlAP - the Automation Peer
Here's an in-depth guide on custom automation peers.
We have a custom XAML element (TermControl). So XAML can't really hold our hands and determine an accessible behavior for us. So this automation peer is responsible for enabling that interaction.
We made it a FrameworkElementAutomationPeer to get as much accessibility as possible from it just being a XAML element (i.e.: where are we on the screen? what are my dimensions?). This is recommended. Any functions with "Core" at the end, are overwritten here to tweak this automation peer into what we really need.
But what kind of interactions can a user expect from this XAML element? Introducing ControlPatterns! There's a ton of interfaces that just define "what can I do". Thankfully, we already know that we're supposed to be
ScreenInfoUiaProviderand that was anITextProvider, so let's just make the TermControlAP anITextProvidertoo.So now we have a way to define what accessible actions can be performed on us, but what should those actions do? Well let's just use the automation providers from ConHost that are now in a shared space! (Note: this is a great place to stop and get some coffee. We're about to hop into the .cpp file in the next section)
Wrapping our shared Automation Providers
Unfortunately, we can't just use the automation providers from ConHost. Or, at least not just hook them up as easily as we wish. ConHost's UIA Providers were written using UIAutomationCore's
ITextProviderandITextRangeProvider. XAML's interfaces (ITextProviderandITextRangeProvider) are lined up to be exactly the same.So we need to wrap our ConHost UIA Providers (UIAutomationCore) with the XAML ones. We had two providers, so that means we have two wrappers.
TermControlAP (XAML) <----> ScreenInfoUiaProvider (UIAutomationCore)
Each of the functions in the pragma region
ITextProviderfor TermControlAP.cpp is just wrapping what we do inScreenInfoUiaProvider, and returning an acceptable version of it.Most of
ScreenInfoUiaProvider's functions returnUiaTextRanges. So we need to wrap that too. That's this next section...XamlUiaTextRange (XAML) <----> UiaTextRange (UIAutomationCore)
Same idea. We're wrapping everything that we could do with
UiaTextRangeand putting it inside ofXamlUiaTextRange.Additional changes to
UiaTextRangeandScreenInfoUiaProviderIf you don't know what I just said, please read this background:
TL;DR:
ScreenInfoUiaProviderlets you interact with the displayed text.UiaTextRangeis specific ranges of text in the display and navigate the text.Thankfully, we didn't do many changes here. I feel like some of it is hacked together but now that we have a somewhat working system, making changes shouldn't be too hard...I hope.
UiaTextRange
We don't have access to the window handle. We really only need it to draw the bounding rects using WinUser's
ScreenToClient()andClientToScreen(). I need to figure out how to get around this.In the meantime, I made the window handle optional. And if we don't have one....well, we need to figure that out. But other than that, we have a
UiaTextRange.ScreenInfoUiaProvider
At some point, we need to hook up this automation provider to the WindowUiaProvider. This should help with navigation of the UIA Tree and make everything just look waaaay better. For now, let's just do the same approach and make the pUiaParent optional.
This one's the one I'm not that proud of, but it works. We need the parent to get a bounding rect of the terminal. While we figure out how to attach the WindowUiaProvider, we should at the very least be able to get a bunch of info from our xaml automation peer. So, I've added a _getBoundingRect optional function. This is what's called when we don't have a WindowUiaProvider as our parent.
Validation Steps Performed
I've been using inspect.exe to see the UIA tree. I was able to interact with the terminal mostly fine. A few known issues below.
Unfortunately, I tried running Narrator on this and it didn't seem to like it (by that I mean WT crashed). Then again, I don't really know how to use narrator other than "click on object" --> "listen voice". I feel like there's a way to get the other interactions with narrator, but I'll be looking into more of that soon. I bet if I fix the two issues below, Narrator will be happy.
Miscellaneous Known Issues
GetSelection()andGetVisibleRanges()crashes. I need to debug through these. I want to include them in this PR.