[fuchsia][a11y] Accessibility Bridge forwards valid updates.

This change refactors parts of the Accessibility Bridge to solve the
following bugs: * Previously, it was deleting nodes that were marked as
reparented or a subtree that was reparented. This assumed that nodes
marked this way would appear in the list of updates sent by the
AxTreeObserver in the final atomic update, but this assumption is not
correct. This fixes the problem where parents were pointing to children
no longer in the tree.

* Allow trees of size 1 to be sent. Previously, the loop that dealt with
  sending updates started from the second item, which would not send
  anything if the tree had only one element.

* Simplifies the order of updates: first deletions and then
  updates. Because atomic updates always finish with the list of nodes
  added / changed, we can use this property to simplify the code to
  always follow this order.

Todos: There are some todos left in the code which I opted to follow-up
in a future cl so that I can split work among people and parallelize.

Test: AccessibilityBridgeTest.*
Bug: 1134384,fuchsia:58989,fuchsia:50187,fuchsia:61155
Change-Id: I6ec09a5d6fa5d5ad37fb42a059d0d0f122d39d97
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2446358
Commit-Queue: Lucas Radaelli <lucasradaelli@google.com>
Reviewed-by: Sergey Ulanov <sergeyu@chromium.org>
Reviewed-by: Sharon Yang <yangsharon@chromium.org>
Cr-Commit-Position: refs/heads/master@{#815346}

fuchsia/engine/browser/accessibility_bridge.cc

@@ -4,6 +4,8 @@
 
 #include "fuchsia/engine/browser/accessibility_bridge.h"
 
+#include <algorithm>
+
 #include <lib/sys/cpp/component_context.h>
 #include <lib/ui/scenic/cpp/view_ref_pair.h>
 
@@ -15,8 +17,6 @@
 
 namespace {
 
-constexpr uint32_t kSemanticNodeRootId = 0;
-
 // TODO(https://crbug.com/973095): Update this value based on average and
 // maximum sizes of serialized Semantic Nodes.
 constexpr size_t kMaxNodesPerUpdate = 16;
@@ -51,60 +51,36 @@ AccessibilityBridge::~AccessibilityBridge() {
 }
 
 void AccessibilityBridge::TryCommit() {
-  if (commit_inflight_ || to_send_.empty())
+  if (commit_inflight_ || (to_delete_.empty() && to_update_.empty()))
     return;
 
-  SemanticUpdateOrDelete::Type current = to_send_.at(0).type;
-  int range_start = 0;
-  for (size_t i = 1; i < to_send_.size(); i++) {
-    if (to_send_.at(i).type == current &&
-        (i - range_start < kMaxNodesPerUpdate)) {
-      continue;
-    } else {
-      DispatchSemanticsMessages(range_start, i - range_start);
-      current = to_send_.at(i).type;
-      range_start = i;
-    }
+  // Deletions come before updates because first the nodes are deleted, and
+  // then we update the parents to no longer point at them.
+  if (!to_delete_.empty())
+    semantic_tree_->DeleteSemanticNodes(std::move(to_delete_));
+
+  size_t start = 0;
+  while (start < to_update_.size()) {
+    // TODO(https://crbug.com/1134727): AccessibilityBridge must respect FIDL
+    // size limits.
+    size_t end =
+        start + std::min(kMaxNodesPerUpdate, to_update_.size() - start);
+    decltype(to_update_) batch;
+    std::move(to_update_.begin() + start, to_update_.begin() + end,
+              std::back_inserter(batch));
+    semantic_tree_->UpdateSemanticNodes(std::move(batch));
+    start = end;
   }
-  DispatchSemanticsMessages(range_start, to_send_.size() - range_start);
-
   semantic_tree_->CommitUpdates(
       fit::bind_member(this, &AccessibilityBridge::OnCommitComplete));
   commit_inflight_ = true;
-  to_send_.clear();
-}
-
-void AccessibilityBridge::DispatchSemanticsMessages(size_t start, size_t size) {
-  if (to_send_.at(start).type == SemanticUpdateOrDelete::Type::UPDATE) {
-    std::vector<fuchsia::accessibility::semantics::Node> updates;
-    for (size_t i = start; i < start + size; i++) {
-      DCHECK(to_send_.at(i).type == SemanticUpdateOrDelete::Type::UPDATE);
-      updates.push_back(std::move(to_send_.at(i).update_node));
-    }
-    semantic_tree_->UpdateSemanticNodes(std::move(updates));
-  } else if (to_send_.at(start).type == SemanticUpdateOrDelete::Type::DELETE) {
-    std::vector<uint32_t> deletes;
-    for (size_t i = start; i < start + size; i++) {
-      DCHECK(to_send_.at(i).type == SemanticUpdateOrDelete::Type::DELETE);
-      deletes.push_back(to_send_.at(i).id_to_delete);
-    }
-    semantic_tree_->DeleteSemanticNodes(deletes);
-  }
+  to_delete_.clear();
+  to_update_.clear();
 }
 
-AccessibilityBridge::SemanticUpdateOrDelete::SemanticUpdateOrDelete(
-    AccessibilityBridge::SemanticUpdateOrDelete&& m)
-    : type(m.type),
-      update_node(std::move(m.update_node)),
-      id_to_delete(m.id_to_delete) {}
-
-AccessibilityBridge::SemanticUpdateOrDelete::SemanticUpdateOrDelete(
-    Type type,
-    fuchsia::accessibility::semantics::Node node,
-    uint32_t id_to_delete)
-    : type(type), update_node(std::move(node)), id_to_delete(id_to_delete) {}
-
 void AccessibilityBridge::OnCommitComplete() {
+  // TODO(https://crbug.com/1134737): Separate updates of atomic updates and
+  // don't allow all of them to be in the same commit.
   commit_inflight_ = false;
   TryCommit();
 }
@@ -200,8 +176,8 @@ void AccessibilityBridge::HitTest(fuchsia::math::PointF local_point,
 void AccessibilityBridge::OnSemanticsModeChanged(
     bool updates_enabled,
     OnSemanticsModeChangedCallback callback) {
-  // TODO(https://crbug.com/1134591): Enabling / disabling semantics can lead to
-  // race conditions.
+  // TODO(https://crbug.com/1134591): Fix the case when enabling / disabling
+  // semantics can lead to race conditions.
   if (enable_semantic_updates_ == updates_enabled)
     return callback();
 
@@ -216,7 +192,8 @@ void AccessibilityBridge::OnSemanticsModeChanged(
     ui::AXMode mode = web_contents_->GetAccessibilityMode();
     mode.set_mode(ui::AXMode::kWebContents, false);
     web_contents_->SetAccessibilityMode(mode);
-    to_send_.clear();
+    to_delete_.clear();
+    to_update_.clear();
     commit_inflight_ = false;
     ax_tree_.Destroy();
     InterruptPendingActions();
@@ -226,29 +203,9 @@ void AccessibilityBridge::OnSemanticsModeChanged(
   callback();
 }
 
-void AccessibilityBridge::DeleteSubtree(ui::AXNode* node) {
-  DCHECK(node);
-
-  // When navigating, page 1, including the root, is deleted after page 2 has
-  // loaded. Since the root id is the same for page 1 and 2, page 2's root id
-  // ends up getting deleted. To handle this, the root will only be updated.
-  if (node->id() != root_id_) {
-    to_send_.push_back(
-        SemanticUpdateOrDelete(SemanticUpdateOrDelete::Type::DELETE, {},
-                               ConvertToFuchsiaNodeId(node->id(), root_id_)));
-  }
-  for (ui::AXNode* child : node->children())
-    DeleteSubtree(child);
-}
-
 void AccessibilityBridge::OnNodeWillBeDeleted(ui::AXTree* tree,
                                               ui::AXNode* node) {
-  DeleteSubtree(node);
-}
-
-void AccessibilityBridge::OnSubtreeWillBeDeleted(ui::AXTree* tree,
-                                                 ui::AXNode* node) {
-  DeleteSubtree(node);
+  to_delete_.push_back(ConvertToFuchsiaNodeId(node->id(), root_id_));
 }
 
 void AccessibilityBridge::OnAtomicUpdateFinished(
@@ -257,26 +214,18 @@ void AccessibilityBridge::OnAtomicUpdateFinished(
     const std::vector<ui::AXTreeObserver::Change>& changes) {
   DCHECK_EQ(tree, &ax_tree_);
   root_id_ = ax_tree_.root()->id();
+  // Changes included here are only nodes that are still on the tree. Since this
+  // indicates the end of an atomic update, it is safe to assume that these
+  // nodes will not change until the next change arrives. Nodes that would be
+  // deleted are already gone, which means that all updates collected here in
+  // |to_update_| are going to be executed after |to_delete_|.
   for (const ui::AXTreeObserver::Change& change : changes) {
-    ui::AXNodeData ax_data;
-    switch (change.type) {
-      case ui::AXTreeObserver::NODE_CREATED:
-      case ui::AXTreeObserver::SUBTREE_CREATED:
-      case ui::AXTreeObserver::NODE_CHANGED:
-        ax_data = change.node->data();
-        if (change.node->id() == root_id_) {
-          ax_data.id = kSemanticNodeRootId;
-        }
-        to_send_.push_back(
-            SemanticUpdateOrDelete(SemanticUpdateOrDelete::Type::UPDATE,
-                                   AXNodeDataToSemanticNode(ax_data), 0));
-        break;
-      case ui::AXTreeObserver::NODE_REPARENTED:
-      case ui::AXTreeObserver::SUBTREE_REPARENTED:
-        DeleteSubtree(change.node);
-        break;
-    }
+    ui::AXNodeData ax_data = change.node->data();
+    ax_data.id = ConvertToFuchsiaNodeId(change.node->id(), root_id_);
+    to_update_.push_back(AXNodeDataToSemanticNode(ax_data));
   }
+  // TODO(https://crbug.com/1134737): Separate updates of atomic updates and
+  // don't allow all of them to be in the same commit.
   TryCommit();
 }
 

fuchsia/engine/browser/accessibility_bridge.h

@@ -58,39 +58,15 @@ class WEB_ENGINE_EXPORT AccessibilityBridge
 
  private:
   FRIEND_TEST_ALL_PREFIXES(AccessibilityBridgeTest, OnSemanticsModeChanged);
-
-  // A struct used for caching semantic information. This allows for updates
-  // and deletes to be stored in the same vector to preserve all ordering
-  // information.
-  struct SemanticUpdateOrDelete {
-    enum Type { UPDATE, DELETE };
-
-    SemanticUpdateOrDelete(SemanticUpdateOrDelete&& m);
-    SemanticUpdateOrDelete(Type type,
-                           fuchsia::accessibility::semantics::Node node,
-                           uint32_t id_to_delete);
-    ~SemanticUpdateOrDelete() = default;
-
-    Type type;
-    fuchsia::accessibility::semantics::Node update_node;
-    uint32_t id_to_delete;
-  };
+  FRIEND_TEST_ALL_PREFIXES(AccessibilityBridgeTest,
+                           TreeModificationsAreForwarded);
 
   // Processes pending data and commits it to the Semantic Tree.
   void TryCommit();
 
-  // Helper function for TryCommit() that sends the contents of |to_send_| to
-  // the Semantic Tree, starting at |start|.
-  void DispatchSemanticsMessages(size_t start, size_t size);
-
   // Callback for SemanticTree::CommitUpdates.
   void OnCommitComplete();
 
-  // Deletes all nodes in subtree rooted at and including |node|, unless
-  // |node| is the root of the tree. |tree| and |node| are owned by the
-  // accessibility bridge.
-  void DeleteSubtree(ui::AXNode* node);
-
   // Interrupts actions that are waiting for a response. This is invoked during
   // destruction time or when semantic updates have been disabled.
   void InterruptPendingActions();
@@ -111,7 +87,6 @@ class WEB_ENGINE_EXPORT AccessibilityBridge
 
   // ui::AXTreeObserver implementation.
   void OnNodeWillBeDeleted(ui::AXTree* tree, ui::AXNode* node) override;
-  void OnSubtreeWillBeDeleted(ui::AXTree* tree, ui::AXNode* node) override;
   void OnAtomicUpdateFinished(
       ui::AXTree* tree,
       bool root_changed,
@@ -126,7 +101,8 @@ class WEB_ENGINE_EXPORT AccessibilityBridge
   bool enable_semantic_updates_ = false;
 
   // Cache for pending data to be sent to the Semantic Tree between commits.
-  std::vector<SemanticUpdateOrDelete> to_send_;
+  std::vector<uint32_t> to_delete_;
+  std::vector<fuchsia::accessibility::semantics::Node> to_update_;
   bool commit_inflight_ = false;
 
   // Maintain a map of callbacks as multiple hit test events can happen at

fuchsia/engine/browser/fake_semantic_tree.cc

@@ -61,6 +61,22 @@ void FakeSemanticTree::RunUntilNodeCountAtLeast(size_t count) {
   run_loop.Run();
 }
 
+void FakeSemanticTree::RunUntilCommitCountIs(size_t count) {
+  DCHECK(!on_commit_updates_);
+  if (count == num_commit_calls_)
+    return;
+
+  base::RunLoop run_loop;
+  base::AutoReset<base::RepeatingClosure> auto_reset(
+      &on_commit_updates_,
+      base::BindLambdaForTesting([this, count, &run_loop]() {
+        if (static_cast<size_t>(num_commit_calls_) == count) {
+          run_loop.Quit();
+        }
+      }));
+  run_loop.Run();
+}
+
 void FakeSemanticTree::SetNodeUpdatedCallback(
     uint32_t node_id,
     base::OnceClosure node_updated_callback) {
@@ -108,6 +124,7 @@ fuchsia::accessibility::semantics::Node* FakeSemanticTree::GetNodeFromRole(
 
 void FakeSemanticTree::UpdateSemanticNodes(
     std::vector<fuchsia::accessibility::semantics::Node> nodes) {
+  num_update_calls_++;
   bool wait_node_updated = false;
   for (auto& node : nodes) {
     if (node.node_id() == node_wait_id_ && on_node_updated_callback_)
@@ -121,11 +138,13 @@ void FakeSemanticTree::UpdateSemanticNodes(
 }
 
 void FakeSemanticTree::DeleteSemanticNodes(std::vector<uint32_t> node_ids) {
+  num_delete_calls_++;
   for (auto id : node_ids)
     nodes_.erase(id);
 }
 
 void FakeSemanticTree::CommitUpdates(CommitUpdatesCallback callback) {
+  num_commit_calls_++;
   callback();
   if (on_commit_updates_)
     on_commit_updates_.Run();

fuchsia/engine/browser/fake_semantic_tree.h

@@ -36,6 +36,7 @@ class FakeSemanticTree
   void Disconnect();
 
   void RunUntilNodeCountAtLeast(size_t count);
+  void RunUntilCommitCountIs(size_t count);
   void SetNodeUpdatedCallback(uint32_t node_id,
                               base::OnceClosure node_updated_callback);
   fuchsia::accessibility::semantics::Node* GetNodeWithId(uint32_t id);
@@ -50,6 +51,10 @@ class FakeSemanticTree
   size_t tree_size() const { return nodes_.size(); }
   void Clear();
 
+  size_t num_delete_calls() const { return num_delete_calls_; }
+  size_t num_update_calls() const { return num_update_calls_; }
+  size_t num_commit_calls() const { return num_commit_calls_; }
+
   // fuchsia::accessibility::semantics::SemanticTree implementation.
   void UpdateSemanticNodes(
       std::vector<fuchsia::accessibility::semantics::Node> nodes) final;
@@ -66,6 +71,10 @@ class FakeSemanticTree
 
   uint32_t node_wait_id_;
   base::OnceClosure on_node_updated_callback_;
+
+  size_t num_delete_calls_ = 0;
+  size_t num_update_calls_ = 0;
+  size_t num_commit_calls_ = 0;
 };
 
 #endif  // FUCHSIA_ENGINE_BROWSER_FAKE_SEMANTIC_TREE_H_