Bug 1040735 - DOM node reinsertion under anonymous content may trigger a11y child adoption, r=bz, tbdaunde, davidb

accessible/base/nsAccessibilityService.cpp

@@ -34,6 +34,7 @@
 #include "States.h"
 #include "Statistics.h"
 #include "TextLeafAccessibleWrap.h"
+#include "TreeWalker.h"
 
 #ifdef MOZ_ACCESSIBILITY_ATK
 #include "AtkSocketAccessible.h"
@@ -391,22 +392,44 @@ nsAccessibilityService::ContentRangeInserted(nsIPresShell* aPresShell,
 
 void
 nsAccessibilityService::ContentRemoved(nsIPresShell* aPresShell,
-                                       nsIContent* aContainer,
-                                       nsIContent* aChild)
+                                       nsIContent* aChildNode)
 {
 #ifdef A11Y_LOG
   if (logging::IsEnabled(logging::eTree)) {
     logging::MsgBegin("TREE", "content removed");
-    logging::Node("container", aContainer);
-    logging::Node("content", aChild);
+    logging::Node("container", aChildNode->GetFlattenedTreeParent());
+    logging::Node("content", aChildNode);
+  }
+#endif
+
+  DocAccessible* document = GetDocAccessible(aPresShell);
+  if (document) {
+    // Flatten hierarchy may be broken at this point so we cannot get a true
+    // container by traversing up the DOM tree. Find a parent of first accessible
+    // from the subtree of the given DOM node, that'll be a container. If no
+    // accessibles in subtree then we don't care about the change.
+    Accessible* child = document->GetAccessible(aChildNode);
+    if (!child) {
+      a11y::TreeWalker walker(document->GetContainerAccessible(aChildNode),
+                              aChildNode, a11y::TreeWalker::eWalkCache);
+      child = walker.NextChild();
+    }
+
+    if (child) {
+      document->ContentRemoved(child->Parent(), aChildNode);
+#ifdef A11Y_LOG
+      if (logging::IsEnabled(logging::eTree))
+        logging::AccessibleNNode("real container", child->Parent());
+#endif
+    }
+  }
+
+#ifdef A11Y_LOG
+  if (logging::IsEnabled(logging::eTree)) {
     logging::MsgEnd();
     logging::Stack();
   }
 #endif
-
-  DocAccessible* docAccessible = GetDocAccessible(aPresShell);
-  if (docAccessible)
-    docAccessible->ContentRemoved(aContainer, aChild);
 }
 
 void

accessible/base/nsAccessibilityService.h

@@ -90,8 +90,7 @@ public:
   /**
    * Notification used to update the accessible tree when content is removed.
    */
-  void ContentRemoved(nsIPresShell* aPresShell, nsIContent* aContainer,
-                      nsIContent* aChild);
+  void ContentRemoved(nsIPresShell* aPresShell, nsIContent* aChild);
 
   virtual void UpdateText(nsIPresShell* aPresShell, nsIContent* aContent);
 

accessible/generic/DocAccessible.cpp

@@ -1417,18 +1417,6 @@ DocAccessible::ContentInserted(nsIContent* aContainerNode,
   }
 }
 
-void
-DocAccessible::ContentRemoved(nsIContent* aContainerNode,
-                              nsIContent* aChildNode)
-{
-  // Update the whole tree of this document accessible when the container is
-  // null (document element is removed).
-  Accessible* container = aContainerNode ?
-    GetAccessibleOrContainer(aContainerNode) : this;
-
-  UpdateTree(container, aChildNode, false);
-}
-
 void
 DocAccessible::RecreateAccessible(nsIContent* aContent)
 {

accessible/generic/DocAccessible.h

@@ -296,7 +296,16 @@ public:
   /**
    * Notify the document accessible that content was removed.
    */
-  void ContentRemoved(nsIContent* aContainerNode, nsIContent* aChildNode);
+  void ContentRemoved(Accessible* aContainer, nsIContent* aChildNode)
+  {
+    // Update the whole tree of this document accessible when the container is
+    // null (document element is removed).
+    UpdateTree((aContainer ? aContainer : this), aChildNode, false);
+  }
+  void ContentRemoved(nsIContent* aContainerNode, nsIContent* aChildNode)
+  {
+    ContentRemoved(GetAccessibleOrContainer(aContainerNode), aChildNode);
+  }
 
   /**
    * Updates accessible tree when rendered text is changed.

accessible/tests/mochitest/treeupdate/a11y.ini

@@ -5,6 +5,7 @@
 [test_bug883708.xhtml]
 [test_bug884251.xhtml]
 [test_bug895082.html]
+[test_bug1040735.html]
 [test_canvas.html]
 [test_colorpicker.xul]
 [test_contextmenu.xul]

accessible/tests/mochitest/treeupdate/test_bug1040735.html

@@ -0,0 +1,42 @@
+<!DOCTYPE html>
+<html>
+<head>
+  <title>Adopt DOM node from anonymous subtree</title>
+
+  <link rel="stylesheet" type="text/css"
+        href="chrome://mochikit/content/tests/SimpleTest/test.css" />
+
+  <script type="application/javascript"
+          src="chrome://mochikit/content/tests/SimpleTest/SimpleTest.js"></script>
+  <script type="application/javascript"
+          src="../common.js"></script>
+
+  <script type="application/javascript">
+    function doTest()
+    {
+      document.body.appendChild(document.getElementById("mw_a"));
+      setTimeout(function() { ok(true, "no crash and assertions"); SimpleTest.finish(); } , 0);
+    }
+
+    SimpleTest.waitForExplicitFinish();
+    addA11yLoadEvent(doTest);
+  </script>
+</head>
+
+<body>
+  <a target="_blank"
+     href="https://bugzilla.mozilla.org/show_bug.cgi?id=1040735"
+     title="Bug 1040735 - DOM node reinsertion under anonymous content may trigger a11y child adoption">
+    Bug 1040735</a>
+  <p id="display"></p>
+  <div id="content" style="display: none"></div>
+  <pre id="test">
+  </pre>
+
+  <marquee>
+    <div id="mw_a" style="visibility: hidden;">
+      <div style="visibility: visible;" id="mw_inside"></div>
+    </div>
+  </marquee>
+</body>
+</html>

accessible/tests/mochitest/treeupdate/test_optgroup.html

@@ -51,7 +51,7 @@
                 { COMBOBOX_OPTION: [
                   { TEXT_LEAF: [] }
                 ] },
-              { COMBOBOX_OPTION: [
+                { COMBOBOX_OPTION: [
                   { TEXT_LEAF: [] }
                 ] },
               ]},
@@ -71,6 +71,7 @@
     {
       this.selectNode = getNode(aID);
       this.select = getAccessible(this.selectNode);
+      this.selectList = this.select.firstChild;
 
       this.invoke = function removeOptGroup_invoke()
       {
@@ -79,7 +80,7 @@
       }
 
       this.eventSeq = [
-        new invokerChecker(EVENT_REORDER, this.select)
+        new invokerChecker(EVENT_REORDER, this.selectList)
       ];
 
       this.finalCheck = function removeOptGroup_finalCheck()

accessible/tests/mochitest/treeupdate/test_select.html

@@ -63,6 +63,7 @@
     {
       this.selectNode = getNode(aID);
       this.select = getAccessible(this.selectNode);
+      this.selectList = this.select.firstChild;
 
       this.invoke = function removeOptions_invoke()
       {
@@ -71,7 +72,7 @@
       }
 
       this.eventSeq = [
-        new invokerChecker(EVENT_REORDER, this.select)
+        new invokerChecker(EVENT_REORDER, this.selectList)
       ];
 
       this.finalCheck = function removeOptions_finalCheck()

layout/base/RestyleManager.cpp

@@ -3409,7 +3409,7 @@ ElementRestyler::SendAccessibilityNotifications()
     if (accService) {
       nsIPresShell* presShell = mFrame->PresContext()->GetPresShell();
       nsIContent* content = mFrame->GetContent();
-      accService->ContentRemoved(presShell, content->GetParent(), content);
+      accService->ContentRemoved(presShell, content);
 
       // Process children staying shown.
       uint32_t visibleContentCount = mVisibleKidsOfHiddenElement.Length();

layout/base/nsCSSFrameConstructor.cpp

@@ -7803,7 +7803,7 @@ nsCSSFrameConstructor::ContentRemoved(nsIContent* aContainer,
 #ifdef ACCESSIBILITY
     nsAccessibilityService* accService = nsIPresShell::AccService();
     if (accService) {
-      accService->ContentRemoved(mPresShell, aContainer, aChild);
+      accService->ContentRemoved(mPresShell, aChild);
     }
 #endif
 

layout/xul/nsListBoxBodyFrame.cpp

@@ -1512,8 +1512,7 @@ nsListBoxBodyFrame::RemoveChildFrame(nsBoxLayoutState &aState,
   nsAccessibilityService* accService = nsIPresShell::AccService();
   if (accService) {
     nsIContent* content = aFrame->GetContent();
-    accService->ContentRemoved(PresContext()->PresShell(), content->GetParent(),
-                               content);
+    accService->ContentRemoved(PresContext()->PresShell(), content);
   }
 #endif