Skip to content
This repository was archived by the owner on Feb 25, 2025. It is now read-only.

Limit selection change to focused node on Windows #38634

Merged
merged 6 commits into from
Jan 6, 2023
Merged

Limit selection change to focused node on Windows #38634

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
15f9811
Limit selection change to focused node on Windows
yaakovschectman Jan 4, 2023
6ff2b0f
Focus fix
yaakovschectman Jan 5, 2023
22a7a52
Test document selection change
yaakovschectman Jan 5, 2023
53a07d8
Comment
yaakovschectman Jan 5, 2023
ea0ccec
Formatting
yaakovschectman Jan 5, 2023
1fd98a6
Update shell/platform/windows/accessibility_bridge_windows.cc
yaakovschectman Jan 5, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 3 additions & 2 deletions shell/platform/common/accessibility_bridge.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -527,11 +527,12 @@ void AccessibilityBridge::SetTooltipFromFlutterUpdate(
void AccessibilityBridge::SetTreeData(const SemanticsNode& node,
ui::AXTreeUpdate& tree_update) {
FlutterSemanticsFlag flags = node.flags;
// Set selection if:
// Set selection of the focused node if:
// 1. this text field has a valid selection
// 2. this text field doesn't have a valid selection but had selection stored
// in the tree.
if (flags & FlutterSemanticsFlag::kFlutterSemanticsFlagIsTextField) {
if (flags & FlutterSemanticsFlag::kFlutterSemanticsFlagIsTextField &&
flags & FlutterSemanticsFlag::kFlutterSemanticsFlagIsFocused) {
if (node.text_selection_base != -1) {
tree_update.tree_data.sel_anchor_object_id = node.id;
tree_update.tree_data.sel_anchor_offset = node.text_selection_base;
Expand Down
4 changes: 3 additions & 1 deletion shell/platform/common/accessibility_bridge_unittests.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -152,7 +152,9 @@ TEST(AccessibilityBridgeTest, canHandleSelectionChangeCorrectly) {
std::shared_ptr<TestAccessibilityBridge> bridge =
std::make_shared<TestAccessibilityBridge>();
FlutterSemanticsNode root = CreateSemanticsNode(0, "root");
root.flags = FlutterSemanticsFlag::kFlutterSemanticsFlagIsTextField;
root.flags = static_cast<FlutterSemanticsFlag>(
FlutterSemanticsFlag::kFlutterSemanticsFlagIsTextField |
FlutterSemanticsFlag::kFlutterSemanticsFlagIsFocused);
bridge->AddFlutterSemanticsNodeUpdate(&root);
bridge->CommitUpdates();

Expand Down
15 changes: 14 additions & 1 deletion shell/platform/windows/accessibility_bridge_windows.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -41,10 +41,23 @@ void AccessibilityBridgeWindows::OnAccessibilityEvent(
DispatchWinAccessibilityEvent(win_delegate,
ax::mojom::Event::kChildrenChanged);
break;
case ui::AXEventGenerator::Event::DOCUMENT_SELECTION_CHANGED:
case ui::AXEventGenerator::Event::DOCUMENT_SELECTION_CHANGED: {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could we add a comment similar to Chromium's comment? https://source.chromium.org/chromium/chromium/src/+/main:content/browser/accessibility/browser_accessibility_manager_win.cc;l=230-234?q=case%20ui::AXEventGenerator::Event::DOCUMENT_SELECTION_CHANGED&ss=chromium

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does this part of the comment make sense for us too?

This is because the focus is the only endpoint that can move, and because the caret (if present) is at the focus.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I do not think so, as we do not have the carat as its own node.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Perhaps I'm misunderstanding, but my reading of that comment is that both the selection & the caret - if they exist - must be on the focus node.

// An event indicating a change in document selection should be fired
// only for the focused node whose selection has changed. If a valid
// caret and selection exist in the app tree, they must both be within
// the focus node.
ui::AXNode::AXID focus_id = GetAXTreeData().sel_focus_object_id;
auto focus_delegate =
GetFlutterPlatformNodeDelegateFromID(focus_id).lock();
Copy link
Member

@loic-sharma loic-sharma Jan 4, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are we guaranteed to get a focus delegate? I see the corresponding Chromium logic checks if it was able to find a focus object: https://source.chromium.org/chromium/chromium/src/+/main:content/browser/accessibility/browser_accessibility_manager_win.cc;l=235-237?q=case%20ui::AXEventGenerator::Event::DOCUMENT_SELECTION_CHANGED&ss=chromium

Copy link
Member

@loic-sharma loic-sharma Jan 5, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we have a test for this scenario? If no, can we add one?

if (!focus_delegate) {
win_delegate =
std::static_pointer_cast<FlutterPlatformNodeDelegateWindows>(
focus_delegate);
}
DispatchWinAccessibilityEvent(
win_delegate, ax::mojom::Event::kDocumentSelectionChanged);
break;
}
case ui::AXEventGenerator::Event::FOCUS_CHANGED:
DispatchWinAccessibilityEvent(win_delegate, ax::mojom::Event::kFocus);
SetFocus(win_delegate);
Expand Down
6 changes: 6 additions & 0 deletions shell/platform/windows/accessibility_bridge_windows_unittests.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -338,5 +338,11 @@ TEST(AccessibilityBridgeWindows, OnAccessibilityStateChanged) {
ax::mojom::Event::kStateChanged);
}

TEST(AccessibilityBridgeWindows, OnDocumentSelectionChanged) {
ExpectWinEventFromAXEvent(
1, ui::AXEventGenerator::Event::DOCUMENT_SELECTION_CHANGED,
ax::mojom::Event::kDocumentSelectionChanged);
}

} // namespace testing
} // namespace flutter