From b9652488f2b0e2a5cf2ddd19ce7f16aa29da5b42 Mon Sep 17 00:00:00 2001 From: Chun-Heng Tai Date: Fri, 23 Apr 2021 13:20:35 -0700 Subject: [PATCH 1/3] Fixes ios accessibility send focus request to an unfocusable node --- .../ios/framework/Source/SemanticsObject.mm | 2 +- .../framework/Source/SemanticsObjectTest.mm | 23 +++++ .../framework/Source/accessibility_bridge.mm | 17 ++-- .../Source/accessibility_bridge_test.mm | 86 ++++++++++++++++++- 4 files changed, 119 insertions(+), 9 deletions(-) diff --git a/shell/platform/darwin/ios/framework/Source/SemanticsObject.mm b/shell/platform/darwin/ios/framework/Source/SemanticsObject.mm index f5e2cc76c3dc5..4ff73e4922961 100644 --- a/shell/platform/darwin/ios/framework/Source/SemanticsObject.mm +++ b/shell/platform/darwin/ios/framework/Source/SemanticsObject.mm @@ -284,7 +284,7 @@ - (BOOL)isAccessibilityElement { // item that is currently off screen but the a11y navigation needs to know // about. return (([self node].flags & ~flutter::kScrollableSemanticsFlags) != 0 && - [self node].flags != static_cast(flutter::SemanticsFlags::kIsHidden)) || + ([self node].flags & ~static_cast(flutter::SemanticsFlags::kIsHidden)) != 1) || ![self node].label.empty() || ![self node].value.empty() || ![self node].hint.empty() || ([self node].actions & ~flutter::kScrollableSemanticsActions) != 0; } diff --git a/shell/platform/darwin/ios/framework/Source/SemanticsObjectTest.mm b/shell/platform/darwin/ios/framework/Source/SemanticsObjectTest.mm index 04212197e4ee1..b9179a27f6ad5 100644 --- a/shell/platform/darwin/ios/framework/Source/SemanticsObjectTest.mm +++ b/shell/platform/darwin/ios/framework/Source/SemanticsObjectTest.mm @@ -100,6 +100,29 @@ - (void)testPlainSemanticsObjectWithLabelHasStaticTextTrait { XCTAssertEqual([object accessibilityTraits], UIAccessibilityTraitStaticText); } +- (void)testNodeWithImplicitScrollIsAnAccessibilityElementWhenItisHidden { + fml::WeakPtrFactory factory( + new flutter::MockAccessibilityBridge()); + fml::WeakPtr bridge = factory.GetWeakPtr(); + flutter::SemanticsNode node; + node.flags = static_cast(flutter::SemanticsFlags::kHasImplicitScrolling) | + static_cast(flutter::SemanticsFlags::kIsHidden); + FlutterSemanticsObject* object = [[FlutterSemanticsObject alloc] initWithBridge:bridge uid:0]; + [object setSemanticsNode:&node]; + XCTAssertEqual(object.isAccessibilityElement, YES); +} + +- (void)testNodeWithImplicitScrollIsNotAnAccessibilityElementWhenItisNotHidden { + fml::WeakPtrFactory factory( + new flutter::MockAccessibilityBridge()); + fml::WeakPtr bridge = factory.GetWeakPtr(); + flutter::SemanticsNode node; + node.flags = static_cast(flutter::SemanticsFlags::kHasImplicitScrolling); + FlutterSemanticsObject* object = [[FlutterSemanticsObject alloc] initWithBridge:bridge uid:0]; + [object setSemanticsNode:&node]; + XCTAssertEqual(object.isAccessibilityElement, NO); +} + - (void)testIntresetingSemanticsObjectWithLabelHasStaticTextTrait { fml::WeakPtrFactory factory( new flutter::MockAccessibilityBridge()); diff --git a/shell/platform/darwin/ios/framework/Source/accessibility_bridge.mm b/shell/platform/darwin/ios/framework/Source/accessibility_bridge.mm index 49c5b145162cb..81663ad58daa7 100644 --- a/shell/platform/darwin/ios/framework/Source/accessibility_bridge.mm +++ b/shell/platform/darwin/ios/framework/Source/accessibility_bridge.mm @@ -222,7 +222,9 @@ void PostAccessibilityNotification(UIAccessibilityNotifications notification, 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) { + if (nextToFocus) { + nextToFocus = FindFirstFocusable(nextToFocus); + } else if (root) { nextToFocus = FindFirstFocusable(root); } } @@ -235,7 +237,9 @@ void PostAccessibilityNotification(UIAccessibilityNotifications notification, SemanticsObject* nextToFocus = nil; if (last_focused_semantics_object_id_ != kSemanticObjectIdInvalid) { nextToFocus = [objects_.get() objectForKey:@(last_focused_semantics_object_id_)]; - if (!nextToFocus && root) { + if (nextToFocus) { + nextToFocus = FindFirstFocusable(nextToFocus); + } else if (root) { nextToFocus = FindFirstFocusable(root); } } @@ -319,6 +323,8 @@ static bool DidFlagChange(const flutter::SemanticsNode& oldNode, void AccessibilityBridge::VisitObjectsRecursivelyAndRemove(SemanticsObject* object, NSMutableArray* doomed_uids) { + NSLog(@"id %u is accessibility element %d has rect: %@", object.uid, + object.isAccessibilityElement, NSStringFromCGRect(object.accessibilityFrame)); [doomed_uids removeObject:@(object.uid)]; for (SemanticsObject* child in [object children]) VisitObjectsRecursivelyAndRemove(child, doomed_uids); @@ -329,14 +335,13 @@ static bool DidFlagChange(const flutter::SemanticsNode& oldNode, return object; } - SemanticsObject* candidate = nil; for (SemanticsObject* child in [object children]) { + SemanticsObject* candidate = FindFirstFocusable(child); if (candidate) { - break; + return candidate; } - candidate = FindFirstFocusable(child); } - return candidate; + return nil; } 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 35fd8aac9ffbc..c734907ef9423 100644 --- a/shell/platform/darwin/ios/framework/Source/accessibility_bridge_test.mm +++ b/shell/platform/darwin/ios/framework/Source/accessibility_bridge_test.mm @@ -344,6 +344,88 @@ - (void)testAnnouncesRouteChanges { UIAccessibilityScreenChangedNotification); } +- (void)testLayoutChangeWithNonAccessibilityElement { + 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, + /*platform_views_controller=*/nil, + /*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.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.label = "root"; + root_node.childrenInTraversalOrder = {1}; + root_node.childrenInHitTestOrder = {1}; + nodes[root_node.id] = root_node; + bridge->UpdateSemantics(/*nodes=*/nodes, /*actions=*/actions); + + // Simulates the focusing on the node 1. + bridge->AccessibilityObjectDidBecomeFocused(1); + + // In this update, we make node 1 unfocusable and trigger the + // layout change. The accessibility bridge should send layoutchange + // notification with the first focusable node under node 1 + flutter::CustomAccessibilityActionUpdates new_actions; + flutter::SemanticsNodeUpdates new_nodes; + + flutter::SemanticsNode new_node1; + new_node1.id = 1; + new_node1.childrenInTraversalOrder = {2}; + new_node1.childrenInHitTestOrder = {2}; + new_nodes[new_node1.id] = new_node1; + bridge->UpdateSemantics(/*nodes=*/new_nodes, /*actions=*/new_actions); + + XCTAssertEqual([accessibility_notifications count], 1ul); + SemanticsObject* focusObject = accessibility_notifications[0][@"argument"]; + // Since node 1 is no longer focusable (no label), it will focus node 2 instead. + XCTAssertEqual([focusObject uid], 2); + XCTAssertEqual([accessibility_notifications[0][@"notification"] unsignedIntValue], + UIAccessibilityLayoutChangedNotification); +} + - (void)testAnnouncesRouteChangesAndLayoutChangeInOneUpdate { flutter::MockDelegate mock_delegate; auto thread_task_runner = CreateNewThread("AccessibilityBridgeTest"); @@ -392,7 +474,7 @@ - (void)testAnnouncesRouteChangesAndLayoutChangeInOneUpdate { nodes[node3.id] = node3; flutter::SemanticsNode root_node; root_node.id = kRootNodeId; - root_node.flags = static_cast(flutter::SemanticsFlags::kScopesRoute); + root_node.label = "root"; root_node.childrenInTraversalOrder = {1, 3}; root_node.childrenInHitTestOrder = {1, 3}; nodes[root_node.id] = root_node; @@ -424,7 +506,7 @@ - (void)testAnnouncesRouteChangesAndLayoutChangeInOneUpdate { new_nodes[new_node2.id] = new_node2; flutter::SemanticsNode new_root_node; new_root_node.id = kRootNodeId; - new_root_node.flags = static_cast(flutter::SemanticsFlags::kScopesRoute); + new_root_node.label = "root"; new_root_node.childrenInTraversalOrder = {1}; new_root_node.childrenInHitTestOrder = {1}; new_nodes[new_root_node.id] = new_root_node; From 929a166699c483c512626d6b1252186bba853af3 Mon Sep 17 00:00:00 2001 From: Chun-Heng Tai Date: Fri, 23 Apr 2021 14:13:22 -0700 Subject: [PATCH 2/3] remove log --- .../darwin/ios/framework/Source/accessibility_bridge.mm | 2 -- 1 file changed, 2 deletions(-) diff --git a/shell/platform/darwin/ios/framework/Source/accessibility_bridge.mm b/shell/platform/darwin/ios/framework/Source/accessibility_bridge.mm index 81663ad58daa7..861130f82af28 100644 --- a/shell/platform/darwin/ios/framework/Source/accessibility_bridge.mm +++ b/shell/platform/darwin/ios/framework/Source/accessibility_bridge.mm @@ -323,8 +323,6 @@ static bool DidFlagChange(const flutter::SemanticsNode& oldNode, void AccessibilityBridge::VisitObjectsRecursivelyAndRemove(SemanticsObject* object, NSMutableArray* doomed_uids) { - NSLog(@"id %u is accessibility element %d has rect: %@", object.uid, - object.isAccessibilityElement, NSStringFromCGRect(object.accessibilityFrame)); [doomed_uids removeObject:@(object.uid)]; for (SemanticsObject* child in [object children]) VisitObjectsRecursivelyAndRemove(child, doomed_uids); From ca9f26eb600f81314b9da5a4f89fcb9690c75168 Mon Sep 17 00:00:00 2001 From: Chun-Heng Tai Date: Mon, 3 May 2021 12:37:37 -0700 Subject: [PATCH 3/3] addressing comments --- .../ios/framework/Source/SemanticsObject.mm | 13 +++-- .../framework/Source/accessibility_bridge.h | 7 ++- .../framework/Source/accessibility_bridge.mm | 49 +++++++++---------- 3 files changed, 34 insertions(+), 35 deletions(-) diff --git a/shell/platform/darwin/ios/framework/Source/SemanticsObject.mm b/shell/platform/darwin/ios/framework/Source/SemanticsObject.mm index 4ff73e4922961..e78fda0b3288e 100644 --- a/shell/platform/darwin/ios/framework/Source/SemanticsObject.mm +++ b/shell/platform/darwin/ios/framework/Source/SemanticsObject.mm @@ -274,17 +274,16 @@ - (BOOL)isAccessibilityElement { if ([self node].HasFlag(flutter::SemanticsFlags::kScopesRoute)) return false; - // If the only flag(s) set are scrolling related AND - // The only flags set are not kIsHidden OR - // The node doesn't have a label, value, or hint OR - // The only actions set are scrolling related actions. + // If the node is scrollable AND hidden OR + // The node has a label, value, or hint OR + // The node has non-scrolling related actions. // - // The kIsHidden flag set with any other flag just means this node is now + // The kIsHidden flag set with the scrollable flag means this node is now // hidden but still is a valid target for a11y focus in the tree, e.g. a list // item that is currently off screen but the a11y navigation needs to know // about. - return (([self node].flags & ~flutter::kScrollableSemanticsFlags) != 0 && - ([self node].flags & ~static_cast(flutter::SemanticsFlags::kIsHidden)) != 1) || + return (([self node].flags & flutter::kScrollableSemanticsFlags) != 0 && + ([self node].flags & static_cast(flutter::SemanticsFlags::kIsHidden)) != 0) || ![self node].label.empty() || ![self node].value.empty() || ![self node].hint.empty() || ([self node].actions & ~flutter::kScrollableSemanticsActions) != 0; } diff --git a/shell/platform/darwin/ios/framework/Source/accessibility_bridge.h b/shell/platform/darwin/ios/framework/Source/accessibility_bridge.h index 1e38a3b0de1ca..bcafa90c478c7 100644 --- a/shell/platform/darwin/ios/framework/Source/accessibility_bridge.h +++ b/shell/platform/darwin/ios/framework/Source/accessibility_bridge.h @@ -78,7 +78,12 @@ class AccessibilityBridge final : public AccessibilityBridgeIos { private: SemanticsObject* GetOrCreateObject(int32_t id, flutter::SemanticsNodeUpdates& updates); - SemanticsObject* FindFirstFocusable(SemanticsObject* object); + SemanticsObject* FindNextFocusableIfNecessary(); + // Finds the first focusable SemanticsObject rooted at the parent. This includes the parent itself + // if it is a focusable SemanticsObject. + // + // If the parent is nil, this function use the root SemanticsObject as the parent. + SemanticsObject* FindFirstFocusable(SemanticsObject* parent); void VisitObjectsRecursivelyAndRemove(SemanticsObject* object, NSMutableArray* doomed_uids); void HandleEvent(NSDictionary* annotatedEvent); diff --git a/shell/platform/darwin/ios/framework/Source/accessibility_bridge.mm b/shell/platform/darwin/ios/framework/Source/accessibility_bridge.mm index 861130f82af28..1c9f6e4f2529d 100644 --- a/shell/platform/darwin/ios/framework/Source/accessibility_bridge.mm +++ b/shell/platform/darwin/ios/framework/Source/accessibility_bridge.mm @@ -216,35 +216,14 @@ void PostAccessibilityNotification(UIAccessibilityNotifications notification, } 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 refocus anything. - 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) { - nextToFocus = FindFirstFocusable(nextToFocus); - } else if (root) { - nextToFocus = FindFirstFocusable(root); - } - } ios_delegate_->PostAccessibilityNotification(UIAccessibilityLayoutChangedNotification, - nextToFocus); + FindNextFocusableIfNecessary()); } 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 = nil; - if (last_focused_semantics_object_id_ != kSemanticObjectIdInvalid) { - nextToFocus = [objects_.get() objectForKey:@(last_focused_semantics_object_id_)]; - if (nextToFocus) { - nextToFocus = FindFirstFocusable(nextToFocus); - } else if (root) { - nextToFocus = FindFirstFocusable(root); - } - } ios_delegate_->PostAccessibilityNotification(UIAccessibilityPageScrolledNotification, - nextToFocus); + FindNextFocusableIfNecessary()); } } @@ -328,12 +307,28 @@ static bool DidFlagChange(const flutter::SemanticsNode& oldNode, VisitObjectsRecursivelyAndRemove(child, doomed_uids); } -SemanticsObject* AccessibilityBridge::FindFirstFocusable(SemanticsObject* object) { - if (object.isAccessibilityElement) { - return object; +SemanticsObject* AccessibilityBridge::FindNextFocusableIfNecessary() { + // 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_ == kSemanticObjectIdInvalid) { + return nil; + } + + // Tries to refocus the previous focused semantics object to avoid random jumps. + return FindFirstFocusable([objects_.get() objectForKey:@(last_focused_semantics_object_id_)]); +} + +SemanticsObject* AccessibilityBridge::FindFirstFocusable(SemanticsObject* parent) { + SemanticsObject* currentObject = parent ?: objects_.get()[@(kRootNodeId)]; + ; + if (!currentObject) + return nil; + + if (currentObject.isAccessibilityElement) { + return currentObject; } - for (SemanticsObject* child in [object children]) { + for (SemanticsObject* child in [currentObject children]) { SemanticsObject* candidate = FindFirstFocusable(child); if (candidate) { return candidate;