Support text editing voiceover feedback in macOS #25600

chunhtai · 2021-04-14T22:14:39Z

design doc go/macos-text-editing-feedback

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-04-21T00:28:14Z

Hi @dnfield @cbracken Can you take a look at this PR? I have a design doc the outline the entire approach go/macos-text-editing-feedback

also cc @gaaclarke for that weird __unsafe_unretained issue for NSTextView

chinmaygarde · 2021-04-22T20:23:38Z

@cbracken is OOO today and @dnfield is probably busy with the Android 1x1 pbuffer effort. Is there anyone else we can ping to unblock? If not, lets wait till next week.

chunhtai · 2021-04-22T20:44:36Z

maybe @dkwingsmt for keyboard changes and @stuartmorgan for generic macos change

dnfield · 2021-04-22T21:11:07Z

Sorry, I lost track of this, but can work on reviewing it. It would probably be good for @stuartmorgan or @cbracken to look as well if they're available.

dnfield · 2021-04-22T21:13:12Z

(mainly because my head ends up stuck in MRC mode, and I'm less certain about how things end up working with ARC :)

dnfield

I didn't look super closely at the test files, and I may be making some MRC minded mistakes, but overall:

We should be cautious about accessing the view without checking that it's loaded, otherwise we could unintentionally force it to load when it shouldn't.
I'm curious about assuming the type of the app delegate. I think if we can avoid doing that it will be better, even if we're not already supporting it elsewhere.
The unsafe unassigned seems fishy to me.

dnfield · 2021-04-22T21:10:45Z

shell/platform/common/flutter_platform_node_delegate.h

+  //------------------------------------------------------------------------------
+  /// @brief      Gets the owner of this platform node delegate. This is useful
+  ///             when you want to get the information surround this platform
+  ///             node delegate, e.g. the global rect of this platform node
+  ///             delegate.


Please mention which task runner this is safe to use on. I assume Platform.

I am not too familiar with this part, what makes some thing safe to run on platform thread vs the other. Also The accessibility birdge class should only be used in embedding code, can embedding code run things on UI thread?

Fml weaks are only safe to use on the thread they were created on. The embedder can access the other task runners.

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

dnfield · 2021-04-22T21:31:43Z

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

-      [strong_engine.viewController.view convertRectFromBacking:ns_local_bounds];
-  NSRect ns_window_bounds = [strong_engine.viewController.view convertRect:ns_view_bounds
-                                                                    toView:nil];
+      [strong_engine.viewController.flutterView convertRectFromBacking:ns_local_bounds];


Here as well: we should either be asserting that the view is loaded, or fizzling out/returning a zero rect if it's not.

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

dnfield · 2021-04-22T21:49:03Z

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

 #import "flutter/shell/platform/darwin/macos/framework/Source/FlutterView.h"

 @interface FlutterViewController ()

 // The FlutterView for this view controller.
-@property(nonatomic, readonly, nullable) FlutterView* flutterView;
+@property(nonatomic, strong, nullable) FlutterView* flutterView;


I know this is the internal header, but I think we probably still want this ot be readonly in the header. We can redefine it as readwrite in the implementation file instead.

dnfield · 2021-04-22T21:49:15Z

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

+/**
+ * The text input plugin that handles text editing state for text fields.
+ */
+@property(nonatomic, strong, nonnull) FlutterTextInputPlugin* textInputPlugin;


Not readonly?

stuartmorgan-g

I did a first pass; I haven't gone over everything in detail (especially once I hit the window delegate issue described below).

stuartmorgan-g · 2021-04-23T18:27:56Z

shell/platform/common/accessibility_bridge.cc

@@ -29,6 +29,7 @@ AccessibilityBridge::AccessibilityBridge(
 AccessibilityBridge::~AccessibilityBridge() {
  event_generator_.ReleaseTree();
  tree_.RemoveObserver(static_cast<ui::AXTreeObserver*>(this));
+  id_wrapper_map_.clear();


Why do you need to clear an ivar in the destructor? Is this enforcing a specific ordering? If so, it absolutely needs a comment explaining that so someone doesn't just remove it.

stuartmorgan-g · 2021-04-23T18:34:30Z

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

 /**
 * A plugin to handle text input.
 *
 * Responsible for bridging the native macOS text input system with the Flutter framework text
 * editing classes, via system channels.
 *
 * This is not an FlutterPlugin since it needs access to FlutterViewController internals, so needs
- * to be managed differently.
+ * to be managed differently. The FlutterViewController has the ownership of this plugin.


What is the reason for adding this comment?

I am generally very skeptical of comments that make assertions about code above this class. If someone refactors FVC at some point and this is no longer the case, what are the chances they will find and update this comment?

Sometimes the benefits outweigh the risk of the comments becoming wrong, but I'm not clear what the benefits are here.

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

stuartmorgan-g · 2021-04-23T18:36:43Z

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

+@interface FlutterTextInputPlugin : NSTextView <FlutterKeySecondaryResponder>
+
+/**
+ * The native text field this backed by this plugin as its field editor.


I'm not able to parse this sentence (and I'm not sure what it's intended to say).

stuartmorgan-g · 2021-04-23T18:37:14Z

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


 /**
 * Initializes a text input plugin that coordinates key event handling with |viewController|.
 */
 - (instancetype)initWithViewController:(FlutterViewController*)viewController;

+/**
+ * Where this plugin is the first responder of this NSWindow.


stuartmorgan-g · 2021-04-23T19:05:11Z

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

+@end
+
+/**
+ * Private interface declaration for FlutterNativeTextFieldHidder.


Hider

Although really this whole line should be removed; comments should not restate obvious aspects of the language itself; anyone familiar with ObjC knows that this is an interface declaration. Just like we don't write comments like:

// This is declaring an integer variable: int x;

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

stuartmorgan-g · 2021-04-23T19:08:59Z

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

+/**
+ * Private interface declaration for FlutterNativeTextFieldHidder.
+ *
+ * This NSView is the content view of the FlutterViewController. This view


s/This NSView is the/The/ (see rationale above)

stuartmorgan-g · 2021-04-23T19:13:38Z

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

 }

 - (void)viewDidLoad {
+  FlutterAppDelegate* appDelegate = (FlutterAppDelegate*)[NSApp delegate];
+  self.windowDelegate = [[FlutterWindowDelegate alloc] initWithController:self];
+  appDelegate.mainFlutterWindow.delegate = _windowDelegate;


This is a non-starter; just as we don't own the application, we don't own the window. This could stomp a developer's window delegate, or they could stomp this.

If a window-level hook is the only way to make this workaround function, then you'll need to wire up an explicit way for an application to delegate to Flutter (likely via FVC) and then make the template set up a skeleton window delegate that explicitly does that forwarding, with a comment explaining why, and what to do it they want to have a different delegate.

Thanks for pointing this out. There may be a way to inject field editor without windows delegate if i can create the custom NSTextFieldCell. I have to test it out.

stuartmorgan-g · 2021-04-23T19:14:47Z

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

+
+- (id)windowWillReturnFieldEditor:(NSWindow*)sender toObject:(id)client {
+  if ([client isKindOfClass:[FlutterTextField class]]) {
+    return _controller.textInputPlugin;


What happens if there is more than one Flutter view in the window?

chunhtai · 2021-04-28T17:11:42Z

@stuartmorgan @dnfield Thanks for the review, I have addressed all the comments, the changes are in the second commit.

chunhtai · 2021-05-04T18:55:30Z

A friendly bump

dnfield · 2021-05-04T18:58:45Z

shell/platform/common/flutter_platform_node_delegate.h

+  /// @brief      Gets the owner of this platform node delegate. This is useful
+  ///             when you want to get the information surround this platform
+  ///             node delegate, e.g. the global rect of this platform node
+  ///             delegate. This pointer is only safe in the platform thread.


Since this is only safe to use on the Platform thread, we should probably use a fml::WeakPtr, which will actually do assertions around usage of this, rather than std::weak_ptr which is meant to be thread safe.

are we allowed to depend on fml in this common library? I remember there is a plan to make fml into its own standalone library, but i am not sure whether that is done yet?

Ahhh you're right sorry.

We're close to being able to use it, but not quite. Can you add a TODO here?

dnfield

All that's left for me is using the fml::WeakPtr instead fo std::weak_ptr for a non-thread-safe weak pointer - as long as @stuartmorgan 's concerns are also addressed.

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

stuartmorgan-g · 2021-05-11T14:07:44Z

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

@@ -451,15 +454,17 @@ - (void)setSemanticsEnabled:(BOOL)enabled {
    return;
  }
  _semanticsEnabled = enabled;
+  // We need to remove the accessibility children from flutter view


Who is "we" here? (go/avoidwe)

stuartmorgan-g · 2021-05-11T14:13:43Z

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

+    //
+    // Since we will nil the handler in dealloc. the weakSelf should
+    // be valid if the handler is ever called.
+    __unsafe_unretained FlutterTextInputPlugin* weakSelf = self;


Please don't call an unsafe pointer weakSelf; this is the idiomatic name for a weak pointer to self, not an unsafe pointer to self.

stuartmorgan-g · 2021-05-11T14:14:32Z

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

+    // NSTextView does not support _weak reference, so we have to
+    // use __unsafe_unretained and manage the reference ourselves.
+    //
+    // Since we will nil the handler in dealloc. the weakSelf should


Remove the stray . here please. Also, as above, use something clearer than "we" (e.g., you can just say "|dealloc| removes the handler, so the pointer should be valid if the handler is ever called.").

stuartmorgan-g · 2021-05-11T14:16:22Z

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

@@ -176,6 +199,17 @@ - (instancetype)initWithViewController:(FlutterViewController*)viewController {
  return self;
 }

+- (BOOL)isFirstResponder {
+  if (!self.flutterViewController.viewLoaded || !self.flutterViewController.view.window) {


The second half of this check isn't doing anything useful; if self.flutterViewController.view.window is nil, then [self.flutterViewController.view.window firstResponder] is also nil, and thus can't be equal to self.

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

stuartmorgan-g · 2021-05-11T14:36:08Z

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

+}
+
+- (instancetype)initWithFlutterView:(FlutterView*)view {
+  self = [super initWithFrame:view.frame];


Taking the frame from its child seems confusing. Why are you structuring it this way instead of following the more normal pattern of setting the outermost view's frame and letting subviews resize accordingly?

stuartmorgan-g · 2021-05-11T14:36:39Z

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

@@ -278,7 +321,11 @@ - (void)loadView {
    }
    flutterView = [[FlutterView alloc] initWithMainContext:mainContext reshapeListener:self];
  }
-  self.view = flutterView;
+  FlutterViewWrapper* nativeTextFieldHidder =


wrapperView

stuartmorgan-g · 2021-05-11T14:37:44Z

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

-  self.view = flutterView;
+  FlutterViewWrapper* nativeTextFieldHidder =
+      [[FlutterViewWrapper alloc] initWithFlutterView:flutterView];
+  [nativeTextFieldHidder addSubview:flutterView];


If you're going to make the Flutter view an init param, why not do this as part of that? Making it possible (and easy) to set up a wrapper that points to a FlutterView that it doesn't actually wrap seems like an anti-feature.

you are right.

stuartmorgan-g · 2021-05-11T14:38:51Z

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

+  if (event.type == NSEventTypeKeyDown) {
+    // macOS only sends keydown for performKeyEquivalent. We need
+    // to synthesize a key up event. This is true for repeat key
+    // events.


How is this change related to the PR?

And why isn't it tested?

oops I think i missed the test.

This will be called when user press shortcut, e.g. ctrl+v. If the framework receive the keydown without receive the keyup, it will assume the ctrl+v is held and never released. This cause other shortcut to break

This wasn't a problem before because the performKeyEquivalent will only be called if the view is a NSControl

chunhtai · 2021-05-11T23:10:27Z

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

@@ -45,8 +45,8 @@
 *    This will invoke [delegate flush:] on raster thread and
 *    [delegate commit:] on platform thread. The requestCommit call will be blocked
 *    until this is done. This is necessary to ensure that rasterizer won't start
- *    rasterizing next frame before we flipped the surface, which must be performed
- *    on platform thread
+ *    rasterizing next frame before the FlutterSurfaceManager flipped the surface,


I think I am correct, the FlutterSurfaceManager is in charged of switching context. Let me know if i am wrong

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

stuartmorgan-g · 2021-05-17T20:04:13Z

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

+}
+
+FlutterTextPlatformNode::~FlutterTextPlatformNode() {
+  [native_text_field_ setFieldEditor:nil];


Could you give an overview of the ownership relation between the various classes involved here? What object ultimately own the FlutterTextPlatformNode? It's not at all obvious to me what guarantee you have that engine.viewController.textInputPlugin will outlive this object instance.

You are right, there is no guarantee. I made the change to store the pointer to the engine directly and access the textinputplugin directly from the engine pointer, so it will either be nil or a valid pointer.

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

cbracken · 2021-05-26T00:07:35Z

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

+    if (start > 0 && end > 0) {
+      selection = NSMakeRange(start, end - start);
+    } else {
+      selection = NSMakeRange([self stringValue].length, 0);


Why is the default to set the cursor at the end of the string?

That is the native behavior when accessibility focus on a textfield without any selection. The problem with calling becomeFirstResponder on a textfield manually is that it will select the entire string.

setting this will sends the selection back to the framework through the textinputplugin

sgtm - can you add a comment to that effect?

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

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

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

third_party/accessibility/ax/platform/ax_platform_node_mac.mm

cbracken

LGTM on the approach taken here; most of the comments are nits or questions, or phrased another way -- requests for comments on the bits that are unclear.

…ine#25600)

flutter-dashboard bot added the platform-macos label Apr 14, 2021

google-cla bot added the cla: yes label Apr 14, 2021

chunhtai force-pushed the issues/77834-real-exp branch from c3cd2ed to 1fbef2b Compare April 16, 2021 19:31

chunhtai requested review from cbracken and dnfield April 21, 2021 00:26

chunhtai marked this pull request as ready for review April 21, 2021 20:32

dnfield reviewed Apr 22, 2021

View reviewed changes

dnfield requested a review from stuartmorgan-g April 22, 2021 21:51

stuartmorgan-g suggested changes Apr 23, 2021

View reviewed changes

chunhtai force-pushed the issues/77834-real-exp branch 4 times, most recently from 9ffa362 to c8d40c2 Compare April 28, 2021 17:10

dnfield reviewed May 4, 2021

View reviewed changes

dnfield suggested changes May 4, 2021

View reviewed changes

dnfield approved these changes May 10, 2021

View reviewed changes

stuartmorgan-g suggested changes May 11, 2021

View reviewed changes

chunhtai force-pushed the issues/77834-real-exp branch from c8d40c2 to 1448ba5 Compare May 11, 2021 23:06

chunhtai commented May 11, 2021

View reviewed changes

chunhtai force-pushed the issues/77834-real-exp branch 2 times, most recently from 87fea82 to 678480b Compare May 11, 2021 23:58

stuartmorgan-g suggested changes May 17, 2021

View reviewed changes

chunhtai force-pushed the issues/77834-real-exp branch from 217016e to a018092 Compare May 18, 2021 18:22