Wires up accessibility bridge in mac embedding #23870

chunhtai · 2021-01-22T20:37:53Z

As title

tech overview: go/flutter-desktop-accessibility

detailed design: go/accessibility-bridge-detailed-design

umbrella issue
flutter/flutter#73819

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.
The reviewer has submitted any presubmit flakes in this PR using the engine presubmit flakes form before re-triggering the failure.

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

chunhtai · 2021-01-23T00:09:31Z

cc @dnfield @gw280

shell/platform/darwin/common/framework/Headers/FlutterMacros.h

dnfield · 2021-01-23T00:37:20Z

shell/platform/darwin/macos/framework/Source/AccessibilityBridgeMacDelegate.mm

+
+namespace flutter {  // namespace
+
+bool SystemVersionEqualTo(NSOperatingSystemVersion target) {


These three methods seem like they should probably be in a commonly accessible target.

shell/platform/darwin/macos/framework/Source/FlutterEngine.mm

dnfield · 2021-01-23T00:42:36Z

shell/platform/darwin/macos/framework/Source/AccessibilityBridgeMacDelegate.h

+  std::unique_ptr<FlutterPlatformNodeDelegate> CreateFlutterPlatformNodeDelegate() override;
+
+ private:
+  FlutterEngine* _flutterEngine;


Added ac omment below about this too - this seems like it wants to be a scoped_nsobject, or possibly some kind of weak_ptr. I'm not sure if that's used in the desktop.

It's an FML thing, but we might want to just move it to the darwin common instead.

I took a look at the file in mac embedding, it looks like all of them are using raw ptr.

macOS uses ARC. scoped_nsobject is not only unnecessary, but we do not currently have an ARC-safe scoped_nsobject implementation.

shell/platform/darwin/macos/framework/Source/AccessibilityBridgeMacDelegate.mm

shell/platform/darwin/macos/framework/Source/FlutterEngine.mm

shell/platform/darwin/macos/framework/Source/FlutterView.mm

stuartmorgan-g

I did a first pass, mostly since I was replying to comments anyway. I'll need to do a deeper pass to understanding the ownership models and interobject communication.

shell/platform/darwin/macos/framework/Source/FlutterEngine.mm

shell/platform/darwin/macos/framework/Source/FlutterView.mm

shell/platform/darwin/macos/framework/Source/AccessibilityBridgeMacDelegate.mm

stuartmorgan-g · 2021-01-25T20:27:02Z

shell/platform/common/cpp/accessibility_bridge.h

  const ui::AXTreeData& GetAXTreeData() const;

+  //------------------------------------------------------------------------------
+  /// @brief      Get all pending accessibility events generated during


stuartmorgan-g · 2021-01-25T20:58:22Z

shell/platform/darwin/macos/framework/Source/FlutterView.mm

+#pragma mark - NSAccessibility overrides
+
+- (BOOL)isAccessibilityElement {
+  return self.accessibilityChildren != nil && [self.accessibilityChildren count] != 0;


The first half of this check is redundant; [nil count] is 0.

stuartmorgan-g · 2021-01-25T20:59:24Z

shell/platform/darwin/macos/framework/Source/FlutterViewController.mm


+- (void)setupNotificationCenterObservers {
+  NSNotificationCenter* center = [NSNotificationCenter defaultCenter];
+  // The MacOS fires this private message When voiceover turns on or off.


s/The MacOS/macOS/
s/When voiceover/when VoiceOver/

stuartmorgan-g · 2021-01-25T21:03:30Z

shell/platform/darwin/macos/framework/Source/FlutterViewController.mm

  }
  controller->_additionalKeyResponders = [[NSMutableOrderedSet alloc] init];
  controller->_mouseTrackingMode = FlutterMouseTrackingModeInKeyWindow;
+  [controller setupNotificationCenterObservers];


Do not call methods on an object during init. It's a common way to get subtle bugs over time. That's why this is CommonInit and not, say, -doCommonInitialization. If you want the code to be separate from this function for some reason, it should be in another, similar function.

stuartmorgan-g · 2021-01-25T21:04:40Z

shell/platform/darwin/macos/framework/Source/FlutterViewController.mm

  _engine.viewController = nil;
 }

+- (void)setupNotificationCenterObservers {


If you keep this (as a function), note that "setup" is a noun, while the verb is "set up", so methods/functions should be capitalized accordingly. Also, this needs a declaration comment.

shell/platform/darwin/macos/framework/Source/FlutterViewController.mm

chunhtai · 2021-01-26T20:47:32Z

Hi @stuartmorgan @dnfield this is ready for another look

chunhtai · 2021-02-01T17:42:06Z

A friendly bump

shell/platform/darwin/macos/framework/Source/AccessibilityBridgeMacDelegate.h

stuartmorgan-g · 2021-02-04T00:55:12Z

shell/platform/darwin/macos/framework/Source/FlutterVersions.h

+///                                     not provided, the function only compares
+///                                     up to minor version.
+bool isOperatingSystemAtLeastVersion(NSInteger majorVersion,
+                                     NSInteger minorVersion = -1,


What is the reason for the use of -1 for these as a special value, rather than just making the default zero? Is it just consistency with the other function?

zero is a valid version number, my intention is to use a invalid version number to be default to do a wild card compare

I understand the goal; my point is that comparing against 13, -1, -1 and against 13, 0, 0 have exactly the same effect for a >= check.

stuartmorgan-g · 2021-02-04T01:00:46Z

shell/platform/darwin/macos/framework/Source/FlutterVersions.h

+///                                     a postive integer if provided. If it is
+///                                     not provided, the function only compares
+///                                     up to minor version.
+bool isOperatingSystemAtMostVersion(NSInteger majorVersion,


Why do we need this form? If we're putting an upper bound on a section of code, then we know the first OS version where that code is no longer needed. Why not just express everything that way and have a simpler API surface?

E.g., you have code that only runs for versions up through 10.13, which is equivalent to not running if it's 10.14+.

stuartmorgan-g · 2021-02-04T01:04:52Z

shell/platform/darwin/macos/framework/Source/FlutterVersions.mm

+  NSCAssert(majorVersion != -1, @"majorVersion must not be ignored");
+  NSCAssert(minorVersion != -1 || patchVersion == -1,
+            @"if minorVersion is ignored, patchVersion must also be ignored");
+  NSOperatingSystemVersion systemVersion = [[NSProcessInfo processInfo] operatingSystemVersion];


As noted in the last review, NSProcessInfo has isOperatingSystemAtLeast.

In conjunction with the other comments (zero being fine as an unset value for >=, and <= being easily expressed with a different >=) that should mean that this entire file can be completely eliminated in favor of just using an existing OS method.

(And then I don't have to ask you where the tests for these functions are 😉 )

ah you are right

shell/platform/darwin/macos/framework/Source/FlutterView.mm

stuartmorgan-g · 2021-02-04T01:07:22Z

shell/platform/darwin/macos/framework/Source/FlutterViewController.mm

  controller->_mouseTrackingMode = FlutterMouseTrackingModeInKeyWindow;
+
+  NSNotificationCenter* center = [NSNotificationCenter defaultCenter];
+  // The macOS fires this private message when voiceover turns on or off.


s/The macOS/macOS/

chunhtai · 2021-02-12T17:55:57Z

A friendly bump

stuartmorgan-g · 2021-02-12T18:50:18Z

A friendly bump

I'm neck-deep in NNBD work at the moment; I can look some next week, or @cbracken can take over for me in reviewing.

cbracken · 2021-02-13T22:34:32Z

Taking a look on my end!

chinmaygarde · 2021-02-18T22:55:45Z

This is on @cbracken s review backlog. The presubmit failures are formatting issues.

cbracken

Not finished, but sending out a first round of comments.

shell/platform/common/accessibility_bridge.h

shell/platform/darwin/macos/framework/Source/FlutterEngine.mm

cbracken · 2021-02-26T18:37:44Z

shell/platform/darwin/macos/framework/Source/FlutterPlatformNodeDelegateMac.mm

+  NSRect window_bounds = [_engine.viewController.view convertRect:view_bounds toView:nil];
+  NSRect global_bounds = [[_engine.viewController.view window] convertRectToScreen:window_bounds];
+  // NSWindow is flipped.
+  NSScreen* screen = [[NSScreen screens] firstObject];


I think we should figure this out/scrounge through docs and sample code, or reach out to the Chromium folks in the blame list. This seems pretty surprising to me. We make this sort of assumption (and shouldn't) in a few places on iOS, but it's likely to break less people there.

Can we use this instead?
https://developer.apple.com/documentation/appkit/nswindow/1419232-screen?language=objc

shell/platform/darwin/macos/framework/Source/FlutterPlatformNodeDelegateMac.h

cbracken · 2021-02-26T19:05:46Z

shell/platform/darwin/macos/framework/Source/FlutterPlatformNodeDelegateMac.mm

+  local_bounds.origin.y = bounds.y();
+  local_bounds.size.width = bounds.width();
+  local_bounds.size.height = bounds.height();
+  // local_bounds is flipped against y axis.


Since we have a mix of platform experience on the team it would be good to provide the tiny bit of additional context that macOS Y co-ords start at bottom-left and increase towards top-right vs Flutter's top-left increasing to bottom-right.

shell/platform/darwin/macos/framework/Source/FlutterPlatformNodeDelegateMac.mm

shell/platform/darwin/macos/framework/Source/AccessibilityBridgeMacDelegate.h

cbracken · 2021-02-26T21:29:01Z

shell/platform/darwin/macos/framework/Source/AccessibilityBridgeMacDelegate.h

+  /// @param[in]  event_type        The original event type.
+  /// @param[in]  ax_node           The original event target.
+  std::vector<NSAccessibilityEvent> ConvertEvent(ui::AXEventGenerator::Event event_type,
+                                                 const ui::AXNode& ax_node) const;


Maybe: MacOSEventFromAXEvent

cbracken · 2021-02-26T21:31:52Z

shell/platform/darwin/macos/framework/Source/AccessibilityBridgeMacDelegate.h

+  //---------------------------------------------------------------------------
+  /// @brief      Whether the given event is in current pending events.
+  /// @param[in]  event_type        The event you would like to look up.
+  bool IsInGeneratedEventBatch(ui::AXEventGenerator::Event event_type) const;


Would bool HasPendingEvent be more accurate?

Why event_type rather than event?

cbracken · 2021-02-26T21:39:33Z

shell/platform/darwin/macos/framework/Source/AccessibilityBridgeMacDelegate.mm

+          .user_info = nil,
+      });
+      if ([[NSProcessInfo processInfo]
+              isOperatingSystemAtLeastVersion:{.majorVersion = 10, .minorVersion = 11}] &&


Just if (@available(macOS 10.11, *)).

cbracken · 2021-02-26T21:40:27Z

shell/platform/darwin/macos/framework/Source/AccessibilityBridgeMacDelegate.mm

+    }
+    case ui::AXEventGenerator::Event::LIVE_REGION_CHANGED: {
+      if (![[NSProcessInfo processInfo]
+              isOperatingSystemAtLeastVersion:{.majorVersion = 10, .minorVersion = 14}]) {


@available(macOS 10.14, *)

cbracken · 2021-02-26T21:45:35Z

shell/platform/darwin/macos/framework/Source/AccessibilityBridgeMacDelegate.mm

+  NSCAssert(flutter_engine_, @"Flutter engine should not be deallocated");
+  auto bridge = flutter_engine_.accessibilityBridge.lock();
+  NSCAssert(bridge, @"Accessibility bridge in flutter engine must not be null.");
+  std::vector<ui::AXEventGenerator::TargetedEvent> events = bridge->GetCurrentEvents();


nit: consider inlining bridge->GetCurrentEvents() into the for below, since this local is never accessed directly and the expression is short.

shell/platform/darwin/macos/framework/Source/AccessibilityBridgeMacDelegate.mm

chunhtai · 2021-03-03T17:36:58Z

Hi @cbracken , This pr is ready for another look.

cbracken · 2021-03-09T21:24:45Z

shell/platform/darwin/macos/framework/Source/AccessibilityBridgeMacDelegate.mm

+    }
+    case ui::AXEventGenerator::Event::LIVE_REGION_CHANGED: {
+      if (@available(macOS 10.14, *)) {
+      } else {


Invert the check logic here and place this in the if block, then kill off the else block.

I got compiler error when i did

if (!@available(macOS 10.14, *)) { }

it seems like it has to be if (@available(..., ...)) inorder to use this annotation?

Ah bummer. Would you mind adding a comment along the lines of // Do nothing on macOS 10.14 and later. just to make 100% clear this is not an oversight for future readers? The comment in the else block hints at this, but useful to add a comment.

cbracken · 2021-03-09T21:33:53Z

shell/platform/darwin/macos/framework/Source/FlutterPlatformNodeDelegateMac.h

+  virtual ~FlutterPlatformNodeDelegateMac();
+
+  //---------------------------------------------------------------------------
+  /// @brief      Gets the live region text of this node in UTF8 format. This is


cbracken · 2021-03-09T21:36:00Z

shell/platform/darwin/macos/framework/Source/AccessibilityBridgeMacDelegate.h

+  ///             macOS native accessibility event[s]
+  /// @param[in]  event_type        The original event type.
+  /// @param[in]  ax_node           The original event target.
+  std::vector<NSAccessibilityEvent> MacOSEventFromAXEvent(ui::AXEventGenerator::Event event_type,


Since this is a vector: MacOSEventsFromAXEvents. (Apologies for missing the 's'es in the original review comment)

cbracken · 2021-03-09T21:39:09Z

shell/platform/darwin/macos/framework/Source/FlutterEngineTest.mm

+  [engine setViewController:nil];
+  [engine shutDownEngine];
+}
+


We should add a test for disabling a11y.

cbracken

Looking much cleaner. Overall lgtm with a few nits.

I don't see any code doing a recursive walk of children in the semantics tree for hit-testing purposes -- though I do see the transform code. Is that not required on macOS, or are we doing that somewhere else?

Discussed some internal reference material for additional testing ideas over chat.

chunhtai · 2021-03-09T22:07:46Z

@cbracken
voiceover does not have hittest, in mac VoiceOver, the mouse behave exactly as before, the only way to control the accessibility focus is using keyboard.

fluttergithubbot · 2021-03-10T17:50:02Z

This pull request is not suitable for automatic merging in its current state.

This pull request has changes requested by @dnfield. Please resolve those before re-applying the label.
This pull request has changes requested by @stuartmorgan. Please resolve those before re-applying the label.

stuartmorgan-g · 2021-03-10T18:22:50Z

(I didn't re-review, but marking as approved since I trust that any remaining issues from my comments have been addressed in subsequent review)

…e#23870)

* Wires up accessibility bridge in mac embedding * addressing comment * addressing comments

google-cla bot added the cla: yes label Jan 22, 2021

chunhtai mentioned this pull request Jan 22, 2021

☂️ Desktop accessibility support flutter/flutter#73819

Closed

10 tasks

chunhtai requested a review from stuartmorgan-g January 23, 2021 00:07

dnfield suggested changes Jan 23, 2021

View reviewed changes

stuartmorgan-g suggested changes Jan 25, 2021

View reviewed changes

chunhtai added the Work in progress (WIP) Not ready (yet) for review! label Jan 25, 2021

chunhtai changed the title ~~Wires up accessibility bridge in mac embedding~~ [WIP]Wires up accessibility bridge in mac embedding Jan 25, 2021

chunhtai force-pushed the mac_embedding branch from 23e5f46 to 20c76f3 Compare January 26, 2021 19:28

chunhtai requested review from dnfield and stuartmorgan-g January 26, 2021 20:46

chunhtai removed the Work in progress (WIP) Not ready (yet) for review! label Jan 26, 2021

chunhtai changed the title ~~[WIP]Wires up accessibility bridge in mac embedding~~ Wires up accessibility bridge in mac embedding Jan 26, 2021

stuartmorgan-g suggested changes Feb 4, 2021

View reviewed changes

chunhtai force-pushed the mac_embedding branch 2 times, most recently from 557f997 to e0fb2d3 Compare February 5, 2021 18:54

chunhtai requested a review from stuartmorgan-g February 8, 2021 18:27

chunhtai force-pushed the mac_embedding branch from c0fe185 to 3c75f62 Compare February 10, 2021 00:41

chunhtai requested a review from cbracken February 10, 2021 20:14

chunhtai force-pushed the mac_embedding branch from 3c75f62 to c551aed Compare February 17, 2021 18:40

cbracken suggested changes Feb 26, 2021

View reviewed changes

cbracken reviewed Feb 26, 2021

View reviewed changes

shell/platform/darwin/macos/framework/Source/AccessibilityBridgeMacDelegate.h Show resolved Hide resolved

cbracken reviewed Feb 26, 2021

View reviewed changes

shell/platform/darwin/macos/framework/Source/AccessibilityBridgeMacDelegate.mm Show resolved Hide resolved

chunhtai force-pushed the mac_embedding branch from bcead86 to fce6747 Compare March 2, 2021 22:07

chunhtai requested a review from cbracken March 4, 2021 18:24

cbracken reviewed Mar 9, 2021

View reviewed changes

cbracken approved these changes Mar 9, 2021

View reviewed changes

chunhtai added 3 commits March 9, 2021 15:50

Wires up accessibility bridge in mac embedding

340293d

addressing comment

a5f30b6

addressing comments

8add1a8

chunhtai force-pushed the mac_embedding branch from 7a71c0e to 8add1a8 Compare March 9, 2021 23:50

chunhtai added the waiting for tree to go green This PR is approved and tested, but waiting for the tree to be green to land. label Mar 10, 2021

fluttergithubbot removed the waiting for tree to go green This PR is approved and tested, but waiting for the tree to be green to land. label Mar 10, 2021

stuartmorgan-g approved these changes Mar 10, 2021

View reviewed changes

chunhtai merged commit afc72ee into flutter:master Mar 10, 2021

engine-flutter-autoroll mentioned this pull request Mar 10, 2021

Roll Engine from 9b5f59ec3d54 to afc72ee1d79c (3 revisions) flutter/flutter#77847

Merged

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Mar 10, 2021

afc72ee Wires up accessibility bridge in mac embedding (flutter/engin…

be27195

…e#23870)

hjfreyer pushed a commit to hjfreyer/engine that referenced this pull request Mar 22, 2021

Wires up accessibility bridge in mac embedding (flutter#23870)

8aaccb3

* Wires up accessibility bridge in mac embedding * addressing comment * addressing comments

chriscraws pushed a commit to chriscraws/engine that referenced this pull request Mar 23, 2021

Wires up accessibility bridge in mac embedding (flutter#23870)

8c3f18d

* Wires up accessibility bridge in mac embedding * addressing comment * addressing comments


		namespace flutter { // namespace

		bool SystemVersionEqualTo(NSOperatingSystemVersion target) {

Wires up accessibility bridge in mac embedding #23870

Wires up accessibility bridge in mac embedding #23870

Uh oh!

Conversation

chunhtai commented Jan 22, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Pre-launch Checklist

Uh oh!

chunhtai commented Jan 23, 2021

Uh oh!

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!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

stuartmorgan-g left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

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!

Uh oh!

chunhtai commented Jan 26, 2021

Uh oh!

chunhtai commented Feb 1, 2021

Uh oh!

Uh oh!

Uh oh!

Uh oh!

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!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

chunhtai commented Feb 12, 2021

Uh oh!

stuartmorgan-g commented Feb 12, 2021

Uh oh!

cbracken commented Feb 13, 2021

Uh oh!

chinmaygarde commented Feb 18, 2021

Uh oh!

cbracken left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

chunhtai commented Jan 22, 2021 •

edited

Loading

cbracken Feb 26, 2021 •

edited

Loading

cbracken Feb 26, 2021 •

edited

Loading

chunhtai Mar 9, 2021 •

edited

Loading

cbracken Mar 9, 2021 •

edited

Loading