Bug 1744009 - Accessibility fixes for new combobox layout code. r=eeejay

In terms of the C++ code, this patch does basically one thing, which is
allowing creating option / optgroup accessibles without a frame for
comboboxes, and tracking mutations like layout does.

It seems this should be straight-forward, but handling mutations got a
bit complicated. We don't want to forcibly re-create accessibles, so we
want to re-use the PruneOrInsertSubtree logic that ContentInserted uses.

But determining whether we need to create the accessible requires
having flushed styles, so I added a ScheduleAccessibilitySubtreeUpdate
API to trigger that from WillRefresh once style and layout are
up-to-date.

The rest of the test updates should be sort of straight-forward. They
reflect two changes:

 * <option> accessibles are leaves now (so they don't have text
   children). Note that we still have the right native name and so on,
   using the same logic we use to render the label.

 * In 1proc tests, the focus no longer goes to the <option>, and uses
   the same code-path that e10s does (moving focus to a <menulist> in
   the parent process). Since that wasn't easy to test for (afaict) and
   we have browser tests to cover that
   (browser_treeupdate_select_dropdown.js, etc), I've decided to just
   remove the tests that relied on the previous code-path, as they were
   testing for a codepath that users weren't hitting anyways.

I've tested this with JAWS and Orca and behavior seems unchanged to my
knowledge.

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

accessible/base/NotificationController.cpp

@@ -678,6 +678,8 @@ void NotificationController::WillRefresh(mozilla::TimeStamp aTime) {
                  "isn't created!");
   }
 
+  mDocument->ProcessPendingUpdates();
+
   nsTArray<CacheData> cache;
 
   // Process rendered text change notifications.

accessible/base/nsAccessibilityService.cpp

@@ -484,6 +484,22 @@ void nsAccessibilityService::ContentRangeInserted(PresShell* aPresShell,
   }
 }
 
+void nsAccessibilityService::ScheduleAccessibilitySubtreeUpdate(
+    PresShell* aPresShell, nsIContent* aContent) {
+  DocAccessible* document = GetDocAccessible(aPresShell);
+#ifdef A11Y_LOG
+  if (logging::IsEnabled(logging::eTree)) {
+    logging::MsgBegin("TREE", "schedule update; doc: %p", document);
+    logging::Node("content node", aContent);
+    logging::MsgEnd();
+  }
+#endif
+
+  if (document) {
+    document->ScheduleTreeUpdate(aContent);
+  }
+}
+
 void nsAccessibilityService::ContentRemoved(PresShell* aPresShell,
                                             nsIContent* aChildNode) {
   DocAccessible* document = GetDocAccessible(aPresShell);
@@ -518,6 +534,27 @@ void nsAccessibilityService::TableLayoutGuessMaybeChanged(
   }
 }
 
+void nsAccessibilityService::ComboboxOptionMaybeChanged(
+    PresShell* aPresShell, nsIContent* aMutatingNode) {
+  DocAccessible* document = GetDocAccessible(aPresShell);
+  if (!document) {
+    return;
+  }
+
+  for (nsIContent* cur = aMutatingNode; cur; cur = cur->GetParent()) {
+    if (cur->IsHTMLElement(nsGkAtoms::option)) {
+      if (LocalAccessible* accessible = document->GetAccessible(cur)) {
+        document->FireDelayedEvent(nsIAccessibleEvent::EVENT_NAME_CHANGE,
+                                   accessible);
+        break;
+      }
+      if (cur->IsHTMLElement(nsGkAtoms::select)) {
+        break;
+      }
+    }
+  }
+}
+
 void nsAccessibilityService::UpdateText(PresShell* aPresShell,
                                         nsIContent* aContent) {
   DocAccessible* document = GetDocAccessible(aPresShell);
@@ -882,7 +919,7 @@ LocalAccessible* nsAccessibilityService::CreateAccessible(
   if (!frame || !frame->StyleVisibility()->IsVisible()) {
     // display:contents element doesn't have a frame, but retains the semantics.
     // All its children are unaffected.
-    if (nsCoreUtils::IsDisplayContents(content)) {
+    if (nsCoreUtils::CanCreateAccessibleWithoutFrame(content)) {
       const MarkupMapInfo* markupMap = GetMarkupMapInfoForNode(content);
       if (markupMap && markupMap->new_func) {
         RefPtr<LocalAccessible> newAcc =

accessible/base/nsAccessibilityService.h

@@ -166,6 +166,14 @@ class nsAccessibilityService final : public mozilla::a11y::DocManager,
   void ContentRangeInserted(mozilla::PresShell* aPresShell,
                             nsIContent* aStartChild, nsIContent* aEndChild);
 
+  /**
+   * Triggers a re-evaluation of the a11y tree of aContent after the next
+   * refresh. This is important because whether we create accessibles may
+   * depend on the frame tree / style.
+   */
+  void ScheduleAccessibilitySubtreeUpdate(mozilla::PresShell* aPresShell,
+                                          nsIContent* aStartChild);
+
   /**
    * Notification used to update the accessible tree when content is removed.
    */
@@ -177,6 +185,12 @@ class nsAccessibilityService final : public mozilla::a11y::DocManager,
   void TableLayoutGuessMaybeChanged(mozilla::PresShell* aPresShell,
                                     nsIContent* aContent);
 
+  /**
+   * Notifies when a combobox <option> text or label changes.
+   */
+  void ComboboxOptionMaybeChanged(mozilla::PresShell*,
+                                  nsIContent* aMutatingNode);
+
   void UpdateText(mozilla::PresShell* aPresShell, nsIContent* aContent);
 
   /**

accessible/base/nsCoreUtils.cpp

@@ -580,8 +580,26 @@ void nsCoreUtils::DispatchAccEvent(RefPtr<nsIAccessibleEvent> event) {
 }
 
 bool nsCoreUtils::IsDisplayContents(nsIContent* aContent) {
-  return aContent && aContent->IsElement() &&
-         aContent->AsElement()->IsDisplayContents();
+  auto* element = Element::FromNodeOrNull(aContent);
+  return element && element->IsDisplayContents();
+}
+
+bool nsCoreUtils::CanCreateAccessibleWithoutFrame(nsIContent* aContent) {
+  auto* element = Element::FromNodeOrNull(aContent);
+  if (!element) {
+    return false;
+  }
+  if (!element->HasServoData() || Servo_Element_IsDisplayNone(element)) {
+    // Out of the flat tree or in a display: none subtree.
+    return false;
+  }
+  if (element->IsDisplayContents()) {
+    return true;
+  }
+  // We don't have a frame, but we're not display: contents either.
+  // For now, only create accessibles for <option>/<optgroup> as our combobox
+  // select code depends on it.
+  return element->IsAnyOfHTMLElements(nsGkAtoms::option, nsGkAtoms::optgroup);
 }
 
 bool nsCoreUtils::IsDocumentVisibleConsideringInProcessAncestors(

accessible/base/nsCoreUtils.h

@@ -318,6 +318,7 @@ class nsCoreUtils {
   static void DispatchAccEvent(RefPtr<nsIAccessibleEvent> aEvent);
 
   static bool IsDisplayContents(nsIContent* aContent);
+  static bool CanCreateAccessibleWithoutFrame(nsIContent* aContent);
 
   /**
    * Return whether the document and all its in-process ancestors are visible in

accessible/generic/DocAccessible.cpp

@@ -131,6 +131,7 @@ NS_IMPL_CYCLE_COLLECTION_TRAVERSE_BEGIN_INHERITED(DocAccessible,
   NS_IMPL_CYCLE_COLLECTION_TRAVERSE(mAccessibleCache)
   NS_IMPL_CYCLE_COLLECTION_TRAVERSE(mAnchorJumpElm)
   NS_IMPL_CYCLE_COLLECTION_TRAVERSE(mInvalidationList)
+  NS_IMPL_CYCLE_COLLECTION_TRAVERSE(mPendingUpdates)
   for (const auto& ar : tmp->mARIAOwnsHash.Values()) {
     for (uint32_t i = 0; i < ar->Length(); i++) {
       NS_CYCLE_COLLECTION_NOTE_EDGE_NAME(cb, "mARIAOwnsHash entry item");
@@ -148,6 +149,7 @@ NS_IMPL_CYCLE_COLLECTION_UNLINK_BEGIN_INHERITED(DocAccessible, LocalAccessible)
   NS_IMPL_CYCLE_COLLECTION_UNLINK(mAccessibleCache)
   NS_IMPL_CYCLE_COLLECTION_UNLINK(mAnchorJumpElm)
   NS_IMPL_CYCLE_COLLECTION_UNLINK(mInvalidationList)
+  NS_IMPL_CYCLE_COLLECTION_UNLINK(mPendingUpdates)
   NS_IMPL_CYCLE_COLLECTION_UNLINK_WEAK_REFERENCE
   tmp->mARIAOwnsHash.Clear();
 NS_IMPL_CYCLE_COLLECTION_UNLINK_END
@@ -457,6 +459,7 @@ void DocAccessible::Shutdown() {
 
   mAnchorJumpElm = nullptr;
   mInvalidationList.Clear();
+  mPendingUpdates.Clear();
 
   for (auto iter = mAccessibleCache.Iter(); !iter.Done(); iter.Next()) {
     LocalAccessible* accessible = iter.Data();
@@ -1164,6 +1167,25 @@ void DocAccessible::ContentInserted(nsIContent* aStartChildNode,
   mNotificationController->ScheduleContentInsertion(container, list);
 }
 
+void DocAccessible::ScheduleTreeUpdate(nsIContent* aContent) {
+  if (mPendingUpdates.Contains(aContent)) {
+    return;
+  }
+  mPendingUpdates.AppendElement(aContent);
+  mNotificationController->ScheduleProcessing();
+}
+
+void DocAccessible::ProcessPendingUpdates() {
+  auto updates = std::move(mPendingUpdates);
+  for (auto update : updates) {
+    if (update->GetComposedDoc() != mDocumentNode) {
+      continue;
+    }
+    // The pruning logic will take care of avoiding unnecessary notifications.
+    ContentInserted(update, update->GetNextSibling());
+  }
+}
+
 bool DocAccessible::PruneOrInsertSubtree(nsIContent* aRoot) {
   bool insert = false;
 
@@ -1178,7 +1200,7 @@ bool DocAccessible::PruneOrInsertSubtree(nsIContent* aRoot) {
     // then remove their accessibles and subtrees.
     while (nsIContent* childNode = iter.GetNextChild()) {
       if (!childNode->GetPrimaryFrame() &&
-          !nsCoreUtils::IsDisplayContents(childNode)) {
+          !nsCoreUtils::CanCreateAccessibleWithoutFrame(childNode)) {
         ContentRemoved(childNode);
       }
     }
@@ -1189,7 +1211,7 @@ bool DocAccessible::PruneOrInsertSubtree(nsIContent* aRoot) {
       for (nsIContent* childNode = aRoot->GetFirstChild(); childNode;
            childNode = childNode->GetNextSibling()) {
         if (!childNode->GetPrimaryFrame() &&
-            !nsCoreUtils::IsDisplayContents(childNode)) {
+            !nsCoreUtils::CanCreateAccessibleWithoutFrame(childNode)) {
           ContentRemoved(childNode);
         }
       }
@@ -1218,7 +1240,7 @@ bool DocAccessible::PruneOrInsertSubtree(nsIContent* aRoot) {
     // As well as removing the a11y subtree, we must also remove Accessibles
     // for DOM descendants, since some of these might be relocated Accessibles
     // and their DOM nodes are now hidden as well.
-    if (!frame && !nsCoreUtils::IsDisplayContents(aRoot)) {
+    if (!frame && !nsCoreUtils::CanCreateAccessibleWithoutFrame(aRoot)) {
       ContentRemoved(aRoot);
       return false;
     }
@@ -1280,7 +1302,8 @@ bool DocAccessible::PruneOrInsertSubtree(nsIContent* aRoot) {
   } else {
     // If there is no current accessible, and the node has a frame, or is
     // display:contents, schedule it for insertion.
-    if (aRoot->GetPrimaryFrame() || nsCoreUtils::IsDisplayContents(aRoot)) {
+    if (aRoot->GetPrimaryFrame() ||
+        nsCoreUtils::CanCreateAccessibleWithoutFrame(aRoot)) {
       // This may be a new subtree, the insertion process will recurse through
       // its descendants.
       if (!GetAccessibleOrDescendant(aRoot)) {

accessible/generic/DocAccessible.h

@@ -335,6 +335,11 @@ class DocAccessible : public HyperTextAccessibleWrap,
    */
   void ContentInserted(nsIContent* aStartChildNode, nsIContent* aEndChildNode);
 
+  /**
+   * @see nsAccessibilityService::ScheduleAccessibilitySubtreeUpdate
+   */
+  void ScheduleTreeUpdate(nsIContent* aContent);
+
   /**
    * Update the tree on content removal.
    */
@@ -490,6 +495,11 @@ class DocAccessible : public HyperTextAccessibleWrap,
    */
   void ProcessInvalidationList();
 
+  /**
+   * Process mPendingUpdates
+   */
+  void ProcessPendingUpdates();
+
   /**
    * Called from NotificationController to process this doc's
    * mMaybeBoundsChanged list. Sends a cache update for each acc in this
@@ -718,6 +728,11 @@ class DocAccessible : public HyperTextAccessibleWrap,
                    nsTArray<RefPtr<LocalAccessible>>>
       mARIAOwnsHash;
 
+  /**
+   * Keeps a list of pending subtrees to update post-refresh.
+   */
+  nsTArray<RefPtr<nsIContent>> mPendingUpdates;
+
   /**
    * Used to process notification from core and accessible events.
    */

accessible/html/HTMLSelectAccessible.cpp

@@ -16,6 +16,7 @@
 
 #include "nsCOMPtr.h"
 #include "mozilla/dom/HTMLOptionElement.h"
+#include "mozilla/dom/HTMLOptGroupElement.h"
 #include "mozilla/dom/HTMLSelectElement.h"
 #include "nsComboboxControlFrame.h"
 #include "nsContainerFrame.h"
@@ -121,22 +122,20 @@ role HTMLSelectOptionAccessible::NativeRole() const {
 }
 
 ENameValueFlag HTMLSelectOptionAccessible::NativeName(nsString& aName) const {
-  // CASE #1 -- great majority of the cases
-  // find the label attribute - this is what the W3C says we should use
-  mContent->AsElement()->GetAttr(kNameSpaceID_None, nsGkAtoms::label, aName);
-  if (!aName.IsEmpty()) return eNameOK;
-
-  // CASE #2 -- no label parameter, get the first child,
-  // use it if it is a text node
-  LocalAccessible* firstChild = LocalFirstChild();
-  nsIContent* text = firstChild ? firstChild->GetContent() : nullptr;
-  if (text && text->IsText()) {
-    nsTextEquivUtils::AppendTextEquivFromTextContent(text, &aName);
-    aName.CompressWhitespace();
+  if (auto* option = dom::HTMLOptionElement::FromNode(mContent)) {
+    option->GetAttr(nsGkAtoms::label, aName);
+    if (!aName.IsEmpty()) {
+      return eNameOK;
+    }
+    option->GetText(aName);
+    return eNameFromSubtree;
+  }
+  if (auto* group = dom::HTMLOptGroupElement::FromNode(mContent)) {
+    group->GetLabel(aName);
     return aName.IsEmpty() ? eNameOK : eNameFromSubtree;
   }
-
-  return eNameOK;
+  MOZ_ASSERT_UNREACHABLE("What content do we have?");
+  return eNameFromSubtree;
 }
 
 void HTMLSelectOptionAccessible::DOMAttributeChanged(

accessible/tests/browser/e10s/browser_treeupdate_optgroup.js

@@ -41,10 +41,7 @@ addAccessibleTask(
         {
           COMBOBOX_LIST: [
             {
-              GROUPING: [
-                { COMBOBOX_OPTION: [{ TEXT_LEAF: [] }] },
-                { COMBOBOX_OPTION: [{ TEXT_LEAF: [] }] },
-              ],
+              GROUPING: [{ COMBOBOX_OPTION: [] }, { COMBOBOX_OPTION: [] }],
             },
             {
               COMBOBOX_OPTION: [],

accessible/tests/browser/e10s/browser_treeupdate_select_dropdown.js

@@ -40,6 +40,7 @@ addAccessibleTask(
       const EventUtils = ContentTaskUtils.getEventUtils(content);
       EventUtils.synthesizeKey("VK_DOWN", { altKey: true }, content);
     });
+    info("Waiting for parent focus");
     let event = await focused;
     let dropdown = event.accessible.parent;
 
@@ -64,13 +65,8 @@ addAccessibleTask(
       "select",
       "select focused after collapsed"
     );
-    await invokeContentTask(browser, [], () => {
-      const { ContentTaskUtils } = ChromeUtils.import(
-        "resource://testing-common/ContentTaskUtils.jsm"
-      );
-      const EventUtils = ContentTaskUtils.getEventUtils(content);
-      EventUtils.synthesizeKey("VK_ESCAPE", {}, content);
-    });
+    EventUtils.synthesizeKey("VK_ESCAPE", {}, window);
+    info("Waiting for child focus");
     await focused;
   },
   { iframe: true, remoteIframe: true }

accessible/tests/mochitest/actions/test_select.html

@@ -32,42 +32,6 @@
             new focusChecker("lb_apple"),
           ],
         },
-        {
-          ID: "combobox",
-          actionIndex: 0,
-          actionName: "open",
-          events: CLICK_EVENTS,
-          eventSeq: [
-            new focusChecker("cb_orange"),
-          ],
-        },
-        {
-          ID: "cb_apple",
-          actionIndex: 0,
-          actionName: "select",
-          events: CLICK_EVENTS,
-          eventSeq: [
-            new focusChecker("combobox"),
-          ],
-        },
-        {
-          ID: "combobox",
-          actionIndex: 0,
-          actionName: "open",
-          events: CLICK_EVENTS,
-          eventSeq: [
-            new focusChecker("cb_apple"),
-          ],
-        },
-        {
-          ID: "combobox",
-          actionIndex: 0,
-          actionName: "close",
-          events: CLICK_EVENTS,
-          eventSeq: [
-            new focusChecker("combobox"),
-          ],
-        },
       ];
 
       testActions(actionsArray);

accessible/tests/mochitest/bounds/a11y.ini

@@ -3,4 +3,3 @@ support-files =
   !/accessible/tests/mochitest/*.js
 
 [test_list.html]
-[test_select.html]