Bug 1487312 - Fix content insertion accessibility notifications in Shadow DOM. r=Jamie,surkov
authorEmilio Cobos Álvarez <emilio@crisal.io>
Thu, 18 Oct 2018 10:02:51 +0200
changeset 490459 011cfa1666cbede441dcb7bc5f9e4b00c41d5878
parent 490457 588d1e2f8c9d9f34ac3d3ac62960e1660fcc9afb
child 490460 53958ec4b3c9c48ee650d2a6e470e44fa47fc8d0
push id247
push userfmarier@mozilla.com
push dateSat, 27 Oct 2018 01:06:44 +0000
reviewersJamie, surkov
bugs1487312, 1427825, 1442207
milestone64.0a1
Bug 1487312 - Fix content insertion accessibility notifications in Shadow DOM. r=Jamie,surkov The issue was specific to content insertion directly under a shadow root, the rest should work (see bug 1427825 for the fix for other similar occurrences). The removal of the aContainer argument follows the same pattern as bug 1442207. Differential Revision: https://phabricator.services.mozilla.com/D6431
accessible/base/NotificationController.cpp
accessible/base/NotificationController.h
accessible/base/nsAccessibilityService.cpp
accessible/generic/DocAccessible.cpp
accessible/generic/DocAccessible.h
accessible/tests/mochitest/treeupdate/test_bug1276857.html
--- a/accessible/base/NotificationController.cpp
+++ b/accessible/base/NotificationController.cpp
@@ -412,37 +412,49 @@ void
 NotificationController::ScheduleChildDocBinding(DocAccessible* aDocument)
 {
   // Schedule child document binding to the tree.
   mHangingChildDocuments.AppendElement(aDocument);
   ScheduleProcessing();
 }
 
 void
-NotificationController::ScheduleContentInsertion(Accessible* aContainer,
-                                                 nsIContent* aStartChildNode,
+NotificationController::ScheduleContentInsertion(nsIContent* aStartChildNode,
                                                  nsIContent* aEndChildNode)
 {
-  nsTArray<nsCOMPtr<nsIContent>> list;
+  // The frame constructor guarantees that only ranges with the same parent
+  // arrive here in presence of dynamic changes to the page, see
+  // nsCSSFrameConstructor::IssueSingleInsertNotifications' callers.
+  nsINode* parent = aStartChildNode->GetFlattenedTreeParentNode();
+  if (!parent) {
+    return;
+  }
 
-  bool needsProcessing = false;
-  nsIContent* node = aStartChildNode;
-  while (node != aEndChildNode) {
+  Accessible* container = mDocument->AccessibleOrTrueContainer(parent);
+  if (!container) {
+    return;
+  }
+
+  AutoTArray<nsCOMPtr<nsIContent>, 10> list;
+  for (nsIContent* node = aStartChildNode;
+       node != aEndChildNode;
+       node = node->GetNextSibling()) {
+    MOZ_ASSERT(parent == node->GetFlattenedTreeParentNode());
     // Notification triggers for content insertion even if no content was
     // actually inserted, check if the given content has a frame to discard
     // this case early.
+    //
+    // TODO(emilio): Should this handle display: contents?
     if (node->GetPrimaryFrame()) {
-      if (list.AppendElement(node))
-        needsProcessing = true;
+      list.AppendElement(node);
     }
-    node = node->GetNextSibling();
   }
 
-  if (needsProcessing) {
-    mContentInsertions.LookupOrAdd(aContainer)->AppendElements(list);
+  if (!list.IsEmpty()) {
+    mContentInsertions.LookupOrAdd(container)->AppendElements(list);
     ScheduleProcessing();
   }
 }
 
 void
 NotificationController::ScheduleProcessing()
 {
   // If notification flush isn't planed yet start notification flush
--- a/accessible/base/NotificationController.h
+++ b/accessible/base/NotificationController.h
@@ -190,18 +190,17 @@ public:
 
     mTextHash.PutEntry(aTextNode);
     ScheduleProcessing();
   }
 
   /**
    * Pend accessible tree update for content insertion.
    */
-  void ScheduleContentInsertion(Accessible* aContainer,
-                                nsIContent* aStartChildNode,
+  void ScheduleContentInsertion(nsIContent* aStartChildNode,
                                 nsIContent* aEndChildNode);
 
   /**
    * Pend an accessible subtree relocation.
    */
   void ScheduleRelocation(Accessible* aOwner)
   {
     if (!mRelocations.Contains(aOwner) && mRelocations.AppendElement(aOwner)) {
--- a/accessible/base/nsAccessibilityService.cpp
+++ b/accessible/base/nsAccessibilityService.cpp
@@ -380,20 +380,17 @@ nsAccessibilityService::ListenersChanged
     for (uint32_t i = 0 ; i < changeCount ; i++) {
       nsIDocument* ownerDoc = node->OwnerDoc();
       DocAccessible* document = GetExistingDocAccessible(ownerDoc);
 
       // Create an accessible for a inaccessible element having click event
       // handler.
       if (document && !document->HasAccessible(node) &&
           nsCoreUtils::HasClickListener(node)) {
-        nsIContent* parentEl = node->GetFlattenedTreeParent();
-        if (parentEl) {
-          document->ContentInserted(parentEl, node, node->GetNextSibling());
-        }
+        document->ContentInserted(node, node->GetNextSibling());
         break;
       }
     }
   }
   return NS_OK;
 }
 
 ////////////////////////////////////////////////////////////////////////////////
@@ -606,17 +603,17 @@ nsAccessibilityService::DeckPanelSwitche
     if (logging::IsEnabled(logging::eTree)) {
       logging::MsgBegin("TREE", "deck panel selected");
       logging::Node("container", panelNode);
       logging::Node("content", aDeckNode);
       logging::MsgEnd();
     }
 #endif
 
-    document->ContentInserted(aDeckNode, panelNode, panelNode->GetNextSibling());
+    document->ContentInserted(panelNode, panelNode->GetNextSibling());
   }
 }
 
 void
 nsAccessibilityService::ContentRangeInserted(nsIPresShell* aPresShell,
                                              nsIContent* aStartChild,
                                              nsIContent* aEndChild)
 {
@@ -630,17 +627,17 @@ nsAccessibilityService::ContentRangeInse
       logging::Node("content", child);
     }
     logging::MsgEnd();
     logging::Stack();
   }
 #endif
 
   if (document) {
-    document->ContentInserted(aStartChild->GetParent(), aStartChild, aEndChild);
+    document->ContentInserted(aStartChild, aEndChild);
   }
 }
 
 void
 nsAccessibilityService::ContentRemoved(nsIPresShell* aPresShell,
                                        nsIContent* aChildNode)
 {
   DocAccessible* document = GetDocAccessible(aPresShell);
--- a/accessible/generic/DocAccessible.cpp
+++ b/accessible/generic/DocAccessible.cpp
@@ -772,20 +772,18 @@ DocAccessible::AttributeChanged(dom::Ele
   if (UpdateAccessibleOnAttrChange(aElement, aAttribute))
     return;
 
   // Update the accessible tree on aria-hidden change. Make sure to not create
   // a tree under aria-hidden='true'.
   if (aAttribute == nsGkAtoms::aria_hidden) {
     if (aria::HasDefinedARIAHidden(aElement)) {
       ContentRemoved(aElement);
-    }
-    else {
-      ContentInserted(aElement->GetFlattenedTreeParent(),
-                      aElement, aElement->GetNextSibling());
+    } else {
+      ContentInserted(aElement, aElement->GetNextSibling());
     }
     return;
   }
 
   // Ignore attribute change if the element doesn't have an accessible (at all
   // or still) iff the element is not a root content of this document accessible
   // (which is treated as attribute change on this document accessible).
   // Note: we don't bail if all the content hasn't finished loading because
@@ -1348,33 +1346,26 @@ DocAccessible::UnbindFromDocument(Access
 
   NS_ASSERTION(!aAccessible->IsDefunct(), "Shutdown the shutdown accessible!");
   aAccessible->Shutdown();
 
   mAccessibleCache.Remove(uniqueID);
 }
 
 void
-DocAccessible::ContentInserted(nsIContent* aContainerNode,
-                               nsIContent* aStartChildNode,
+DocAccessible::ContentInserted(nsIContent* aStartChildNode,
                                nsIContent* aEndChildNode)
 {
   // Ignore content insertions until we constructed accessible tree. Otherwise
   // schedule tree update on content insertion after layout.
   if (mNotificationController && HasLoadState(eTreeConstructed)) {
     // Update the whole tree of this document accessible when the container is
     // null (document element is inserted or removed).
-    Accessible* container = aContainerNode ?
-      AccessibleOrTrueContainer(aContainerNode) : this;
-    if (container) {
-      // Ignore notification if the container node is no longer in the DOM tree.
-      mNotificationController->ScheduleContentInsertion(container,
-                                                        aStartChildNode,
-                                                        aEndChildNode);
-    }
+    mNotificationController->ScheduleContentInsertion(aStartChildNode,
+                                                      aEndChildNode);
   }
 }
 
 void
 DocAccessible::RecreateAccessible(nsIContent* aContent)
 {
 #ifdef A11Y_LOG
   if (logging::IsEnabled(logging::eTree)) {
@@ -1383,20 +1374,18 @@ DocAccessible::RecreateAccessible(nsICon
     logging::MsgEnd();
   }
 #endif
 
   // XXX: we shouldn't recreate whole accessible subtree, instead we should
   // subclass hide and show events to handle them separately and implement their
   // coalescence with normal hide and show events. Note, in this case they
   // should be coalesced with normal show/hide events.
-
-  nsIContent* parent = aContent->GetFlattenedTreeParent();
   ContentRemoved(aContent);
-  ContentInserted(parent, aContent, aContent->GetNextSibling());
+  ContentInserted(aContent, aContent->GetNextSibling());
 }
 
 void
 DocAccessible::ProcessInvalidationList()
 {
   // Invalidate children of container accessible for each element in
   // invalidation list. Allow invalidation list insertions while container
   // children are recached.
--- a/accessible/generic/DocAccessible.h
+++ b/accessible/generic/DocAccessible.h
@@ -345,19 +345,17 @@ public:
   /**
    * Remove from document and shutdown the given accessible.
    */
   void UnbindFromDocument(Accessible* aAccessible);
 
   /**
    * Notify the document accessible that content was inserted.
    */
-  void ContentInserted(nsIContent* aContainerNode,
-                       nsIContent* aStartChildNode,
-                       nsIContent* aEndChildNode);
+  void ContentInserted(nsIContent* aStartChildNode, nsIContent* aEndChildNode);
 
   /**
    * Update the tree on content removal.
    */
   void ContentRemoved(Accessible* aAccessible);
   void ContentRemoved(nsIContent* aContentNode);
 
   /**
--- a/accessible/tests/mochitest/treeupdate/test_bug1276857.html
+++ b/accessible/tests/mochitest/treeupdate/test_bug1276857.html
@@ -76,23 +76,27 @@
             ] },
             { TEXT_LEAF: [] } // More text
           ]
         };
         testAccessibleTree(iframe.contentDocument.getElementById("c2"), tree);
 
         var shadowRoot = iframe.contentDocument.getElementById("c2_c").shadowRoot;
         shadowRoot.firstElementChild.querySelector("span").remove();
+        // bug 1487312
+        shadowRoot.firstElementChild.offsetTop;
+        shadowRoot.appendChild(document.createElement("button"));
       };
 
       this.finalCheck = function runShadowTest_finalCheck() {
         var tree = {
           SECTION: [ // c2
             { TEXT_LEAF: [] }, // Some text
-            { TEXT_LEAF: [] } // More text
+            { TEXT_LEAF: [] }, // More text
+            { PUSHBUTTON: [] } // The button we appended.
           ]
         };
         testAccessibleTree(iframe.contentDocument.getElementById("c2"), tree);
       };
 
       this.getID = function runShadowTest_getID() {
         return "child DOM node is removed before the layout notifies the a11y about parent removal/show in shadow DOM";
       };