addressing comments

Author: chunhtai

shell/platform/common/accessibility_bridge.cc

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

shell/platform/common/flutter_platform_node_delegate.h

@@ -137,7 +137,7 @@ 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.
+  ///             delegate. This pointer is only safe in the platform thread.
   std::weak_ptr<OwnerBridge> GetOwnerBridge() const;
 
  private:

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

@@ -208,7 +208,7 @@
           .target = native_node,
           .user_info = nil,
       });
-      // Voiceover requires a live region changed notification to actually
+      // VoiceOver requires a live region changed notification to actually
       // announce the live region.
       auto live_region_events =
           MacOSEventsFromAXEvent(ui::AXEventGenerator::Event::LIVE_REGION_CHANGED, ax_node);

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

@@ -96,6 +96,9 @@ - (instancetype)initWithPlugin:(NSString*)pluginKey flutterEngine:(FlutterEngine
 }
 
 - (NSView*)view {
+  if (!_flutterEngine.viewController.viewLoaded) {
+    return nil;
+  }
   return _flutterEngine.viewController.flutterView;
 }
 
@@ -417,7 +420,7 @@ - (void)updateDisplayConfig {
 }
 
 - (void)updateWindowMetrics {
-  if (!_engine) {
+  if (!_engine || !_viewController.viewLoaded) {
     return;
   }
   NSView* view = _viewController.flutterView;
@@ -453,7 +456,7 @@ - (void)setSemanticsEnabled:(BOOL)enabled {
   _semanticsEnabled = enabled;
   // We need to remove the accessibility children from flutter view
   // before we reset the bridge.
-  if (!_semanticsEnabled) {
+  if (!_semanticsEnabled && self.viewController.viewLoaded) {
     self.viewController.flutterView.accessibilityChildren = nil;
   }
   if (!_semanticsEnabled && _bridge) {
@@ -669,6 +672,10 @@ - (void)updateSemanticsCustomActions:(const FlutterSemanticsCustomAction*)action
     // Custom action with id = kFlutterSemanticsNodeIdBatchEnd indicates this is
     // the end of the update batch.
     _bridge->CommitUpdates();
+    // Accessibility tree can only be used when the view is loaded.
+    if (!self.viewController.viewLoaded) {
+      return;
+    }
     // Attaches the accessibility root to the flutter view.
     auto root = _bridge->GetFlutterPlatformNodeDelegateFromID(0).lock();
     if (root) {

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

@@ -47,6 +47,7 @@
   gfx::NativeViewAccessible parent = FlutterPlatformNodeDelegate::GetParent();
   if (!parent) {
     NSCAssert(engine_, @"Flutter engine should not be deallocated");
+    NSCAssert(engine_.viewController.viewLoaded, @"Flutter view must be loaded");
     return engine_.viewController.flutterView;
   }
   return parent;
@@ -101,22 +102,23 @@
   // increasing to bottom-right. Therefore, We need to flip the y coordinate when
   // we convert from flutter coordinates to macOS coordinates.
   ns_local_bounds.origin.y = -ns_local_bounds.origin.y - ns_local_bounds.size.height;
-  __strong FlutterEngine* strong_engine = engine_;
-  NSCAssert(strong_engine, @"Flutter engine should not be deallocated");
+
+  NSCAssert(engine_, @"Flutter engine should not be deallocated");
+  NSCAssert(engine_.viewController.viewLoaded, @"Flutter view must be loaded.");
   NSRect ns_view_bounds =
-      [strong_engine.viewController.flutterView convertRectFromBacking:ns_local_bounds];
-  NSRect ns_window_bounds = [strong_engine.viewController.flutterView convertRect:ns_view_bounds
-                                                                           toView:nil];
+      [engine_.viewController.flutterView convertRectFromBacking:ns_local_bounds];
+  NSRect ns_window_bounds = [engine_.viewController.flutterView convertRect:ns_view_bounds
+                                                                     toView:nil];
   NSRect ns_screen_bounds =
-      [[strong_engine.viewController.flutterView window] convertRectToScreen:ns_window_bounds];
+      [[engine_.viewController.flutterView window] convertRectToScreen:ns_window_bounds];
   gfx::RectF screen_bounds(ns_screen_bounds.origin.x, ns_screen_bounds.origin.y,
                            ns_screen_bounds.size.width, ns_screen_bounds.size.height);
   return screen_bounds;
 }
 
 gfx::RectF FlutterPlatformNodeDelegateMac::ConvertBoundsFromScreenToGlobal(
     const gfx::RectF& screen_bounds) const {
-  // The voiceover seems to only accept bounds that are relative to primary screen.
+  // The VoiceOver seems to only accept bounds that are relative to primary screen.
   // Thus, we use [[NSScreen screens] firstObject] instead of [NSScreen mainScreen].
   NSScreen* screen = [[NSScreen screens] firstObject];
   NSRect primary_screen_bounds = [screen frame];

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

@@ -8,9 +8,9 @@
 #import "flutter/shell/platform/darwin/macos/framework/Source/FlutterDartProject_Internal.h"
 #import "flutter/shell/platform/darwin/macos/framework/Source/FlutterEngine_Internal.h"
 #import "flutter/shell/platform/darwin/macos/framework/Source/FlutterPlatformNodeDelegateMac.h"
+#import "flutter/shell/platform/darwin/macos/framework/Source/FlutterTextInputSemanticsObject.h"
 #import "flutter/shell/platform/darwin/macos/framework/Source/FlutterViewControllerTestUtils.h"
 #import "flutter/shell/platform/darwin/macos/framework/Source/FlutterViewController_Internal.h"
-#import "flutter/shell/platform/darwin/macos/framework/Source/FlutterTextInputSemanticsObject.h"
 
 #include "flutter/shell/platform/common/accessibility_bridge.h"
 #include "flutter/shell/platform/embedder/test_utils/proc_table_replacement.h"

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

@@ -17,7 +17,7 @@
  * editing classes, via system channels.
  *
  * This is not an FlutterPlugin since it needs access to FlutterViewController internals, so needs
- * to be managed differently. The FlutterViewController has the ownership of this plugin.
+ * 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
@@ -26,7 +26,7 @@
 @interface FlutterTextInputPlugin : NSTextView <FlutterKeySecondaryResponder>
 
 /**
- * The native text field this backed by this plugin as its field editor.
+ * The native text field that currently has this plugin as its field editor.
  *
  * Must be nil if accessibility is off.
  */
@@ -38,10 +38,10 @@
 - (instancetype)initWithViewController:(FlutterViewController*)viewController;
 
 /**
- * Where this plugin is the first responder of this NSWindow.
+ * Whether this plugin is the first responder of this NSWindow.
  *
  * When accessibility is on, this plugin is set as the first responder to act as the field
- * editor for FlutterTextField client.
+ * editor for FlutterTextFields.
  *
  * Returns false if accessibility is off.
  */

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

@@ -135,6 +135,20 @@ - (void)handleMethodCall:(FlutterMethodCall*)call result:(FlutterResult)result;
  */
 - (void)setEditingState:(NSDictionary*)state;
 
+/**
+ * Informs the Flutter framework of changes to the text input model's state.
+ */
+- (void)updateEditState;
+
+/**
+ * Updates the stringValue and selectedRange that stored in the NSTextView interface
+ * that this plugin inherits from.
+ *
+ * If there is a FlutterTextField uses this plugin as its field editor, this method
+ * will update the stringValue and selectedRange through the API of the FlutterTextField.
+ */
+- (void)updateTextAndSelection;
+
 @end
 
 @implementation FlutterTextInputPlugin {
@@ -163,6 +177,11 @@ - (instancetype)initWithViewController:(FlutterViewController*)viewController {
                                            binaryMessenger:viewController.engine.binaryMessenger
                                                      codec:[FlutterJSONMethodCodec sharedInstance]];
     _shown = FALSE;
+    // 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
+    // be valid if the handler is ever called.
     __unsafe_unretained FlutterTextInputPlugin* weakSelf = self;
     [_channel setMethodCallHandler:^(FlutterMethodCall* call, FlutterResult result) {
       [weakSelf handleMethodCall:call result:result];
@@ -181,8 +200,10 @@ - (instancetype)initWithViewController:(FlutterViewController*)viewController {
 }
 
 - (BOOL)isFirstResponder {
-  FlutterAppDelegate* appDelegate = (FlutterAppDelegate*)[NSApp delegate];
-  return [appDelegate.mainFlutterWindow firstResponder] == self;
+  if (!self.flutterViewController.viewLoaded || !self.flutterViewController.view.window) {
+    return false;
+  }
+  return [self.flutterViewController.view.window firstResponder] == self;
 }
 
 - (void)dealloc {
@@ -298,15 +319,10 @@ - (void)setEditingState:(NSDictionary*)state {
       state[kComposingBaseKey], state[kComposingExtentKey], _activeModel->composing_range());
   size_t cursor_offset = selected_range.base() - composing_range.start();
   _activeModel->SetComposingRange(composing_range, cursor_offset);
-  if (_client) {
-    [_client becomeFirstResponder];
-  }
+  [_client becomeFirstResponder];
   [self updateTextAndSelection];
 }
 
-/**
- * Informs the Flutter framework of changes to the text input model's state.
- */
 - (void)updateEditState {
   if (_activeModel == nullptr) {
     return;
@@ -328,10 +344,29 @@ - (void)updateEditState {
     kComposingExtentKey : @(composingExtent),
     kTextKey : [NSString stringWithUTF8String:_activeModel->GetText().c_str()]
   };
+
   [_channel invokeMethod:kUpdateEditStateResponseMethod arguments:@[ self.clientID, state ]];
   [self updateTextAndSelection];
 }
 
+- (void)updateTextAndSelection {
+  NSAssert(_activeModel != nullptr, @"Flutter text model must not be null.");
+  NSString* text = @(_activeModel->GetText().data());
+  int start = _activeModel->selection().base();
+  int extend = _activeModel->selection().extent();
+  NSRange selection = NSMakeRange(MIN(start, extend), ABS(start - extend));
+  // There may be a native text field client if VoiceOver is on.
+  // In this case, we have to update text and selection through
+  // the client in order for VoiceOver to announce the text editing
+  // properly.
+  if (_client) {
+    [_client updateString:text withSelection:selection];
+  } else {
+    self.string = text;
+    [self setSelectedRange:selection];
+  }
+}
+
 #pragma mark -
 #pragma mark FlutterKeySecondaryResponder
 
@@ -357,117 +392,74 @@ - (BOOL)handleKeyEvent:(NSEvent*)event {
   return [_textInputContext handleEvent:event];
 }
 
+#pragma mark -
 #pragma mark NSResponder
 
 - (void)keyDown:(NSEvent*)event {
-  if (!self.flutterViewController) {
-    return;
-  }
   [self.flutterViewController keyDown:event];
 }
 
 - (void)keyUp:(NSEvent*)event {
-  if (!self.flutterViewController) {
-    return;
-  }
   [self.flutterViewController keyUp:event];
 }
 
+- (BOOL)performKeyEquivalent:(NSEvent*)event {
+  return [self.flutterViewController performKeyEquivalent:event];
+}
+
 - (void)flagsChanged:(NSEvent*)event {
-  if (!self.flutterViewController) {
-    return;
-  }
   [self.flutterViewController flagsChanged:event];
 }
 
 - (void)mouseEntered:(NSEvent*)event {
-  if (!self.flutterViewController) {
-    return;
-  }
   [self.flutterViewController mouseEntered:event];
 }
 
 - (void)mouseExited:(NSEvent*)event {
-  if (!self.flutterViewController) {
-    return;
-  }
   [self.flutterViewController mouseExited:event];
 }
 
 - (void)mouseDown:(NSEvent*)event {
-  if (!self.flutterViewController) {
-    return;
-  }
   [self.flutterViewController mouseDown:event];
 }
 
 - (void)mouseUp:(NSEvent*)event {
-  if (!self.flutterViewController) {
-    return;
-  }
   [self.flutterViewController mouseUp:event];
 }
 
 - (void)mouseDragged:(NSEvent*)event {
-  if (!self.flutterViewController) {
-    return;
-  }
   [self.flutterViewController mouseDragged:event];
 }
 
 - (void)rightMouseDown:(NSEvent*)event {
-  if (!self.flutterViewController) {
-    return;
-  }
   [self.flutterViewController rightMouseDown:event];
 }
 
 - (void)rightMouseUp:(NSEvent*)event {
-  if (!self.flutterViewController) {
-    return;
-  }
   [self.flutterViewController rightMouseUp:event];
 }
 
 - (void)rightMouseDragged:(NSEvent*)event {
-  if (!self.flutterViewController) {
-    return;
-  }
   [self.flutterViewController rightMouseDragged:event];
 }
 
 - (void)otherMouseDown:(NSEvent*)event {
-  if (!self.flutterViewController) {
-    return;
-  }
   [self.flutterViewController otherMouseDown:event];
 }
 
 - (void)otherMouseUp:(NSEvent*)event {
-  if (!self.flutterViewController) {
-    return;
-  }
   [self.flutterViewController otherMouseUp:event];
 }
 
 - (void)otherMouseDragged:(NSEvent*)event {
-  if (!self.flutterViewController) {
-    return;
-  }
   [self.flutterViewController otherMouseDragged:event];
 }
 
 - (void)mouseMoved:(NSEvent*)event {
-  if (!self.flutterViewController) {
-    return;
-  }
   [self.flutterViewController mouseMoved:event];
 }
 
 - (void)scrollWheel:(NSEvent*)event {
-  if (!self.flutterViewController) {
-    return;
-  }
   [self.flutterViewController scrollWheel:event];
 }
 
@@ -553,24 +545,6 @@ - (void)unmarkText {
   [self updateEditState];
 }
 
-- (void)updateTextAndSelection {
-  NSAssert(_activeModel != nullptr, @"Flutter text model must not be null.");
-  NSString* text = @(_activeModel->GetText().data());
-  int start = _activeModel->selection().base();
-  int extend = _activeModel->selection().extent();
-  NSRange selection = NSMakeRange(MIN(start, extend), ABS(start - extend));
-  // There may be a native text field client if voiceover is on.
-  // In this case, we have to update text and selection through
-  // the client in order for voiceover to announce the text editing
-  // properly.
-  if (_client) {
-    [_client updateString:text withSelection:selection];
-  } else {
-    self.string = text;
-    [self setSelectedRange:selection];
-  }
-}
-
 - (NSRange)markedRange {
   if (_activeModel == nullptr) {
     return NSMakeRange(NSNotFound, 0);
@@ -602,6 +576,9 @@ - (NSAttributedString*)attributedSubstringForProposedRange:(NSRange)range
 }
 
 - (NSRect)firstRectForCharacterRange:(NSRange)range actualRange:(NSRangePointer)actualRange {
+  if (!self.flutterViewController.viewLoaded) {
+    return CGRectZero;
+  }
   // This only determines position of caret instead of any arbitrary range, but it's enough
   // to properly position accent selection popup
   if (CATransform3DIsAffine(_editableTransform) && !CGRectEqualToRect(_caretRect, CGRectNull)) {