From c6d1b80f3239608af5730b1f76fbec18475adf46 Mon Sep 17 00:00:00 2001 From: Chun-Heng Tai Date: Thu, 30 Jul 2020 17:00:26 -0700 Subject: [PATCH 1/3] fix ios layout change cause the accessibility focus to jump randomly. --- .../ios/framework/Source/SemanticsObject.mm | 1 + .../framework/Source/SemanticsObjectTest.mm | 1 + .../framework/Source/accessibility_bridge.h | 2 + .../framework/Source/accessibility_bridge.mm | 19 +- .../Source/accessibility_bridge_ios.h | 1 + .../Source/accessibility_bridge_test.mm | 202 +++++++++++++++++- 6 files changed, 221 insertions(+), 5 deletions(-) diff --git a/shell/platform/darwin/ios/framework/Source/SemanticsObject.mm b/shell/platform/darwin/ios/framework/Source/SemanticsObject.mm index 2addc2a50932f..1470bca82270a 100644 --- a/shell/platform/darwin/ios/framework/Source/SemanticsObject.mm +++ b/shell/platform/darwin/ios/framework/Source/SemanticsObject.mm @@ -453,6 +453,7 @@ - (BOOL)accessibilityPerformEscape { - (void)accessibilityElementDidBecomeFocused { if (![self isAccessibilityBridgeAlive]) return; + [self bridge]->AccessibilityFocusDidChange([self uid]); if ([self node].HasFlag(flutter::SemanticsFlags::kIsHidden) || [self node].HasFlag(flutter::SemanticsFlags::kIsHeader)) { [self bridge]->DispatchSemanticsAction([self uid], flutter::SemanticsAction::kShowOnScreen); diff --git a/shell/platform/darwin/ios/framework/Source/SemanticsObjectTest.mm b/shell/platform/darwin/ios/framework/Source/SemanticsObjectTest.mm index 3c42aeacb3efb..196cd09272777 100644 --- a/shell/platform/darwin/ios/framework/Source/SemanticsObjectTest.mm +++ b/shell/platform/darwin/ios/framework/Source/SemanticsObjectTest.mm @@ -33,6 +33,7 @@ void DispatchSemanticsAction(int32_t id, SemanticsActionObservation observation(id, action); observations.push_back(observation); } + void AccessibilityFocusDidChange(int32_t focused_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 a1e3c2d7fb18f..c1db1588bd7ce 100644 --- a/shell/platform/darwin/ios/framework/Source/accessibility_bridge.h +++ b/shell/platform/darwin/ios/framework/Source/accessibility_bridge.h @@ -61,6 +61,7 @@ class AccessibilityBridge final : public AccessibilityBridgeIos { void DispatchSemanticsAction(int32_t id, flutter::SemanticsAction action, std::vector args) override; + void AccessibilityFocusDidChange(int32_t focused_id) override; UIView* textInputView() override; @@ -83,6 +84,7 @@ class AccessibilityBridge final : public AccessibilityBridgeIos { FlutterViewController* view_controller_; PlatformViewIOS* platform_view_; FlutterPlatformViewsController* platform_views_controller_; + int32_t last_focused_semantics_object_id_; fml::scoped_nsobject> objects_; fml::scoped_nsprotocol accessibility_channel_; fml::WeakPtrFactory weak_factory_; diff --git a/shell/platform/darwin/ios/framework/Source/accessibility_bridge.mm b/shell/platform/darwin/ios/framework/Source/accessibility_bridge.mm index 0d42cce730a9b..89e3d3d134dd2 100644 --- a/shell/platform/darwin/ios/framework/Source/accessibility_bridge.mm +++ b/shell/platform/darwin/ios/framework/Source/accessibility_bridge.mm @@ -41,6 +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), objects_([[NSMutableDictionary alloc] init]), weak_factory_(this), previous_route_id_(0), @@ -66,6 +67,10 @@ void PostAccessibilityNotification(UIAccessibilityNotifications notification, return [[platform_view_->GetOwnerViewController().get().engine textInputPlugin] textInputView]; } +void AccessibilityBridge::AccessibilityFocusDidChange(int32_t focused_id) { + last_focused_semantics_object_id_ = focused_id; +} + void AccessibilityBridge::UpdateSemantics(flutter::SemanticsNodeUpdates nodes, flutter::CustomAccessibilityActionUpdates actions) { BOOL layoutChanged = NO; @@ -188,12 +193,18 @@ void PostAccessibilityNotification(UIAccessibilityNotifications notification, [lastAdded routeFocusObject]); } } else if (layoutChanged) { - // TODO(goderbauer): figure out which node to focus next. - ios_delegate_->PostAccessibilityNotification(UIAccessibilityLayoutChangedNotification, nil); + // Tries to refocus the previous focused semantics object to avoid random jumps. + ios_delegate_->PostAccessibilityNotification( + UIAccessibilityLayoutChangedNotification, + [objects_.get() objectForKey:@(last_focused_semantics_object_id_)] + ); } if (scrollOccured) { - // TODO(tvolkert): provide meaningful string (e.g. "page 2 of 5") - ios_delegate_->PostAccessibilityNotification(UIAccessibilityPageScrolledNotification, @""); + // Tries to refocus the previous focused semantics object to avoid random jumps. + ios_delegate_->PostAccessibilityNotification( + UIAccessibilityPageScrolledNotification, + [objects_.get() objectForKey:@(last_focused_semantics_object_id_)] + ); } } 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 c2546ac7c3a2c..8cfa7cdecc338 100644 --- a/shell/platform/darwin/ios/framework/Source/accessibility_bridge_ios.h +++ b/shell/platform/darwin/ios/framework/Source/accessibility_bridge_ios.h @@ -24,6 +24,7 @@ class AccessibilityBridgeIos { virtual void DispatchSemanticsAction(int32_t id, flutter::SemanticsAction action, std::vector args) = 0; + virtual void AccessibilityFocusDidChange(int32_t focused_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 300c30b47bfe6..900b805825afe 100644 --- a/shell/platform/darwin/ios/framework/Source/accessibility_bridge_test.mm +++ b/shell/platform/darwin/ios/framework/Source/accessibility_bridge_test.mm @@ -306,7 +306,6 @@ - (void)testAnnouncesRouteChanges { flutter::SemanticsNode route_node; route_node.id = 1; - route_node.label = label; route_node.flags = static_cast(flutter::SemanticsFlags::kScopesRoute) | static_cast(flutter::SemanticsFlags::kNamesRoute); route_node.label = "route"; @@ -327,6 +326,207 @@ - (void)testAnnouncesRouteChanges { UIAccessibilityScreenChangedNotification); } +- (void)testAnnouncesLayoutChangeWithNilIfLastFocusIsRemoved{ + 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 mockFlutterView = OCMClassMock([FlutterView class]); + + 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=*/mockFlutterView, + /*platform_view=*/platform_view.get(), + /*platform_views_controller=*/nil, + /*ios_delegate=*/std::move(ios_delegate)); + + flutter::CustomAccessibilityActionUpdates actions; + flutter::SemanticsNodeUpdates first_update; + + flutter::SemanticsNode route_node; + route_node.id = 1; + route_node.label = "route"; + first_update[route_node.id] = route_node; + flutter::SemanticsNode root_node; + root_node.id = kRootNodeId; + root_node.label = "root"; + root_node.childrenInTraversalOrder = {1}; + root_node.childrenInHitTestOrder = {1}; + 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->AccessibilityFocusDidChange(1); + + flutter::SemanticsNodeUpdates second_update; + // Simulates the removal of the node 1 + flutter::SemanticsNode new_root_node; + new_root_node.id = kRootNodeId; + new_root_node.label = "root"; + second_update[root_node.id] = new_root_node; + bridge->UpdateSemantics(/*nodes=*/second_update, /*actions=*/actions); + NSNull* focusObject = accessibility_notifications[0][@"argument"]; + // The node 1 was removed, so the bridge will set the focus object to nil. + XCTAssertEqual(focusObject, [NSNull null]); + XCTAssertEqual([accessibility_notifications[0][@"notification"] unsignedIntValue], + UIAccessibilityLayoutChangedNotification); +} + +- (void)testAnnouncesLayoutChangeWithLastFocused { + 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 mockFlutterView = OCMClassMock([FlutterView class]); + + 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=*/mockFlutterView, + /*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->AccessibilityFocusDidChange(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); + SemanticsObject* focusObject = accessibility_notifications[0][@"argument"]; + // Since we have focused on the node 1 right before the layout changed, the bridge should refocus + // the node 1. + XCTAssertEqual([focusObject uid], 1); + XCTAssertEqual([accessibility_notifications[0][@"notification"] unsignedIntValue], + UIAccessibilityLayoutChangedNotification); +} + +- (void)testAnnouncesScrollChangeWithLastFocused { + 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 mockFlutterView = OCMClassMock([FlutterView class]); + + 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=*/mockFlutterView, + /*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"; + node_one.scrollPosition = 0.0; + first_update[node_one.id] = node_one; + flutter::SemanticsNode root_node; + root_node.id = kRootNodeId; + root_node.label = "root"; + root_node.childrenInTraversalOrder = {1}; + root_node.childrenInHitTestOrder = {1}; + first_update[root_node.id] = root_node; + bridge->UpdateSemantics(/*nodes=*/first_update, /*actions=*/actions); + + // The first update will trigger a scroll announcement, but we are not interested in it. + [accessibility_notifications removeAllObjects]; + + // Simulates the focusing on the node 1. + bridge->AccessibilityFocusDidChange(1); + + flutter::SemanticsNodeUpdates second_update; + // Simulates the scrolling on the node 1. + flutter::SemanticsNode new_node_one; + new_node_one.id = 1; + new_node_one.label = "route1"; + new_node_one.scrollPosition = 1.0; + second_update[new_node_one.id] = new_node_one; + bridge->UpdateSemantics(/*nodes=*/second_update, /*actions=*/actions); + SemanticsObject* focusObject = accessibility_notifications[0][@"argument"]; + // Since we have focused on the node 1 right before the scrolling, the bridge should refocus the + // node 1. + XCTAssertEqual([focusObject uid], 1); + XCTAssertEqual([accessibility_notifications[0][@"notification"] unsignedIntValue], + UIAccessibilityPageScrolledNotification); +} + - (void)testAnnouncesIgnoresRouteChangesWhenModal { flutter::MockDelegate mock_delegate; auto thread_task_runner = CreateNewThread("AccessibilityBridgeTest"); From d0c54a7c2d5a9280ac7ebf8645ec6282c4c51f83 Mon Sep 17 00:00:00 2001 From: Chun-Heng Tai Date: Fri, 31 Jul 2020 12:30:55 -0700 Subject: [PATCH 2/3] format --- .../ios/framework/Source/accessibility_bridge.mm | 10 ++++------ .../ios/framework/Source/accessibility_bridge_test.mm | 2 +- 2 files changed, 5 insertions(+), 7 deletions(-) diff --git a/shell/platform/darwin/ios/framework/Source/accessibility_bridge.mm b/shell/platform/darwin/ios/framework/Source/accessibility_bridge.mm index 89e3d3d134dd2..b48bff9875fe6 100644 --- a/shell/platform/darwin/ios/framework/Source/accessibility_bridge.mm +++ b/shell/platform/darwin/ios/framework/Source/accessibility_bridge.mm @@ -195,16 +195,14 @@ void PostAccessibilityNotification(UIAccessibilityNotifications notification, } else if (layoutChanged) { // Tries to refocus the previous focused semantics object to avoid random jumps. ios_delegate_->PostAccessibilityNotification( - UIAccessibilityLayoutChangedNotification, - [objects_.get() objectForKey:@(last_focused_semantics_object_id_)] - ); + UIAccessibilityLayoutChangedNotification, + [objects_.get() objectForKey:@(last_focused_semantics_object_id_)]); } if (scrollOccured) { // Tries to refocus the previous focused semantics object to avoid random jumps. ios_delegate_->PostAccessibilityNotification( - UIAccessibilityPageScrolledNotification, - [objects_.get() objectForKey:@(last_focused_semantics_object_id_)] - ); + UIAccessibilityPageScrolledNotification, + [objects_.get() objectForKey:@(last_focused_semantics_object_id_)]); } } 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 900b805825afe..affabb9d3baab 100644 --- a/shell/platform/darwin/ios/framework/Source/accessibility_bridge_test.mm +++ b/shell/platform/darwin/ios/framework/Source/accessibility_bridge_test.mm @@ -326,7 +326,7 @@ - (void)testAnnouncesRouteChanges { UIAccessibilityScreenChangedNotification); } -- (void)testAnnouncesLayoutChangeWithNilIfLastFocusIsRemoved{ +- (void)testAnnouncesLayoutChangeWithNilIfLastFocusIsRemoved { flutter::MockDelegate mock_delegate; auto thread_task_runner = CreateNewThread("AccessibilityBridgeTest"); flutter::TaskRunners runners(/*label=*/self.name.UTF8String, From 55cca605abcb7bf0dc36b54608553c0e820f5d56 Mon Sep 17 00:00:00 2001 From: Chun-Heng Tai Date: Wed, 5 Aug 2020 12:22:28 -0700 Subject: [PATCH 3/3] addressing review comments --- .../darwin/ios/framework/Source/SemanticsObjectTest.mm | 2 +- .../darwin/ios/framework/Source/accessibility_bridge.h | 2 +- .../darwin/ios/framework/Source/accessibility_bridge.mm | 4 ++-- .../ios/framework/Source/accessibility_bridge_ios.h | 8 +++++++- 4 files changed, 11 insertions(+), 5 deletions(-) diff --git a/shell/platform/darwin/ios/framework/Source/SemanticsObjectTest.mm b/shell/platform/darwin/ios/framework/Source/SemanticsObjectTest.mm index 196cd09272777..8b23f8d9369f0 100644 --- a/shell/platform/darwin/ios/framework/Source/SemanticsObjectTest.mm +++ b/shell/platform/darwin/ios/framework/Source/SemanticsObjectTest.mm @@ -33,7 +33,7 @@ void DispatchSemanticsAction(int32_t id, SemanticsActionObservation observation(id, action); observations.push_back(observation); } - void AccessibilityFocusDidChange(int32_t focused_id) override {} + void AccessibilityFocusDidChange(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 c1db1588bd7ce..758b63fdf7545 100644 --- a/shell/platform/darwin/ios/framework/Source/accessibility_bridge.h +++ b/shell/platform/darwin/ios/framework/Source/accessibility_bridge.h @@ -61,7 +61,7 @@ class AccessibilityBridge final : public AccessibilityBridgeIos { void DispatchSemanticsAction(int32_t id, flutter::SemanticsAction action, std::vector args) override; - void AccessibilityFocusDidChange(int32_t focused_id) override; + void AccessibilityFocusDidChange(int32_t id) override; UIView* textInputView() override; diff --git a/shell/platform/darwin/ios/framework/Source/accessibility_bridge.mm b/shell/platform/darwin/ios/framework/Source/accessibility_bridge.mm index b48bff9875fe6..d8f99a3b198b4 100644 --- a/shell/platform/darwin/ios/framework/Source/accessibility_bridge.mm +++ b/shell/platform/darwin/ios/framework/Source/accessibility_bridge.mm @@ -67,8 +67,8 @@ void PostAccessibilityNotification(UIAccessibilityNotifications notification, return [[platform_view_->GetOwnerViewController().get().engine textInputPlugin] textInputView]; } -void AccessibilityBridge::AccessibilityFocusDidChange(int32_t focused_id) { - last_focused_semantics_object_id_ = focused_id; +void AccessibilityBridge::AccessibilityFocusDidChange(int32_t id) { + last_focused_semantics_object_id_ = id; } void AccessibilityBridge::UpdateSemantics(flutter::SemanticsNodeUpdates nodes, 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 8cfa7cdecc338..19b49140edc54 100644 --- a/shell/platform/darwin/ios/framework/Source/accessibility_bridge_ios.h +++ b/shell/platform/darwin/ios/framework/Source/accessibility_bridge_ios.h @@ -24,7 +24,13 @@ class AccessibilityBridgeIos { virtual void DispatchSemanticsAction(int32_t id, flutter::SemanticsAction action, std::vector args) = 0; - virtual void AccessibilityFocusDidChange(int32_t focused_id) = 0; + /** + * A callback that is called after the accessibility focus has moved to a new + * SemanticObject. + * + * The input id is the uid of the newly focused SemanticObject. + */ + virtual void AccessibilityFocusDidChange(int32_t id) = 0; virtual FlutterPlatformViewsController* GetPlatformViewsController() const = 0; };