Skip to content

Commit

Permalink
[fuchsia][a11y] Refactor node updates.
Browse files Browse the repository at this point in the history 
Currently, it's possible for chromium to send multiple updates for the
same node in the same atomic tree update. This change ensures that
fuchsia only receives one update per node per tree update.

TEST: manual, unit/browser tests
Bug: fuchsia:77217
Change-Id: I8be5c5e1eb574e881f285714f15dd1861f396797
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2910478
Reviewed-by: Sergey Ulanov <sergeyu@chromium.org>
Commit-Queue: Alexander Brusher <abrusher@google.com>
Cr-Commit-Position: refs/heads/master@{#887043}
  • Loading branch information
@abrush21
abrush21 authored and Chromium LUCI CQ committed May 27, 2021
1 parent 2dd7453 commit a0b666e
Show file tree
Hide file tree
Showing 3 changed files with 189 additions and 47 deletions.
130 changes: 87 additions & 43 deletions fuchsia/engine/browser/accessibility_bridge.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -109,8 +109,17 @@ AccessibilityBridge::~AccessibilityBridge() {
ax_trees_.clear();
}

void AccessibilityBridge::AddNodeToOffsetMapping(
const ui::AXTree* tree,
const ui::AXNodeData& node_data) {
auto ax_tree_id = tree->GetAXTreeID();
auto offset_container_id = GetOffsetContainerId(tree, node_data);
offset_container_children_[std::make_pair(ax_tree_id, offset_container_id)]
.insert(std::make_pair(ax_tree_id, node_data.id));
}

void AccessibilityBridge::RemoveNodeFromOffsetMapping(
ui::AXTree* tree,
const ui::AXTree* tree,
const ui::AXNodeData& node_data) {
auto offset_container_children_it =
offset_container_children_.find(std::make_pair(
Expand Down Expand Up @@ -324,8 +333,18 @@ void AccessibilityBridge::OnSemanticsModeChanged(
callback();
}

void AccessibilityBridge::OnNodeCreated(ui::AXTree* tree, ui::AXNode* node) {
DCHECK(tree);
DCHECK(node);

AddNodeToOffsetMapping(tree, node->data());
}

void AccessibilityBridge::OnNodeWillBeDeleted(ui::AXTree* tree,
ui::AXNode* node) {
DCHECK(tree);
DCHECK(node);

// Remove the node from its offset container's list of children.
RemoveNodeFromOffsetMapping(tree, node->data());

Expand All @@ -335,6 +354,8 @@ void AccessibilityBridge::OnNodeWillBeDeleted(ui::AXTree* tree,
}

void AccessibilityBridge::OnNodeDeleted(ui::AXTree* tree, int32_t node_id) {
DCHECK(tree);

to_delete_.push_back(
id_mapper_->ToFuchsiaNodeID(tree->GetAXTreeID(), node_id, false));
}
Expand All @@ -343,45 +364,59 @@ void AccessibilityBridge::OnNodeDataChanged(
ui::AXTree* tree,
const ui::AXNodeData& old_node_data,
const ui::AXNodeData& new_node_data) {
if (!tree)
return;
DCHECK(tree);

// If this node's offset container has changed, then we should remove it from
// its old offset container's offset children and add it to its new offset
// container's children.
if (old_node_data.relative_bounds.offset_container_id !=
new_node_data.relative_bounds.offset_container_id) {
RemoveNodeFromOffsetMapping(tree, old_node_data);
AddNodeToOffsetMapping(tree, new_node_data);
}

// If this node's bounds have changed, then we should update its offset
// children's transforms to reflect the new bounds.
if (old_node_data.relative_bounds.bounds ==
new_node_data.relative_bounds.bounds) {
return;
}

auto offset_container_children_it = offset_container_children_.find(
std::make_pair(tree->GetAXTreeID(), old_node_data.id));

// If any descendants have this node as their offset containers, and this
// node's bounds have changed, then we need to update those descendants'
// transforms to reflect the new bounds.
if (offset_container_children_it != offset_container_children_.end() &&
old_node_data.relative_bounds.bounds !=
new_node_data.relative_bounds.bounds) {
for (auto offset_child_id : offset_container_children_it->second) {
auto* child_node = tree->GetFromId(offset_child_id.second);
if (!child_node) {
continue;
}
if (offset_container_children_it == offset_container_children_.end())
return;

auto child_node_data = child_node->data();
to_update_.push_back(AXNodeDataToSemanticNode(
child_node_data, new_node_data, tree->GetAXTreeID(), false,
id_mapper_.get()));
}
}
for (auto offset_child_id : offset_container_children_it->second) {
auto* child_node = tree->GetFromId(offset_child_id.second);
if (!child_node)
continue;

// If this node's offset container has changed, then we should remove it from
// its old offset container's offset children.
if (old_node_data.relative_bounds.offset_container_id !=
new_node_data.relative_bounds.offset_container_id) {
RemoveNodeFromOffsetMapping(tree, old_node_data);
auto child_node_data = child_node->data();

// If the offset container for |child_node| does NOT change during this
// atomic update, then the update produced here will be correct.
//
// If the offset container for |child_node| DOES change during this atomic
// update, then depending on the order of the individual node updates, the
// update we produce here could be incorrect. However, in that case,
// OnAtomicUpdateFinished() will see a change for |child_node|. By the time
// that OnAtomicUpdateFinished() is called, offset_container_children_ will
// be correct, so we can simply overwrite the existing update.
auto* fuchsia_node =
GetUpdatedNode(tree->GetAXTreeID(), child_node->data().id,
/*replace_existing=*/true);
DCHECK(fuchsia_node);
}
}

void AccessibilityBridge::OnAtomicUpdateFinished(
ui::AXTree* tree,
bool root_changed,
const std::vector<ui::AXTreeObserver::Change>& changes) {
DCHECK(tree);

if (root_changed)
MaybeDisconnectTreeFromParentTree(tree);

Expand All @@ -398,25 +433,21 @@ void AccessibilityBridge::OnAtomicUpdateFinished(
for (const ui::AXTreeObserver::Change& change : changes) {
const auto& node = change.node->data();

int32_t offset_container_id =
GetOffsetContainerId(tree, change.node->data());
const auto* container = tree->GetFromId(offset_container_id);
DCHECK(container);

offset_container_children_[std::make_pair(tree->GetAXTreeID(),
offset_container_id)]
.insert(std::make_pair(tree->GetAXTreeID(), node.id));
// Get the updated fuchsia representation of the node. It's possible that
// there's an existing update for this node from OnNodeDataChanged(). This
// update may not have the correct offset container and/or transform, so we
// should replace it.
auto* fuchsia_node = GetUpdatedNode(tree->GetAXTreeID(), node.id,
/*replace_existing=*/true);
DCHECK(fuchsia_node);

const bool is_root = is_main_frame_tree ? node.id == root_id_ : false;
to_update_.push_back(AXNodeDataToSemanticNode(node, container->data(),
tree->GetAXTreeID(), is_root,
id_mapper_.get()));
if (node.HasStringAttribute(ax::mojom::StringAttribute::kChildTreeId)) {
const auto child_tree_id = ui::AXTreeID::FromString(
node.GetStringAttribute(ax::mojom::StringAttribute::kChildTreeId));
tree_connections_[child_tree_id] = {node.id, tree->GetAXTreeID(), false};
}
}

UpdateTreeConnections();
UpdateFocus();
// TODO(https://crbug.com/1134737): Separate updates of atomic updates and
Expand Down Expand Up @@ -492,7 +523,8 @@ void AccessibilityBridge::UpdateTreeConnections() {
if (kv.second.is_connected)
continue; // No work to do, trees connected and present.

auto* fuchsia_node = GetUpdatedNode(parent_ax_tree_id, ax_node->id());
auto* fuchsia_node = GetUpdatedNode(parent_ax_tree_id, ax_node->id(),
/*replace_existing=*/false);
DCHECK(fuchsia_node);
// Now, the connection really happens:
// This node, from the parent tree, will have a child that points to the
Expand Down Expand Up @@ -523,7 +555,8 @@ void AccessibilityBridge::UpdateFocus() {
// as it is redundant.
auto* node =
focus_changed
? GetUpdatedNode(new_focused_node->first, new_focused_node->second)
? GetUpdatedNode(new_focused_node->first, new_focused_node->second,
/*replace_existing=*/false)
: GetNodeIfChangingInUpdate(new_focused_node->first,
new_focused_node->second);
if (node)
Expand All @@ -532,7 +565,8 @@ void AccessibilityBridge::UpdateFocus() {

if (last_focused_node_id_) {
auto* node = focus_changed ? GetUpdatedNode(last_focused_node_id_->first,
last_focused_node_id_->second)
last_focused_node_id_->second,
/*replace_existing=*/false)
: nullptr /*already updated above*/;
if (node)
node->mutable_states()->set_has_input_focus(false);
Expand Down Expand Up @@ -697,9 +731,10 @@ AccessibilityBridge::GetNodeIfChangingInUpdate(const ui::AXTreeID& tree_id,

fuchsia::accessibility::semantics::Node* AccessibilityBridge::GetUpdatedNode(
const ui::AXTreeID& tree_id,
ui::AXNodeID node_id) {
ui::AXNodeID node_id,
bool replace_existing) {
auto* fuchsia_node = GetNodeIfChangingInUpdate(tree_id, node_id);
if (fuchsia_node)
if (fuchsia_node && !replace_existing)
return fuchsia_node;

auto ax_tree_it = ax_trees_.find(tree_id);
Expand All @@ -715,8 +750,17 @@ fuchsia::accessibility::semantics::Node* AccessibilityBridge::GetUpdatedNode(
const auto* container = tree->GetFromId(offset_container_id);
DCHECK(container);

const bool is_main_frame_tree =
tree->GetAXTreeID() == web_contents_->GetMainFrame()->GetAXTreeID();
const bool is_root = is_main_frame_tree ? node_id == root_id_ : false;
auto new_fuchsia_node = AXNodeDataToSemanticNode(
ax_node->data(), container->data(), tree_id, false, id_mapper_.get());
ax_node->data(), container->data(), tree_id, is_root, id_mapper_.get());

if (replace_existing && fuchsia_node) {
*fuchsia_node = std::move(new_fuchsia_node);
return fuchsia_node;
}

to_update_.push_back(std::move(new_fuchsia_node));
return &to_update_.back();
}
19 changes: 15 additions & 4 deletions fuchsia/engine/browser/accessibility_bridge.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -80,6 +80,7 @@ class WEB_ENGINE_EXPORT AccessibilityBridge
UpdateTransformWhenContainerBoundsChange);
FRIEND_TEST_ALL_PREFIXES(AccessibilityBridgeTest,
OffsetContainerBookkeepingIsUpdated);
FRIEND_TEST_ALL_PREFIXES(AccessibilityBridgeTest, OneUpdatePerNode);

// Represents a connection between two AXTrees that are in different frames.
struct TreeConnection {
Expand Down Expand Up @@ -131,9 +132,14 @@ class WEB_ENGINE_EXPORT AccessibilityBridge
// in tests.
float GetDeviceScaleFactor();

// Helper method to add a node to its offset container's offset children
// mapping.
void AddNodeToOffsetMapping(const ui::AXTree* tree,
const ui::AXNodeData& node_data);

// Helper method to remove a node id from its offset container's offset
// children mapping.
void RemoveNodeFromOffsetMapping(ui::AXTree* tree,
void RemoveNodeFromOffsetMapping(const ui::AXTree* tree,
const ui::AXNodeData& node_data);

// Helper method to return the node in focus. Returns nullptr if the main
Expand All @@ -150,11 +156,15 @@ class WEB_ENGINE_EXPORT AccessibilityBridge

// Helper method to get the most recently updated fuchsia representation of
// the node. Note that it differs from |GetNodeIfChangingInUpdate| because
// here a node will be created to be part of the update if it is not. Returns
// nullptr if the node does not exist.
// here a node will be created to be part of the update if it is not. If
// |replace_existing| is set to true, then this method will overwrite the
// existing update for the node (if one exists).
//
// Returns nullptr if the node does not exist.
fuchsia::accessibility::semantics::Node* GetUpdatedNode(
const ui::AXTreeID& tree_id,
ui::AXNodeID node_id);
ui::AXNodeID node_id,
bool replace_existing);

// Returns the node in focus in this frame or in one of its descendants if the
// node in focus points to a child frame.
Expand All @@ -177,6 +187,7 @@ class WEB_ENGINE_EXPORT AccessibilityBridge
OnSemanticsModeChangedCallback callback) final;

// ui::AXTreeObserver implementation.
void OnNodeCreated(ui::AXTree* tree, ui::AXNode* node) override;
void OnNodeWillBeDeleted(ui::AXTree* tree, ui::AXNode* node) override;
void OnNodeDeleted(ui::AXTree* tree, int32_t node_id) override;
void OnAtomicUpdateFinished(
Expand Down
87 changes: 87 additions & 0 deletions fuchsia/engine/browser/accessibility_bridge_browsertest.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -957,6 +957,93 @@ IN_PROC_BROWSER_TEST_F(AccessibilityBridgeTest,
EXPECT_EQ(fuchsia_node->transform().matrix[13], 3);
}

// This test ensures that fuchsia only receives one update per node.
IN_PROC_BROWSER_TEST_F(AccessibilityBridgeTest, OneUpdatePerNode) {
// Loads a page, so a real frame is created for this test. Then, several tree
// operations are applied on top of it, using the AXTreeID that corresponds to
// that frame.
LoadPage(kPage1Path, kPage1Title);
semantics_manager_.semantic_tree()->RunUntilNodeCountAtLeast(kPage1NodeCount);

// Fetch the AXTreeID of the main frame (the page just loaded). This ID will
// be used in the operations that follow to simulate new data coming in.
auto tree_id =
frame_impl_->web_contents_for_test()->GetMainFrame()->GetAXTreeID();

AccessibilityBridge* bridge = frame_impl_->accessibility_bridge_for_test();
size_t tree_size = 5;

// The tree has the following form: (1 (2 (3 (4 (5)))))
auto tree_accessibility_event = CreateTreeAccessibilityEvent(tree_size);
tree_accessibility_event.ax_tree_id = tree_id;

// The root of this tree needs to be cleared (because it holds the page just
// loaded, and we are loading something completely new).
tree_accessibility_event.updates[0].node_id_to_clear =
bridge->ax_tree_for_test()->root()->id();

// Set a name in a node so we can wait for this node to appear. This pattern
// is used throughout this test to ensure that the new data we are waiting for
// arrived.
tree_accessibility_event.updates[0].nodes[0].SetName(kUpdate1Name);

bridge->AccessibilityEventReceived(tree_accessibility_event);

semantics_manager_.semantic_tree()->RunUntilNodeWithLabelIsInTree(
kUpdate1Name);

// Mark node 2 as node 3's offset container. Below, we will send an update to
// change node 3's offset container to node 1, so that we can verify that the
// fuchsia node produced reflects that update.
bridge->offset_container_children_[std::make_pair(tree_id, 2)].insert(
std::make_pair(tree_id, 3));

// Send three updates:
// 1. Change bounds for node 1.
// 2. Change bounds for node 2. Since node 3 is marked as node 2's offset
// child (above), OnNodeDataChanged() should produce an update for node 3 that
// includes a transform accounting for node 2's new bounds.
// 3. Change offset container for node 3 from node 2 to node 1.
// OnAtomicUpdateFinished() should replace the now-incorrect update from step
// (2) with a new update that includes a transform accounting for node 1's
// bounds.
const char kNodeName[] = "transform should update";
// Changes the bounds of node 1.
// (1 (2 (3 (4 (5)))))
ui::AXTreeUpdate update;
update.root_id = 1;
update.nodes.resize(3);
update.nodes[0].id = 1;
// Update the relative bounds of node 1, which is node 2's offset container.
auto new_root_bounds = gfx::RectF(2, 3, 4, 5);
update.nodes[0].relative_bounds.bounds = new_root_bounds;
update.nodes[0].child_ids = {2};
update.nodes[0].SetName(kUpdate2Name);
update.nodes[1].id = 2;
update.nodes[1].relative_bounds.bounds = gfx::RectF(20, 30, 40, 50);
update.nodes[1].child_ids = {3};
update.nodes[2].id = 3;
update.nodes[2].relative_bounds.offset_container_id = 1u;
update.nodes[2].SetName(kNodeName);

bridge->AccessibilityEventReceived(
CreateAccessibilityEventWithUpdate(std::move(update), tree_id));
semantics_manager_.semantic_tree()->RunUntilNodeWithLabelIsInTree(kNodeName);

// Verify that the transform for the Fuchsia semantic node corresponding to
// node 3 reflects the new bounds of node 1.
fuchsia::accessibility::semantics::Node* fuchsia_node =
semantics_manager_.semantic_tree()->GetNodeFromLabel(kNodeName);

// A Fuchsia node's semantic transform should include an offset for its parent
// node as a post-translation on top of its existing transform. Therefore, the
// x, y, and z scale (indices 0, 5, and 10, respectively) should remain
// unchanged, and the x and y bounds of the offset container should be added
// to the node's existing translation entries (indices 12 and 13).
EXPECT_EQ(fuchsia_node->transform().matrix[12], new_root_bounds.x());
EXPECT_EQ(fuchsia_node->transform().matrix[13], new_root_bounds.y());
}

IN_PROC_BROWSER_TEST_F(AccessibilityBridgeTest, OutOfProcessIframe) {
constexpr int64_t kBindingsId = 1234;

Expand Down

0 comments on commit a0b666e

Please sign in to comment.