From 60335376ac6f54a0daaa5ce6e3cc691ba7fdfbed Mon Sep 17 00:00:00 2001 From: Chun-Heng Tai Date: Tue, 8 Sep 2020 15:48:53 -0700 Subject: [PATCH 1/3] Fixed ios layout change to not refocus semantics object if the focus is outside of flutter --- .../ios/framework/Source/SemanticsObject.mm | 3 +- .../framework/Source/SemanticsObjectTest.mm | 3 +- .../framework/Source/accessibility_bridge.h | 6 +- .../framework/Source/accessibility_bridge.mm | 34 +++++--- .../Source/accessibility_bridge_ios.h | 11 ++- .../Source/accessibility_bridge_test.mm | 80 ++++++++++++++++++- 6 files changed, 117 insertions(+), 20 deletions(-) diff --git a/shell/platform/darwin/ios/framework/Source/SemanticsObject.mm b/shell/platform/darwin/ios/framework/Source/SemanticsObject.mm index 1470bca82270a..ba2f8abbd4dc2 100644 --- a/shell/platform/darwin/ios/framework/Source/SemanticsObject.mm +++ b/shell/platform/darwin/ios/framework/Source/SemanticsObject.mm @@ -453,7 +453,7 @@ - (BOOL)accessibilityPerformEscape { - (void)accessibilityElementDidBecomeFocused { if (![self isAccessibilityBridgeAlive]) return; - [self bridge]->AccessibilityFocusDidChange([self uid]); + [self bridge]->AccessibilityObjectDidBecomeFocused([self uid]); if ([self node].HasFlag(flutter::SemanticsFlags::kIsHidden) || [self node].HasFlag(flutter::SemanticsFlags::kIsHeader)) { [self bridge]->DispatchSemanticsAction([self uid], flutter::SemanticsAction::kShowOnScreen); @@ -467,6 +467,7 @@ - (void)accessibilityElementDidBecomeFocused { - (void)accessibilityElementDidLoseFocus { if (![self isAccessibilityBridgeAlive]) return; + [self bridge]->AccessibilityObjectDidLoseFocus([self uid]); if ([self node].HasAction(flutter::SemanticsAction::kDidLoseAccessibilityFocus)) { [self bridge]->DispatchSemanticsAction([self uid], flutter::SemanticsAction::kDidLoseAccessibilityFocus); diff --git a/shell/platform/darwin/ios/framework/Source/SemanticsObjectTest.mm b/shell/platform/darwin/ios/framework/Source/SemanticsObjectTest.mm index 8b23f8d9369f0..ee5540508ff39 100644 --- a/shell/platform/darwin/ios/framework/Source/SemanticsObjectTest.mm +++ b/shell/platform/darwin/ios/framework/Source/SemanticsObjectTest.mm @@ -33,7 +33,8 @@ void DispatchSemanticsAction(int32_t id, SemanticsActionObservation observation(id, action); observations.push_back(observation); } - void AccessibilityFocusDidChange(int32_t id) override {} + void AccessibilityObjectDidBecomeFocused(int32_t id) override {} + void AccessibilityObjectDidLoseFocus(int32_t id) override {} FlutterPlatformViewsController* GetPlatformViewsController() const override { return nil; } std::vector observations; diff --git a/shell/platform/darwin/ios/framework/Source/accessibility_bridge.h b/shell/platform/darwin/ios/framework/Source/accessibility_bridge.h index 098be11cd210d..65ca0a9834fc6 100644 --- a/shell/platform/darwin/ios/framework/Source/accessibility_bridge.h +++ b/shell/platform/darwin/ios/framework/Source/accessibility_bridge.h @@ -61,7 +61,8 @@ class AccessibilityBridge final : public AccessibilityBridgeIos { void DispatchSemanticsAction(int32_t id, flutter::SemanticsAction action, std::vector args) override; - void AccessibilityFocusDidChange(int32_t id) override; + void AccessibilityObjectDidBecomeFocused(int32_t id) override; + void AccessibilityObjectDidLoseFocus(int32_t id) override; UIView* textInputView() override; @@ -85,6 +86,9 @@ class AccessibilityBridge final : public AccessibilityBridgeIos { FlutterViewController* view_controller_; PlatformViewIOS* platform_view_; FlutterPlatformViewsController* platform_views_controller_; + // If the this id is -1, it means either nothing has been focused or the + // focus is currently outside of the flutter application (status bar or + // the keyboard) int32_t last_focused_semantics_object_id_; fml::scoped_nsobject> objects_; fml::scoped_nsprotocol accessibility_channel_; diff --git a/shell/platform/darwin/ios/framework/Source/accessibility_bridge.mm b/shell/platform/darwin/ios/framework/Source/accessibility_bridge.mm index eee0ac4823469..037b7168b4fd1 100644 --- a/shell/platform/darwin/ios/framework/Source/accessibility_bridge.mm +++ b/shell/platform/darwin/ios/framework/Source/accessibility_bridge.mm @@ -41,7 +41,7 @@ void PostAccessibilityNotification(UIAccessibilityNotifications notification, : view_controller_(view_controller), platform_view_(platform_view), platform_views_controller_(platform_views_controller), - last_focused_semantics_object_id_(0), + last_focused_semantics_object_id_(-1), objects_([[NSMutableDictionary alloc] init]), weak_factory_(this), previous_route_id_(0), @@ -67,10 +67,15 @@ void PostAccessibilityNotification(UIAccessibilityNotifications notification, return [[platform_view_->GetOwnerViewController().get().engine textInputPlugin] textInputView]; } -void AccessibilityBridge::AccessibilityFocusDidChange(int32_t id) { +void AccessibilityBridge::AccessibilityObjectDidBecomeFocused(int32_t id) { last_focused_semantics_object_id_ = id; } +void AccessibilityBridge::AccessibilityObjectDidLoseFocus(int32_t id) { + if (last_focused_semantics_object_id_ == id) + last_focused_semantics_object_id_ = -1; +} + void AccessibilityBridge::UpdateSemantics(flutter::SemanticsNodeUpdates nodes, flutter::CustomAccessibilityActionUpdates actions) { BOOL layoutChanged = NO; @@ -169,6 +174,7 @@ void PostAccessibilityNotification(UIAccessibilityNotifications notification, int index = [newRoutes count] - 1; lastAdded = [newRoutes objectAtIndex:index]; } + if (lastAdded != nil && [lastAdded uid] != previous_route_id_) { previous_route_id_ = [lastAdded uid]; routeChanged = true; @@ -198,11 +204,15 @@ void PostAccessibilityNotification(UIAccessibilityNotifications notification, nextToFocus); } } else if (layoutChanged) { - // Tries to refocus the previous focused semantics object to avoid random jumps. - SemanticsObject* nextToFocus = - [objects_.get() objectForKey:@(last_focused_semantics_object_id_)]; - if (!nextToFocus && root) { - nextToFocus = FindFirstFocusable(root); + SemanticsObject* nextToFocus = nil; + // This property will be -1 if the focus is outside of the flutter + // application. In this case, we should not try to refocus anything. + if (last_focused_semantics_object_id_ != -1) { + // Tries to refocus the previous focused semantics object to avoid random jumps. + nextToFocus = [objects_.get() objectForKey:@(last_focused_semantics_object_id_)]; + if (!nextToFocus && root) { + nextToFocus = FindFirstFocusable(root); + } } ios_delegate_->PostAccessibilityNotification(UIAccessibilityLayoutChangedNotification, nextToFocus); @@ -210,10 +220,12 @@ void PostAccessibilityNotification(UIAccessibilityNotifications notification, // TODO(chunhtai): figure out what string to use for notification. At this // point, it is guarantee the previous focused object is still in the tree // so that we don't need to worry about focus lost. (e.g. "Screen 0 of 3") - SemanticsObject* nextToFocus = - [objects_.get() objectForKey:@(last_focused_semantics_object_id_)]; - if (!nextToFocus && root) { - nextToFocus = FindFirstFocusable(root); + SemanticsObject* nextToFocus = nil; + if (last_focused_semantics_object_id_ != -1) { + nextToFocus = [objects_.get() objectForKey:@(last_focused_semantics_object_id_)]; + if (!nextToFocus && root) { + nextToFocus = FindFirstFocusable(root); + } } ios_delegate_->PostAccessibilityNotification(UIAccessibilityPageScrolledNotification, nextToFocus); diff --git a/shell/platform/darwin/ios/framework/Source/accessibility_bridge_ios.h b/shell/platform/darwin/ios/framework/Source/accessibility_bridge_ios.h index 19b49140edc54..3b28a9529241e 100644 --- a/shell/platform/darwin/ios/framework/Source/accessibility_bridge_ios.h +++ b/shell/platform/darwin/ios/framework/Source/accessibility_bridge_ios.h @@ -25,12 +25,17 @@ class AccessibilityBridgeIos { flutter::SemanticsAction action, std::vector args) = 0; /** - * A callback that is called after the accessibility focus has moved to a new - * SemanticObject. + * A callback that is called when a SemanticObject receives focus. * * The input id is the uid of the newly focused SemanticObject. */ - virtual void AccessibilityFocusDidChange(int32_t id) = 0; + virtual void AccessibilityObjectDidBecomeFocused(int32_t id) = 0; + /** + * A callback that is called when a SemanticObject loses focus + * + * The input id is the uid of the newly focused SemanticObject. + */ + virtual void AccessibilityObjectDidLoseFocus(int32_t id) = 0; virtual FlutterPlatformViewsController* GetPlatformViewsController() const = 0; }; diff --git a/shell/platform/darwin/ios/framework/Source/accessibility_bridge_test.mm b/shell/platform/darwin/ios/framework/Source/accessibility_bridge_test.mm index 01bca1cd6b7f9..e554cb3331d24 100644 --- a/shell/platform/darwin/ios/framework/Source/accessibility_bridge_test.mm +++ b/shell/platform/darwin/ios/framework/Source/accessibility_bridge_test.mm @@ -450,7 +450,7 @@ - (void)testAnnouncesLayoutChangeWithNilIfLastFocusIsRemoved { XCTAssertEqual([accessibility_notifications count], 0ul); // Simulates the focusing on the node 1. - bridge->AccessibilityFocusDidChange(1); + bridge->AccessibilityObjectDidBecomeFocused(1); flutter::SemanticsNodeUpdates second_update; // Simulates the removal of the node 1 @@ -520,7 +520,7 @@ - (void)testAnnouncesLayoutChangeWithLastFocused { XCTAssertEqual([accessibility_notifications count], 0ul); // Simulates the focusing on the node 1. - bridge->AccessibilityFocusDidChange(1); + bridge->AccessibilityObjectDidBecomeFocused(1); flutter::SemanticsNodeUpdates second_update; // Simulates the removal of the node 2. @@ -539,6 +539,80 @@ - (void)testAnnouncesLayoutChangeWithLastFocused { UIAccessibilityLayoutChangedNotification); } +- (void)testAnnouncesLayoutChangeWhenFocusMovedOutside { + flutter::MockDelegate mock_delegate; + auto thread_task_runner = CreateNewThread("AccessibilityBridgeTest"); + flutter::TaskRunners runners(/*label=*/self.name.UTF8String, + /*platform=*/thread_task_runner, + /*raster=*/thread_task_runner, + /*ui=*/thread_task_runner, + /*io=*/thread_task_runner); + auto platform_view = std::make_unique( + /*delegate=*/mock_delegate, + /*rendering_api=*/flutter::IOSRenderingAPI::kSoftware, + /*task_runners=*/runners); + id mockFlutterViewController = OCMClassMock([FlutterViewController class]); + id mockFlutterView = OCMClassMock([FlutterView class]); + OCMStub([mockFlutterViewController view]).andReturn(mockFlutterView); + + NSMutableArray*>* accessibility_notifications = + [[[NSMutableArray alloc] init] autorelease]; + auto ios_delegate = std::make_unique(); + ios_delegate->on_PostAccessibilityNotification_ = + [accessibility_notifications](UIAccessibilityNotifications notification, id argument) { + [accessibility_notifications addObject:@{ + @"notification" : @(notification), + @"argument" : argument ? argument : [NSNull null], + }]; + }; + __block auto bridge = + std::make_unique(/*view_controller=*/mockFlutterViewController, + /*platform_view=*/platform_view.get(), + /*platform_views_controller=*/nil, + /*ios_delegate=*/std::move(ios_delegate)); + + flutter::CustomAccessibilityActionUpdates actions; + flutter::SemanticsNodeUpdates first_update; + + flutter::SemanticsNode node_one; + node_one.id = 1; + node_one.label = "route1"; + first_update[node_one.id] = node_one; + flutter::SemanticsNode node_two; + node_two.id = 2; + node_two.label = "route2"; + first_update[node_two.id] = node_two; + flutter::SemanticsNode root_node; + root_node.id = kRootNodeId; + root_node.label = "root"; + root_node.childrenInTraversalOrder = {1, 2}; + root_node.childrenInHitTestOrder = {1, 2}; + first_update[root_node.id] = root_node; + bridge->UpdateSemantics(/*nodes=*/first_update, /*actions=*/actions); + + XCTAssertEqual([accessibility_notifications count], 0ul); + // Simulates the focusing on the node 1. + bridge->AccessibilityObjectDidBecomeFocused(1); + // Simulates that the focus move outside of flutter. + bridge->AccessibilityObjectDidLoseFocus(1); + + flutter::SemanticsNodeUpdates second_update; + // Simulates the removal of the node 2. + flutter::SemanticsNode new_root_node; + new_root_node.id = kRootNodeId; + new_root_node.label = "root"; + new_root_node.childrenInTraversalOrder = {1}; + new_root_node.childrenInHitTestOrder = {1}; + second_update[root_node.id] = new_root_node; + bridge->UpdateSemantics(/*nodes=*/second_update, /*actions=*/actions); + NSNull* focusObject = accessibility_notifications[0][@"argument"]; + // Since the focus is moved outside of the app right before the layout + // changed, the bridge should not try to refocus anything . + XCTAssertEqual(focusObject, [NSNull null]); + XCTAssertEqual([accessibility_notifications[0][@"notification"] unsignedIntValue], + UIAccessibilityLayoutChangedNotification); +} + - (void)testAnnouncesScrollChangeWithLastFocused { flutter::MockDelegate mock_delegate; auto thread_task_runner = CreateNewThread("AccessibilityBridgeTest"); @@ -591,7 +665,7 @@ - (void)testAnnouncesScrollChangeWithLastFocused { [accessibility_notifications removeAllObjects]; // Simulates the focusing on the node 1. - bridge->AccessibilityFocusDidChange(1); + bridge->AccessibilityObjectDidBecomeFocused(1); flutter::SemanticsNodeUpdates second_update; // Simulates the scrolling on the node 1. From 71fa7a9914bdb2a29a87d0b4b39d5c3f43c59bee Mon Sep 17 00:00:00 2001 From: Chun-Heng Tai Date: Tue, 8 Sep 2020 16:03:39 -0700 Subject: [PATCH 2/3] update --- .../darwin/ios/framework/Source/accessibility_bridge.h | 4 ++-- .../darwin/ios/framework/Source/accessibility_bridge.mm | 3 +-- 2 files changed, 3 insertions(+), 4 deletions(-) diff --git a/shell/platform/darwin/ios/framework/Source/accessibility_bridge.h b/shell/platform/darwin/ios/framework/Source/accessibility_bridge.h index 65ca0a9834fc6..896a2c622e7ab 100644 --- a/shell/platform/darwin/ios/framework/Source/accessibility_bridge.h +++ b/shell/platform/darwin/ios/framework/Source/accessibility_bridge.h @@ -87,8 +87,8 @@ class AccessibilityBridge final : public AccessibilityBridgeIos { PlatformViewIOS* platform_view_; FlutterPlatformViewsController* platform_views_controller_; // If the this id is -1, it means either nothing has been focused or the - // focus is currently outside of the flutter application (status bar or - // the keyboard) + // focus is currently outside of the flutter application (i.e. the status bar + // or keyboard) int32_t last_focused_semantics_object_id_; fml::scoped_nsobject> objects_; fml::scoped_nsprotocol accessibility_channel_; diff --git a/shell/platform/darwin/ios/framework/Source/accessibility_bridge.mm b/shell/platform/darwin/ios/framework/Source/accessibility_bridge.mm index 037b7168b4fd1..d5f834b5dc364 100644 --- a/shell/platform/darwin/ios/framework/Source/accessibility_bridge.mm +++ b/shell/platform/darwin/ios/framework/Source/accessibility_bridge.mm @@ -174,7 +174,6 @@ void PostAccessibilityNotification(UIAccessibilityNotifications notification, int index = [newRoutes count] - 1; lastAdded = [newRoutes objectAtIndex:index]; } - if (lastAdded != nil && [lastAdded uid] != previous_route_id_) { previous_route_id_ = [lastAdded uid]; routeChanged = true; @@ -206,7 +205,7 @@ void PostAccessibilityNotification(UIAccessibilityNotifications notification, } else if (layoutChanged) { SemanticsObject* nextToFocus = nil; // This property will be -1 if the focus is outside of the flutter - // application. In this case, we should not try to refocus anything. + // application. In this case, we should not refocus anything. if (last_focused_semantics_object_id_ != -1) { // Tries to refocus the previous focused semantics object to avoid random jumps. nextToFocus = [objects_.get() objectForKey:@(last_focused_semantics_object_id_)]; From 01e24d6a974918f255387c4e1a0f7d1b576d66d2 Mon Sep 17 00:00:00 2001 From: Chun-Heng Tai Date: Wed, 9 Sep 2020 11:45:29 -0700 Subject: [PATCH 3/3] addressing comments --- .../ios/framework/Source/accessibility_bridge.h | 6 +++--- .../ios/framework/Source/accessibility_bridge.mm | 13 ++++++++----- 2 files changed, 11 insertions(+), 8 deletions(-) diff --git a/shell/platform/darwin/ios/framework/Source/accessibility_bridge.h b/shell/platform/darwin/ios/framework/Source/accessibility_bridge.h index 896a2c622e7ab..3d0fdeb84092c 100644 --- a/shell/platform/darwin/ios/framework/Source/accessibility_bridge.h +++ b/shell/platform/darwin/ios/framework/Source/accessibility_bridge.h @@ -86,9 +86,9 @@ class AccessibilityBridge final : public AccessibilityBridgeIos { FlutterViewController* view_controller_; PlatformViewIOS* platform_view_; FlutterPlatformViewsController* platform_views_controller_; - // If the this id is -1, it means either nothing has been focused or the - // focus is currently outside of the flutter application (i.e. the status bar - // or keyboard) + // If the this id is kSemanticObjectIdInvalid, it means either nothing has + // been focused or the focus is currently outside of the flutter application + // (i.e. the status bar or keyboard) int32_t last_focused_semantics_object_id_; fml::scoped_nsobject> objects_; fml::scoped_nsprotocol accessibility_channel_; diff --git a/shell/platform/darwin/ios/framework/Source/accessibility_bridge.mm b/shell/platform/darwin/ios/framework/Source/accessibility_bridge.mm index d5f834b5dc364..ca4b2618a62f0 100644 --- a/shell/platform/darwin/ios/framework/Source/accessibility_bridge.mm +++ b/shell/platform/darwin/ios/framework/Source/accessibility_bridge.mm @@ -16,6 +16,8 @@ namespace flutter { namespace { +constexpr int32_t kSemanticObjectIdInvalid = -1; + class DefaultIosDelegate : public AccessibilityBridge::IosDelegate { public: bool IsFlutterViewControllerPresentingModalViewController( @@ -41,7 +43,7 @@ void PostAccessibilityNotification(UIAccessibilityNotifications notification, : view_controller_(view_controller), platform_view_(platform_view), platform_views_controller_(platform_views_controller), - last_focused_semantics_object_id_(-1), + last_focused_semantics_object_id_(kSemanticObjectIdInvalid), objects_([[NSMutableDictionary alloc] init]), weak_factory_(this), previous_route_id_(0), @@ -72,8 +74,9 @@ void PostAccessibilityNotification(UIAccessibilityNotifications notification, } void AccessibilityBridge::AccessibilityObjectDidLoseFocus(int32_t id) { - if (last_focused_semantics_object_id_ == id) - last_focused_semantics_object_id_ = -1; + if (last_focused_semantics_object_id_ == id) { + last_focused_semantics_object_id_ = kSemanticObjectIdInvalid; + } } void AccessibilityBridge::UpdateSemantics(flutter::SemanticsNodeUpdates nodes, @@ -206,7 +209,7 @@ void PostAccessibilityNotification(UIAccessibilityNotifications notification, SemanticsObject* nextToFocus = nil; // This property will be -1 if the focus is outside of the flutter // application. In this case, we should not refocus anything. - if (last_focused_semantics_object_id_ != -1) { + if (last_focused_semantics_object_id_ != kSemanticObjectIdInvalid) { // Tries to refocus the previous focused semantics object to avoid random jumps. nextToFocus = [objects_.get() objectForKey:@(last_focused_semantics_object_id_)]; if (!nextToFocus && root) { @@ -220,7 +223,7 @@ void PostAccessibilityNotification(UIAccessibilityNotifications notification, // point, it is guarantee the previous focused object is still in the tree // so that we don't need to worry about focus lost. (e.g. "Screen 0 of 3") SemanticsObject* nextToFocus = nil; - if (last_focused_semantics_object_id_ != -1) { + if (last_focused_semantics_object_id_ != kSemanticObjectIdInvalid) { nextToFocus = [objects_.get() objectForKey:@(last_focused_semantics_object_id_)]; if (!nextToFocus && root) { nextToFocus = FindFirstFocusable(root);