Reuse BrowserAccessibility when reparenting so that events are correct

When a node is reparented we currently destroy it rather than actually
reusing it, causing it to look like a new node. New nodes do not fire
change events for name, state, value or live regions.

Impact:
- We need to reuse nodes if we want them to be able to fire change
events when they are reparented at the same time.
- Accessibility nodes can be unexpectedly reparented by the layout
engine, as shown in the included test.

Bug: 469254
Change-Id: I7aec0d7852c712724b97be4a723df3c928720d62
Reviewed-on: https://chromium-review.googlesource.com/1152477
Commit-Queue: Aaron Leventhal <aleventhal@chromium.org>
Reviewed-by: Dominic Mazzoni <dmazzoni@chromium.org>
Cr-Commit-Position: refs/heads/master@{#578750}

content/browser/accessibility/browser_accessibility_manager.cc

@@ -1101,32 +1101,6 @@ void BrowserAccessibilityManager::OnSubtreeWillBeDeleted(ui::AXTree* tree,
     obj->OnSubtreeWillBeDeleted();
 }
 
-void BrowserAccessibilityManager::OnNodeWillBeReparented(ui::AXTree* tree,
-                                                         ui::AXNode* node) {
-  AXEventGenerator::OnNodeWillBeReparented(tree, node);
-  // BrowserAccessibility should probably ask the tree source for the AXNode via
-  // an id rather than weakly holding a pointer to a AXNode that might have been
-  // destroyed under the hood and re-created later on. Treat this as a delete to
-  // make things work.
-  if (id_wrapper_map_.find(node->id()) == id_wrapper_map_.end())
-    return;
-  GetFromAXNode(node)->Destroy();
-  id_wrapper_map_.erase(node->id());
-}
-
-void BrowserAccessibilityManager::OnSubtreeWillBeReparented(ui::AXTree* tree,
-                                                            ui::AXNode* node) {
-  AXEventGenerator::OnSubtreeWillBeReparented(tree, node);
-  // BrowserAccessibility should probably ask the tree source for the AXNode via
-  // an id rather than weakly holding a pointer to a AXNode that might have been
-  // destroyed under the hood and re-created later on. Treat this as a delete to
-  // make things work.
-  DCHECK(node);
-  BrowserAccessibility* obj = GetFromAXNode(node);
-  if (obj)
-    obj->OnSubtreeWillBeDeleted();
-}
-
 void BrowserAccessibilityManager::OnNodeCreated(ui::AXTree* tree,
                                                 ui::AXNode* node) {
   AXEventGenerator::OnNodeCreated(tree, node);
@@ -1139,13 +1113,13 @@ void BrowserAccessibilityManager::OnNodeCreated(ui::AXTree* tree,
 void BrowserAccessibilityManager::OnNodeReparented(ui::AXTree* tree,
                                                    ui::AXNode* node) {
   AXEventGenerator::OnNodeReparented(tree, node);
-  // BrowserAccessibility should probably ask the tree source for the AXNode via
-  // an id rather than weakly holding a pointer to a AXNode that might have been
-  // destroyed under the hood and re-created later on. Treat this as a create to
-  // make things work.
-  BrowserAccessibility* wrapper = factory_->Create();
+  BrowserAccessibility* wrapper = GetFromID(node->id());
+  if (!wrapper) {
+    wrapper = factory_->Create();
+    id_wrapper_map_[node->id()] = wrapper;
+  }
+
   wrapper->Init(this, node);
-  id_wrapper_map_[node->id()] = wrapper;
   wrapper->OnDataChanged();
 }
 

content/browser/accessibility/browser_accessibility_manager.h

@@ -347,8 +347,6 @@ class CONTENT_EXPORT BrowserAccessibilityManager : public ui::AXEventGenerator {
   // AXTreeDelegate implementation.
   void OnNodeWillBeDeleted(ui::AXTree* tree, ui::AXNode* node) override;
   void OnSubtreeWillBeDeleted(ui::AXTree* tree, ui::AXNode* node) override;
-  void OnNodeWillBeReparented(ui::AXTree* tree, ui::AXNode* node) override;
-  void OnSubtreeWillBeReparented(ui::AXTree* tree, ui::AXNode* node) override;
   void OnNodeCreated(ui::AXTree* tree, ui::AXNode* node) override;
   void OnNodeReparented(ui::AXTree* tree, ui::AXNode* node) override;
   void OnNodeChanged(ui::AXTree* tree, ui::AXNode* node) override;

content/browser/accessibility/dump_accessibility_events_browsertest.cc

@@ -339,6 +339,11 @@ IN_PROC_BROWSER_TEST_F(DumpAccessibilityEventsTest,
   RunEventTest(FILE_PATH_LITERAL("live-region-create.html"));
 }
 
+IN_PROC_BROWSER_TEST_F(DumpAccessibilityEventsTest,
+                       AccessibilityEventsLiveRegionElemReparent) {
+  RunEventTest(FILE_PATH_LITERAL("live-region-elem-reparent.html"));
+}
+
 IN_PROC_BROWSER_TEST_F(DumpAccessibilityEventsTest,
                        AccessibilityEventsLiveRegionIgnoresClick) {
   RunEventTest(FILE_PATH_LITERAL("live-region-ignores-click.html"));

content/test/data/accessibility/event/live-region-elem-reparent-expected-mac.txt

+AXLiveRegionChanged on AXGroup

content/test/data/accessibility/event/live-region-elem-reparent-expected-win.txt

+IA2_EVENT_TEXT_INSERTED on <#document> role=ROLE_SYSTEM_DOCUMENT value~=[doc-url] FOCUSED,FOCUSABLE new_text={'<obj><obj><obj>' start=0 end=3}
+IA2_EVENT_TEXT_INSERTED on <p#message> role=P new_text={'This is important!' start=0 end=18}
\ No newline at end of file

content/test/data/accessibility/event/live-region-elem-reparent.html

+<!--
+@WIN-DENY:*
+@WIN-ALLOW:IA2_EVENT_TEXT_INSERTED*
+-->
+<!DOCTYPE html>
+<html>
+<body>
+<button onclick="onClick();">Click</button>
+<div id="something" style="display: none;"></div>
+<span aria-live="polite">
+  <p id="message" style="position:absolute"></p>
+</span>
+
+<script>
+  function go() {
+    document.querySelector('button').click();
+  }
+
+  function onClick() {
+    /* The <p> to gets reparented and updated text at the same time */
+    var something = document.querySelector('#something');
+    something.style.display = 'block';
+    var message = document.querySelector('#message');
+    message.textContent = 'This is important!';
+  }
+</script>
+</body>
+</html>
\ No newline at end of file