Avoid resetting AX tree when node becomes ignored

SerializeNodeChanges() checks every descendant node to see if it was
reparented, which is considered an error condition, because reparented
nodes should be handled in the caller SerializeChanges(). When this
error condition is reached, it means that the helper
AnyDescendantWasReparented() did not detect a reparented node, and the
tree is Reset() -- meaning it will need to be rebuilt. In addition,
generated events on reparented nodes will be lost.

This error condition occurred when nodes change their ignored status,
because of a line in AnyDescendantWasReparented() that causes it to skip
checking children recursively for valid nodes. Skipping this check when
node is ignored avoids the error condition.

In addition, a DumpWithoutCrashing() calls is added so that the
accessibility team can understand how often this error condition occurs.

This change has the side effect of detecting more reparenting changes,
because when a subtree becomes display:none, a new accessible is
created for each node in the subtree -- unless one of the objects in
the subtree was already display:none. Those nodes will effectively
get new parents. As a result of this side effect, menupopupend
changes need to be fired on any menu nodes that are deleted,
not just when they're the root of a subtree that's deleted. This is
a more robust solution for menupopupend events in any case.

Bug: 1059394
Change-Id: I0433690d6f7ac9708bcc54dafe4b3f9b09d30ee5
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2090736
Commit-Queue: Aaron Leventhal <aleventhal@chromium.org>
Reviewed-by: Dominic Mazzoni <dmazzoni@chromium.org>
Cr-Commit-Position: refs/heads/master@{#751870}

content/browser/accessibility/browser_accessibility_manager_win.cc

@@ -601,10 +601,16 @@ void BrowserAccessibilityManagerWin::OnSubtreeWillBeDeleted(ui::AXTree* tree,
   if (obj) {
     FireWinAccessibilityEvent(EVENT_OBJECT_HIDE, obj);
     FireUiaStructureChangedEvent(StructureChangeType_ChildRemoved, obj);
-    if (obj->GetRole() == ax::mojom::Role::kMenu) {
-      FireWinAccessibilityEvent(EVENT_SYSTEM_MENUPOPUPEND, obj);
-      FireUiaAccessibilityEvent(UIA_MenuClosedEventId, obj);
-    }
+  }
+}
+
+void BrowserAccessibilityManagerWin::OnNodeWillBeDeleted(ui::AXTree* tree,
+                                                         ui::AXNode* node) {
+  if (node->data().role == ax::mojom::Role::kMenu) {
+    BrowserAccessibility* obj = GetFromAXNode(node);
+    DCHECK(obj);
+    FireWinAccessibilityEvent(EVENT_SYSTEM_MENUPOPUPEND, obj);
+    FireUiaAccessibilityEvent(UIA_MenuClosedEventId, obj);
   }
 }
 

content/browser/accessibility/browser_accessibility_manager_win.h

@@ -80,6 +80,7 @@ class CONTENT_EXPORT BrowserAccessibilityManagerWin
  protected:
   // AXTreeObserver methods.
   void OnSubtreeWillBeDeleted(ui::AXTree* tree, ui::AXNode* node) override;
+  void OnNodeWillBeDeleted(ui::AXTree* tree, ui::AXNode* node) override;
   void OnAtomicUpdateFinished(
       ui::AXTree* tree,
       bool root_changed,

ui/accessibility/ax_tree_serializer.h

@@ -12,6 +12,7 @@
 #include <unordered_set>
 #include <vector>
 
+#include "base/debug/dump_without_crashing.h"
 #include "base/logging.h"
 #include "ui/accessibility/ax_export.h"
 #include "ui/accessibility/ax_tree_source.h"
@@ -341,7 +342,10 @@ bool AXTreeSerializer<AXSourceNode, AXNodeData, AXTreeData>::
         // This child is already in the client tree and valid, we won't
         // recursively serialize it so we don't need to check this
         // subtree recursively for reparenting.
-        continue;
+        // However, if the child is ignored, the children may now be
+        // considered as reparented, so continue recursion in that case.
+        if (!client_child->ignored)
+          continue;
       }
     }
 
@@ -548,6 +552,7 @@ bool AXTreeSerializer<AXSourceNode, AXNodeData, AXTreeData>::
     ClientTreeNode* client_child = ClientTreeNodeById(new_child_id);
     if (client_child && client_child->parent != client_node) {
       DVLOG(1) << "Reparenting detected";
+      base::debug::DumpWithoutCrashing();
       Reset();
       return false;
     }