From 29d338d31dbd84e14b0b5392cdbeca030c210c0b Mon Sep 17 00:00:00 2001 From: Chris Bracken Date: Thu, 9 Dec 2021 18:06:28 -0800 Subject: [PATCH] Fix TestAccessibilityBridgeDelegate event caching TestAccessibilityBridgeDelegate::accessibility_events previously held values of type ui::AXEventGenerator::TargetedEvent. TargetedEvent contains an AXNode pointer and a const reference to a ui::AXEventGenerator::EventParams object, and as such it's unsafe to make or read copies of TargetedEvent values outside the scope of the AccessibilityBridgeDelegate::OnAccessibilityEvent callback. In this patch, we update the accessibility_events vector to simply hold EventType values since this is the only part of the value we use in our existing tests. If in future we need the full TargetedEvent, we'll need to properly copy these values. This patch also fixes a typo in the accessibility_events identifier and converts an EXPECT_EQ to an ASSERT_EQ in a case where the following test expectations are meaningless/could crash if the accessibility_events size isn't as expected. Issue: https://github.com/flutter/flutter/issues/77838 --- .../common/accessibility_bridge_unittests.cc | 29 ++++++++++--------- .../common/test_accessibility_bridge.cc | 2 +- .../common/test_accessibility_bridge.h | 2 +- 3 files changed, 18 insertions(+), 15 deletions(-) diff --git a/shell/platform/common/accessibility_bridge_unittests.cc b/shell/platform/common/accessibility_bridge_unittests.cc index c7119a0071dda..e98ea4b527339 100644 --- a/shell/platform/common/accessibility_bridge_unittests.cc +++ b/shell/platform/common/accessibility_bridge_unittests.cc @@ -4,6 +4,7 @@ #include "accessibility_bridge.h" +#include "gmock/gmock.h" #include "gtest/gtest.h" #include "test_accessibility_bridge.h" @@ -11,6 +12,8 @@ namespace flutter { namespace testing { +using ::testing::Contains; + TEST(AccessibilityBridgeTest, basicTest) { std::shared_ptr bridge = std::make_shared( @@ -115,7 +118,7 @@ TEST(AccessibilityBridgeTest, canFireChildrenChangedCorrectly) { EXPECT_EQ(child1_node->GetChildCount(), 0); EXPECT_EQ(child1_node->GetName(), "child 1"); - delegate->accessibilitiy_events.clear(); + delegate->accessibility_events.clear(); // Add a child to root. root.child_count = 2; @@ -145,14 +148,14 @@ TEST(AccessibilityBridgeTest, canFireChildrenChangedCorrectly) { EXPECT_EQ(root_node->GetChildCount(), 2); EXPECT_EQ(root_node->GetData().child_ids[0], 1); EXPECT_EQ(root_node->GetData().child_ids[1], 2); - EXPECT_EQ(delegate->accessibilitiy_events.size(), size_t{2}); - std::set actual_event; - actual_event.insert(delegate->accessibilitiy_events[0].event_params.event); - actual_event.insert(delegate->accessibilitiy_events[1].event_params.event); - EXPECT_NE(actual_event.find(ui::AXEventGenerator::Event::CHILDREN_CHANGED), - actual_event.end()); - EXPECT_NE(actual_event.find(ui::AXEventGenerator::Event::SUBTREE_CREATED), - actual_event.end()); + EXPECT_EQ(delegate->accessibility_events.size(), size_t{2}); + std::set actual_event{ + delegate->accessibility_events.begin(), + delegate->accessibility_events.end()}; + EXPECT_THAT(actual_event, + Contains(ui::AXEventGenerator::Event::CHILDREN_CHANGED)); + EXPECT_THAT(actual_event, + Contains(ui::AXEventGenerator::Event::SUBTREE_CREATED)); } TEST(AccessibilityBridgeTest, canUpdateDelegate) { @@ -240,7 +243,7 @@ TEST(AccessibilityBridgeTest, canHandleSelectionChangeCorrectly) { const ui::AXTreeData& tree = bridge->GetAXTreeData(); EXPECT_EQ(tree.sel_anchor_object_id, ui::AXNode::kInvalidAXID); - delegate->accessibilitiy_events.clear(); + delegate->accessibility_events.clear(); // Update the selection. root.text_selection_base = 0; @@ -253,10 +256,10 @@ TEST(AccessibilityBridgeTest, canHandleSelectionChangeCorrectly) { EXPECT_EQ(tree.sel_anchor_offset, 0); EXPECT_EQ(tree.sel_focus_object_id, 0); EXPECT_EQ(tree.sel_focus_offset, 5); - EXPECT_EQ(delegate->accessibilitiy_events.size(), size_t{2}); - EXPECT_EQ(delegate->accessibilitiy_events[0].event_params.event, + ASSERT_EQ(delegate->accessibility_events.size(), size_t{2}); + EXPECT_EQ(delegate->accessibility_events[0], ui::AXEventGenerator::Event::DOCUMENT_SELECTION_CHANGED); - EXPECT_EQ(delegate->accessibilitiy_events[1].event_params.event, + EXPECT_EQ(delegate->accessibility_events[1], ui::AXEventGenerator::Event::OTHER_ATTRIBUTE_CHANGED); } diff --git a/shell/platform/common/test_accessibility_bridge.cc b/shell/platform/common/test_accessibility_bridge.cc index af92c638660f3..299f25a9f2536 100644 --- a/shell/platform/common/test_accessibility_bridge.cc +++ b/shell/platform/common/test_accessibility_bridge.cc @@ -13,7 +13,7 @@ TestAccessibilityBridgeDelegate::CreateFlutterPlatformNodeDelegate() { void TestAccessibilityBridgeDelegate::OnAccessibilityEvent( ui::AXEventGenerator::TargetedEvent targeted_event) { - accessibilitiy_events.push_back(targeted_event); + accessibility_events.push_back(targeted_event.event_params.event); } void TestAccessibilityBridgeDelegate::DispatchAccessibilityAction( diff --git a/shell/platform/common/test_accessibility_bridge.h b/shell/platform/common/test_accessibility_bridge.h index 8f618409fc5ba..bb833cb2ea0fa 100644 --- a/shell/platform/common/test_accessibility_bridge.h +++ b/shell/platform/common/test_accessibility_bridge.h @@ -22,7 +22,7 @@ class TestAccessibilityBridgeDelegate std::shared_ptr CreateFlutterPlatformNodeDelegate() override; - std::vector accessibilitiy_events; + std::vector accessibility_events; std::vector performed_actions; };