Initial AccessibilityInfo implementation #5849

acoates-ms · 2020-08-26T18:33:18Z

There are several things wrong with this implementation.

isTouchExplorationEnabled always returns false. As far as I know windows does not expose a hook to know if someone is running narrator or similar tools. ~~Open question, should we default to true instead of false?~~ Using UiaClientsAreListening for now)
setAccessibilityFocus is unimplemented. - Turns out this method on this native module isn't called by RN. The AccessibilityInfo.setAccessibilityFocus method calls directly into UIManager.sendAccessibilityEvent (which we also need to implement) (Implement UIManager.sendAccessibilityEvent #5907)
announceForAccessibility Tries to find an element to raise the event from. All the elements I tried do not seem to return anything in the call FrameworkElementAutomationPeer::FromElement, so the method ends up being a no-op. Someone with more XAML expertise should see if they can come up with something better. -- We might want to look into adding an argument to the core function to allow users to specify which element should do the announce, since in desktop environments that might make more difference. (AccessibilityInfo::announceForAccessibility fails to announce #5908)

I expect to open an issue to track all the unresolved issues above when this goes in. But having this implementation does provide a good impl for isReduceMotionEnabled, and fixes the Redbox on the existing AccessibilityExample test page.

Microsoft Reviewers: Open in CodeFlow

NickGerleman · 2020-08-26T21:39:48Z

I think there might be some issues openaround api completion for AccessibilityInfo. Feel free to have this PR close them if you're wanting to track specific implementation issues.

NickGerleman · 2020-08-26T21:41:39Z

vnext/Microsoft.ReactNative/Modules/AccessibilityInfoModule.cpp

+  onSuccess(false);
+}
+
+void AccessibilityInfo::setAccessibilityFocus(double reactTag) noexcept {


Can't remember if we generate warnings for unused params or not, but consider commenting out the name.

vnext/src/RNTester/js/examples/Accessibility/AccessibilityInfoExample.js

aeulitz · 2020-08-27T18:49:35Z

vnext/Microsoft.ReactNative/Modules/AccessibilityInfoModule.cpp

+
+void AccessibilityInfo::isTouchExplorationEnabled(
+    std::function<void(React::JSValue const &)> const &onSuccess) noexcept {
+  onSuccess(false);


false [](start = 12, length = 5)

Re: - isTouchExplorationEnabled always returns false. As far as I know windows does not expose a hook to know if someone is running narrator or similar tools. Open question, should we default to true instead of false?

Idk what "touch exploration" means & how it's used, but as far as telling whether an a11y tool is running goes: There's UiaClientsAreListening (https://docs.microsoft.com/en-us/windows/win32/api/uiautomationcoreapi/nf-uiautomationcoreapi-uiaclientsarelistening). This is about events which is a subset of UIA functionality, but I'd assume that any "real" a11y tool would do that. If you should decide to use UiaClientsAreListening, do a sanity check on its returns; I vaguely recall that on earlier Win10 versions this returned true even if there was no real a11y tool running.

Is that API safe for UWP usage?

There's also IRawElementProviderAdviseEvents (https://docs.microsoft.com/en-us/windows/win32/api/uiautomationcore/nn-uiautomationcore-irawelementprovideradviseevents). This is something that needs to be implemented in the UI framework, and we'd have to connect to that - probably too complicated for what we want to achieve here.

In reply to: 478625704 [](ancestors = 478625704)

I believe so (we're using it in WinRT platform adaptors in FastAcc). Forrest would know for sure.

In reply to: 478626922 [](ancestors = 478626922)

Per otools/bin/mox/mosdk-shaped-fullanno.xml, it looks like UiaClientsAreListening is allowed.

In reply to: 478626922 [](ancestors = 478626922)

It still seems to return true even if nothing is running. Not sure how useful that hook up is then.

in-box XAML uses UiaClientsAreListening for this purpose, and I am pretty sure this is working reliably there (ie it returns true only when an AT like Narrator is running).

In reply to: 478640914 [](ancestors = 478640914,478626922)

@kmelmon it returns true if there is a UIA client that is subscribed to events - but UIA clients don't have to do that, instead they can just poll properties every N seconds. Narrator does subscribe to UIA events, so if the intent is to only get this working with narrator and maybe not support other UIA clients then that API is fine.

It's been a long time since I looked at this API, but IIRC it could return false positives. For example, I think it returns true on touch devices even if there no ATs listening.

kmelmon · 2020-09-01T03:11:31Z

vnext/Microsoft.ReactNative/Modules/AccessibilityInfoModule.cpp

+  onSuccess(false);
+}
+
+void AccessibilityInfo::setAccessibilityFocus(double /*reactTag*/) noexcept {


AccessibilityInfo::setAccessibilityFocus [](start = 5, length = 40)

We can use AutomationPeer::SetFocusCore() to implement this. See:
https://docs.microsoft.com/en-us/uwp/api/windows.ui.xaml.automation.peers.automationpeer.setfocuscore?view=winrt-19041
Also see XAML code that implements it here:
%SDXROOT%\onecoreuap\windows\dxaml\xcp\dxaml\lib\winrtgeneratedclasses\AutomationPeer.g.cpp

That moves actual focus to that element. This API is just trying to move the narrator focus. -- So, when running narrator, it would move the blue rectangle without moving keyboard focus.

(Otherwise, this API is pointless, since there is a similar API that sets normal focus)

Oops I didn't realize it's just supposed to move narrator focus. I'm asking around, meanwhile I see some hits in Windows that suggest raising the AutomationFocusChanged event might work (eg %SDXROOT%\xbox\shellapps\Shared\Xbox.Foundation.UI\Narrator\NarratorReader.cpp line 125)

// Raise the focus changed automation event, to ensure that this element receives narrator focus so that it reads the element and textToRead together. auto automationPeer = FrameworkElementAutomationPeer::FromElement(focusedElement); if (automationPeer != nullptr) { automationPeer.RaiseAutomationEvent(AutomationEvents::AutomationFocusChanged); }

In reply to: 481482991 [](ancestors = 481482991)

kmelmon · 2020-09-01T03:21:33Z

vnext/Microsoft.ReactNative/Modules/AccessibilityInfoModule.cpp

+      return;
+    }
+
+    auto peer = xaml::Automation::Peers::FrameworkElementAutomationPeer::FromElement(element);


auto peer = xaml::Automation::Peers::FrameworkElementAutomationPeer::FromElement(element); [](start = 4, length = 90)

Is this still returning null in all cases? If so feel free to file an issue and assign to me, I can look into it.

ghost · 2020-09-02T18:05:03Z

Hello @acoates-ms!

Because this pull request has the AutoMerge label, I will be glad to assist with helping to merge this pull request once all check-in policies pass.

p.s. you can customize the way I help with merging this pull request, such as holding this pull request until a specific person approves. Simply @mention me (`@msftbot`) and give me an instruction to get started! Learn more here.

NikoAri · 2020-09-02T18:08:43Z

vnext/Microsoft.ReactNative/Modules/AccessibilityInfoModule.cpp

+  m_context = reactContext;
+}
+
+void AccessibilityInfo::isReduceMotionEnabled(std::function<void(React::JSValue const &)> const &onSuccess) noexcept {


[](start = 88, length = 1)

(nit) can this be && to avoid std::function copy here and while passing into further lambdas?

NikoAri · 2020-09-02T18:09:16Z

vnext/Microsoft.ReactNative/Modules/AccessibilityInfoModule.cpp

+}
+
+void AccessibilityInfo::isReduceMotionEnabled(std::function<void(React::JSValue const &)> const &onSuccess) noexcept {
+  auto jsDispatcher = m_context.JSDispatcher();


jsDispatcher [](start = 7, length = 12)

std::move?

NikoAri · 2020-09-02T18:12:08Z

vnext/Microsoft.ReactNative/Modules/AccessibilityInfoModule.cpp

+  // no-op - This appears to be unused in RN
+}
+
+void AccessibilityInfo::announceForAccessibility(std::string announcement) noexcept {


string [](start = 54, length = 6)

Similar comment, can this be either && or const& to avoid accidental copying?

NikoAri

Summary: Cherry picks: microsoft#5849 microsoft#7220 microsoft#7290 Test Plan: See D29689294 Test Plan Reviewers: ericroz Reviewed By: ericroz Subscribers: eliwhite Differential Revision: https://phabricator.intern.facebook.com/D30940769 Tasks: T85606613 Signature: 30940769:1631724353:41db1d978132f06502803cfb4df0323f6da84732

Initial AccessibilityInfo implementation

74661c0

acoates-ms requested a review from a team as a code owner August 26, 2020 18:33

Andrew Coates added 3 commits August 26, 2020 11:33

formatting

8ceafe5

Change files

3e8aa91

build fixes

195a2b1

NickGerleman approved these changes Aug 26, 2020

View reviewed changes

review feedback

1c67c76

aeulitz reviewed Aug 27, 2020

View reviewed changes

ZihanChen-MSFT approved these changes Aug 27, 2020

View reviewed changes

This was linked to issues Aug 28, 2020

"Unknown" Exception When Viewing AccessibilityExample RNTester Page #5604

Closed

AccessibilityInfo announceForAccessibility #3117

Closed

AccessibilityInfo fetch() #3157

Closed

AccessibilityInfo isReduceMotionEnabled #3110

Closed

kmelmon reviewed Sep 1, 2020

View reviewed changes

Andrew Coates added 2 commits September 2, 2020 10:03

Merge remote-tracking branch 'upstream/master' into accessinfo

50a0d66

Use UiaClientsAreListening

5a54995

acoates-ms mentioned this pull request Sep 2, 2020

AccessibilityInfo::announceForAccessibility fails to announce #5908

Closed

lint and formatting

f53a2eb

acoates-ms mentioned this pull request Sep 2, 2020

Implement UIManager.sendAccessibilityEvent #5907

Closed

acoates-ms added the AutoMerge Causes a PR to be automatically merged once all requirements are passed (label drives bot activity) label Sep 2, 2020

NikoAri reviewed Sep 2, 2020

View reviewed changes

NikoAri approved these changes Sep 2, 2020

View reviewed changes

ghost merged commit d391879 into microsoft:master Sep 2, 2020

acoates-ms deleted the accessinfo branch July 23, 2021 16:07

This pull request was closed.

Initial AccessibilityInfo implementation #5849

Initial AccessibilityInfo implementation #5849

Uh oh!

Conversation

acoates-ms commented Aug 26, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Microsoft Reviewers: Open in CodeFlow

Uh oh!

NickGerleman commented Aug 26, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ghost commented Sep 2, 2020

p.s. you can customize the way I help with merging this pull request, such as holding this pull request until a specific person approves. Simply @mention me (@msftbot) and give me an instruction to get started! Learn more here.

Uh oh!

NikoAri Sep 2, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

NikoAri left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants

acoates-ms commented Aug 26, 2020 •

edited

Loading

p.s. you can customize the way I help with merging this pull request, such as holding this pull request until a specific person approves. Simply @mention me (`@msftbot`) and give me an instruction to get started! Learn more here.

NikoAri Sep 2, 2020 •

edited

Loading