Support UIA ::SetFocus and basic property change notifications #11674
Conversation
| #define IN | ||
| #define OUT |
There was a problem hiding this comment.
What's this for? SAFEARRAY?
Most of the code is using std::vector, so where's the SAFEARRAY dependency being forced?
There was a problem hiding this comment.
yea, a lot of the UIA APIs use SAFEARRAY for certain parameters.
| #pragma warning(disable : 4229) | ||
| #define IN | ||
| #define OUT | ||
| #include <atlsafe.h> |
There was a problem hiding this comment.
this PR adds SAFEARRAY support to WIL which we should be using instead:
microsoft/wil#145
could you perhaps work with @chrisglein 's team to see that PR through and use it here?
There was a problem hiding this comment.
+@carlos-zamora who filed the original wil issue. Can you help? :)
There was a problem hiding this comment.
Sure! Here's the way we did it in the Terminal repo:
https://github.com/microsoft/terminal/blob/c9e993a38ec04833e15edac230300948a0fab83b/src/cascadia/inc/cppwinrt_utils.h#L222-L250
And here's a quick example of us using it:
https://github.com/microsoft/terminal/blob/c9e993a38ec04833e15edac230300948a0fab83b/src/cascadia/TerminalControl/InteractivityAutomationPeer.cpp#L206
Hope this helps!
There was a problem hiding this comment.
Nice, I wasn't aware of the WIL API. @asklar I've been pulled off this work for now. Would you be OK with me just using the SAFEARRAY directly and opening an issue to move this to the WIL stuff?
There was a problem hiding this comment.
@jonthysell we can interface manually (which I think is what @FalseLobster is proposing) but it's going to be a lot more code than necessary and hard to maintain/reason over. WIL is header only and designed to simplify calling win32 apis so your code becomes a lot more concise. Ideally we can move to that sooner than later.
There was a problem hiding this comment.
@FalseLobster I'm okay with taking things as you've written them and opening an issue to replace ATL with WIL and assign it to me. There's probably other places the (Fabric) codebase could benefit from access to WIL helpers to deal with all this old Win32 stuff.
@asklar Yes, WIL is header only, but it's also "yet another re-wrapping of standard C++" to add to this project. Like do we really need every implementation of smart pointers that have ever been made? We have stl, boost, Mso, cppwinrt, now wil.
There was a problem hiding this comment.
the alternative being used here is to use ATL. We should not be using ATL.
WIL provides convenient wrappers for this specific use case. If STL, Boost, MSO or CppWinRT had RAII wrappers for these types, great. I don't believe they do though.
There was a problem hiding this comment.
@asklar , @jonthysell , could we avoid dependency on the new library?
SAFEARRAY are not so difficult to use directly.
I would expect that C++/WinRT has all the required wrappers for them.
C++/WinRT is also header-only library, and we know the overall impact on the developer experience.
I would stay away from the header-only libraries since they put a huge pressure on the compiler: such headers must be often compiled again and again for each .cpp that slows down compilation no matter how powerful your computer is.
There was a problem hiding this comment.
C++/WinRT is for working with WinRT, but these are not WinRT apis, so C++/WinRT doesn't help in this case. Feel free to use SAFEARRAY directly.
microsoft-github-policy-service
bot
added
the
Needs: Author Feedback
| return rootProvider->WasPropertyAdvised(propId); | ||
| } | ||
|
|
||
| void UpdateUiaProperty(winrt::IInspectable provider, PROPERTYID propId, bool oldValue, bool newValue) noexcept { |
There was a problem hiding this comment.
Was this the function you were talking about maybe needing to be templatized?
There was a problem hiding this comment.
Yea, I was considering templatizing this but I was fumbling a little with the SFINAE, and maybe we can just add it later when there are more overloads needed. Having two functions isn't too bad for now.
microsoft-github-policy-service
bot
removed
the
Needs: Author Feedback
| #pragma warning(disable : 4229) | ||
| #define IN | ||
| #define OUT | ||
| #include <atlsafe.h> |
There was a problem hiding this comment.
@FalseLobster I'm okay with taking things as you've written them and opening an issue to replace ATL with WIL and assign it to me. There's probably other places the (Fabric) codebase could benefit from access to WIL helpers to deal with all this old Win32 stuff.
@asklar Yes, WIL is header only, but it's also "yet another re-wrapping of standard C++" to add to this project. Like do we really need every implementation of smart pointers that have ever been made? We have stl, boost, Mso, cppwinrt, now wil.
|
@asklar can I get a sign off? |
Description
Improves the navigation and focus management experience for screen readers and other assistive technologies (ATs) and creates a pattern for property invalidation for UIA-related RN props.
Type of Change
Why
Improve the ability of ATs to navigate RNW Fabric and manage focus.
What
Testing
I performed some testing using Narrator, NVDA, and Accessibility Insights to confirm that AT navigation is improved and property and event changes happen as expected.
Microsoft Reviewers: Open in CodeFlow