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

Implement UIA Invoke Method #11874

Merged
merged 23 commits into from
Aug 9, 2023
Merged

Implement UIA Invoke Method #11874

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
23 commits
Select commit Hold shift + click to select a range
2f77b71
Implement Invoke
chiaramooney Jul 6, 2023
e0779e4
Change files
chiaramooney Jul 11, 2023
ddad53f
Format
chiaramooney Jul 11, 2023
da2dbbd
Adjust Call
chiaramooney Jul 11, 2023
50a5e90
Adjust Call
chiaramooney Jul 11, 2023
e39a92d
Address Feedback
chiaramooney Jul 13, 2023
352b505
Format
chiaramooney Jul 13, 2023
2c1bcc9
Fix
chiaramooney Jul 14, 2023
10b6a31
Merge
chiaramooney Jul 20, 2023
c80f7bc
Address Feedback
chiaramooney Jul 20, 2023
223257c
Merge
chiaramooney Jul 20, 2023
50d480a
Fix
chiaramooney Jul 20, 2023
c2d7e04
Remove
chiaramooney Jul 20, 2023
2680a00
Fix
chiaramooney Jul 24, 2023
57fe0ba
Format
chiaramooney Jul 24, 2023
b41a392
Fix UIA Tree Issue
chiaramooney Aug 1, 2023
f44b187
Address Feedback
chiaramooney Aug 3, 2023
82749ed
Implement Invoke
chiaramooney Aug 3, 2023
b7fd10f
Change files
chiaramooney Aug 3, 2023
83e4197
Merge branch 'main' of https://github.com/microsoft/react-native-wind…
chiaramooney Aug 3, 2023
1497fec
Remove Unneeded
chiaramooney Aug 3, 2023
beaab47
Format
chiaramooney Aug 3, 2023
4d84bd8
Merge branch 'main' into cm-fabric-invoke
chiaramooney Aug 7, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 7 additions & 0 deletions change/react-native-windows-8ece1df1-5ba8-4b14-8258-bdb6ccc9948b.json
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
{
"type": "prerelease",
"comment": "Implement Invoke",
"packageName": "react-native-windows",
"email": "[email protected]",
"dependentChangeType": "patch"
}
30 changes: 30 additions & 0 deletions vnext/Microsoft.ReactNative/Fabric/Composition/CompositionDynamicAutomationProvider.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -272,4 +272,34 @@ HRESULT __stdcall CompositionDynamicAutomationProvider::get_HostRawElementProvid
return S_OK;
}

HRESULT __stdcall CompositionDynamicAutomationProvider::Invoke() {
FalseLobster marked this conversation as resolved.
Show resolved Hide resolved
auto strongView = m_view.view();

if (!strongView)
return UIA_E_ELEMENTNOTAVAILABLE;

auto baseView = std::static_pointer_cast<::Microsoft::ReactNative::CompositionBaseComponentView>(strongView);
if (baseView == nullptr)
return UIA_E_ELEMENTNOTAVAILABLE;

// Currently calls both onAccessibilityTap and onClick.
// To match Paper behavior, onAccessibilityTap only called if onClick is not defined.
Copy link
Contributor

Choose a reason for hiding this comment

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

I do wonder if this is correct? What if we have some special code that needs to behave differently for pointer and AT users? (e.g. Telemetry recording invocation method).

I feel like if both are implemented, maybe we should only call onAccessibilityTap and rely on users to handle the normal onClick behavior in the JS callback. Or maybe both? Maybe this was already discussed with the Paper impl, but I don't know that I like the idea that we deprive people of an AT-specific code path.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

For sure! Just to confirm I understand what you're saying, you're wondering that maybe the behavior we had on Paper was wrong. And that instead we should either...
(1) always call onAccessibilityTap
(2) always call both onAccessibilityTap and onClick

That makes sense to me! It did seem odd that the onAccessibilityTap functionality would be ignored when onClick functionality is set. Option 2 seems like the right option to me, do you have an instinct?

Copy link
Contributor

Choose a reason for hiding this comment

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

I just checked and this:

maybe we should only call onAccessibilityTap and rely on users to handle the normal onClick behavior in the JS callback

is what we do in Office, though I do want to get this right. So while (2) sounds reasonable, I wouldn't mind a second opinion here from @jonthysell or @acoates-ms

Copy link
Contributor

Choose a reason for hiding this comment

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

I'd go with (1). I dont see any indication in code that core calls onClick when invoking onAccessibilityTap. @Saadnajmi might be able to verify the behavior on iOS. (I think onClick might be fabric only in iOS)

Copy link
Contributor

@jonthysell jonthysell Aug 2, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

It appears that onAccessibilityTap is its own, independent event in RN code, and technically an iOS-only event (though not documented that way, see facebook/react-native#34136).

See also https://reactnative.dev/docs/view#onaccessibilitytap

I think our Paper impl is too tied up with how we interacted with XAML and XAML's behaviors. onClick is a windows-only event, and I don't really like that we assume to send and onClick when hit via UIA. Why not an onPress? Nowhere do I see code that defaults to calling RN's own built-in onPress during an onAccessibilityTap.

I'd rather leave that up to app devs in JS rather than assume a default behavior "by default". Every example online where I can find people who care about accessibility, they define both onPress and onAccessibilityTap (even if it's usually to the same function). If any particular dev wants it to pass through they can easily define their own JS components to do so.

That said, it appears in RN for iOS, there is a kind of event chaining where the 'activate' accessibilityAction presumes to call onAccessibilityTap if it's available:

https://github.com/facebook/react-native/blob/8d8455c3c34e9554908781485718d8eee252267f/packages/react-native/React/Fabric/Mounting/ComponentViews/View/RCTViewComponentView.mm#L802-L810

- (BOOL)accessibilityActivate
{
  if (_eventEmitter && _props->onAccessibilityTap) {
    _eventEmitter->onAccessibilityTap();
    return YES;
  } else {
    return NO;
  }
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Couple of additional pieces of info:

  • Upstream has recently added a topClick/onClick event, so it won't be Windows only anymore. See Use Upstream TopClick Type Instead of Windows-Specific Type #11969. More of a fyi than something actionable on this PR.
  • The value of props->onClick matches whether onPress has been set. And the call to dispatchEvent("topClick") runs the onPress function. I haven't dug into the code to figure out why this is the case, but that's the behavior we get.
  • From testing, it does NOT appear that if onAccessibilityTap is not set, then the onPress function is run instead. The program simply does a noop. So if the Invoke method expects the behavior to match when you click without AT, then just calling onAccessibilityTap might not be sufficient.
  • Based on the RN docs, I believe we'll want to implement the accessibilityAction "activate" to match whatever behavior we land on for Invoke. They are both described in the same way. What's interesting is that with this description "Typically this should perform the same action as when the user touches or clicks the component when not using an assistive technology. This is generated when a screen reader user double taps the component." I would expect _props->onClick to be called if onAccessibilityTap isn't implemented.

Copy link
Contributor

Choose a reason for hiding this comment

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

"Typically this should perform the same action as when the user touches or clicks the component when not using an assistive technology. This is generated when a screen reader user double taps the component."

I would expect _props->onClick to be called if onAccessibilityTap isn't implemented.

I think that it is a note to the developer that they should typically set onAccessibilityTap to the same value of onClick. Not that the platform would do it for you. If the platform does it, then it is no longer typically, but always, which could be problematic for some use cases.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I see that makes sense. Seems consensus is to only calling onAccessibilityTap. I'll make those edits.

// Events dispatched for any control.
// To match Paper, Event should only dispatch for pressable controls without state.
baseView.get()->GetEventEmitter().get()->onAccessibilityTap();
baseView.get()->GetEventEmitter().get()->onClick();
FalseLobster marked this conversation as resolved.
Show resolved Hide resolved

auto rootCV = strongView->rootComponentView();
chiaramooney marked this conversation as resolved.
Show resolved Hide resolved
if (rootCV == nullptr)
return UIA_E_ELEMENTNOTAVAILABLE;

auto uiaProvider = rootCV->EnsureUiaProvider();
auto spProviderSimple = uiaProvider.try_as<IRawElementProviderSimple>();
if (spProviderSimple != nullptr) {
UiaRaiseAutomationEvent(spProviderSimple.get(), UIA_Invoke_InvokedEventId);
}

return S_OK;
}

} // namespace winrt::Microsoft::ReactNative::implementation
7 changes: 6 additions & 1 deletion vnext/Microsoft.ReactNative/Fabric/Composition/CompositionDynamicAutomationProvider.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,14 +4,16 @@
#include <Fabric/ReactTaggedView.h>
#include <UIAutomation.h>
#include <inspectable.h>
#include <uiautomationcore.h>

namespace winrt::Microsoft::ReactNative::implementation {

class CompositionDynamicAutomationProvider : public winrt::implements<
CompositionDynamicAutomationProvider,
IInspectable,
IRawElementProviderFragment,
IRawElementProviderSimple> {
IRawElementProviderSimple,
IInvokeProvider> {
public:
CompositionDynamicAutomationProvider(
const std::shared_ptr<::Microsoft::ReactNative::CompositionBaseComponentView> &componentView) noexcept;
Expand All @@ -31,6 +33,9 @@ class CompositionDynamicAutomationProvider : public winrt::implements<
virtual HRESULT __stdcall get_HostRawElementProvider(IRawElementProviderSimple **pRetVal) override;
// virtual HRESULT __stdcall ShowContextMenu() noexcept override;

// inherited via IInvokeProvider
virtual HRESULT __stdcall Invoke() override;

private:
::Microsoft::ReactNative::ReactTaggedView m_view;
};
Expand Down
4 changes: 4 additions & 0 deletions vnext/Microsoft.ReactNative/Fabric/Composition/CompositionRootAutomationProvider.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -65,6 +65,10 @@ HRESULT __stdcall CompositionRootAutomationProvider::get_HostRawElementProvider(
return S_OK;
}

HRESULT __stdcall CompositionRootAutomationProvider::Invoke() {
return S_OK;
}

HRESULT __stdcall CompositionRootAutomationProvider::get_BoundingRectangle(UiaRect *pRetVal) {
if (pRetVal == nullptr)
return E_POINTER;
Expand Down
5 changes: 5 additions & 0 deletions vnext/Microsoft.ReactNative/Fabric/Composition/CompositionRootAutomationProvider.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@
#include <Fabric/ReactTaggedView.h>
#include <UIAutomation.h>
#include <inspectable.h>
#include <uiautomationcore.h>

namespace winrt::Microsoft::ReactNative::implementation {
struct CompositionRootView;
Expand All @@ -14,6 +15,7 @@ class CompositionRootAutomationProvider : public winrt::implements<
IRawElementProviderFragmentRoot,
IRawElementProviderFragment,
IRawElementProviderSimple,
IInvokeProvider,
chiaramooney marked this conversation as resolved.
Show resolved Hide resolved
IRawElementProviderAdviseEvents> {
public:
// inherited via IRawElementProviderFragmentRoot
Expand All @@ -35,6 +37,9 @@ class CompositionRootAutomationProvider : public winrt::implements<
virtual HRESULT __stdcall GetPropertyValue(PROPERTYID propertyId, VARIANT *pRetVal) override;
virtual HRESULT __stdcall get_HostRawElementProvider(IRawElementProviderSimple **pRetVal) override;

// inherited via IInvokeProvider
virtual HRESULT __stdcall Invoke() override;

// IRawElementProviderAdviseEvents
virtual HRESULT __stdcall AdviseEventAdded(EVENTID idEvent, SAFEARRAY *psaProperties) override;
virtual HRESULT __stdcall AdviseEventRemoved(EVENTID idEvent, SAFEARRAY *psaProperties) override;
Expand Down
5 changes: 5 additions & 0 deletions ...actCommon/TEMP_UntilReactCommonUpdate/react/renderer/components/view/ViewEventEmitter.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -156,4 +156,9 @@ void ViewEventEmitter::onKeyDown(KeyboardEvent const &event) const {
});
}

// [Windows]
void ViewEventEmitter::onClick() const {
Copy link
Contributor

@jonthysell jonthysell Aug 2, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Do we even need a native onClick when Pressability is a thing now since 0.63? https://reactnative.dev/docs/pressable

Maybe we should just define onClick in the View js and have it pass through to onPress? Is that possible?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Copying note from above: The value of props->onClick matches whether onPress has been set. And the call to dispatchEvent("topClick") runs the onPress function. I haven't dug into the code to figure out why this is the case, but that's the behavior we get.

So we don't need to pass any additional props through on the JS side to get onClick data. This code is just to allow us to access the onPress data on the native side.

dispatchEvent("topClick");
}

} // namespace facebook::react
2 changes: 2 additions & 0 deletions ...ReactCommon/TEMP_UntilReactCommonUpdate/react/renderer/components/view/ViewEventEmitter.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -57,6 +57,8 @@ class ViewEventEmitter : public TouchEventEmitter {
void onKeyUp(KeyboardEvent const &event) const; // [Windows]
void onKeyDown(KeyboardEvent const &event) const; // [Windows]

void onClick() const; // [Windows]

private:
/*
* Contains the most recent `frame` and a `mutex` protecting access to it.
Expand Down