Merge MSAA alert functionality with UIA #38745

yaakovschectman · 2023-01-10T16:56:51Z

Merges the existing MSAA alert implementation with UIA making use of the existing AXFragmentRoot class rather than using separate implementations for each. Also use the above class for holding alert nodes, making AccessibilityRootNode and AccessibilityAlert redundant.

Part of flutter/flutter#116220

Pre-launch Checklist

I read the Contributor Guide and followed the process outlined there for submitting PRs.
I read the Tree Hygiene wiki page, which explains my responsibilities.
I read and followed the Flutter Style Guide and the C++, Objective-C, Java style guides.
I listed at least one issue that this PR fixes in the description above.
I added new tests to check the change I am making or feature I am adding, or Hixie said the PR is test-exempt. See testing the engine for instructions on writing and running engine tests.
I updated/added relevant documentation (doc comments with ///).
I signed the CLA.
All existing and new tests are passing.

If you need help, consider asking for advice on the #hackers-new channel on Discord.

yaakovschectman · 2023-01-10T16:57:50Z

shell/platform/windows/flutter_windows_view.h

-                                     HWND hwnd,
-                                     LONG idObject,
-                                     LONG idChild);
+  virtual void NotifyWinEventWrapper(ui::AXPlatformNodeWin* node, ax::mojom::Event event);


AXPlatformNodeWin already has a helper method for dispatching accessibility events, so rather than dispatching them directly from the view, we use this helper method. This also means UIA events are handled alongside MSAA events.

yaakovschectman · 2023-01-10T16:58:48Z

shell/platform/windows/window.cc

@@ -204,35 +204,35 @@ LRESULT Window::OnGetObject(UINT const message,
  gfx::NativeViewAccessible root_view = GetNativeViewAccessible();
  // TODO(schectman): UIA is currently disabled by default.
  // https://github.com/flutter/flutter/issues/114547
-  if (is_uia_request && root_view) {
-#ifdef FLUTTER_ENGINE_USE_UIA
+  if (root_view) {


The AXFragmentRootWin is now used by both UIA and MSAA, so check if we need to create it in either case.

yaakovschectman · 2023-01-10T17:00:16Z

third_party/accessibility/ax/platform/ax_platform_node_win.cc

@@ -5408,7 +5418,7 @@ AXPlatformNodeWin* AXPlatformNodeWin::GetTargetFromChildID(

  AXPlatformNodeBase* base =
      FromNativeViewAccessible(node->GetNativeViewAccessible());
-  if (base && !IsDescendant(base))
+  if (base && !base->IsDescendantOf(this))


Allows using the descendant's override of IsDescendantOf to handle whichever delegate type it has.

yaakovschectman · 2023-01-10T17:01:02Z

shell/platform/windows/flutter_window.h

+  void Alert(const std::wstring& text) override;
+
+  // |WindowBindingHandler|
+  ui::AXPlatformNodeWin* GetAlert() override;


The two separate methods make unit testing simpler.

loic-sharma · 2023-01-10T20:27:01Z

shell/platform/common/alert_platform_node_delegate.h

+
+class AlertPlatformNodeDelegate : public ui::AXPlatformNodeDelegateBase {
+ public:
+  AlertPlatformNodeDelegate(ui::AXPlatformNodeDelegate* parent_delegate);


Suggested change

AlertPlatformNodeDelegate(ui::AXPlatformNodeDelegate* parent_delegate);

explicit AlertPlatformNodeDelegate(ui::AXPlatformNodeDelegate* parent_delegate);

See: https://google.github.io/styleguide/cppguide.html#Implicit_Conversions

loic-sharma · 2023-01-10T20:28:29Z

shell/platform/common/alert_platform_node_delegate.h

+
+class AlertPlatformNodeDelegate : public ui::AXPlatformNodeDelegateBase {
+ public:
+  AlertPlatformNodeDelegate(ui::AXPlatformNodeDelegate* parent_delegate);


Should this disallow copy and assign?

loic-sharma · 2023-01-10T20:29:48Z

shell/platform/windows/flutter_window_unittests.cc

  view.AnnounceAlert(message);
-  IAccessible* alert = root_node->GetOrCreateAlert();
+
+  IAccessible* alert = view.AlertNode();  // root_node->GetOrCreateAlert();


Suggested change

IAccessible* alert = view.AlertNode(); // root_node->GetOrCreateAlert();

IAccessible* alert = view.AlertNode();

loic-sharma · 2023-01-10T20:32:45Z

shell/platform/windows/flutter_platform_node_delegate_windows.cc

@@ -11,6 +11,7 @@
 #include "flutter/third_party/accessibility/ax/ax_clipping_behavior.h"
 #include "flutter/third_party/accessibility/ax/ax_coordinate_system.h"
 #include "flutter/third_party/accessibility/ax/platform/ax_fragment_root_win.h"
+#include "flutter/third_party/accessibility/ax/platform/ax_platform_node_win.h"


Is this necessary? Feel free to close if yes

loic-sharma · 2023-01-10T21:07:29Z

shell/platform/windows/window.cc

  }
-  accessibility_root_ = AccessibilityRootNode::Create();
+  alert_delegate_ =
+      std::make_unique<AlertPlatformNodeDelegate>(ax_fragment_root_.get());


What happens if the framework requests an alert before the app receives its first WM_GETOBJECT message? From my understanding, ax_fragment_root_ will be nullptr, and the alert node's parent will never be updated. Is that correct?

In that case nothing would happen, but if there has been no getobject message, I believe that would mean there is no screen reader open, so an alert would do nothing, is that correct?

To clarify, here's the scenario I'm concerned about:

The Windows embedder receives an alert message from the framework

The user opens a screen reader & the Windows embedder receives the first WM_GETOBJECT message

The Windows embedder receives a second alert message from the framework

In this scenario, I expect that the screen reader is notified of an alert for step 3. However, I suspect it won't. From my understanding, step 1 creates an alert delegate that will always have a nullptr parent delegate. Step 2 creates the ax_fragment_root_, but, the alert delegate is never notified of this parent. Finally for step 3, both AlertPlatformNodeDelegate::GetTargetForNativeAccessibilityEvent and AlertPlatformNodeDelegate::GetParent will return nullptr when called by AXPlatformNodeWin::NotifyAccessibilityEvent. Please let me know if I'm missing something!

loic-sharma · 2023-01-10T21:12:39Z

shell/platform/windows/flutter_windows_view.cc

-                   AccessibilityRootNode::kAlertChildId);
+  binding_handler_->Alert(text);
+  ui::AXPlatformNodeWin* alert_node = binding_handler_->GetAlert();
+  NotifyWinEventWrapper(alert_node, ax::mojom::Event::kAlert);


~~Why doesn't WindowBindingHandler::Alert call AXPlatformNodeWin::NotifyAccessibilityEvent?~~

EDIT: See message below instead

For mocking in the unit tests.

Taking a step back, the value of the FlutterWindowsView / FlutterWindow split is to enable mock testing by decoupling Flutter from native win32 APIs. We can't mock win32 APIs, so instead we wrap them in FlutterWindow and mock that.

Ideally FlutterWindow is as small as possible. Any code that's unit testable should be moved out of FlutterWindow and into FlutterWindowsView.

Currently, the alerting logic is split across FlutterWindow and FlutterWindowsView. Do you think we could refactor FlutterWindow to push up more alerting logic to FlutterWindowsView?

Nothing for this patch, but to add to this discussion, I sort of wonder if, from a testing point of view, longer term we should consider wrapping Win32 APIs like we do with, for example the embedder API. It feels like overkill, but that code would be really low-maintenance, and when we've done that with other embedders (e.g. the one we wrote for the google home hub), it's generally worked out well, and makes win32 mockable where desired.

loic-sharma · 2023-01-10T21:28:51Z

shell/platform/windows/window.cc

+      FlutterPlatformNodeDelegate* child_delegate =
+          static_cast<FlutterPlatformNodeDelegate*>(
+              ax_fragment_root_->GetChildNodeDelegate());
+      child_delegate->GetAXNode();


What does this do?

loic-sharma · 2023-01-10T21:29:54Z

shell/platform/windows/window.cc

+    } else if (is_msaa_request) {
+      // Create the accessibility root if it does not already exist.
+      // Return the IAccessible for the root view.
+      // Microsoft::WRL::ComPtr<IAccessible> root(root_view);


Suggested change

// Microsoft::WRL::ComPtr<IAccessible> root(root_view);

loic-sharma · 2023-01-10T21:33:42Z

third_party/accessibility/ax/platform/ax_fragment_root_win.cc

@@ -289,7 +289,7 @@ class AXFragmentRootMapWin {

 AXFragmentRootWin::AXFragmentRootWin(gfx::AcceleratedWidget widget,
                                     AXFragmentRootDelegateWin* delegate)
-    : widget_(widget), delegate_(delegate) {
+    : widget_(widget), delegate_(delegate), alert_node_(nullptr) {


What's the benefit of creating the alert node lazily? Could we create an alert node every time we create the root?

The alert node, delegate, and fragment root refer to each other, so one pointer or more will be set after creation.

Right... but the creation of the fragment root node and alert node is decoupled: a fragment root is created only when I attach a screen reader, and, an alert node is created only when the framework sends an alert. Why is that? Why doesn't creating a fragment root also create the alert node? Do we need to create an alert node if there's no screen reader?

If we do need to create an alert node even without a screen reader, perhaps that should also create a fragment root? That would address concerns like: #38745 (comment)

AXFragmentRootWin must be created with a non-null delegate, which does not exist when we are just testing FlutterWindowsView without an engine.

I'll see if I can mock out that method in the mock window.

yaakovschectman · 2023-01-10T22:41:16Z

With the logic of dispatching the alert message moved to the windows view, I will need to change the platform message unit test a bit more.

cbracken · 2023-01-11T18:22:23Z

shell/platform/common/alert_platform_node_delegate.h

+
+namespace flutter {
+
+class AlertPlatformNodeDelegate : public ui::AXPlatformNodeDelegateBase {


Add a doc comment. (True, we lack a lot in existing code but good to remedy in new code)

Please do a pass, there's several other fields that are also missing comments

cbracken · 2023-01-11T18:34:09Z

shell/platform/common/alert_platform_node_delegate.h

+  AlertPlatformNodeDelegate operator=(const AlertPlatformNodeDelegate& other) =
+      delete;
+
+  void SetText(const std::u16string& text);


Consider void SetText(std::u16string_view text);. It's super lightweight, sometimes better performance, and allows for passing a char16_t* string as well (unlikely as that is).

The AXNodeData methods that this method calls take string& parameters. Am I mistaken that constructing a string from a string_view effectively copies it?

I'm also leaning towards keeping this a string reference, but, the string reference is also copied when AXNodeData stores it as an attribute.

The AXNodeData methods that this method calls take string& parameters. Am I mistaken that constructing a string from a string_view effectively copies it?

Yes - to benefit from this, you'd need any methods this gets passed to to also take a view rather than a string ref. If the current implementation is doing so, then leave as-is; at some point we can do a sweep of the codebase to clean these up across the board.

cbracken · 2023-01-11T18:38:54Z

shell/platform/windows/flutter_window_unittests.cc

-  IAccessible* alert = root_node->GetOrCreateAlert();
+
+  IAccessible* alert = view.AlertNode();
+  FML_LOG(ERROR) << "Alert = " << (void*)alert;


Delete before checking in.

shell/platform/windows/flutter_windows_view.cc

cbracken · 2023-01-11T18:53:55Z

shell/platform/windows/window.h

@@ -214,6 +215,8 @@ class Window : public KeyboardManager::WindowDelegate {
  // Check if the high contrast feature is enabled on the OS
  virtual bool GetHighContrastEnabled();

+  void CreateAlertNode();


Add a doc comment. Ideally the questions it would address are what the lifetime of alert_node_ looks like:

Once created is it ever reset to nullptr? (doesn't look like it)

What happens if called when there already is a valid alert_node_ (it's a no-op so long as there's a valid parent or there's no fragment root)

cbracken · 2023-01-11T18:55:17Z

third_party/accessibility/ax/platform/ax_fragment_root_win.cc

@@ -425,4 +427,18 @@ int AXFragmentRootWin::GetIndexInParentOfChild() const {
  return 0;
 }

+void AXFragmentRootWin::SetAlertNode(AXPlatformNodeWin* alert_node) {
+  alert_node_ = alert_node;


Are there any assertions we should be applying? For example, is setting to nullptr valid?

I do not think we have a use case for this, but setting to nullptr would remove the alert node from the root. The only time it is accessed by AXFragmentRootWin is in ChildAtIndex, where it is checked for nullity.

If it would currently be a developer error to set it null, consider adding an assert(alert_node != nullptr); so we catch it in dev mode. If doing so is harmless, but unexpected, maybe just covering that in the doc comment is sufficient.

third_party/accessibility/ax/platform/ax_fragment_root_win.h

third_party/accessibility/ax/platform/ax_platform_node_win.cc

Some unused code remains to be removed. Merge MSAA to AXFragmentRootWin

loic-sharma · 2023-01-13T01:38:53Z

@yaakovschectman Please tag me when this is ready for another review 😄

loic-sharma · 2023-01-14T00:13:06Z

shell/platform/windows/flutter_windows_view.cc

  }
 }

 ui::AXFragmentRootDelegateWin* FlutterWindowsView::GetAxFragmentRootDelegate() {
+  auto bridge = engine_->accessibility_bridge().lock().get();


Can this be removed?

shell/platform/common/alert_platform_node_delegate.h

loic-sharma · 2023-01-17T21:11:20Z

third_party/accessibility/ax/platform/ax_platform_node_win.h

@@ -473,6 +473,9 @@ class AX_EXPORT __declspec(uuid("26f5641a-246d-457b-a96d-07f3fae6acf2"))
  static std::optional<PROPERTYID> MojoEventToUIAProperty(
      ax::mojom::Event event);

+  // |AXPlatformNodeBase|
+  bool IsDescendantOf(AXPlatformNode* acnestor) const override;


Suggested change

bool IsDescendantOf(AXPlatformNode* acnestor) const override;

bool IsDescendantOf(AXPlatformNode* ancestor) const override;

loic-sharma

LGTM, nice work!

cbracken

Nice!

cbracken · 2023-01-19T18:16:47Z

shell/platform/windows/flutter_windows_view.cc

-                   AccessibilityRootNode::kAlertChildId);
+  binding_handler_->Alert(text);
+  ui::AXPlatformNodeWin* alert_node = binding_handler_->GetAlert();
+  NotifyWinEventWrapper(alert_node, ax::mojom::Event::kAlert);


Nothing for this patch, but to add to this discussion, I sort of wonder if, from a testing point of view, longer term we should consider wrapping Win32 APIs like we do with, for example the embedder API. It feels like overkill, but that code would be really low-maintenance, and when we've done that with other embedders (e.g. the one we wrote for the google home hub), it's generally worked out well, and makes win32 mockable where desired.

…118829) * a0e3c14d4 Merge MSAA alert functionality with UIA (flutter/engine#38745) * 78bbea005 [web] dont look up webgl params if no GPU is available (flutter/engine#38948)

* Use AXFragmentRootWin for MSAA functionality. Some unused code remains to be removed. Merge MSAA to AXFragmentRootWin * Removing unused code * Remove unused files * Flip macro * Formatting * Licenses * Make reference * Disable copy constructor/assignment * Unused import * Formatting * Relocate alert logic * Remove comment and unused mock * Fix unit test * Idempotency * Formatting * PR feedback * Doc comments * Undo string change for now * Couple fragment root and alert node * Formatting * Add comments * Pointer to reference * Typo fix

yaakovschectman added accessibility affects: desktop platform-windows labels Jan 10, 2023

yaakovschectman requested review from cbracken, loic-sharma, a-wallen and chunhtai January 10, 2023 16:56

yaakovschectman self-assigned this Jan 10, 2023

yaakovschectman commented Jan 10, 2023

View reviewed changes

yaakovschectman marked this pull request as ready for review January 10, 2023 19:26

loic-sharma reviewed Jan 10, 2023

View reviewed changes

cbracken reviewed Jan 11, 2023

View reviewed changes

yaakovschectman added 8 commits January 11, 2023 16:21

Use AXFragmentRootWin for MSAA functionality.

3be3387

Some unused code remains to be removed. Merge MSAA to AXFragmentRootWin

Removing unused code

3465272

Remove unused files

1a7171b

Flip macro

0a20b2f

Formatting

7f78a69

Licenses

8982f08

Make reference

77ee7b9

Disable copy constructor/assignment

6352acd

yaakovschectman added 6 commits January 11, 2023 16:21

Fix unit test

463b74b

Idempotency

4abf2a1

Formatting

2dfe876

PR feedback

0ab2478

Doc comments

efe2657

Undo string change for now

42d7721

yaakovschectman force-pushed the uia_alert branch from b57d097 to 42d7721 Compare January 11, 2023 21:21

yaakovschectman added 2 commits January 13, 2023 10:37

Couple fragment root and alert node

cfedb56

Formatting

5373390

yaakovschectman requested a review from loic-sharma January 13, 2023 19:38

loic-sharma reviewed Jan 14, 2023

View reviewed changes

shell/platform/common/alert_platform_node_delegate.h Outdated Show resolved Hide resolved

yaakovschectman added 2 commits January 17, 2023 09:39

Add comments

48f0315

Pointer to reference

111ad30

loic-sharma reviewed Jan 17, 2023

View reviewed changes

loic-sharma approved these changes Jan 17, 2023

View reviewed changes

Typo fix

d32c151

cbracken approved these changes Jan 19, 2023

View reviewed changes

yaakovschectman merged commit a0e3c14 into flutter:main Jan 19, 2023

yaakovschectman deleted the uia_alert branch January 19, 2023 18:28

engine-flutter-autoroll mentioned this pull request Jan 19, 2023

Roll Flutter Engine from 29a0582a1d5f to 78bbea005d27 (2 revisions) flutter/flutter#118824

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Jan 19, 2023

a0e3c14d4 Merge MSAA alert functionality with UIA (flutter/engine#38745)

fe146e9

engine-flutter-autoroll mentioned this pull request Jan 19, 2023

Roll Flutter Engine from 29a0582a1d5f to 78bbea005d27 (2 revisions) flutter/flutter#118829

Merged

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Jan 19, 2023

a0e3c14d4 Merge MSAA alert functionality with UIA (flutter/engine#38745)

52b3463

	AlertPlatformNodeDelegate(ui::AXPlatformNodeDelegate* parent_delegate);
	explicit AlertPlatformNodeDelegate(ui::AXPlatformNodeDelegate* parent_delegate);

	IAccessible* alert = view.AlertNode(); // root_node->GetOrCreateAlert();
	IAccessible* alert = view.AlertNode();


		namespace flutter {

		class AlertPlatformNodeDelegate : public ui::AXPlatformNodeDelegateBase {

	bool IsDescendantOf(AXPlatformNode* acnestor) const override;
	bool IsDescendantOf(AXPlatformNode* ancestor) const override;

Merge MSAA alert functionality with UIA #38745

Merge MSAA alert functionality with UIA #38745

Conversation

yaakovschectman commented Jan 10, 2023

Pre-launch Checklist

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

loic-sharma Jan 10, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

loic-sharma Jan 10, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

loic-sharma Jan 10, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yaakovschectman Jan 13, 2023 • edited

Choose a reason for hiding this comment

yaakovschectman commented Jan 10, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

loic-sharma commented Jan 13, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

loic-sharma left a comment

Choose a reason for hiding this comment

cbracken left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

loic-sharma Jan 10, 2023 •

edited

loic-sharma Jan 10, 2023 •

edited

loic-sharma Jan 10, 2023 •

edited

yaakovschectman Jan 13, 2023 •

edited