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

Implement UiaConnectionBoundObject in abstraction API #95

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

michaelDCurran
Copy link
Contributor

@michaelDCurran michaelDCurran commented Jun 3, 2022

Partially implements #93

Background

In pr #84 I added initial support for checking for and calling custom extensions to UiaElement through IsExtensionSupported and CallExtension methods.
However, this was somewhat incomplete, as UI Automation remote operations also allow for checking for and calling extensions on text ranges, and generic connection-bound objects (which currently have no representation in the abstraction library at all).

The general pattern for custom extensions is usually:

  • Given a UIA element or text range, call its CallExtension method with a custom pattern guid to fetch a custom pattern object. This custom pattern object is implemented on the provider side as a generic connection-bound object.
  • On the connection-bound object representing the custom pattern, call its CallExtension method with a custom method guid to execute the required custom method on the pattern.

The work around would be to use a UiaElement in place of the connection-bound object, as that is the only class that implemented CallExtension. This is both semantically incorrect and dangerous (as calling any of the other UiaElement methods would fail or may crash the client.

PR #92 added support for connection-bound objects to the lower-level winrt library through the addition of the AutomationRemoteExtensionTarget class.

However, this also was slightly incomplete as AutomationRemoteExtensionTarget was missing a Set method, and AutomationRemoteAnyObject was missing IsExtensionTarget and AsExtensionTarget methods.

Description of changes

This new PR enhances the abstraction library, adding a new UiaConnectionBoundObject class, which exposes IsExtensionSupported and CallExtension, and should be used where a connection-bound object is required, I.e. represents a custom pattern object.
This PR also exposes IsExtensionSupported and CallExtension on UiaTextRange, completing all the places where custom extensions are supported.

The specific changes to the abstraction library are as follows:

  • Add a UiaTypeBaseWithExtensionSupport template class which subclasses UiaTypeBase and adds IsExtensionSupported and CallExtension methods, moved from UiaElement.
  • UiaElement and UiaTextRange now inherit from UiaTypeBaseWithExtensionSupport rather than UiaTypeBase. This makes IsExtensionSupported and CallExtension methods again available on UiaElement, but also now UiaTextRange.
  • Add a new UiaConnectionBoundObject class, which inherits from UiaTypeBaseWithExtensionSupport. It contains no other real features of its own apart from being constructable from / convertable to AutomationRemoteExtensionTarget remotely and IUnknown locally. This can be used as a generic connection-bound object, such as sometimes returned from CallExtension, which represents a custom extension pattern.

The above work also required addressing the limitations of the AutomationRemoteExtensionTarget implementation in the winrt library.

The specific changes to the winrt library are as follows:

  • Add a 'Set' method to AutomationRemoteExtensionTarget.
  • Add 'IsExtensionTarget' and 'AsExtensionTarget' methods to AutomationRemoteAnyObject to enable check / casting to AutomationRemoteExtensionTarget.
  • Add a deprecation comment to AutomationRemoteExtensionTarget::IsExtensionTarget. I think this was a misunderstanding in PR Implement wrappers for CustomExtensions #92 of where this needed to be.

Testing

I have not yet added tests for these changes. I should be able to add some general tests for UiaConnectionBoundObject in regards to construction / conversions. But as mentioned in previous PRs, there is currently no simple way to test IsExtesnionSupported / CallExtension as this requires a provider that has implemented these.

Questions / thoughts

  1. I wonder whether having UiaConnectionBoundObject's local type as IUnknown is suitable. It seems like the only choice, as the older COM interfaces don't have a representation for connection-bound objects E.g. a IUIAutomationconnectionBoundObject. Thus IUnknown really is the only valid representation. But are there any disadvantages to this. Are there features or conversions of UiaConnectionBoundObject that we should deliberately remove or disable as IUnknown is so generic? I can't think of any so far. But just bringing up the point here in case.
  2. Am I right that IsExtensionTarget on AutomationRemoteExtensionTarget was an error? Can someone please verify that the IsExtensionTarget op code in the remote ops VM really does ask if an anyObject is an extensionTarget (similar to all the other Is* op codes)?
  3. in the winrt library, standins.cpp, AutomationRemoteElement::GetUpdatedCacheElement: it was necessary for me to static_cast the *this parameter on result.Set to AutomationRemoteElement, otherwise I would get an ambiguous overload error when compiling. This was caused due to me adding a Set method to AutomationRemoteExtensionTarget, which is inherited by AutomationRemoteElement. I don't entirely understand why the static_cast is necessary or why the compiler is clearly converting to a base class when searching for the correct overload of Set. Did I address this issue in the right way?

* Add a 'Set' method to AutomationRemoteExtensionTarget. All AutomationRemoteObject subclasses require this but seemed to have been left out of the original implementation.
* Add 'IsExtensionTarget' and 'AsExtensionTarget' methods to AutomationRemoteAnyObject to enable check / casting to AutomationRemoteExtensionTarget.
* Add a deprecation comment to AutomationRemoteExtensionTarget::IsExtensionTarget. I think this was a misunderstanding of where this needed to be.
* Add a UiaTypeBaseWithExtensionSupport template class which subclasses UiaTypeBase and adds IsExtensionSupported and CallExtension methods, moved from UiaElement.
* UiaElement and UiaTextRange now inherit from UiaTypeBaseWithExtensionSupport rather than UiaTypeBase. This makes IsExtensionSupported and CallExtension methods again available on UiaElement, but also now UiaTextRange.
* Add a new UiaConnectionBoundObject class, which inherits from UiaTypeBaseWithExtensionSupport. It contains no other real features of its own a part from being constructable from / convertable to AutomationRemoteExtensionTarget remotely and IUnknown locally. This can be used as a generic connection-bound object, such as sometimes returned from CallExtension, which represents a custom extension pattern. Previously the caller had to treat the object as a UiaElement, even though it was not really.
@michaelDCurran michaelDCurran marked this pull request as ready for review June 3, 2022 01:18
@michaelDCurran michaelDCurran changed the title Implement UiaConnectionBoundObject Implement UiaConnectionBoundObject in abstraction API Jun 3, 2022
@rahulkhanna1605
Copy link

I ran the functional tests to ensure nothing breaks but please mention how these changes were tested.

@abinashojha83
Copy link

Hi @michaelDCurran , I also have a similar question as @rahulkhanna1605.
Can you please mention details about the test performed?

@michaelDCurran
Copy link
Contributor Author

michaelDCurran commented Jul 19, 2022 via email

@michaelDCurran
Copy link
Contributor Author

The only testing I have done is:

  • All existing functional tests continue to pass
  • I have made use of UiaConnectionBoundObject in the MS Word custom patterns code in the NVDA screen Reader project. See commit nvaccess/nvda@506ba8f6ea9 . Running this commit of NVDA, I can open Microsoft Word, type several lines of text. Enable reporting of line numbers in NvDA in NVDA's document settings. Arrow up and down the lines of text hearing the line numbers announced. These line numbers are fetched via a MS Word UIA custom pattern, using a UiaConnectionBoundObject.
    To integrate tests for this directly into this project, either the test machine needs to contain Microsoft Word, or a custom app needs to be written that exposes a UIA provider that supports custom patterns.
    I don't have the time or knowhow to write that kind of provider I'm sorry. So I'll leave this branch/pr for anyone else to pick up if they feel they no how to move it further.

@@ -144,9 +144,10 @@ namespace Microsoft.UI.UIAutomation

unsealed runtimeclass AutomationRemoteExtensionTarget : AutomationRemoteObject
{
AutomationRemoteBool IsExtensionTarget();
/* Deprecated */ AutomationRemoteBool IsExtensionTarget();
Copy link

@rahulkhanna1605 rahulkhanna1605 Oct 20, 2022

Choose a reason for hiding this comment

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

Does this need to be marked as deprecated? @mavangur could you also confirm if this would be required.

Copy link
Contributor

Choose a reason for hiding this comment

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

I don't know when this got deprecated Rahul, At the time I introduced this API, we are in a view that it would be useful. I don't know who marked it deprecated.

Copy link
Contributor

Choose a reason for hiding this comment

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

(Sorry for parachuting in, just hoping to provide some historic context :))

Is<Type> and As<Type> methods should be on AutomationRemoteAnyObject, since that's what facilitates "casting" between the generic "any" type and a "concrete" type. Basically it helps "inspect" the actual type and convert between them safely. (Sort of like a QI...)

This method was originally added to AutomationRemoteExtensionTarget where it doesn't do much good -- it's already representing a specific type. This PR is moving it to the right place and deprecating this one, presumably so that if anyone took a dependency on this so far for some reason they're not immediately broken.

Consider just introducing a (technically) breaking change and removing this, though.

Mick also did have a question in the PR:

Can someone please verify that the IsExtensionTarget op code in the remote ops VM really does ask if an anyObject is an extensionTarget (similar to all the other Is* op codes)?

I don't know for sure - I no longer have access to the Windows code base :( - but I would assume that it'd follow the same pattern as the others; I think someone would've had to go out of their way to implement it any differently given how things were originally structured.

@dpy013
Copy link

dpy013 commented Jun 10, 2023

cc @michaelDCurran and @beweedon
Is there any progress on this pull request?

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.

6 participants