From 8072a110735d2c91fbca1dcac5d752140924eb20 Mon Sep 17 00:00:00 2001 From: Chun-Heng Tai Date: Fri, 21 Aug 2020 14:41:04 -0700 Subject: [PATCH 1/4] Make IOS accessibility announcement more robust --- .../framework/Source/accessibility_bridge.h | 6 +- .../framework/Source/accessibility_bridge.mm | 48 ++++++--- .../Source/accessibility_bridge_test.mm | 102 +++++++++++++++--- 3 files changed, 126 insertions(+), 30 deletions(-) diff --git a/shell/platform/darwin/ios/framework/Source/accessibility_bridge.h b/shell/platform/darwin/ios/framework/Source/accessibility_bridge.h index 758b63fdf7545..f451949f0c67d 100644 --- a/shell/platform/darwin/ios/framework/Source/accessibility_bridge.h +++ b/shell/platform/darwin/ios/framework/Source/accessibility_bridge.h @@ -77,13 +77,15 @@ class AccessibilityBridge final : public AccessibilityBridgeIos { private: SemanticsObject* GetOrCreateObject(int32_t id, flutter::SemanticsNodeUpdates& updates); - void VisitObjectsRecursivelyAndRemove(SemanticsObject* object, - NSMutableArray* doomed_uids); + void UpdateFirstFocusable(SemanticsObject* object); + void WalkAndProccessTree(SemanticsObject* object); void HandleEvent(NSDictionary* annotatedEvent); FlutterViewController* view_controller_; PlatformViewIOS* platform_view_; FlutterPlatformViewsController* platform_views_controller_; + SemanticsObject* first_focusable_; + NSMutableArray* doomed_uids_; 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 d8f99a3b198b4..fc3f3b9dee024 100644 --- a/shell/platform/darwin/ios/framework/Source/accessibility_bridge.mm +++ b/shell/platform/darwin/ios/framework/Source/accessibility_bridge.mm @@ -181,28 +181,40 @@ void PostAccessibilityNotification(UIAccessibilityNotifications notification, view_controller_.view.accessibilityElements = nil; } - NSMutableArray* doomed_uids = [NSMutableArray arrayWithArray:[objects_.get() allKeys]]; + doomed_uids_ = [NSMutableArray arrayWithArray:[objects_.get() allKeys]]; + first_focusable_ = nil; if (root) - VisitObjectsRecursivelyAndRemove(root, doomed_uids); - [objects_ removeObjectsForKeys:doomed_uids]; - - layoutChanged = layoutChanged || [doomed_uids count] > 0; + WalkAndProccessTree(root); + [objects_ removeObjectsForKeys:doomed_uids_]; + layoutChanged = layoutChanged || [doomed_uids_ count] > 0; + doomed_uids_ = nil; + // We should send out only one notification per semantics update. if (routeChanged) { if (!ios_delegate_->IsFlutterViewControllerPresentingModalViewController(view_controller_)) { + SemanticsObject* nextToFocus = [lastAdded routeFocusObject]; + if (!nextToFocus) + nextToFocus = first_focusable_; ios_delegate_->PostAccessibilityNotification(UIAccessibilityScreenChangedNotification, - [lastAdded routeFocusObject]); + 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) + nextToFocus = first_focusable_; ios_delegate_->PostAccessibilityNotification( UIAccessibilityLayoutChangedNotification, - [objects_.get() objectForKey:@(last_focused_semantics_object_id_)]); - } - if (scrollOccured) { - // Tries to refocus the previous focused semantics object to avoid random jumps. + nextToFocus); + } else if (scrollOccured) { + // 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) + nextToFocus = first_focusable_; ios_delegate_->PostAccessibilityNotification( UIAccessibilityPageScrolledNotification, - [objects_.get() objectForKey:@(last_focused_semantics_object_id_)]); + nextToFocus); } } @@ -279,11 +291,17 @@ static bool DidFlagChange(const flutter::SemanticsNode& oldNode, return object; } -void AccessibilityBridge::VisitObjectsRecursivelyAndRemove(SemanticsObject* object, - NSMutableArray* doomed_uids) { - [doomed_uids removeObject:@(object.uid)]; +void AccessibilityBridge::UpdateFirstFocusable(SemanticsObject* object) { + if (first_focusable_ || !object.isAccessibilityElement) + return; + first_focusable_ = object; +} + +void AccessibilityBridge::WalkAndProccessTree(SemanticsObject* object) { + UpdateFirstFocusable(object); + [doomed_uids_ removeObject:@(object.uid)]; for (SemanticsObject* child in [object children]) - VisitObjectsRecursivelyAndRemove(child, doomed_uids); + WalkAndProccessTree(child); } void AccessibilityBridge::HandleEvent(NSDictionary* annotatedEvent) { 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 d5f972aa62ffc..01bca1cd6b7f9 100644 --- a/shell/platform/darwin/ios/framework/Source/accessibility_bridge_test.mm +++ b/shell/platform/darwin/ios/framework/Source/accessibility_bridge_test.mm @@ -283,7 +283,6 @@ - (void)testAnnouncesRouteChanges { id mockFlutterView = OCMClassMock([FlutterView class]); id mockFlutterViewController = OCMClassMock([FlutterViewController class]); OCMStub([mockFlutterViewController view]).andReturn(mockFlutterView); - std::string label = "some label"; NSMutableArray*>* accessibility_notifications = [[[NSMutableArray alloc] init] autorelease]; @@ -304,15 +303,25 @@ - (void)testAnnouncesRouteChanges { flutter::CustomAccessibilityActionUpdates actions; flutter::SemanticsNodeUpdates nodes; - flutter::SemanticsNode route_node; - route_node.id = 1; - route_node.flags = static_cast(flutter::SemanticsFlags::kScopesRoute) | - static_cast(flutter::SemanticsFlags::kNamesRoute); - route_node.label = "route"; - nodes[route_node.id] = route_node; + flutter::SemanticsNode node1; + node1.id = 1; + node1.label = "node1"; + node1.flags = static_cast(flutter::SemanticsFlags::kScopesRoute); + node1.childrenInTraversalOrder = {2, 3}; + node1.childrenInHitTestOrder = {2, 3}; + nodes[node1.id] = node1; + flutter::SemanticsNode node2; + node2.id = 2; + node2.label = "node2"; + nodes[node2.id] = node2; + flutter::SemanticsNode node3; + node3.id = 3; + node3.flags = static_cast(flutter::SemanticsFlags::kNamesRoute); + node3.label = "node3"; + nodes[node3.id] = node3; flutter::SemanticsNode root_node; root_node.id = kRootNodeId; - root_node.label = label; + root_node.flags = static_cast(flutter::SemanticsFlags::kScopesRoute); root_node.childrenInTraversalOrder = {1}; root_node.childrenInHitTestOrder = {1}; nodes[root_node.id] = root_node; @@ -320,8 +329,74 @@ - (void)testAnnouncesRouteChanges { XCTAssertEqual([accessibility_notifications count], 1ul); SemanticsObject* focusObject = accessibility_notifications[0][@"argument"]; - XCTAssertEqual([focusObject uid], 1); - XCTAssertEqualObjects([focusObject accessibilityLabel], @"route"); + XCTAssertEqual([focusObject uid], 3); + XCTAssertEqualObjects([focusObject accessibilityLabel], @"node3"); + XCTAssertEqual([accessibility_notifications[0][@"notification"] unsignedIntValue], + UIAccessibilityScreenChangedNotification); +} + +- (void)testAnnouncesRouteChangesWhenNoNamesRoute { + 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]); + id mockFlutterViewController = OCMClassMock([FlutterViewController 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 nodes; + + flutter::SemanticsNode node1; + node1.id = 1; + node1.label = "node1"; + node1.flags = static_cast(flutter::SemanticsFlags::kScopesRoute); + node1.childrenInTraversalOrder = {2, 3}; + node1.childrenInHitTestOrder = {2, 3}; + nodes[node1.id] = node1; + flutter::SemanticsNode node2; + node2.id = 2; + node2.label = "node2"; + nodes[node2.id] = node2; + flutter::SemanticsNode node3; + node3.id = 3; + node3.label = "node3"; + nodes[node3.id] = node3; + flutter::SemanticsNode root_node; + root_node.id = kRootNodeId; + root_node.childrenInTraversalOrder = {1}; + root_node.childrenInHitTestOrder = {1}; + nodes[root_node.id] = root_node; + bridge->UpdateSemantics(/*nodes=*/nodes, /*actions=*/actions); + + // Notification should focus first focusable node, which is node1. + XCTAssertEqual([accessibility_notifications count], 1ul); + SemanticsObject* focusObject = accessibility_notifications[0][@"argument"]; + XCTAssertEqual([focusObject uid], 2); + XCTAssertEqualObjects([focusObject accessibilityLabel], @"node2"); XCTAssertEqual([accessibility_notifications[0][@"notification"] unsignedIntValue], UIAccessibilityScreenChangedNotification); } @@ -384,9 +459,10 @@ - (void)testAnnouncesLayoutChangeWithNilIfLastFocusIsRemoved { 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]); + SemanticsObject* focusObject = accessibility_notifications[0][@"argument"]; + // The node 1 was removed, so the bridge will set the focus object to root. + XCTAssertEqual([focusObject uid], 0); + XCTAssertEqualObjects([focusObject accessibilityLabel], @"root"); XCTAssertEqual([accessibility_notifications[0][@"notification"] unsignedIntValue], UIAccessibilityLayoutChangedNotification); } From 8beec080999dd80366206e5c7d9e4f82633f295e Mon Sep 17 00:00:00 2001 From: Chun-Heng Tai Date: Fri, 21 Aug 2020 14:53:25 -0700 Subject: [PATCH 2/4] format --- .../ios/framework/Source/accessibility_bridge.mm | 16 ++++++++-------- 1 file changed, 8 insertions(+), 8 deletions(-) diff --git a/shell/platform/darwin/ios/framework/Source/accessibility_bridge.mm b/shell/platform/darwin/ios/framework/Source/accessibility_bridge.mm index fc3f3b9dee024..734cd9bf745f4 100644 --- a/shell/platform/darwin/ios/framework/Source/accessibility_bridge.mm +++ b/shell/platform/darwin/ios/framework/Source/accessibility_bridge.mm @@ -199,22 +199,22 @@ void PostAccessibilityNotification(UIAccessibilityNotifications notification, } } 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_)]; + SemanticsObject* nextToFocus = + [objects_.get() objectForKey:@(last_focused_semantics_object_id_)]; if (!nextToFocus) nextToFocus = first_focusable_; - ios_delegate_->PostAccessibilityNotification( - UIAccessibilityLayoutChangedNotification, - nextToFocus); + ios_delegate_->PostAccessibilityNotification(UIAccessibilityLayoutChangedNotification, + nextToFocus); } else if (scrollOccured) { // 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_)]; + SemanticsObject* nextToFocus = + [objects_.get() objectForKey:@(last_focused_semantics_object_id_)]; if (!nextToFocus) nextToFocus = first_focusable_; - ios_delegate_->PostAccessibilityNotification( - UIAccessibilityPageScrolledNotification, - nextToFocus); + ios_delegate_->PostAccessibilityNotification(UIAccessibilityPageScrolledNotification, + nextToFocus); } } From 06f934175bb266665cbfdd8872004a653637b633 Mon Sep 17 00:00:00 2001 From: Chun-Heng Tai Date: Mon, 24 Aug 2020 12:02:52 -0700 Subject: [PATCH 3/4] addressing review comment --- .../framework/Source/accessibility_bridge.h | 7 ++- .../framework/Source/accessibility_bridge.mm | 53 +++++++++++-------- 2 files changed, 35 insertions(+), 25 deletions(-) diff --git a/shell/platform/darwin/ios/framework/Source/accessibility_bridge.h b/shell/platform/darwin/ios/framework/Source/accessibility_bridge.h index f451949f0c67d..098be11cd210d 100644 --- a/shell/platform/darwin/ios/framework/Source/accessibility_bridge.h +++ b/shell/platform/darwin/ios/framework/Source/accessibility_bridge.h @@ -77,15 +77,14 @@ class AccessibilityBridge final : public AccessibilityBridgeIos { private: SemanticsObject* GetOrCreateObject(int32_t id, flutter::SemanticsNodeUpdates& updates); - void UpdateFirstFocusable(SemanticsObject* object); - void WalkAndProccessTree(SemanticsObject* object); + SemanticsObject* FindFirstFocusable(SemanticsObject* object); + void VisitObjectsRecursivelyAndRemove(SemanticsObject* object, + NSMutableArray* doomed_uids); void HandleEvent(NSDictionary* annotatedEvent); FlutterViewController* view_controller_; PlatformViewIOS* platform_view_; FlutterPlatformViewsController* platform_views_controller_; - SemanticsObject* first_focusable_; - NSMutableArray* doomed_uids_; 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 734cd9bf745f4..1febef39f5190 100644 --- a/shell/platform/darwin/ios/framework/Source/accessibility_bridge.mm +++ b/shell/platform/darwin/ios/framework/Source/accessibility_bridge.mm @@ -181,19 +181,19 @@ void PostAccessibilityNotification(UIAccessibilityNotifications notification, view_controller_.view.accessibilityElements = nil; } - doomed_uids_ = [NSMutableArray arrayWithArray:[objects_.get() allKeys]]; - first_focusable_ = nil; + NSMutableArray* doomed_uids = [NSMutableArray arrayWithArray:[objects_.get() allKeys]]; if (root) - WalkAndProccessTree(root); - [objects_ removeObjectsForKeys:doomed_uids_]; - layoutChanged = layoutChanged || [doomed_uids_ count] > 0; - doomed_uids_ = nil; + VisitObjectsRecursivelyAndRemove(root, doomed_uids); + [objects_ removeObjectsForKeys:doomed_uids]; + + layoutChanged = layoutChanged || [doomed_uids count] > 0; // We should send out only one notification per semantics update. if (routeChanged) { if (!ios_delegate_->IsFlutterViewControllerPresentingModalViewController(view_controller_)) { SemanticsObject* nextToFocus = [lastAdded routeFocusObject]; - if (!nextToFocus) - nextToFocus = first_focusable_; + if (!nextToFocus && root) { + nextToFocus = FindFirstFocusable(root); + } ios_delegate_->PostAccessibilityNotification(UIAccessibilityScreenChangedNotification, nextToFocus); } @@ -201,8 +201,9 @@ void PostAccessibilityNotification(UIAccessibilityNotifications notification, // Tries to refocus the previous focused semantics object to avoid random jumps. SemanticsObject* nextToFocus = [objects_.get() objectForKey:@(last_focused_semantics_object_id_)]; - if (!nextToFocus) - nextToFocus = first_focusable_; + if (!nextToFocus && root) { + nextToFocus = FindFirstFocusable(root); + } ios_delegate_->PostAccessibilityNotification(UIAccessibilityLayoutChangedNotification, nextToFocus); } else if (scrollOccured) { @@ -211,8 +212,9 @@ void PostAccessibilityNotification(UIAccessibilityNotifications notification, // 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) - nextToFocus = first_focusable_; + if (!nextToFocus && root) { + nextToFocus = FindFirstFocusable(root); + } ios_delegate_->PostAccessibilityNotification(UIAccessibilityPageScrolledNotification, nextToFocus); } @@ -291,17 +293,26 @@ static bool DidFlagChange(const flutter::SemanticsNode& oldNode, return object; } -void AccessibilityBridge::UpdateFirstFocusable(SemanticsObject* object) { - if (first_focusable_ || !object.isAccessibilityElement) - return; - first_focusable_ = object; +void AccessibilityBridge::VisitObjectsRecursivelyAndRemove(SemanticsObject* object, + NSMutableArray* doomed_uids) { + [doomed_uids removeObject:@(object.uid)]; + for (SemanticsObject* child in [object children]) + VisitObjectsRecursivelyAndRemove(child, doomed_uids); } -void AccessibilityBridge::WalkAndProccessTree(SemanticsObject* object) { - UpdateFirstFocusable(object); - [doomed_uids_ removeObject:@(object.uid)]; - for (SemanticsObject* child in [object children]) - WalkAndProccessTree(child); +SemanticsObject* AccessibilityBridge::FindFirstFocusable(SemanticsObject* object) { + if (object.isAccessibilityElement) { + return object; + } + + SemanticsObject* candidate = nil; + for (SemanticsObject* child in [object children]) { + if (candidate) { + break; + } + candidate = FindFirstFocusable(child); + } + return candidate; } void AccessibilityBridge::HandleEvent(NSDictionary* annotatedEvent) { From 0271f8bea9d64e3bd0b201e9f89f30edaca857ca Mon Sep 17 00:00:00 2001 From: Chun-Heng Tai Date: Mon, 24 Aug 2020 14:31:12 -0700 Subject: [PATCH 4/4] format --- .../darwin/ios/framework/Source/accessibility_bridge.mm | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/shell/platform/darwin/ios/framework/Source/accessibility_bridge.mm b/shell/platform/darwin/ios/framework/Source/accessibility_bridge.mm index 1febef39f5190..eee0ac4823469 100644 --- a/shell/platform/darwin/ios/framework/Source/accessibility_bridge.mm +++ b/shell/platform/darwin/ios/framework/Source/accessibility_bridge.mm @@ -202,8 +202,8 @@ void PostAccessibilityNotification(UIAccessibilityNotifications notification, SemanticsObject* nextToFocus = [objects_.get() objectForKey:@(last_focused_semantics_object_id_)]; if (!nextToFocus && root) { - nextToFocus = FindFirstFocusable(root); - } + nextToFocus = FindFirstFocusable(root); + } ios_delegate_->PostAccessibilityNotification(UIAccessibilityLayoutChangedNotification, nextToFocus); } else if (scrollOccured) { @@ -213,8 +213,8 @@ void PostAccessibilityNotification(UIAccessibilityNotifications notification, SemanticsObject* nextToFocus = [objects_.get() objectForKey:@(last_focused_semantics_object_id_)]; if (!nextToFocus && root) { - nextToFocus = FindFirstFocusable(root); - } + nextToFocus = FindFirstFocusable(root); + } ios_delegate_->PostAccessibilityNotification(UIAccessibilityPageScrolledNotification, nextToFocus); }