Accessibility: Refactor Providers #2414

carlos-zamora · 2019-08-12T20:32:55Z

Summary of the Pull Request

Refactors the accessibility providers (ScreenInfoUiaProvider and UiaTextRange) into a better separated model between ConHost and Windows Terminal.

ScreenInfoUiaProviderBase and UiaTextRangeBase are introduced. ConHost and Windows Terminal implement their own versions of ScreenInfoUiaProvider and UiaTextRange that inherit from their respective base classes.

References

PR Checklist

Closes Refactor accessibility providers into better separated model #2120
CLA signed.
~~Tests added/passed~~ old tests still valid
~~Requires documentation to be updated~~ N/A
I am a core contributor

Detailed Description of the Pull Request / Additional comments

Types now has ScreenInfoUiaProviderBase and UiaTextRangeBase to allow for shared code
UiaTextRange
- Creates are used to properly create itself
  - this is needed because we need to use some virtual functions AFTER we are created to fully initialize ourselves. A bit annoying.
- ChangeViewport, TranslatePointToScreen, and TranslatePointFromScreen are unique to CH vs WT (helper functions)
- Clone and FindText are unique to CH vs WT (public interactions)
ScreenInfoUiaProvider
- CreateUTRs are used to properly create UiaTextRanges
- Navigate, get_BoundingRectangle, and get_FragmentRoot are unique to CH vs WT

Now that we know if we're calling FindText from WT or CH, we don't need it in IRenderData. So that got moved.

🚨NOTE: There will be some conflicts with #2296. Notably, the issue mentioned above. resolving that merge conflict will be pretty easy though.

Validation Steps Performed

Used inspect on ConHost and WT. Bounding rects still appear as expected in each model

src/cascadia/TerminalControl/UiaTextRange.cpp

DHowett-MSFT · 2019-08-14T23:55:05Z

src/cascadia/TerminalControl/UiaTextRange.cpp

+        return E_OUTOFMEMORY;
+    }
+
+#if defined(_DEBUG) && defined(UiaTextRangeBase_DEBUG_MSGS)


? did you add this?

Nope. There's a bunch of these in UiaTextRangeBase. Should I move the tracing and debugging stuff to UiaTextRangeBase and have the UiaTextRange implementations call that at the end?

src/cascadia/WindowsTerminal/WindowUiaProvider.cpp

zadjii-msft

Yea I want some answers to these questions before I sign off

src/cascadia/TerminalControl/ScreenInfoUiaProvider.cpp

src/types/ScreenInfoUiaProviderBase.h

src/cascadia/TerminalControl/UiaTextRange.cpp

src/interactivity/win32/screenInfoUiaProvider.cpp

src/cascadia/TerminalControl/ScreenInfoUiaProvider.hpp

src/interactivity/win32/uiaTextRange.cpp

UiaTextRange now has a com_ptr reference to its parent ScreenInfoUiaProvider's CreateUTRs and GetSelectionRangeUTRs got renamed WindowUiaProvider TODO comment fixes + remove layering violation

zadjii-msft

Overall much happier with this. Did we ever figure out why there were all those mysterious extra AddRef / Releases? I'm a little worried they're being removed if we didn't understand fully why they were there originally. If they were there originally completely as an oversight and should never have been, the no worries.

carlos-zamora · 2019-08-19T17:14:38Z

Overall much happier with this. Did we ever figure out why there were all those mysterious extra AddRef / Releases? I'm a little worried they're being removed if we didn't understand fully why they were there originally. If they were there originally completely as an oversight and should never have been, the no worries.

Originally, we were manually keeping track of the refs. @DHowett-MSFT suggests that we should switch over to wil::com_ptrs for CodeHealth reasons and the same concerns you have. I just created this task (#2474) to continue doing this since we'll have to do it in ConHost as well (and other parts of the accessibility model).

src/cascadia/TerminalControl/UiaTextRange.cpp

src/types/UiaTextRangeBase.cpp

src/interactivity/win32/screenInfoUiaProvider.cpp

src/interactivity/win32/uiaTextRange.cpp

Fix merge conflicts regarding refacroting of IRenderData # Conflicts: # src/cascadia/TerminalControl/TermControlAutomationPeer.cpp # src/cascadia/TerminalCore/Terminal.hpp # src/cascadia/TerminalCore/terminalrenderdata.cpp # src/host/renderData.cpp # src/host/renderData.hpp # src/interactivity/win32/ut_interactivity_win32/UiaTextRangeTests.cpp # src/interactivity/win32/windowUiaProvider.cpp # src/renderer/inc/IRenderData.hpp # src/types/ScreenInfoUiaProviderBase.cpp # src/types/ScreenInfoUiaProviderBase.h # src/types/UiaTextRangeBase.cpp # src/types/UiaTextRangeBase.hpp

- RETURN_HR_IF(E_INVALIDARG, ppProvider == nullptr) checks

src/types/UiaTextRangeBase.cpp

I trust the Mikes more than myself here.

carlos-zamora self-assigned this Aug 12, 2019

Refactor Accessibility providers

1bf9a38

carlos-zamora force-pushed the dev/cazamor/acc-providers branch from 3b39fe9 to 1bf9a38 Compare August 12, 2019 20:58

carlos-zamora mentioned this pull request Aug 13, 2019

Accessibility: Add BoundingRects to UiaTextRanges #2423

Merged

5 tasks

carlos-zamora added the Area-Accessibility Issues related to accessibility label Aug 13, 2019

carlos-zamora changed the title ~~Refactor Accessibility Providers~~ Accessibility: Refactor Providers Aug 13, 2019

DHowett-MSFT previously requested changes Aug 15, 2019

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 Aug 15, 2019

zadjii-msft requested changes Aug 15, 2019

View reviewed changes

ghost removed the Needs-Author-Feedback The original author of the issue/PR needs to come back and respond to something label Aug 15, 2019

WindowsTerminal's ScreenInfoUiaProvider --> TermControlUiaProvider

9b857de

UiaTextRange now has a com_ptr reference to its parent ScreenInfoUiaProvider's CreateUTRs and GetSelectionRangeUTRs got renamed WindowUiaProvider TODO comment fixes + remove layering violation

zadjii-msft approved these changes Aug 19, 2019

View reviewed changes

miniksa reviewed Aug 19, 2019

View reviewed changes

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

miniksa reviewed Aug 19, 2019

View reviewed changes

src/cascadia/TerminalControl/UiaTextRange.cpp Show resolved Hide resolved

miniksa requested changes Aug 19, 2019

View reviewed changes

src/types/UiaTextRangeBase.cpp Outdated Show resolved Hide resolved

src/interactivity/win32/screenInfoUiaProvider.cpp Show resolved Hide resolved

src/interactivity/win32/uiaTextRange.cpp Outdated Show resolved Hide resolved

src/interactivity/win32/uiaTextRange.cpp Show resolved Hide resolved

ghost added the Needs-Author-Feedback The original author of the issue/PR needs to come back and respond to something label Aug 19, 2019

carlos-zamora added 2 commits August 19, 2019 12:51

PR Feedback

5738188

- RETURN_HR_IF(E_INVALIDARG, ppProvider == nullptr) checks

ghost removed the Needs-Author-Feedback The original author of the issue/PR needs to come back and respond to something label Aug 19, 2019

miniksa approved these changes Aug 20, 2019

View reviewed changes

src/types/UiaTextRangeBase.cpp Show resolved Hide resolved

PR change nit

f56e1dc

carlos-zamora merged commit 667c028 into master Aug 20, 2019

carlos-zamora deleted the dev/cazamor/acc-providers branch August 20, 2019 23:39

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Accessibility: Refactor Providers #2414

Accessibility: Refactor Providers #2414

carlos-zamora commented Aug 12, 2019 •

edited

Loading

DHowett-MSFT Aug 14, 2019

carlos-zamora Aug 15, 2019

zadjii-msft left a comment

zadjii-msft left a comment

carlos-zamora commented Aug 19, 2019

Accessibility: Refactor Providers #2414

Accessibility: Refactor Providers #2414

Conversation

carlos-zamora commented Aug 12, 2019 • edited Loading

Summary of the Pull Request

References

PR Checklist

Detailed Description of the Pull Request / Additional comments

Validation Steps Performed

DHowett-MSFT Aug 14, 2019

Choose a reason for hiding this comment

carlos-zamora Aug 15, 2019

Choose a reason for hiding this comment

zadjii-msft left a comment

Choose a reason for hiding this comment

zadjii-msft left a comment

Choose a reason for hiding this comment

carlos-zamora commented Aug 19, 2019

carlos-zamora commented Aug 12, 2019 •

edited

Loading