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

UIA Operation Abstraction lib: implement support for CallExtension / IsExtensionSupported #84

Merged
merged 9 commits into from
Mar 22, 2022

Conversation

michaelDCurran
Copy link
Contributor

UI Automation recently introduced the concept of custom patterns. This is the ability for a provider to expose custom methods, identified by a GUID, that a client an execute via UIA remote Operations. For example, MS Word might expose a custom pattern for moving by sentence - something that UIA by itself does not support.

This PR implements the ability for a client to execute these custom methods (extensions) by using the UIA Operation Abstraction library. Support for custom patterns has already been introduced to the Microsoft::UI::UIAutomation winrt library.

Specific changes:

  • Added a new UiaGuid class, which represents either a local or remote GUID.
    • It can be constructed locally from a winrt::guid.
    • It can be assigned to, and compaired with == / !=.
    • It exposes LookupAnnotationType and LookupPropertyId methods, which work both remotely and locally.
  • Implement UiaElement::IsExtensionSupported: this method takes a UiaGuid as an argument, and returns a UiaBool denoting if the extension identified by the given GUID is supported by the remote provider.
    • If remote, this method calls AutomationRemoteElement::IsExtensionSupported.
    • This method has no local equivilant and therefore throws NotImplemented if executed locally.
  • Implemented UiaElement::CallExtension: this method takes a UiaGuid, plus a variable number of further arguments of any Uia* type, and executes the extension identified by the given GUID, passing the given arguments as its in/out arguments.
    • If remote, this method calls AutomationRemoteElement::CallExtension.
    • This method has no local equivilant and therefore throws NotImplemented if executed locally.

No tests have been written yet. These will be hard to write unless they can be run against a specific app that exposes 1 or more custom patterns.
Manually I have tested UiaGuid::LookupAnnotationType / UiaGuid::LookupPropertyId. But I am so far unable to test UiaElement::IsExtensionSupported / UIAElement::CallExtension until I have an app that actually implements 1 or more custom patterns...

…omationRemoteObject so that the initializer list is implisitly cast to an array_view of AutomationRemoteObject values as expected.
@michaelDCurran michaelDCurran marked this pull request as ready for review January 14, 2022 01:15
@michaelDCurran
Copy link
Contributor Author

I have been able to manually confirm that these changes work, at least for my use cases, by testing them against Microsoft Word's custom patterns implementation, from within the NVDA screen reader. However, I really am not sure how this can be tested specifically within this project as access to Microsoft Word, or something that implemented custom patterns, would be required on the build system.

@beweedon
Copy link
Contributor

Sorry for us ignoring this PR for so long! I'll give it a look next week. @mavangur @zhayuli @pengyuanjin for FYI.

Honestly, we haven't even had the time to get our tests running in CI yet. There's a general issue with launching apps and we haven't really gotten to look at why. I'll give some thought as to what we can do with regard to automated testing here.

class UiaGuid : public UiaTypeBase<winrt::guid, winrt::Microsoft::UI::UIAutomation::AutomationRemoteGuid>
{
public:
//static constexpr VARTYPE c_comVariantType = VT_CLSID;
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we need these comment lines?

@michaelDCurran
Copy link
Contributor Author

michaelDCurran commented Jan 15, 2022 via email


UiaPropertyId UiaGuid::LookupPropertyId()
{
if (ShouldUseRemoteApi())
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: This should be indented in by 4 more spaces.

return;
}

// No local equivilant
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: Should be spelled "equivalent"

@beweedon
Copy link
Contributor

@michaelDCurran I'd say as long as we don't need GUIDs in UiaVariant for this PR, we can remove those comment lines. It might make sense to store GUIDs in VT_SAFEARRAY in the variant, though, when we want to implement that.

Also, we could add support for extension tests by creating a provider that supports custom extensions, and loading it with the ModernApp test utility (or creating a Win32 executable). If you feel you're up to that in this PR feel free (and of course ask us if you need help!), but if not I've created #86 to track adding those tests in the future. I can try to get around to confirming your manual testing next week.

@beweedon
Copy link
Contributor

I'm really sorry for all the slow responses here!

My remaining comment is can we add tests for UiaGuid? We don't currently have a huge amount of tests for each individual type, but I figure we should aim for good test coverage on new types. So basically calling all its methods, operating locally and remotely, etc.

Otherwise, this looks great!!!

@beweedon
Copy link
Contributor

beweedon commented Mar 3, 2022

@michaelDCurran just in case you missed these notifications.

@michaelDCurran
Copy link
Contributor Author

michaelDCurran commented Mar 3, 2022 via email

@beweedon
Copy link
Contributor

beweedon commented Mar 3, 2022

No need to apologize I just wanted to make sure you didn't think you were waiting on me :)

@michaelDCurran
Copy link
Contributor Author

@beweedon I'm afraid that I can't think of any ways I can write remote tests for UiaGuid.

  • There is no method (other than UIA custom extensions) that take a remote guid
  • There are no UIA methods that give back a guid
  • A guid to / from a custom property or annotation only makes sense if there are custom annotations or properties already registered in the remote provider.

In other words, to create any meaningful tests for UiaGuid, (just like for custom extensions) we'd need to write a custom provider or test with an MS Office product such as Microsoft Word.
I don't believe that I am qualified at the moment to pull that one off at this stage. Plus, some of the other additions we have talked about are probably more high priority for me. Thankfully those ones are actually quite easy to write tests for ;)
I'll leave it up to you then as to if you wish to merge this with no custom provider / tests. I'm already using this code real-world in NVDA 2022.1 via my fork michaeldcurran/microsoft-ui-uiautomation with the code from this pr merged.

@beweedon
Copy link
Contributor

@michaelDCurran I was thinking mostly about testing the simple stuff just to make sure it works.

  • Test that UiaGuid can go from local to remote back to local again.
  • Test that UiaGuid assignment and equality work.
  • Test that LookupPropertyId works if you set the UiaGuid to one of the known property GUIDs in UIAutomationCoreApi.h (e.g. Name_Property_GUID should correspond to UIA_NamePropertyId).

I'm glad you're not blocked, though :). If you don't want to add the tests, one of us can look into that at some point and get this merged in.

…d annotation type IDs.

This also required:
* UIAGuid: add cast operator for winrt::guid to allow getting back to local after remote execution.
* UiaOperationAbstraction library now also links against UiAutomationCore.lib as UiaLookupId is required.
@michaelDCurran
Copy link
Contributor Author

@beweedon I've now added tests for UiaGuid, including equality checks, and looking up property IDs and annotation type IDs.

@beweedon
Copy link
Contributor

@michaelDCurran This looks great to me! I'm going to see if other reviews filter in before merging.

@beweedon
Copy link
Contributor

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@beweedon beweedon merged commit a219679 into microsoft:main Mar 22, 2022
feerrenrut added a commit to nvaccess/nvda-microsoft-ui-uiautomation that referenced this pull request Oct 10, 2022
The new commit being used (d8c87fa) is effectively the same as the commit used in
NVDA currently (224b22f).

microsoft main branch HEAD commit:
d8c87fa Merge of microsoft/Microsoft-UI-UIAutomation#84
From repo:branch michaelDCurran:callExtension

The callExtension branch includes the commit that NVDA was using (224b22f), but
it was not the HEAD of the branch when the branch was merged upstream.

There was one commit on that branch after this:
See commit cdabf41ed5c277d7761222e947748ee860ec97d2 in the callExtension
branch:
https://github.com/michaelDCurran/Microsoft-UI-UIAutomation/commits/callExtension
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.

4 participants