[A11y] Stability improvements for AXMenuList*

Helps land CL:4873421.

Note that the long term plan is for AXMenuList* and AXMockObject to go
away and instead use ordinary AXLayoutObjects backed by the shadow DOM.
This is already true on some platforms.

Bug: none
Change-Id: Ia062c8746c83dbbfb741244f3b93acef239e2652
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4968953
Commit-Queue: Aaron Leventhal <aleventhal@chromium.org>
Reviewed-by: Chris Harrelson <chrishtr@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1216983}

third_party/blink/renderer/modules/accessibility/ax_menu_list.cc

@@ -65,63 +65,59 @@ void AXMenuList::Detach() {
   // from the AXObjectCache.
   DCHECK_LE(children_.size(), 1U);
 
-  // Clear the popup.
+  // Clear the children.
   if (children_.size()) {
     // Do not call Remove() while AXObjectCacheImpl() is detaching all objects,
     // because the hash map of objects does not allow simultaneous iteration and
     // removal of objects.
+    CHECK(popup_ == children_[0]);
+    children_.clear();
+  }
+
+  // Clear the popup.
+  if (popup_) {
     if (!AXObjectCache().HasBeenDisposed()) {
       auto& cache = AXObjectCache();
       for (auto* const option_grandchild :
            To<HTMLSelectElement>(GetNode())->GetOptionList()) {
         cache.Remove(option_grandchild, /* notify_parent */ false);
       }
-      cache.Remove(children_[0], /* notify_parent */ false);
+      cache.Remove(popup_, /* notify_parent */ false);
     }
-    children_.clear();
+    popup_->Detach();
+    popup_ = nullptr;
   }
 
   AXLayoutObject::Detach();
 }
 
 void AXMenuList::ChildrenChangedWithCleanLayout() {
   if (!children_.empty()) {
-    if (AXObject* child_popup = children_[0]) {
-      // If we have a child popup, update its children at the same time.
-      DCHECK(IsA<AXMenuListPopup>(child_popup));
-      child_popup->ChildrenChangedWithCleanLayout();
-    }
+    CHECK_EQ(children_.size(), 1U);
+    CHECK_EQ(children_[0], popup_);
+    // If we have a child popup, update its children at the same time.
+    popup_->ChildrenChangedWithCleanLayout();
   }
 
-  AXObject::ChildrenChangedWithCleanLayout();
+  AXLayoutObject::ChildrenChangedWithCleanLayout();
 }
 
 void AXMenuList::SetNeedsToUpdateChildren() const {
   if (!children_.empty()) {
-    if (AXObject* child_popup = children_[0]) {
-      // If we have a child popup, update its children at the same time.
-      DCHECK(IsA<AXMenuListPopup>(child_popup));
-      child_popup->SetNeedsToUpdateChildren();
-    }
+    CHECK_EQ(children_.size(), 1U);
+    CHECK_EQ(children_[0], popup_);
+    // If we have a child popup, update its children at the same time.
+    popup_->SetNeedsToUpdateChildren();
   }
 
-  AXObject::SetNeedsToUpdateChildren();
+  AXLayoutObject::SetNeedsToUpdateChildren();
 }
 
 void AXMenuList::ClearChildren() const {
-  if (children_.empty())
-    return;
-
-  // Unless the menu list is detached, there's no reason to clear our
-  // AXMenuListPopup child. If we get a call to ClearChildren, it's because the
-  // options might have changed, so call it on our popup. Clearing the
-  // AXMenuListPopup child would cause additional thrashing and events that the
-  // AT would need to process, potentially causing the AT to believe that the
-  // popup had closed and a new popup and reopened.
-  // The mock AXMenuListPopup child will be cleared when this object is
-  // detached, as it has no use without this object as an owner.
-  DCHECK_EQ(children_.size(), 1U);
-  children_[0]->ClearChildren();
+  if (popup_) {
+    popup_->ClearChildren();
+  }
+  AXLayoutObject::ClearChildren();
 }
 
 void AXMenuList::AddChildren() {
@@ -130,38 +126,42 @@ void AXMenuList::AddChildren() {
   DCHECK(!is_adding_children_) << " Reentering method on " << GetNode();
   base::AutoReset<bool> reentrancy_protector(&is_adding_children_, true);
   // ClearChildren() does not clear the menulist popup chld.
-  DCHECK_LE(children_.size(), 1U)
-      << "Parent still has " << children_.size() << " children before adding:"
-      << "\nParent is " << ToString(true, true) << "\nFirst child is "
-      << children_[0]->ToString(true, true);
+  CHECK(children_.empty()) << "Parent still has " << children_.size()
+                           << " children before adding:"
+                           << "\nParent is " << ToString(true, true)
+                           << "\nFirst child is "
+                           << children_[0]->ToString(true, true);
 #endif
 
-  DCHECK(children_dirty_);
-  children_dirty_ = false;
+  CHECK(children_dirty_);
 
-  AXObject* ax_popup_child = GetOrCreateMockPopupChild();
+  GetOrCreateMockPopupChild();
+  CHECK(popup_);
+  children_.push_back(popup_);
+  popup_->SetParent(this);
+
+  children_dirty_ = false;
 
   // Update mock AXMenuListPopup children.
-  ax_popup_child->SetNeedsToUpdateChildren();
-  ax_popup_child->UpdateChildrenIfNecessary();
+  popup_->UpdateChildrenIfNecessary();
 }
 
 AXObject* AXMenuList::GetOrCreateMockPopupChild() {
-  if (IsDetached())
+  if (IsDetached()) {
+    popup_ = nullptr;
     return nullptr;
+  }
 
-  // Ensure mock AXMenuListPopup exists as first and only child.
-  if (children_.empty()) {
-    AXObjectCacheImpl& cache = AXObjectCache();
-    AXObject* popup =
-        cache.CreateAndInit(ax::mojom::blink::Role::kMenuListPopup, this);
-    DCHECK(popup);
-    DCHECK(!popup->IsDetached());
-    DCHECK(popup->CachedParentObject());
-    children_.push_back(popup);
+  if (!popup_) {
+    popup_ = AXObjectCache().CreateAndInit(
+        ax::mojom::blink::Role::kMenuListPopup, this);
+    CHECK(popup_);
+    CHECK(!popup_->IsDetached());
+    CHECK_EQ(popup_->CachedParentObject(), this);
+    CHECK(popup_->IsMenuListPopup());
   }
-  DCHECK_EQ(children_.size(), 1U);
-  return children_[0].Get();
+
+  return popup_;
 }
 
 bool AXMenuList::IsCollapsed() const {
@@ -226,4 +226,9 @@ void AXMenuList::DidHidePopup() {
     AXObjectCache().PostNotification(this, ax::mojom::Event::kFocus);
 }
 
+void AXMenuList::Trace(Visitor* visitor) const {
+  visitor->Trace(popup_);
+  AXLayoutObject::Trace(visitor);
+}
+
 }  // namespace blink

third_party/blink/renderer/modules/accessibility/ax_menu_list.h

@@ -40,6 +40,8 @@ class AXMenuList final : public AXLayoutObject {
   AXMenuList(const AXMenuList&) = delete;
   AXMenuList& operator=(const AXMenuList&) = delete;
 
+  void Trace(Visitor*) const override;
+
   AccessibilityExpanded IsExpanded() const final;
   bool OnNativeClickAction() override;
   void ChildrenChangedWithCleanLayout() override;
@@ -72,6 +74,8 @@ class AXMenuList final : public AXLayoutObject {
   bool IsCollapsed() const;
 
   WTF::Vector<gfx::Rect> options_bounds_;
+
+  Member<AXObject> popup_;
 };
 
 template <>

third_party/blink/renderer/modules/accessibility/ax_menu_list_popup.cc

@@ -35,6 +35,16 @@ namespace blink {
 AXMenuListPopup::AXMenuListPopup(AXObjectCacheImpl& ax_object_cache)
     : AXMockObject(ax_object_cache), active_index_(-1) {}
 
+void AXMenuListPopup::Init(AXObject* parent) {
+  owner_ = parent;
+  AXMockObject::Init(parent);
+}
+
+void AXMenuListPopup::Detach() {
+  owner_ = nullptr;
+  AXMockObject::Detach();
+}
+
 ax::mojom::blink::Role AXMenuListPopup::NativeRoleIgnoringAria() const {
   return ax::mojom::blink::Role::kMenuListPopup;
 }
@@ -134,6 +144,7 @@ void AXMenuListPopup::AddChildren() {
         << ax_preexisting->CachedParentObject()->ToString(true, true);
 #endif
     AXMenuListOption* option = MenuListOptionAXObject(option_element);
+    CHECK(!option->IsMissingParent());
     if (option && option->AccessibilityIsIncludedInTree()) {
       DCHECK(!option->IsDetached());
       children_.push_back(option);
@@ -208,4 +219,9 @@ AXObject* AXMenuListPopup::ActiveDescendant() {
   return AXObjectCache().Get(option);
 }
 
+void AXMenuListPopup::Trace(Visitor* visitor) const {
+  visitor->Trace(owner_);
+  AXMockObject::Trace(visitor);
+}
+
 }  // namespace blink

third_party/blink/renderer/modules/accessibility/ax_menu_list_popup.h

@@ -42,13 +42,19 @@ class AXMenuListPopup final : public AXMockObject {
   AXMenuListPopup(const AXMenuListPopup&) = delete;
   AXMenuListPopup& operator=(const AXMenuListPopup&) = delete;
 
+  void Trace(Visitor*) const override;
+
+  void Init(AXObject* parent) override;
+  void Detach() override;
+
   AXRestriction Restriction() const override;
   bool IsOffScreen() const override;
 
   void DidUpdateActiveOption(int option_index, bool fire_notifications = true);
   void DidShow();
   void DidHide();
   AXObject* ActiveDescendant() final;
+  AXObject* owner() const { return owner_; }
 
  private:
   bool IsMenuListPopup() const override { return true; }
@@ -65,6 +71,8 @@ class AXMenuListPopup final : public AXMockObject {
 
   // Note that this may be -1 if nothing is selected.
   int active_index_;
+
+  Member<AXObject> owner_;
 };
 
 template <>

third_party/blink/renderer/modules/accessibility/ax_mock_object.cc

@@ -35,7 +35,9 @@ AXMockObject::AXMockObject(AXObjectCacheImpl& ax_object_cache)
 AXMockObject::~AXMockObject() = default;
 
 Document* AXMockObject::GetDocument() const {
-  return ParentObject() ? ParentObject()->GetDocument() : nullptr;
+  // This assumes that the mock object (only used for <select> popups) will not
+  // occur within another popup document (a popup within another popup).
+  return &AXObjectCache().GetDocument();
 }
 
 ax::mojom::blink::Role AXMockObject::NativeRoleIgnoringAria() const {

third_party/blink/renderer/modules/accessibility/ax_object.cc

@@ -974,20 +974,19 @@ AXObject* AXObject::ComputeParent() const {
 // Same as ComputeParent, but without the extra check for valid parent in the
 // end. This is for use in RestoreParentOrPrune.
 AXObject* AXObject::ComputeParentOrNull() const {
-#if defined(AX_FAIL_FAST_BUILD)
-  SANITIZER_CHECK(!IsDetached());
+  CHECK(!IsDetached());
 
-  SANITIZER_CHECK(!IsMockObject())
-      << "A mock object must have a parent, and cannot exist without one. "
-         "The parent is set when the object is constructed.";
+  if (IsMockObject()) {
+    const AXMenuListPopup* popup = To<AXMenuListPopup>(this);
+    return popup->owner();
+  }
 
-  SANITIZER_CHECK(GetNode() || GetLayoutObject() || IsVirtualObject())
+  CHECK(GetNode() || GetLayoutObject() || IsVirtualObject())
       << "Can't compute parent on AXObjects without a backing Node "
          "LayoutObject, "
          " or AccessibleNode. Objects without those must set the "
          "parent in Init(), |this| = "
       << RoleValue();
-#endif
 
   AXObject* ax_parent = nullptr;
   if (IsAXInlineTextBox()) {
@@ -3116,16 +3115,6 @@ void AXObject::UpdateCachedAttributeValuesIfNeeded(
     return;
   }
 
-  // Mock objects are created by, owned and dependent on their parents.
-  // If the mock object's values change, recompute the parent's as well.
-  // Note: The only remaining use of mock objects is AXMenuListPopup.
-  // TODO(accessibility) Remove this when we remove AXMenuList* and create the
-  // AX hierarchy for <select> from the shadow dom instead.
-  if (IsMockObject()) {
-    DCHECK(parent_);
-    parent_->UpdateCachedAttributeValuesIfNeeded();
-  }
-
   if (!cached_values_need_update_) {
     return;
   }
@@ -3164,7 +3153,7 @@ void AXObject::UpdateCachedAttributeValuesIfNeeded(
   // TODO(accessibility) Can this be fixed by instead invalidating the parent
   // when invalidating the child?
   if (IsMockObject()) {
-    DCHECK(parent_);
+    CHECK(parent_) << "Mock object missing parent: " << ToString(true, true);
     parent_->UpdateCachedAttributeValuesIfNeeded();
   }