Skip to content

Commit

Permalink
[Read Anything] Fix crash when triple-clicking selection.
Browse files Browse the repository at this point in the history
Also helps with issue in Gmail when dragging the selection beyond its
bounds.

Broken off from: https://chromium-review.googlesource.com/c/chromium/src/+/4437503

Bug: 1266555
Change-Id: Iea1af273ed9e3d128ccda81113ae1d94aaf26eea
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4443470
Reviewed-by: Abigail Klein <abigailbklein@google.com>
Commit-Queue: Lauren Winston <lwinston@google.com>
Cr-Commit-Position: refs/heads/main@{#1132693}
  • Loading branch information
Lauren Winston authored and Chromium LUCI CQ committed Apr 19, 2023
1 parent 22b21c7 commit 139c711
Show file tree
Hide file tree
Showing 2 changed files with 57 additions and 0 deletions.
13 changes: 13 additions & 0 deletions chrome/renderer/accessibility/read_anything_app_controller.cc
Original file line number Diff line number Diff line change
Expand Up @@ -697,6 +697,19 @@ void ReadAnythingAppController::OnSelectionChange(ui::AXNodeID anchor_node_id,
return;
}

ui::AXNode* focus_node = model_.GetAXNode(focus_node_id);
ui::AXNode* anchor_node = model_.GetAXNode(anchor_node_id);
// Some text fields, like Gmail, allow a <div> to be returned as a focus
// node for selection, most frequently when a triple click causes an entire
// range of text to be selected, including non-text nodes. This can cause
// inconsistencies in how the selection is handled. e.g. the focus node can
// be before the anchor node and set to a non-text node, which can cause
// page_handler_->OnSelectionChange to be incorrectly triggered, resulting in
// a failing DCHECK. Therefore, return early if this happens.
if (!focus_node->IsText() || !anchor_node->IsText()) {
return;
}

// If the selection change matches the tree's selection, this means it was
// set by the controller. Javascript selections set by the controller are
// always forward selections. This means the anchor node always comes before
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -1077,6 +1077,16 @@ TEST_F(ReadAnythingAppControllerTest, OnLinkClicked_DistillationInProgress) {
}

TEST_F(ReadAnythingAppControllerTest, OnSelectionChange) {
ui::AXTreeUpdate update;
SetUpdateTreeID(&update);
update.nodes.resize(3);
update.nodes[0].id = 2;
update.nodes[1].id = 3;
update.nodes[2].id = 4;
update.nodes[0].role = ax::mojom::Role::kStaticText;
update.nodes[1].role = ax::mojom::Role::kStaticText;
update.nodes[2].role = ax::mojom::Role::kStaticText;
AccessibilityEventReceived({update});
ui::AXNodeID anchor_node_id = 2;
int anchor_offset = 0;
ui::AXNodeID focus_node_id = 3;
Expand All @@ -1090,6 +1100,15 @@ TEST_F(ReadAnythingAppControllerTest, OnSelectionChange) {

TEST_F(ReadAnythingAppControllerTest,
OnSelectionChange_ClickDoesNotUpdateSelection) {
ui::AXTreeUpdate update;
SetUpdateTreeID(&update);
update.nodes.resize(3);
update.nodes[0].id = 2;
update.nodes[1].id = 3;
update.nodes[2].id = 4;
update.nodes[0].role = ax::mojom::Role::kStaticText;
update.nodes[1].role = ax::mojom::Role::kStaticText;
update.nodes[2].role = ax::mojom::Role::kStaticText;
ui::AXNodeID anchor_node_id = 2;
int anchor_offset = 15;
ui::AXNodeID focus_node_id = 2;
Expand All @@ -1109,6 +1128,7 @@ TEST_F(ReadAnythingAppControllerTest,
SetUpdateTreeID(&update, new_tree_id);
update.root_id = 1;
update.nodes.resize(1);
update.nodes[0].role = ax::mojom::Role::kStaticText;
update.nodes[0].id = 1;
AccessibilityEventReceived({update});
EXPECT_CALL(*distiller_, Distill).Times(1);
Expand All @@ -1120,6 +1140,30 @@ TEST_F(ReadAnythingAppControllerTest,
page_handler_.FlushForTesting();
}

TEST_F(ReadAnythingAppControllerTest,
OnSelectionChange_NonTextFieldDoesNotUpdateSelection) {
ui::AXTreeUpdate update;
SetUpdateTreeID(&update);
update.nodes.resize(3);
update.nodes[0].id = 2;
update.nodes[1].id = 3;
update.nodes[2].id = 4;
update.nodes[0].role = ax::mojom::Role::kTextField;
update.nodes[1].role = ax::mojom::Role::kGenericContainer;
update.nodes[2].role = ax::mojom::Role::kTextField;
AccessibilityEventReceived({update});
ui::AXNodeID anchor_node_id = 2;
int anchor_offset = 0;
ui::AXNodeID focus_node_id = 3;
int focus_offset = 1;
EXPECT_CALL(page_handler_,
OnSelectionChange(tree_id_, anchor_node_id, anchor_offset,
focus_node_id, focus_offset))
.Times(0);
OnSelectionChange(anchor_node_id, anchor_offset, focus_node_id, focus_offset);
page_handler_.FlushForTesting();
}

TEST_F(ReadAnythingAppControllerTest, Selection_Forward) {
// Create selection from node 3-4.
ui::AXTreeUpdate update;
Expand Down

0 comments on commit 139c711

Please sign in to comment.