addressing comments

chunhtai · chunhtai · commit 5345209c77c6 · 2021-05-26T15:36:25.000-07:00
diff --git a/shell/platform/common/accessibility_bridge.cc b/shell/platform/common/accessibility_bridge.cc
@@ -114,17 +114,16 @@ AccessibilityBridge::GetPendingEvents() {
 void AccessibilityBridge::UpdateDelegate(
     std::unique_ptr<AccessibilityBridgeDelegate> delegate) {
   delegate_ = std::move(delegate);
-  // FlutterPlatformNodeDelegate[s] are created by the
-  // AccessibilityBridgeDelegate and may contain states from previous
-  // AccessibilityBridgeDelegate. Need to recreate them.
-  for (const auto& pair : id_wrapper_map_) {
+  // Recreate FlutterPlatformNodeDelegates since they may contain stale state
+  // from the previous AccessibilityBridgeDelegate.
+  for (const auto& [node_id, old_platform_node_delegate] : id_wrapper_map_) {
     std::shared_ptr<FlutterPlatformNodeDelegate> platform_node_delegate =
         delegate_->CreateFlutterPlatformNodeDelegate();
     platform_node_delegate->Init(
         std::static_pointer_cast<FlutterPlatformNodeDelegate::OwnerBridge>(
             shared_from_this()),
-        pair.second->GetAXNode());
-    id_wrapper_map_[pair.first] = platform_node_delegate;
+        old_platform_node_delegate->GetAXNode());
+    id_wrapper_map_[node_id] = platform_node_delegate;
   }
 }
 
diff --git a/shell/platform/common/accessibility_bridge_unittests.cc b/shell/platform/common/accessibility_bridge_unittests.cc
@@ -156,11 +156,9 @@ TEST(AccessibilityBridgeTest, canFireChildrenChangedCorrectly) {
 }
 
 TEST(AccessibilityBridgeTest, canUpdateDelegate) {
-  TestAccessibilityBridgeDelegate* delegate =
-      new TestAccessibilityBridgeDelegate();
-  std::unique_ptr<TestAccessibilityBridgeDelegate> ptr(delegate);
   std::shared_ptr<AccessibilityBridge> bridge =
-      std::make_shared<AccessibilityBridge>(std::move(ptr));
+      std::make_shared<AccessibilityBridge>(
+          std::make_unique<TestAccessibilityBridgeDelegate>());
   FlutterSemanticsNode root;
   root.id = 0;
   root.flags = static_cast<FlutterSemanticsFlag>(0);
@@ -200,10 +198,7 @@ TEST(AccessibilityBridgeTest, canUpdateDelegate) {
   EXPECT_FALSE(root_node.expired());
   EXPECT_FALSE(child1_node.expired());
   // Update Delegate
-  TestAccessibilityBridgeDelegate* new_delegate =
-      new TestAccessibilityBridgeDelegate();
-  std::unique_ptr<TestAccessibilityBridgeDelegate> new_ptr(new_delegate);
-  bridge->UpdateDelegate(std::move(new_ptr));
+  bridge->UpdateDelegate(std::make_unique<TestAccessibilityBridgeDelegate>());
 
   // Old tree is destroyed.
   EXPECT_TRUE(root_node.expired());
@@ -266,11 +261,9 @@ TEST(AccessibilityBridgeTest, canHandleSelectionChangeCorrectly) {
 }
 
 TEST(AccessibilityBridgeTest, doesNotAssignEditableRootToSelectableText) {
-  TestAccessibilityBridgeDelegate* delegate =
-      new TestAccessibilityBridgeDelegate();
-  std::unique_ptr<TestAccessibilityBridgeDelegate> ptr(delegate);
   std::shared_ptr<AccessibilityBridge> bridge =
-      std::make_shared<AccessibilityBridge>(std::move(ptr));
+      std::make_shared<AccessibilityBridge>(
+          std::make_unique<TestAccessibilityBridgeDelegate>());
   FlutterSemanticsNode root;
   root.id = 0;
   root.flags = static_cast<FlutterSemanticsFlag>(
@@ -292,9 +285,8 @@ TEST(AccessibilityBridgeTest, doesNotAssignEditableRootToSelectableText) {
 
   auto root_node = bridge->GetFlutterPlatformNodeDelegateFromID(0).lock();
 
-  EXPECT_EQ(root_node->GetData().GetBoolAttribute(
-                ax::mojom::BoolAttribute::kEditableRoot),
-            false);
+  EXPECT_FALSE(root_node->GetData().GetBoolAttribute(
+      ax::mojom::BoolAttribute::kEditableRoot));
 }
 
 }  // namespace testing
diff --git a/shell/platform/common/flutter_platform_node_delegate.h b/shell/platform/common/flutter_platform_node_delegate.h
@@ -135,9 +135,10 @@ class FlutterPlatformNodeDelegate : public ui::AXPlatformNodeDelegateBase {
 
   //------------------------------------------------------------------------------
   /// @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.
+  ///             when you want to get the information about surrounding nodes
+  ///             of this platform node delegate, e.g. the global rect of this
+  ///             platform node delegate. This pointer is only safe in the
+  ///             platform thread.
   std::weak_ptr<OwnerBridge> GetOwnerBridge() const;
 
  private:
diff --git a/shell/platform/darwin/macos/framework/Source/AccessibilityBridgeMacDelegate.mm b/shell/platform/darwin/macos/framework/Source/AccessibilityBridgeMacDelegate.mm
@@ -77,7 +77,7 @@
             .user_info = nil,
         });
       }
-      // In all other cases this delegate should post
+      // In all other cases, this delegate should post
       // |NSAccessibilityFocusedUIElementChangedNotification|, but this is
       // handled elsewhere.
       break;
diff --git a/shell/platform/darwin/macos/framework/Source/FlutterCompositor.h b/shell/platform/darwin/macos/framework/Source/FlutterCompositor.h
@@ -26,7 +26,7 @@ class FlutterCompositor {
   // data with the new BackingStore data.
   // If the backing store is being requested for the first time
   // for a given frame, this compositor does not create a new backing
-  // store but rather return the backing store associated with the
+  // store but rather returns the backing store associated with the
   // FlutterView's FlutterSurfaceManager.
   //
   // Any additional state allocated for the backing store and
diff --git a/shell/platform/darwin/macos/framework/Source/FlutterGLCompositor.h b/shell/platform/darwin/macos/framework/Source/FlutterGLCompositor.h
@@ -28,7 +28,7 @@ class FlutterGLCompositor : public FlutterCompositor {
   // data with the new BackingStore data.
   // If the backing store is being requested for the first time
   // for a given frame, this compositor does not create a new backing
-  // store but rather return the backing store associated with the
+  // store but rather returns the backing store associated with the
   // FlutterView's FlutterSurfaceManager.
   //
   // Any additional state allocated for the backing store and
diff --git a/shell/platform/darwin/macos/framework/Source/FlutterMetalCompositor.h b/shell/platform/darwin/macos/framework/Source/FlutterMetalCompositor.h
@@ -22,7 +22,7 @@ class FlutterMetalCompositor : public FlutterCompositor {
   //
   // If the backing store is being requested for the first time
   // for a given frame, this compositor does not create a new backing
-  // store but rather return the backing store associated with the
+  // store but rather returns the backing store associated with the
   // FlutterView's FlutterSurfaceManager.
   //
   // Any additional state allocated for the backing store and
diff --git a/shell/platform/darwin/macos/framework/Source/FlutterTextInputPlugin.h b/shell/platform/darwin/macos/framework/Source/FlutterTextInputPlugin.h
@@ -19,14 +19,13 @@
  * This is not an FlutterPlugin since it needs access to FlutterViewController internals, so needs
  * to be managed differently.
  *
- * When accessibility is on, accessibility bridge creates a native text field, i.e.
- * FlutterTextField, for every text field in the Flutter. This plugin acts as a field editor for
- * those native text fields.
+ * When accessibility is on, accessibility bridge creates a NSTextField, i.e. FlutterTextField,
+ * for every text field in the Flutter. This plugin acts as a field editor for those NSTextField[s].
  */
 @interface FlutterTextInputPlugin : NSTextView <FlutterKeySecondaryResponder>
 
 /**
- * The native text field that currently has this plugin as its field editor.
+ * The NSTextField that currently has this plugin as its field editor.
  *
  * Must be nil if accessibility is off.
  */
diff --git a/shell/platform/darwin/macos/framework/Source/FlutterTextInputPlugin.mm b/shell/platform/darwin/macos/framework/Source/FlutterTextInputPlugin.mm
@@ -466,8 +466,7 @@ - (void)insertText:(id)string replacementRange:(NSRange)range {
   if (range.location != NSNotFound) {
     // The selected range can actually have negative numbers, since it can start
     // at the end of the range if the user selected the text going backwards.
-    // NSRange uses NSUIntegers, however, so they have to be casted to know if the
-    // selection is reversed or not.
+    // Cast to a signed type to determine whether or not the selection is reversed.
     long signedLength = static_cast<long>(range.length);
     long location = range.location;
     long textLength = _activeModel->text_range().end();
diff --git a/shell/platform/darwin/macos/framework/Source/FlutterTextInputPluginTest.mm b/shell/platform/darwin/macos/framework/Source/FlutterTextInputPluginTest.mm
@@ -181,7 +181,7 @@ - (bool)testFirstRectForCharacterRange {
 namespace flutter::testing {
 
 namespace {
-// Returns an engine configured for the text fixture resource configuration.
+// Allocates and returns an engine configured for the text fixture resource configuration.
 FlutterEngine* CreateTestEngine() {
   NSString* fixtures = @(testing::GetFixturesPath());
   FlutterDartProject* project = [[FlutterDartProject alloc]
@@ -208,7 +208,7 @@ - (bool)testFirstRectForCharacterRange {
   FlutterViewController* viewController = [[FlutterViewController alloc] initWithProject:project];
   [viewController loadView];
   [engine setViewController:viewController];
-  // Creates a NSWindow so that the native text field can become first responder.
+  // Create a NSWindow so that the native text field can become first responder.
   NSWindow* window = [[NSWindow alloc] initWithContentRect:NSMakeRect(0, 0, 800, 600)
                                                  styleMask:NSBorderlessWindowMask
                                                    backing:NSBackingStoreBuffered
@@ -233,7 +233,6 @@ - (bool)testFirstRectForCharacterRange {
   [viewController.view addSubview:mockTextField];
   [mockTextField becomeFirstResponder];
 
-  // Sets Client.
   NSDictionary* arguments = @{
     @"inputAction" : @"action",
     @"inputType" : @{@"name" : @"inputName"},
@@ -244,7 +243,6 @@ - (bool)testFirstRectForCharacterRange {
   };
   [viewController.textInputPlugin handleMethodCall:methodCall result:result];
 
-  // Sets editing state.
   arguments = @{
     @"text" : @"new text",
     @"selectionBase" : @(1),
diff --git a/shell/platform/darwin/macos/framework/Source/FlutterTextInputSemanticsObject.h b/shell/platform/darwin/macos/framework/Source/FlutterTextInputSemanticsObject.h
@@ -34,21 +34,22 @@ class FlutterTextPlatformNode : public ui::AXPlatformNodeBase {
   ///        FlutterViewController. This is used by the FlutterTextField to get its
   ///        frame rect because the FlutterTextField is a subview of the
   ///        FlutterViewController.view.
-  NSRect GetFrameRelativeToControllerView();
+  NSRect GetFrame();
 
   // |ui::AXPlatformNodeBase|
   gfx::NativeViewAccessible GetNativeViewAccessible() override;
 
  private:
-  FlutterTextField* native_text_field_;
+  FlutterTextField* appkit_text_field_;
   __weak FlutterViewController* view_controller_;
 
   //------------------------------------------------------------------------------
   /// @brief Ensures the FlutterTextField is attached to the FlutterView. This
-  ///        method returns true if it is attached after this method excuted.
-  ///        If this method returns false, that means the FlutterTextField cannot
-  ///        attach to the FlutterView. This can happen when the FlutterEngine does
-  ///        not have a FlutterViewController or the FlutterView is not loaded yet.
+  ///        method returns true if the text field is succesfully attached. If
+  ///        this method returns false, that means the FlutterTextField could not
+  ///        be attached to the FlutterView. This can happen when the FlutterEngine
+  ///        does not have a FlutterViewController or the FlutterView is not loaded
+  ///        yet.
   bool EnsureAttachedToView();
 
   //------------------------------------------------------------------------------
@@ -60,15 +61,16 @@ class FlutterTextPlatformNode : public ui::AXPlatformNodeBase {
 }  // namespace flutter
 
 /**
- * A native text field that represents the NativeViewAccessible for the FlutterTextPlatformNode
+ * An NSTextField implementation that represents the NativeViewAccessible for the
+ * FlutterTextPlatformNode
  *
- * The NSAccessibility protocol does not provide full support for text editing. This native text
- * field is used to get around this problem. The FlutterTextPlatformNode creates a hidden
- * FlutterTextField and makes macOS think that it is interacting with a real native text field.
- * This causes VoiceOver to announce text editing feedbacks.
+ * The NSAccessibility protocol does not provide full support for text editing. This
+ * appkit text field is used to get around this problem. The FlutterTextPlatformNode
+ * creates a hidden FlutterTextField, since VoiceOver only provides text editing
+ * announcements for NSTextField subclasses.
  *
  * All of the text editing events in this native text field are redirected to the
- * FlutterTextInputPlugin
+ * FlutterTextInputPlugin.
  */
 @interface FlutterTextField : NSTextField
 
@@ -82,7 +84,8 @@ class FlutterTextPlatformNode : public ui::AXPlatformNodeBase {
 /**
  * Updates the string value and the selection of this text field.
  *
- * Calling this method is necesarry for macOS to get notified about string and selection changes.
+ * Calling this method is necessary for macOS to get notified about string and selection
+ * changes.
  */
 - (void)updateString:(NSString*)string withSelection:(NSRange)selection;
 
diff --git a/shell/platform/darwin/macos/framework/Source/FlutterTextInputSemanticsObject.mm b/shell/platform/darwin/macos/framework/Source/FlutterTextInputSemanticsObject.mm
@@ -14,7 +14,7 @@
 
 #pragma mark - FlutterTextFieldCell
 /**
- * A convenient class that can be used to set a custom field editor for a
+ * A convenient class that can be used to set a custom field editor for an
  * NSTextField.
  *
  * The FlutterTextField uses this class set the FlutterTextInputPlugin as
@@ -40,8 +40,8 @@ - (instancetype)initWithTextField:(NSTextField*)textField fieldEditor:(NSTextVie
   if (self) {
     _editor = editor;
     [self setControlView:textField];
-    // Read only text fields are sent to the mac embedding as static
-    // texts. This text field must be editable and selectable at this
+    // Read-only text fields are sent to the mac embedding as static
+    // text. This text field must be editable and selectable at this
     // point.
     self.editable = YES;
     self.selectable = YES;
@@ -91,7 +91,7 @@ - (void)updateString:(NSString*)string withSelection:(NSRange)selection {
 #pragma mark - NSView
 
 - (NSRect)frame {
-  return _node->GetFrameRelativeToControllerView();
+  return _node->GetFrame();
 }
 
 #pragma mark - NSAccessibilityProtocol
@@ -121,9 +121,10 @@ - (BOOL)becomeFirstResponder {
     NSString* textValue = @(_node->GetStringAttribute(ax::mojom::StringAttribute::kValue).data());
     int start = _node->GetIntAttribute(ax::mojom::IntAttribute::kTextSelStart);
     int end = _node->GetIntAttribute(ax::mojom::IntAttribute::kTextSelEnd);
+    NSAssert((start >= 0 && end >= 0) || (start == -1 && end == -1), @"selection is invalid");
     NSRange selection;
-    if (start > 0 && end > 0) {
-      selection = NSMakeRange(start, end - start);
+    if (start >= 0 && end >= 0) {
+      selection = NSMakeRange(MIN(start, end), ABS(end - start));
     } else {
       selection = NSMakeRange([self stringValue].length, 0);
     }
@@ -154,13 +155,13 @@ - (void)dealloc {
                                                  __weak FlutterViewController* view_controller) {
   Init(delegate);
   view_controller_ = view_controller;
-  native_text_field_ =
+  appkit_text_field_ =
       [[FlutterTextField alloc] initWithPlatformNode:this
                                          fieldEditor:view_controller.textInputPlugin];
-  native_text_field_.bezeled = NO;
-  native_text_field_.drawsBackground = NO;
-  native_text_field_.bordered = NO;
-  native_text_field_.focusRingType = NSFocusRingTypeNone;
+  appkit_text_field_.bezeled = NO;
+  appkit_text_field_.drawsBackground = NO;
+  appkit_text_field_.bordered = NO;
+  appkit_text_field_.focusRingType = NSFocusRingTypeNone;
 }
 
 FlutterTextPlatformNode::~FlutterTextPlatformNode() {
@@ -169,12 +170,12 @@ - (void)dealloc {
 
 gfx::NativeViewAccessible FlutterTextPlatformNode::GetNativeViewAccessible() {
   if (EnsureAttachedToView()) {
-    return native_text_field_;
+    return appkit_text_field_;
   }
   return nil;
 }
 
-NSRect FlutterTextPlatformNode::GetFrameRelativeToControllerView() {
+NSRect FlutterTextPlatformNode::GetFrame() {
   if (!view_controller_.viewLoaded) {
     return NSZeroRect;
   }
@@ -198,17 +199,17 @@ - (void)dealloc {
   if (!view_controller_.viewLoaded) {
     return false;
   }
-  if ([native_text_field_ isDescendantOf:view_controller_.view]) {
+  if ([appkit_text_field_ isDescendantOf:view_controller_.view]) {
     return true;
   }
-  [view_controller_.view addSubview:native_text_field_
+  [view_controller_.view addSubview:appkit_text_field_
                          positioned:NSWindowBelow
                          relativeTo:view_controller_.flutterView];
   return true;
 }
 
 void FlutterTextPlatformNode::EnsureDetachedFromView() {
-  [native_text_field_ removeFromSuperview];
+  [appkit_text_field_ removeFromSuperview];
 }
 
 }
diff --git a/shell/platform/darwin/macos/framework/Source/FlutterTextInputSemanticsObjectTest.mm b/shell/platform/darwin/macos/framework/Source/FlutterTextInputSemanticsObjectTest.mm
@@ -34,7 +34,7 @@
   FlutterViewController* viewController = [[FlutterViewController alloc] initWithProject:project];
   [viewController loadView];
   [engine setViewController:viewController];
-  // Creates a NSWindow so that the native text field can become first responder.
+  // Create a NSWindow so that the native text field can become first responder.
   NSWindow* window = [[NSWindow alloc] initWithContentRect:NSMakeRect(0, 0, 800, 600)
                                                  styleMask:NSBorderlessWindowMask
                                                    backing:NSBackingStoreBuffered
@@ -51,7 +51,7 @@
   node_data.SetValue("initial text");
   ax_node.SetData(node_data);
   delegate.Init(engine.accessibilityBridge, &ax_node);
-  // This should attaches a FlutterTextField to the view
+  // Verify that a FlutterTextField is attached to the view.
   FlutterTextPlatformNode text_platform_node(&delegate, viewController);
   id native_accessibility = text_platform_node.GetNativeViewAccessible();
   EXPECT_TRUE([native_accessibility isKindOfClass:[FlutterTextField class]]);
diff --git a/shell/platform/darwin/macos/framework/Source/FlutterViewController.mm b/shell/platform/darwin/macos/framework/Source/FlutterViewController.mm
diff --git a/shell/platform/darwin/macos/framework/Source/FlutterViewControllerTest.mm b/shell/platform/darwin/macos/framework/Source/FlutterViewControllerTest.mm

Original file line number	Diff line number	Diff line change
`@@ -77,7 +77,7 @@`
`77`	`77`	`.user_info = nil,`
`78`	`78`	`});`
`79`	`79`	`}`
`80`		`- // In all other cases this delegate should post`
	`80`	`+ // In all other cases, this delegate should post`
`81`	`81`	`// \|NSAccessibilityFocusedUIElementChangedNotification\|, but this is`
`82`	`82`	`// handled elsewhere.`
`83`	`83`	`break;`