From bb97ac1d3b685319ed9e2bed60a2fafb7d5b4df7 Mon Sep 17 00:00:00 2001 From: yaakovschectman <109111084+yaakovschectman@users.noreply.github.com> Date: Fri, 17 Nov 2023 14:21:23 -0500 Subject: [PATCH] Assign mojom `kSwitch` role to switches (flutter/engine#48146) We have previously been using the `kToggleButton` role for any widget that has a toggled state. Our AX library does not expect this role to be used for switches, and so would not assign the checked state to switches for MSAA. As far as I can tell, the `Switch` is the only widget that uses the `toggled` semantic property (`ToggleButtons`, ironically, does not), so we ought to be able to swap in the `kSwitch` role, for which the proper states are presented. This will allow screen readers to announce the state of a switch. https://github.com/flutter/flutter/issues/138500 ## Pre-launch Checklist - [x] I read the [Contributor Guide] and followed the process outlined there for submitting PRs. - [x] I read the [Tree Hygiene] wiki page, which explains my responsibilities. - [x] I read and followed the [Flutter Style Guide] and the [C++, Objective-C, Java style guides]. - [x] I listed at least one issue that this PR fixes in the description above. - [x] I added new tests to check the change I am making or feature I am adding, or the PR is [test-exempt]. See [testing the engine] for instructions on writing and running engine tests. - [x] I updated/added relevant documentation (doc comments with `///`). - [ ] I signed the [CLA]. - [x] All existing and new tests are passing. If you need help, consider asking for advice on the #hackers-new channel on [Discord]. [Contributor Guide]: https://github.com/flutter/flutter/wiki/Tree-hygiene#overview [Tree Hygiene]: https://github.com/flutter/flutter/wiki/Tree-hygiene [test-exempt]: https://github.com/flutter/flutter/wiki/Tree-hygiene#tests [Flutter Style Guide]: https://github.com/flutter/flutter/wiki/Style-guide-for-Flutter-repo [C++, Objective-C, Java style guides]: https://github.com/flutter/engine/blob/main/CONTRIBUTING.md#style [testing the engine]: https://github.com/flutter/flutter/wiki/Testing-the-engine [CLA]: https://cla.developers.google.com/ [flutter/tests]: https://github.com/flutter/tests [breaking change policy]: https://github.com/flutter/flutter/wiki/Tree-hygiene#handling-breaking-changes [Discord]: https://github.com/flutter/flutter/wiki/Chat --- .../flutter/shell/platform/common/accessibility_bridge.cc | 4 ++-- .../shell/platform/common/accessibility_bridge_unittests.cc | 4 ++-- .../platform/windows/flutter_windows_view_unittests.cc | 6 ++++-- 3 files changed, 8 insertions(+), 6 deletions(-) diff --git a/engine/src/flutter/shell/platform/common/accessibility_bridge.cc b/engine/src/flutter/shell/platform/common/accessibility_bridge.cc index db5f507fe8..9a6e02065e 100644 --- a/engine/src/flutter/shell/platform/common/accessibility_bridge.cc +++ b/engine/src/flutter/shell/platform/common/accessibility_bridge.cc @@ -333,7 +333,7 @@ void AccessibilityBridge::SetRoleFromFlutterUpdate(ui::AXNodeData& node_data, return; } if (flags & kFlutterSemanticsFlagHasToggledState) { - node_data.role = ax::mojom::Role::kToggleButton; + node_data.role = ax::mojom::Role::kSwitch; return; } if (flags & kFlutterSemanticsFlagIsSlider) { @@ -477,7 +477,7 @@ void AccessibilityBridge::SetIntAttributesFromFlutterUpdate( : flags & FlutterSemanticsFlag::kFlutterSemanticsFlagIsChecked ? ax::mojom::CheckedState::kTrue : ax::mojom::CheckedState::kFalse)); - } else if (node_data.role == ax::mojom::Role::kToggleButton) { + } else if (node_data.role == ax::mojom::Role::kSwitch) { 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 fb0c839b77..796670e3ab 100644 --- a/engine/src/flutter/shell/platform/common/accessibility_bridge_unittests.cc +++ b/engine/src/flutter/shell/platform/common/accessibility_bridge_unittests.cc @@ -253,7 +253,7 @@ TEST(AccessibilityBridgeTest, DoesNotAssignEditableRootToSelectableText) { ax::mojom::BoolAttribute::kEditableRoot)); } -TEST(AccessibilityBridgeTest, ToggleHasToggleButtonRole) { +TEST(AccessibilityBridgeTest, SwitchHasSwitchRole) { std::shared_ptr bridge = std::make_shared(); FlutterSemanticsNode2 root = CreateSemanticsNode(0, "root"); @@ -265,7 +265,7 @@ TEST(AccessibilityBridgeTest, ToggleHasToggleButtonRole) { bridge->CommitUpdates(); auto root_node = bridge->GetFlutterPlatformNodeDelegateFromID(0).lock(); - EXPECT_EQ(root_node->GetData().role, ax::mojom::Role::kToggleButton); + EXPECT_EQ(root_node->GetData().role, ax::mojom::Role::kSwitch); } TEST(AccessibilityBridgeTest, SliderHasSliderRole) { 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 6f4d5fc856..39cea644c7 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 @@ -1081,7 +1081,7 @@ TEST(FlutterWindowsViewTest, SwitchNativeState) { { auto root_node = bridge->GetFlutterPlatformNodeDelegateFromID(0).lock(); - EXPECT_EQ(root_node->GetData().role, ax::mojom::Role::kToggleButton); + EXPECT_EQ(root_node->GetData().role, ax::mojom::Role::kSwitch); EXPECT_EQ(root_node->GetData().GetCheckedState(), ax::mojom::CheckedState::kTrue); @@ -1104,6 +1104,7 @@ TEST(FlutterWindowsViewTest, SwitchNativeState) { VARIANT native_state = {}; ASSERT_TRUE(SUCCEEDED(native_view->get_accState(varchild, &native_state))); EXPECT_TRUE(native_state.lVal & STATE_SYSTEM_PRESSED); + EXPECT_TRUE(native_state.lVal & STATE_SYSTEM_CHECKED); // Test similarly on UIA node. IRawElementProviderSimple* uia_node; @@ -1129,7 +1130,7 @@ TEST(FlutterWindowsViewTest, SwitchNativeState) { { auto root_node = bridge->GetFlutterPlatformNodeDelegateFromID(0).lock(); - EXPECT_EQ(root_node->GetData().role, ax::mojom::Role::kToggleButton); + EXPECT_EQ(root_node->GetData().role, ax::mojom::Role::kSwitch); EXPECT_EQ(root_node->GetData().GetCheckedState(), ax::mojom::CheckedState::kFalse); @@ -1146,6 +1147,7 @@ TEST(FlutterWindowsViewTest, SwitchNativeState) { VARIANT native_state = {}; ASSERT_TRUE(SUCCEEDED(native_view->get_accState(varchild, &native_state))); EXPECT_FALSE(native_state.lVal & STATE_SYSTEM_PRESSED); + EXPECT_FALSE(native_state.lVal & STATE_SYSTEM_CHECKED); // Test similarly on UIA node. IRawElementProviderSimple* uia_node;