From 53c321a3e39b1acfcd4f3bb37b3f3691df92282d Mon Sep 17 00:00:00 2001 From: yaakovschectman <109111084+yaakovschectman@users.noreply.github.com> Date: Mon, 22 Aug 2022 16:01:02 -0400 Subject: [PATCH] Include checkbox in check state update (flutter/engine#35557) * Include checkbox in check state update * Windows test for checkbox native state * Reformat to appease linux_unopt test * More format hoops * Update accessibility_bridge_unittests.cc * Update flutter_windows_view_unittests.cc --- .../platform/common/accessibility_bridge.cc | 3 +- .../common/accessibility_bridge_unittests.cc | 31 +++++++ .../windows/flutter_windows_view_unittests.cc | 90 +++++++++++++++++++ 3 files changed, 123 insertions(+), 1 deletion(-) diff --git a/engine/src/flutter/shell/platform/common/accessibility_bridge.cc b/engine/src/flutter/shell/platform/common/accessibility_bridge.cc index ab2468e331c..576edfac6e6 100644 --- a/engine/src/flutter/shell/platform/common/accessibility_bridge.cc +++ b/engine/src/flutter/shell/platform/common/accessibility_bridge.cc @@ -375,7 +375,8 @@ void AccessibilityBridge::SetIntAttributesFromFlutterUpdate( node_data.AddIntAttribute(ax::mojom::IntAttribute::kTextSelStart, sel_start); node_data.AddIntAttribute(ax::mojom::IntAttribute::kTextSelEnd, sel_end); - if (node_data.role == ax::mojom::Role::kRadioButton) { + if (node_data.role == ax::mojom::Role::kRadioButton || + node_data.role == ax::mojom::Role::kCheckBox) { node_data.AddIntAttribute( ax::mojom::IntAttribute::kCheckedState, static_cast( diff --git a/engine/src/flutter/shell/platform/common/accessibility_bridge_unittests.cc b/engine/src/flutter/shell/platform/common/accessibility_bridge_unittests.cc index 38e03826662..41834b107a4 100644 --- a/engine/src/flutter/shell/platform/common/accessibility_bridge_unittests.cc +++ b/engine/src/flutter/shell/platform/common/accessibility_bridge_unittests.cc @@ -337,5 +337,36 @@ TEST(AccessibilityBridgeTest, SliderHasSliderRole) { EXPECT_EQ(root_node->GetData().role, ax::mojom::Role::kSlider); } +// Ensure that checkboxes have their checked status set apropriately +// Previously, only Radios could have this flag updated +// Resulted in the issue seen at +// https://github.com/flutter/flutter/issues/96218 +// As this fix involved code run on all platforms, it is included here. +TEST(AccessibilityBridgeTest, CanSetCheckboxChecked) { + std::shared_ptr bridge = + std::make_shared( + std::make_unique()); + FlutterSemanticsNode root; + root.id = 0; + root.label = "root"; + root.hint = ""; + root.value = ""; + root.increased_value = ""; + root.decreased_value = ""; + root.child_count = 0; + root.custom_accessibility_actions_count = 0; + root.flags = static_cast( + FlutterSemanticsFlag::kFlutterSemanticsFlagHasCheckedState | + FlutterSemanticsFlag::kFlutterSemanticsFlagIsChecked); + bridge->AddFlutterSemanticsNodeUpdate(&root); + + bridge->CommitUpdates(); + + auto root_node = bridge->GetFlutterPlatformNodeDelegateFromID(0).lock(); + EXPECT_EQ(root_node->GetData().role, ax::mojom::Role::kCheckBox); + EXPECT_EQ(root_node->GetData().GetCheckedState(), + ax::mojom::CheckedState::kTrue); +} + } // namespace testing } // namespace flutter diff --git a/engine/src/flutter/shell/platform/windows/flutter_windows_view_unittests.cc b/engine/src/flutter/shell/platform/windows/flutter_windows_view_unittests.cc index 9513322daa2..d6830d69081 100644 --- a/engine/src/flutter/shell/platform/windows/flutter_windows_view_unittests.cc +++ b/engine/src/flutter/shell/platform/windows/flutter_windows_view_unittests.cc @@ -597,5 +597,95 @@ TEST(FlutterWindowsViewTest, WindowRepaintTests) { EXPECT_TRUE(schedule_frame_called); } +// Ensure that checkboxes have their checked status set apropriately +// Previously, only Radios could have this flag updated +// Resulted in the issue seen at +// https://github.com/flutter/flutter/issues/96218 +// This test ensures that the native state of Checkboxes on Windows, +// specifically, is updated as desired. +TEST(FlutterWindowsViewTest, CheckboxNativeState) { + std::unique_ptr engine = GetTestEngine(); + EngineModifier modifier(engine.get()); + modifier.embedder_api().UpdateSemanticsEnabled = + [](FLUTTER_API_SYMBOL(FlutterEngine) engine, bool enabled) { + return kSuccess; + }; + + auto window_binding_handler = + std::make_unique<::testing::NiceMock>(); + FlutterWindowsView view(std::move(window_binding_handler)); + view.SetEngine(std::move(engine)); + + // Enable semantics to instantiate accessibility bridge. + view.OnUpdateSemanticsEnabled(true); + + auto bridge = view.GetEngine()->accessibility_bridge().lock(); + ASSERT_TRUE(bridge); + + FlutterSemanticsNode root{sizeof(FlutterSemanticsNode), 0}; + root.id = 0; + root.label = "root"; + root.hint = ""; + root.value = ""; + root.increased_value = ""; + root.decreased_value = ""; + root.child_count = 0; + root.custom_accessibility_actions_count = 0; + root.flags = static_cast( + FlutterSemanticsFlag::kFlutterSemanticsFlagHasCheckedState | + FlutterSemanticsFlag::kFlutterSemanticsFlagIsChecked); + bridge->AddFlutterSemanticsNodeUpdate(&root); + + bridge->CommitUpdates(); + + auto root_node = bridge + ->GetFlutterPlatformNodeDelegateFromID( + AccessibilityBridge::kRootNodeId) + .lock(); + EXPECT_EQ(root_node->GetData().role, ax::mojom::Role::kCheckBox); + EXPECT_EQ(root_node->GetData().GetCheckedState(), + ax::mojom::CheckedState::kTrue); + + // Get the IAccessible for the root node. + IAccessible* native_view = root_node->GetNativeViewAccessible(); + ASSERT_TRUE(native_view != nullptr); + + // Look up against the node itself (not one of its children) + VARIANT varchild = {}; + varchild.vt = VT_I4; + + // Verify the checkbox is checked. + varchild.lVal = CHILDID_SELF; + VARIANT native_state = {}; + ASSERT_TRUE(SUCCEEDED(native_view->get_accState(varchild, &native_state))); + EXPECT_TRUE(native_state.lVal & STATE_SYSTEM_CHECKED); + + // Test unchecked too + root.flags = static_cast( + FlutterSemanticsFlag::kFlutterSemanticsFlagHasCheckedState); + bridge->AddFlutterSemanticsNodeUpdate(&root); + bridge->CommitUpdates(); + root_node = bridge + ->GetFlutterPlatformNodeDelegateFromID( + AccessibilityBridge::kRootNodeId) + .lock(); + EXPECT_EQ(root_node->GetData().role, ax::mojom::Role::kCheckBox); + EXPECT_EQ(root_node->GetData().GetCheckedState(), + ax::mojom::CheckedState::kFalse); + + // Get the IAccessible for the root node. + native_view = root_node->GetNativeViewAccessible(); + + // Look up against the node itself (not one of its children) + varchild = {}; + varchild.vt = VT_I4; + + // Verify the checkbox is checked. + varchild.lVal = CHILDID_SELF; + native_state = {}; + ASSERT_TRUE(SUCCEEDED(native_view->get_accState(varchild, &native_state))); + EXPECT_FALSE(native_state.lVal & STATE_SYSTEM_CHECKED); +} + } // namespace testing } // namespace flutter