Bug 1249253 - content removal processing can wrongly remove ARIA owned children, r=yzen

accessible/base/TreeWalker.cpp

@@ -43,9 +43,8 @@ TreeWalker::
   mChildFilter(nsIContent::eSkipPlaceholderContent), mFlags(aFlags),
   mPhase(eAtStart)
 {
+  MOZ_ASSERT(mFlags & eWalkCache, "This constructor cannot be used for tree creation");
   MOZ_ASSERT(aAnchorNode, "No anchor node for the accessible tree walker");
-  MOZ_ASSERT(mDoc->GetAccessibleOrContainer(aAnchorNode) == mContext,
-             "Unexpected anchor node was given");
 
   mChildFilter |= mContext->NoXBLKids() ?
     nsIContent::eAllButXBL : nsIContent::eAllChildren;
@@ -169,9 +168,16 @@ TreeWalker::Next()
   }
 
   // If we traversed the whole subtree of the anchor node. Move to next node
-  // relative anchor node within the context subtree if possible.
-  if (mFlags != eWalkContextTree)
+  // relative anchor node within the context subtree if asked.
+  if (mFlags != eWalkContextTree) {
+    // eWalkCache flag presence indicates that the search is scoped to the
+    // anchor (no ARIA owns stuff).
+    if (mFlags & eWalkCache) {
+      mPhase = eAtEnd;
+      return nullptr;
+    }
     return Next();
+  }
 
   nsINode* contextNode = mContext->GetNode();
   while (mAnchorNode != contextNode) {

accessible/base/TreeWalker.h

@@ -45,7 +45,7 @@ public:
    * @param aAnchorNode [in] the node the search will be prepared relative to
    * @param aFlags   [in] flags (see enum above)
    */
-  TreeWalker(Accessible* aContext, nsIContent* aAnchorNode, uint32_t aFlags = 0);
+  TreeWalker(Accessible* aContext, nsIContent* aAnchorNode, uint32_t aFlags = eWalkCache);
 
   ~TreeWalker();
 

accessible/generic/DocAccessible.cpp

@@ -1795,34 +1795,9 @@ DocAccessible::UpdateTreeOnRemoval(Accessible* aContainer, nsIContent* aChildNod
   if (child) {
     updateFlags |= UpdateTreeInternal(child, false, reorderEvent);
   } else {
-    // aChildNode may not coorespond to a particular accessible, to handle
-    // this we go through all the children of aContainer.  Then if a child
-    // has aChildNode as an ancestor, or does not have the node for
-    // aContainer as an ancestor remove that child of aContainer.  Note that
-    // when we are called aChildNode may already have been removed from the DOM
-    // so we can't expect it to have a parent or what was it's parent to have
-    // it as a child.
-    nsINode* containerNode = aContainer->GetNode();
-    for (uint32_t idx = 0; idx < aContainer->ContentChildCount();) {
-      Accessible* child = aContainer->ContentChildAt(idx);
-
-      // If accessible doesn't have its own content then we assume parent
-      // will handle its update.  If child is DocAccessible then we don't
-      // handle updating it here either.
-      if (!child->HasOwnContent() || child->IsDoc()) {
-        idx++;
-        continue;
-      }
-
-      nsINode* childNode = child->GetContent();
-      while (childNode != aChildNode && childNode != containerNode &&
-             (childNode = childNode->GetParentNode()));
-
-      if (childNode != containerNode) {
-        updateFlags |= UpdateTreeInternal(child, false, reorderEvent);
-      } else {
-        idx++;
-      }
+    TreeWalker walker(aContainer, aChildNode, TreeWalker::eWalkCache);
+    while (Accessible* child = walker.Next()) {
+      updateFlags |= UpdateTreeInternal(child, false, reorderEvent);
     }
   }
 

accessible/tests/mochitest/treeupdate/test_ariaowns.html

@@ -472,6 +472,43 @@
       }
     }
 
+    function removeNotARIAOwnedEl(aContainer, aChild)
+    {
+      this.eventSeq = [
+        new invokerChecker(EVENT_REORDER, aContainer)
+      ];
+
+      this.invoke = function removeNotARIAOwnedEl_invoke()
+      {
+        dumpTree(aContainer, "before");
+        var tree = {
+          SECTION: [
+            { TEXT_LEAF: [ ] },
+            { GROUPING: [ ] }
+          ]
+        };
+        testAccessibleTree(aContainer, tree);
+
+        getNode(aContainer).removeChild(getNode(aChild));
+      }
+
+      this.finalCheck = function removeNotARIAOwnedEl_finalCheck()
+      {
+        dumpTree(aContainer, "after");
+        var tree = {
+          SECTION: [
+            { GROUPING: [ ] }
+          ]
+        };
+        testAccessibleTree(aContainer, tree);
+      }
+
+      this.getID = function removeNotARIAOwnedEl_getID()
+      {
+        return `remove not ARIA owned child`;
+      }
+    }
+
 
     ////////////////////////////////////////////////////////////////////////////
     // Test
@@ -516,6 +553,8 @@
         [ "t5_radio", "t5_button" ],
         [ "RADIOBUTTON", "PUSHBUTTON", "CHECKBUTTON" ]));
 
+      gQueue.push(new removeNotARIAOwnedEl("t6_container", "t6_span"));
+
       gQueue.invoke(); // SimpleTest.finish() will be called in the end
     }
 
@@ -563,6 +602,11 @@
     <div role="checkbox" id="t5_checkbox"></div>
     <div role="radio" id="t5_radio"></div>
   </div>
+
+  <div id="t6_container" aria-owns="t6_fake">
+    <span id="t6_span">hey</span>
+  </div>
+  <div id="t6_fake" role="group"></div>
 </body>
 
 </html>