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

Accessibility: Refactor Providers #2414

Merged
merged 5 commits into from
Aug 20, 2019
Merged

Accessibility: Refactor Providers #2414

merged 5 commits into from
Aug 20, 2019

Conversation

carlos-zamora
Copy link
Member

@carlos-zamora carlos-zamora commented Aug 12, 2019 •
edited
Loading

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

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

@carlos-zamora carlos-zamora self-assigned this Aug 12, 2019
@carlos-zamora carlos-zamora mentioned this pull request Aug 13, 2019
Merged
5 tasks
@carlos-zamora carlos-zamora added the Area-Accessibility Issues related to accessibility label Aug 13, 2019
@carlos-zamora carlos-zamora changed the title Refactor Accessibility Providers Accessibility: Refactor Providers Aug 13, 2019
DHowett-MSFT
DHowett-MSFT previously requested changes Aug 15, 2019
View reviewed changes
src/cascadia/TerminalControl/UiaTextRange.cpp Outdated Show resolved Hide resolved
src/cascadia/TerminalControl/UiaTextRange.cpp Show resolved Hide resolved
src/cascadia/TerminalControl/UiaTextRange.cpp Outdated Show resolved Hide resolved
src/cascadia/TerminalControl/UiaTextRange.cpp Outdated Show resolved Hide resolved
src/cascadia/TerminalControl/UiaTextRange.cpp Outdated Show resolved Hide resolved
src/cascadia/TerminalControl/UiaTextRange.cpp
return E_OUTOFMEMORY;
}

#if defined(_DEBUG) && defined(UiaTextRangeBase_DEBUG_MSGS)
Copy link
Contributor

Choose a reason for hiding this comment

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

? did you add this?

Copy link
Member Author

Choose a reason for hiding this comment

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

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 Outdated Show resolved Hide resolved
@ghost 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
zadjii-msft requested changes Aug 15, 2019
View reviewed changes
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.

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

src/cascadia/TerminalControl/ScreenInfoUiaProvider.cpp Outdated Show resolved Hide resolved
src/types/ScreenInfoUiaProviderBase.h Outdated Show resolved Hide resolved
src/types/ScreenInfoUiaProviderBase.h Outdated Show resolved Hide resolved
src/cascadia/TerminalControl/UiaTextRange.cpp Outdated Show resolved Hide resolved
src/interactivity/win32/screenInfoUiaProvider.cpp Outdated Show resolved Hide resolved
src/cascadia/TerminalControl/ScreenInfoUiaProvider.hpp Outdated Show resolved Hide resolved
src/interactivity/win32/uiaTextRange.cpp Show resolved Hide resolved
@ghost 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
@carlos-zamora
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
zadjii-msft approved these changes Aug 19, 2019
View reviewed changes
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.

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
Copy link
Member Author

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).

miniksa
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 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
Merge branch 'master' into dev/cazamor/acc-providers
1f771a2 
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
@carlos-zamora
PR Feedback
5738188 
- RETURN_HR_IF(E_INVALIDARG, ppProvider == nullptr) checks
@ghost 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
miniksa approved these changes Aug 20, 2019
View reviewed changes
src/types/UiaTextRangeBase.cpp Show resolved Hide resolved
@DHowett-MSFT DHowett-MSFT dismissed their stale review August 20, 2019 23:31

I trust the Mikes more than myself here.

@carlos-zamora carlos-zamora merged commit 667c028 into master Aug 20, 2019
@carlos-zamora carlos-zamora deleted the dev/cazamor/acc-providers branch August 20, 2019 23:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@miniksa miniksa miniksa approved these changes

@zadjii-msft zadjii-msft zadjii-msft approved these changes

@DHowett-MSFT DHowett-MSFT DHowett-MSFT left review comments

Assignees

@carlos-zamora carlos-zamora

Labels
Area-Accessibility Issues related to accessibility
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Refactor accessibility providers into better separated model
4 participants
@carlos-zamora @DHowett-MSFT @miniksa @zadjii-msft