Bug 1879255 part 3: Notify accessibility about changes to explicitly set attr-elements. r=smaug,eeejay

Accessibility needs to keep track of changes to explicitly set attr-elements.
Since the popovertarget content attribute is "" for any explicitly set attr-element, we won't always get attribute change notifications for the content attribute when .popoverTargetElement is set.
For example, if e1's popovertarget content attribute is absent and you set e1.popoverTargetElement to e2, the popovertarget content attribute will be "".
If you later set e1.popoverTargetElement to e3, there won't be a notification for the content attribute change, since it remains "".
Even if there were, it might occur before the element has changed, which means we can't detect any relevant state changes there; e.g. mPrevStateBits.
To deal with this, we now have DOM notify accessibility before and after the explicitly set attr-element is changed.
Within DocAccessible, this is treated like any other attribute change, but the notification methods get called consistently and at the appropriate time.

Differential Revision: https://phabricator.services.mozilla.com/D201662

accessible/base/nsAccessibilityService.cpp

@@ -606,6 +606,24 @@ void nsAccessibilityService::NotifyOfDevPixelRatioChange(
   }
 }
 
+void nsAccessibilityService::NotifyAttrElementWillChange(
+    mozilla::dom::Element* aElement, nsAtom* aAttr) {
+  mozilla::dom::Document* doc = aElement->OwnerDoc();
+  MOZ_ASSERT(doc);
+  if (DocAccessible* docAcc = GetDocAccessible(doc)) {
+    docAcc->AttrElementWillChange(aElement, aAttr);
+  }
+}
+
+void nsAccessibilityService::NotifyAttrElementChanged(
+    mozilla::dom::Element* aElement, nsAtom* aAttr) {
+  mozilla::dom::Document* doc = aElement->OwnerDoc();
+  MOZ_ASSERT(doc);
+  if (DocAccessible* docAcc = GetDocAccessible(doc)) {
+    docAcc->AttrElementChanged(aElement, aAttr);
+  }
+}
+
 LocalAccessible* nsAccessibilityService::GetRootDocumentAccessible(
     PresShell* aPresShell, bool aCanCreate) {
   PresShell* presShell = aPresShell;

accessible/base/nsAccessibilityService.h

@@ -244,6 +244,19 @@ class nsAccessibilityService final : public mozilla::a11y::DocManager,
   void NotifyOfDevPixelRatioChange(mozilla::PresShell* aPresShell,
                                    int32_t aAppUnitsPerDevPixel);
 
+  /**
+   * Notify accessibility that an element explicitly set for an attribute is
+   * about to change. See dom::Element::ExplicitlySetAttrElement.
+   */
+  void NotifyAttrElementWillChange(mozilla::dom::Element* aElement,
+                                   nsAtom* aAttr);
+
+  /**
+   * Notify accessibility that an element explicitly set for an attribute has
+   * changed. See dom::Element::ExplicitlySetAttrElement.
+   */
+  void NotifyAttrElementChanged(mozilla::dom::Element* aElement, nsAtom* aAttr);
+
   // nsAccessibiltiyService
 
   /**

accessible/generic/DocAccessible.cpp

@@ -733,9 +733,26 @@ std::pair<nsPoint, nsRect> DocAccessible::ComputeScrollData(
 NS_IMPL_NSIDOCUMENTOBSERVER_CORE_STUB(DocAccessible)
 NS_IMPL_NSIDOCUMENTOBSERVER_LOAD_STUB(DocAccessible)
 
+// When a reflected element IDL attribute changes, we might get the following
+// synchronous calls:
+// 1. AttributeWillChange for the element.
+// 2. AttributeWillChange for the content attribute.
+// 3. AttributeChanged for the content attribute.
+// 4. AttributeChanged for the element.
+// Since the content attribute value is "" for any element, we won't always get
+// 2 or 3. Even if we do, they might occur after the element has already
+// changed, which means we can't detect any relevant state changes there; e.g.
+// mPrevStateBits. Thus, we need 1 and 4, and we must ignore 2 and 3. To
+// facilitate this, sIsAttrElementChanging will be set to true for 2 and 3.
+static bool sIsAttrElementChanging = false;
+
 void DocAccessible::AttributeWillChange(dom::Element* aElement,
                                         int32_t aNameSpaceID,
                                         nsAtom* aAttribute, int32_t aModType) {
+  if (sIsAttrElementChanging) {
+    // See the comment above the definition of sIsAttrElementChanging.
+    return;
+  }
   LocalAccessible* accessible = GetAccessible(aElement);
   if (!accessible) {
     if (aElement != mContent) return;
@@ -748,6 +765,7 @@ void DocAccessible::AttributeWillChange(dom::Element* aElement,
   // elements.
   if (aModType != dom::MutationEvent_Binding::ADDITION) {
     RemoveDependentIDsFor(accessible, aAttribute);
+    RemoveDependentElementsFor(accessible, aAttribute);
   }
 
   if (aAttribute == nsGkAtoms::id) {
@@ -782,6 +800,10 @@ void DocAccessible::AttributeChanged(dom::Element* aElement,
                                      int32_t aNameSpaceID, nsAtom* aAttribute,
                                      int32_t aModType,
                                      const nsAttrValue* aOldValue) {
+  if (sIsAttrElementChanging) {
+    // See the comment above the definition of sIsAttrElementChanging.
+    return;
+  }
   NS_ASSERTION(!IsDefunct(),
                "Attribute changed called on defunct document accessible!");
 
@@ -855,6 +877,7 @@ void DocAccessible::AttributeChanged(dom::Element* aElement,
   if (aModType == dom::MutationEvent_Binding::MODIFICATION ||
       aModType == dom::MutationEvent_Binding::ADDITION) {
     AddDependentIDsFor(accessible, aAttribute);
+    AddDependentElementsFor(accessible, aAttribute);
   }
 }
 
@@ -2906,3 +2929,22 @@ void DocAccessible::MaybeHandleChangeToHiddenNameOrDescription(
     }
   }
 }
+
+void DocAccessible::AttrElementWillChange(dom::Element* aElement,
+                                          nsAtom* aAttr) {
+  MOZ_ASSERT(!sIsAttrElementChanging);
+  AttributeWillChange(aElement, kNameSpaceID_None, aAttr,
+                      dom::MutationEvent_Binding::MODIFICATION);
+  // We might get notified about a related content attribute change. Ignore
+  // it.
+  sIsAttrElementChanging = true;
+}
+
+void DocAccessible::AttrElementChanged(dom::Element* aElement, nsAtom* aAttr) {
+  MOZ_ASSERT(sIsAttrElementChanging);
+  // The element has changed and the content attribute change notifications
+  // (if any) have been sent.
+  sIsAttrElementChanging = false;
+  AttributeChanged(aElement, kNameSpaceID_None, aAttr,
+                   dom::MutationEvent_Binding::MODIFICATION, nullptr);
+}

accessible/generic/DocAccessible.h

@@ -410,6 +410,9 @@ class DocAccessible : public HyperTextAccessible,
     return mMovedAccessibles.Contains(aAcc);
   }
 
+  void AttrElementWillChange(dom::Element* aElement, nsAtom* aAttr);
+  void AttrElementChanged(dom::Element* aElement, nsAtom* aAttr);
+
  protected:
   virtual ~DocAccessible();
 

accessible/tests/browser/e10s/browser_caching_relations_002.js

@@ -383,10 +383,8 @@ between
 </template></div>
 <script>
   const toggle1 = document.getElementById("toggle1");
-  const toggle2 = document.getElementById("toggle2");
   const popover1 = document.getElementById("popover1");
   toggle1.popoverTargetElement = popover1;
-  toggle2.popoverTargetElement = popover1;
   const toggle3 = document.getElementById("toggle3");
   const shadow = document.getElementById("shadowHost").shadowRoot;
   const toggle4 = shadow.getElementById("toggle4");
@@ -415,15 +413,35 @@ between
     toggle1.doAction(0);
     const popover1 = (await shown).accessible;
     await testCachedRelation(toggle1, RELATION_DETAILS, popover1);
-    await testCachedRelation(toggle2, RELATION_DETAILS, popover1);
     // toggle5 is inside the shadow DOM and popover1 is outside, so the target
     // is valid.
     await testCachedRelation(toggle5, RELATION_DETAILS, popover1);
+    await testCachedRelation(popover1, RELATION_DETAILS_FOR, [
+      toggle1,
+      toggle5,
+    ]);
+    info("Setting toggle2's popover target to popover1");
+    await invokeContentTask(browser, [], () => {
+      const toggle2Dom = content.document.getElementById("toggle2");
+      const popover1Dom = content.document.getElementById("popover1");
+      toggle2Dom.popoverTargetElement = popover1Dom;
+    });
+    await testCachedRelation(toggle2, RELATION_DETAILS, popover1);
     await testCachedRelation(popover1, RELATION_DETAILS_FOR, [
       toggle1,
       toggle2,
       toggle5,
     ]);
+    info("Clearing toggle2's popover target");
+    await invokeContentTask(browser, [], () => {
+      const toggle2Dom = content.document.getElementById("toggle2");
+      toggle2Dom.popoverTargetElement = null;
+    });
+    await testCachedRelation(toggle2, RELATION_DETAILS, []);
+    await testCachedRelation(popover1, RELATION_DETAILS_FOR, [
+      toggle1,
+      toggle5,
+    ]);
     info("Hiding popover1");
     let hidden = waitForEvent(EVENT_HIDE, popover1);
     toggle1.doAction(0);

accessible/tests/browser/e10s/browser_caching_states.js

@@ -571,7 +571,6 @@ addAccessibleTask(
   const toggle1 = document.getElementById("toggle1");
   const popover1 = document.getElementById("popover1");
   toggle1.popoverTargetElement = popover1;
-  toggle2.popoverTargetElement = popover1;
   const toggle3 = document.getElementById("toggle3");
   const shadow = document.getElementById("shadowHost").shadowRoot;
   const toggle4 = shadow.getElementById("toggle4");
@@ -584,9 +583,28 @@ addAccessibleTask(
   `,
   async function (browser, docAcc) {
     const toggle1 = findAccessibleChildByID(docAcc, "toggle1");
+    // toggle1's popover target is set and connected to the document.
     testStates(toggle1, STATE_COLLAPSED);
+
     const toggle2 = findAccessibleChildByID(docAcc, "toggle2");
+    // toggle2's popover target isn't set yet.
+    testStates(
+      toggle2,
+      0,
+      0,
+      STATE_EXPANDED | STATE_COLLAPSED,
+      EXT_STATE_EXPANDABLE
+    );
+    info("Setting toggle2's popoverTargetElement");
+    let changed = waitForStateChange(toggle2, EXT_STATE_EXPANDABLE, true, true);
+    await invokeContentTask(browser, [], () => {
+      const toggle2Dom = content.document.getElementById("toggle2");
+      const popover1 = content.document.getElementById("popover1");
+      toggle2Dom.popoverTargetElement = popover1;
+    });
+    await changed;
     testStates(toggle2, STATE_COLLAPSED);
+
     const toggle5 = findAccessibleChildByID(docAcc, "toggle5");
     // toggle5 is inside the shadow DOM and popover1 is outside, so the target
     // is valid.
@@ -599,7 +617,7 @@ addAccessibleTask(
       [EVENT_STATE_CHANGE, toggle5],
     ];
     info("Showing popover1");
-    let changed = waitForEvents(changeEvents);
+    changed = waitForEvents(changeEvents);
     toggle1.doAction(0);
     await changed;
     testStates(toggle1, STATE_EXPANDED);
@@ -612,6 +630,40 @@ addAccessibleTask(
     testStates(toggle1, STATE_COLLAPSED);
     testStates(toggle2, STATE_COLLAPSED);
 
+    info("Clearing toggle1's popover target");
+    changed = waitForStateChange(toggle1, EXT_STATE_EXPANDABLE, false, true);
+    await invokeContentTask(browser, [], () => {
+      const toggle1Dom = content.document.getElementById("toggle1");
+      toggle1Dom.popoverTargetElement = null;
+    });
+    await changed;
+    testStates(
+      toggle1,
+      0,
+      0,
+      STATE_EXPANDED | STATE_COLLAPSED,
+      EXT_STATE_EXPANDABLE
+    );
+
+    info("Setting toggle2's popover target to a disconnected node");
+    changed = waitForStateChange(toggle2, EXT_STATE_EXPANDABLE, false, true);
+    await invokeContentTask(browser, [], () => {
+      const toggle2Dom = content.document.getElementById("toggle2");
+      const popover3 = content.document.createElement("div");
+      popover3.popover = "auto";
+      popover3.textContent = "popover3";
+      // We don't append popover3 anywhere, so it is disconnected.
+      toggle2Dom.popoverTargetElement = popover3;
+    });
+    await changed;
+    testStates(
+      toggle2,
+      0,
+      0,
+      STATE_EXPANDED | STATE_COLLAPSED,
+      EXT_STATE_EXPANDABLE
+    );
+
     const toggle3 = findAccessibleChildByID(docAcc, "toggle3");
     // toggle3 is outside popover2's shadow DOM, so the target isn't valid.
     testStates(

dom/base/Element.cpp

@@ -1774,16 +1774,46 @@ void Element::ClearExplicitlySetAttrElement(nsAtom* aAttr) {
 }
 
 void Element::ExplicitlySetAttrElement(nsAtom* aAttr, Element* aElement) {
+#ifdef ACCESSIBILITY
+  nsAccessibilityService* accService = GetAccService();
+#endif
+  // Accessibility requires that no other attribute changes occur between
+  // AttrElementWillChange and AttrElementChanged. Scripts could cause
+  // this, so don't let them run here. We do this even if accessibility isn't
+  // running so that the JS behavior is consistent regardless of accessibility.
+  // Otherwise, JS might be able to use this difference to determine whether
+  // accessibility is running, which would be a privacy concern.
+  nsAutoScriptBlocker scriptBlocker;
   if (aElement) {
+#ifdef ACCESSIBILITY
+    if (accService) {
+      accService->NotifyAttrElementWillChange(this, aAttr);
+    }
+#endif
     SetAttr(aAttr, EmptyString(), IgnoreErrors());
     nsExtendedDOMSlots* slots = ExtendedDOMSlots();
     slots->mExplicitlySetAttrElements.InsertOrUpdate(
         aAttr, do_GetWeakReference(aElement));
+#ifdef ACCESSIBILITY
+    if (accService) {
+      accService->NotifyAttrElementChanged(this, aAttr);
+    }
+#endif
     return;
   }
 
+#ifdef ACCESSIBILITY
+  if (accService) {
+    accService->NotifyAttrElementWillChange(this, aAttr);
+  }
+#endif
   ClearExplicitlySetAttrElement(aAttr);
   UnsetAttr(aAttr, IgnoreErrors());
+#ifdef ACCESSIBILITY
+  if (accService) {
+    accService->NotifyAttrElementChanged(this, aAttr);
+  }
+#endif
 }
 
 Element* Element::GetExplicitlySetAttrElement(nsAtom* aAttr) const {