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: Finalized Shared UIA Tree Model #1915

Merged
merged 5 commits into from
Jul 16, 2019

Conversation

carlos-zamora
Copy link
Member

@carlos-zamora carlos-zamora commented Jul 10, 2019

Summary of the Pull Request

The UIA provider classes are now shared properly. They're hooked up to ConHost and Windows Terminal. You should actually have a UIA Tree when you try to use them.

The actual text buffer UIA element is not hooked up for Windows Terminal, however. That'll be a later PR.

References

Extension of #1691
Please read the description in that PR for background information.

Detailed Description of the Pull Request / Additional comments

Changes to the WindowUiaProvider

As mentioned earlier, the WindowUiaProvider is the top-level UIA provider for our projects. To reuse as much code as possible, I created Microsoft::Console::Types::WindowUiaProviderBase. Any existing functions that reference a ScreenInfoUiaProvider were virtual-ized.

In each project, a WindowUiaProvider : WindowUiaProviderBase was created to define those virtual functions. Note that that will be the main difference between ConHost and Windows Terminal moving forward: how many TextBuffers are on the screen.

So, ConHost should be the same as before, with only one ScreenInfoUiaProvider, whereas Windows Terminal needs to (1) update which one is on the screen and (2) may have multiple on the screen.

🚨 Windows Terminal doesn't have the ScreenInfoUiaProvider hooked up yet. We'll have all the XAML elements in the UIA tree. But, since TermControl is a custom XAML Control, I need to hook up the ScreenInfoUiaProvider to it. This work will be done in a new PR and resolve GitHub Issue #1352.

Moved to Microsoft::Console::Types

These files got moved to a shared area so that they can be used by both ConHost and Windows Terminal.
This means that any references to the ServiceLocator had to be removed.

  • IConsoleWindow
    • Windows Terminal: IslandWindow : IConsoleWindow
  • ScreenInfoUiaProvider
    • all references to ServiceLocator and SCREEN_INFORMATION were removed. IRenderData was used to accomplish this. Refer to next section for more details.
  • UiaTextRange
    • all references to ServiceLocator and SCREEN_INFORMATION were removed. IRenderData was used to accomplish this. Refer to next section for more details.
    • since most of the functions were static, that means that an IRenderData had to be added into most of them.

Changes to IRenderData

Since IRenderData is now being used to abstract out ServiceLocator and SCREEN_INFORMATION, I had to add a few functions here:

  • bool IsAreaSelected()
  • void ClearSelection()
  • void SelectNewRegion(...)
  • HRESULT SearchForText(...)

image

SearchForText() is a problem here. The overall new design is great! But Windows Terminal doesn't have a way to search for text in the buffer yet, whereas ConHost does. So I'm punting on this issue for now. It looks nasty, but just look at all the other pretty things here. :)

Miscellaneous Known Issues

Things I'll fix in this PR (just need to add commits on top of it):

  • ConHost: the UIA Rects for the caption buttons are missing. Still investigating this issue.
  • I would be VERY surprised if the tests for ConHost pass.

Validation Steps Performed

  • Deployed ConHost from master and ConHost from this branch. Did some quick validation using inspect.exe.
  • Used inspect.exe to see UIA tree in Windows Terminal.

Both projects must now implement the virtual functions.
ConHost should be MOSTLY set up.
WT needs a way to implement these (need access to Terminal info)

NEXT: Fix IScreenInfoUiaProvider's references to Selection and UiaTextRange
TODOs:
- bugfix: get rects for Min/Max/Close buttons
- Clean public/protected/private functions from UIA Providers (some are in wrong place)
- re-attach tracing
- Hook up Windows Terminal
Got WindowsTerminal UIA Tree up and running again

TODO ConHost:
- bugfix: get rects for Caption buttons
- re-attach tracing

TODO WT:
- attach ScreenInfoUiaProvider to TermControl
src/types/UiaTextRange.hpp Show resolved Hide resolved
src/types/UiaTextRange.cpp Outdated Show resolved Hide resolved
src/renderer/inc/IRenderData.hpp Show resolved Hide resolved
src/interactivity/win32/windowUiaProvider.cpp Show resolved Hide resolved
src/host/tracing.hpp Outdated Show resolved Hide resolved
src/cascadia/TerminalCore/terminalrenderdata.cpp Outdated Show resolved Hide resolved
src/types/ScreenInfoUiaProvider.cpp Outdated Show resolved Hide resolved
src/types/ScreenInfoUiaProvider.cpp Outdated Show resolved Hide resolved
@carlos-zamora carlos-zamora self-assigned this Jul 15, 2019
Copy link
Member

@zadjii-msft zadjii-msft left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have a lot of the same concerns Michael does. I haven't read everything, but I figured I'd get this feedback in sooner than later

src/host/renderData.hpp Show resolved Hide resolved
src/host/tracing.cpp Show resolved Hide resolved
src/types/UiaTextRange.hpp Show resolved Hide resolved
Copy link
Member

@miniksa miniksa left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is good enough for me to get this going. We can refine the design and layout of the functions after this is in. It's easier to build from SOMETHING than from nothing.

@carlos-zamora
Copy link
Member Author

Just gonna merge this into my dev/cazamor/accessibility branch so that we can get things moving. I'll update dev/cazamor/accessibility with any further changes you guys want (though I think everything's addressed now).

I'll mark it ready for review when it's ready for review (so, like, later today)

@carlos-zamora carlos-zamora merged commit 030ae7a into dev/cazamor/accessibility Jul 16, 2019
@carlos-zamora carlos-zamora deleted the dev/cazamor/acc/screen_info branch July 16, 2019 23:24
DHowett-MSFT pushed a commit that referenced this pull request Jul 30, 2019
Builds on the work of #1691 and #1915 

Let's start with the easy change:
- `TermControl`'s `controlRoot` was removed. `TermControl` is a `UserControl`
  now.

Ok. Now we've got a story to tell here....

### TermControlAP - the Automation Peer
Here's an in-depth guide on custom automation peers:
https://docs.microsoft.com/en-us/windows/uwp/design/accessibility/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
`ScreenInfoUiaProvider` and that was an `ITextProvider`, so let's just make the
TermControlAP an `ITextProvider` too.

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 and ITextRangeProiuder. XAML's interfaces
ITextProvider and ITextRangeProvider 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 `ITextProvider` for
TermControlAP.cpp is just wrapping what we do in `ScreenInfoUiaProvider`, and
returning an acceptable version of it.

Most of `ScreenInfoUiaProvider`'s functions return `UiaTextRange`s. 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 `UiaTextRange` and
putting it inside of `XamlUiaTextRange`.


### Additional changes to `UiaTextRange` and `ScreenInfoUiaProvider`
If you don't know what I just said, please read this background:
- #1691: how accessibility works and the general responsibility of these two
  classes
- #1915: how we pulled these Accessibility Providers into a shared area

TL;DR: `ScreenInfoUiaProvider` lets you interact with the displayed text.
`UiaTextRange` is 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()` and `ClientToScreen()`. 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()` and `GetVisibleRanges()` crashes. I need to debug through
  these. I want to include them in this PR.

Fixes #1353.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants