AX: Any addition of children should funnel through AccessibilityObjec…

…t::addChild

https://bugs.webkit.org/show_bug.cgi?id=231914

Patch by Tyler Wilcock <tyler_w@apple.com> on 2021-10-21
Reviewed by Chris Fleizach.

All addition of children now goes through
AccessibilityObject::addChild. This is good for two reasons:

1. It ensures we aren't inserting ignored elements into the tree.
`insertChild` (downstream of `addChild`) checks this. Prior to this
patch, there were cases where we could insert ignored children into the
tree because no check was made.

2. We can reliably set state on the child based on the state of the
parent at insertion time. For example, children can set a flag if
any of their ancestors have an application or document role, which can
be useful for some AX clients.

* accessibility/AccessibilityARIAGrid.cpp:
(WebCore::AccessibilityARIAGrid::addTableCellChild):
(WebCore::AccessibilityARIAGrid::addChildren):
* accessibility/AccessibilityListBox.cpp:
(WebCore::AccessibilityListBox::addChildren):
* accessibility/AccessibilityMenuList.cpp:
(WebCore::AccessibilityMenuList::addChildren):
* accessibility/AccessibilityMenuListPopup.cpp:
(WebCore::AccessibilityMenuListPopup::addChildren):
* accessibility/AccessibilityObject.cpp:
(WebCore::isAutofillButton):
(WebCore::isTableComponent):
(WebCore::AccessibilityObject::insertChild):
* accessibility/AccessibilityRenderObject.cpp:
(WebCore::AccessibilityRenderObject::addImageMapChildren):
(WebCore::AccessibilityRenderObject::addTextFieldChildren):
(WebCore::AccessibilityRenderObject::addRemoteSVGChildren):
* accessibility/AccessibilityScrollView.cpp:
(WebCore::AccessibilityScrollView::addChildScrollbar):
* accessibility/AccessibilitySlider.cpp:
(WebCore::AccessibilitySlider::addChildren):
* accessibility/AccessibilitySpinButton.cpp:
(WebCore::AccessibilitySpinButton::addChildren):
* accessibility/AccessibilityTable.cpp:
(WebCore::AccessibilityTable::addChildren):
(WebCore::AccessibilityTable::addTableCellChild):
* accessibility/AccessibilityTableColumn.cpp:
(WebCore::AccessibilityTableColumn::addChildren):
* accessibility/AccessibilityTableHeaderContainer.cpp:
(WebCore::AccessibilityTableHeaderContainer::addChildren):
* accessibility/AccessibilityTableRow.cpp:
(WebCore::AccessibilityTableRow::addChildren):

Canonical link: https://commits.webkit.org/243335@main
git-svn-id: https://svn.webkit.org/repository/webkit/trunk@284606 268f45cc-cd09-0410-ab3c-d52691b4dbfc

Source/WebCore/ChangeLog

@@ -1,3 +1,56 @@
+2021-10-21  Tyler Wilcock  <tyler_w@apple.com>
+
+        AX: Any addition of children should funnel through AccessibilityObject::addChild
+        https://bugs.webkit.org/show_bug.cgi?id=231914
+
+        Reviewed by Chris Fleizach.
+
+        All addition of children now goes through
+        AccessibilityObject::addChild. This is good for two reasons:
+
+        1. It ensures we aren't inserting ignored elements into the tree.
+        `insertChild` (downstream of `addChild`) checks this. Prior to this
+        patch, there were cases where we could insert ignored children into the
+        tree because no check was made.
+
+        2. We can reliably set state on the child based on the state of the
+        parent at insertion time. For example, children can set a flag if
+        any of their ancestors have an application or document role, which can
+        be useful for some AX clients.
+
+        * accessibility/AccessibilityARIAGrid.cpp:
+        (WebCore::AccessibilityARIAGrid::addTableCellChild):
+        (WebCore::AccessibilityARIAGrid::addChildren):
+        * accessibility/AccessibilityListBox.cpp:
+        (WebCore::AccessibilityListBox::addChildren):
+        * accessibility/AccessibilityMenuList.cpp:
+        (WebCore::AccessibilityMenuList::addChildren):
+        * accessibility/AccessibilityMenuListPopup.cpp:
+        (WebCore::AccessibilityMenuListPopup::addChildren):
+        * accessibility/AccessibilityObject.cpp:
+        (WebCore::isAutofillButton):
+        (WebCore::isTableComponent):
+        (WebCore::AccessibilityObject::insertChild):
+        * accessibility/AccessibilityRenderObject.cpp:
+        (WebCore::AccessibilityRenderObject::addImageMapChildren):
+        (WebCore::AccessibilityRenderObject::addTextFieldChildren):
+        (WebCore::AccessibilityRenderObject::addRemoteSVGChildren):
+        * accessibility/AccessibilityScrollView.cpp:
+        (WebCore::AccessibilityScrollView::addChildScrollbar):
+        * accessibility/AccessibilitySlider.cpp:
+        (WebCore::AccessibilitySlider::addChildren):
+        * accessibility/AccessibilitySpinButton.cpp:
+        (WebCore::AccessibilitySpinButton::addChildren):
+        * accessibility/AccessibilityTable.cpp:
+        (WebCore::AccessibilityTable::addChildren):
+        (WebCore::AccessibilityTable::addTableCellChild):
+        * accessibility/AccessibilityTableColumn.cpp:
+        (WebCore::AccessibilityTableColumn::addChildren):
+        * accessibility/AccessibilityTableHeaderContainer.cpp:
+        (WebCore::AccessibilityTableHeaderContainer::addChildren):
+        * accessibility/AccessibilityTableRow.cpp:
+        (WebCore::AccessibilityTableRow::addChildren):
+
 2021-10-21  Antti Koivisto  <antti@apple.com>
 
         Move Style::Resolver::State out of header

Source/WebCore/accessibility/AccessibilityARIAGrid.cpp

@@ -67,14 +67,7 @@ bool AccessibilityARIAGrid::addTableCellChild(AXCoreObject* child, HashSet<Acces
 
     row.setRowIndex((int)m_rows.size());
     m_rows.append(&row);
-
-    // Try adding the row if it's not ignoring accessibility,
-    // otherwise add its children (the cells) as the grid's children.
-    if (!row.accessibilityIsIgnored())
-        m_children.append(&row);
-    else
-        m_children.appendVector(row.children());
-
+    addChild(&row);
     appendedRows.add(&row);
     return true;
 }
@@ -142,13 +135,10 @@ void AccessibilityARIAGrid::addChildren()
         column.setColumnIndex(i);
         column.setParent(this);
         m_columns.append(&column);
-        if (!column.accessibilityIsIgnored())
-            m_children.append(&column);
+        addChild(&column);
     }
 
-    auto* headerContainerObject = headerContainer();
-    if (headerContainerObject && !headerContainerObject->accessibilityIsIgnored())
-        m_children.append(headerContainerObject);
+    addChild(headerContainer());
 }
 
 } // namespace WebCore

Source/WebCore/accessibility/AccessibilityListBox.cpp

@@ -73,11 +73,8 @@ void AccessibilityListBox::addChildren()
 
     m_haveChildren = true;
 
-    for (const auto& listItem : downcast<HTMLSelectElement>(*selectNode).listItems()) {
-        AccessibilityObject* listOption = listBoxOptionAccessibilityObject(listItem);
-        if (listOption && !listOption->accessibilityIsIgnored())
-            m_children.append(listOption);
-    }
+    for (const auto& listItem : downcast<HTMLSelectElement>(*selectNode).listItems())
+        addChild(listBoxOptionAccessibilityObject(listItem));
 }
 
 void AccessibilityListBox::setSelectedChildren(const AccessibilityChildrenVector& children)

Source/WebCore/accessibility/AccessibilityMenuList.cpp

@@ -82,8 +82,7 @@ void AccessibilityMenuList::addChildren()
     }
 
     m_haveChildren = true;
-    m_children.append(list);
-
+    addChild(list);
     list->addChildren();
 }
 

Source/WebCore/accessibility/AccessibilityMenuListPopup.cpp

@@ -96,11 +96,8 @@ void AccessibilityMenuListPopup::addChildren()
 
     m_haveChildren = true;
 
-    for (const auto& listItem : downcast<HTMLSelectElement>(*selectNode).listItems()) {
-        // FIXME: Why does AccessibilityListBox::addChildren check accessibilityIsIgnored but this does not?
-        if (auto option = menuListOptionAccessibilityObject(listItem))
-            m_children.append(option);
-    }
+    for (const auto& listItem : downcast<HTMLSelectElement>(*selectNode).listItems())
+        addChild(menuListOptionAccessibilityObject(listItem));
 }
 
 void AccessibilityMenuListPopup::childrenChanged()

Source/WebCore/accessibility/AccessibilityObject.cpp

@@ -483,7 +483,14 @@ static void appendAccessibilityObject(AXCoreObject* object, AccessibilityObject:
     if (object)
         results.append(object);
 }
-
+
+#ifndef NDEBUG
+static bool isTableComponent(AXCoreObject& axObject)
+{
+    return axObject.isTable() || axObject.isTableColumn() || axObject.isTableRow() || axObject.isTableCell();
+}
+#endif
+
 void AccessibilityObject::insertChild(AXCoreObject* child, unsigned index)
 {
     if (!child)
@@ -516,7 +523,9 @@ void AccessibilityObject::insertChild(AXCoreObject* child, unsigned index)
         for (size_t i = 0; i < length; ++i)
             m_children.insert(index + i, children[i]);
     } else {
-        ASSERT(child->parentObject() == this);
+        // Table component child-parent relationships often don't line up properly, hence the need for methods
+        // like parentTable() and parentRow(). Exclude them from this ASSERT.
+        ASSERT((!isTableComponent(*child) && !isTableComponent(*this)) ? child->parentObject() == this : true);
         m_children.insert(index, child);
     }
 

Source/WebCore/accessibility/AccessibilityRenderObject.cpp

@@ -3283,7 +3283,7 @@ void AccessibilityRenderObject::addImageMapChildren()
         areaObject.setHTMLMapElement(map);
         areaObject.setParent(this);
         if (!areaObject.accessibilityIsIgnored())
-            m_children.append(&areaObject);
+            addChild(&areaObject);
         else
             axObjectCache()->remove(areaObject.objectID());
     }
@@ -3316,7 +3316,7 @@ void AccessibilityRenderObject::addTextFieldChildren()
     auto& axSpinButton = downcast<AccessibilitySpinButton>(*axObjectCache()->create(AccessibilityRole::SpinButton));
     axSpinButton.setSpinButtonElement(downcast<SpinButtonElement>(spinButtonElement));
     axSpinButton.setParent(this);
-    m_children.append(&axSpinButton);
+    addChild(&axSpinButton);
 }
 
 bool AccessibilityRenderObject::isSVGImage() const
@@ -3380,12 +3380,7 @@ void AccessibilityRenderObject::addRemoteSVGChildren()
     // In order to connect the AX hierarchy from the SVG root element from the loaded resource
     // the parent must be set, because there's no other way to get back to who created the image.
     root->setParent(this);
-
-    if (root->accessibilityIsIgnored()) {
-        for (const auto& child : root->children())
-            m_children.append(child);
-    } else
-        m_children.append(root);
+    addChild(root);
 }
 
 void AccessibilityRenderObject::addCanvasChildren()

Source/WebCore/accessibility/AccessibilityScrollView.cpp

@@ -163,7 +163,7 @@ AccessibilityScrollbar* AccessibilityScrollView::addChildScrollbar(Scrollbar* sc
 
     auto& scrollBarObject = downcast<AccessibilityScrollbar>(*cache->getOrCreate(scrollbar));
     scrollBarObject.setParent(this);
-    m_children.append(&scrollBarObject);
+    addChild(&scrollBarObject);
     return &scrollBarObject;
 }
 

Source/WebCore/accessibility/AccessibilitySlider.cpp

@@ -100,7 +100,7 @@ void AccessibilitySlider::addChildren()
     if (thumb.accessibilityIsIgnored())
         cache->remove(thumb.objectID());
     else
-        m_children.append(&thumb);
+        addChild(&thumb);
 }
 
 const AtomString& AccessibilitySlider::getAttribute(const QualifiedName& attribute) const

Source/WebCore/accessibility/AccessibilitySpinButton.cpp

@@ -91,12 +91,12 @@ void AccessibilitySpinButton::addChildren()
     auto& incrementor = downcast<AccessibilitySpinButtonPart>(*cache->create(AccessibilityRole::SpinButtonPart));
     incrementor.setIsIncrementor(true);
     incrementor.setParent(this);
-    m_children.append(&incrementor);
+    addChild(&incrementor);
 
     auto& decrementor = downcast<AccessibilitySpinButtonPart>(*cache->create(AccessibilityRole::SpinButtonPart));
     decrementor.setIsIncrementor(false);
     decrementor.setParent(this);
-    m_children.append(&decrementor);
+    addChild(&decrementor);
 }
 
 void AccessibilitySpinButton::step(int amount)

Source/WebCore/accessibility/AccessibilityTable.cpp

@@ -394,8 +394,11 @@ void AccessibilityTable::addChildren()
     if (HTMLTableElement* tableElement = this->tableElement()) {
         if (auto caption = tableElement->caption()) {
             AccessibilityObject* axCaption = axObjectCache()->getOrCreate(caption.get());
+            // While `addChild` won't insert ignored children, we still need this accessibilityIsIgnored
+            // check so that `addChild` doesn't try to add the caption's children in its stead. Basically,
+            // explicitly checking accessibilityIsIgnored() ignores the caption and any of its children.
             if (axCaption && !axCaption->accessibilityIsIgnored())
-                m_children.append(axCaption);
+                addChild(axCaption);
         }
     }
 
@@ -420,13 +423,9 @@ void AccessibilityTable::addChildren()
         column.setColumnIndex(i);
         column.setParent(this);
         m_columns.append(&column);
-        if (!column.accessibilityIsIgnored())
-            m_children.append(&column);
+        addChild(&column);
     }
-
-    auto* headerContainerObject = headerContainer();
-    if (headerContainerObject && !headerContainerObject->accessibilityIsIgnored())
-        m_children.append(headerContainerObject);
+    addChild(headerContainer());
 
     // Sometimes the cell gets the wrong role initially because it is created before the parent
     // determines whether it is an accessibility table. Iterate all the cells and allow them to
@@ -451,8 +450,7 @@ void AccessibilityTable::addTableCellChild(AccessibilityObject* rowObject, HashS
 
     row.setRowIndex(static_cast<int>(m_rows.size()));
     m_rows.append(&row);
-    if (!row.accessibilityIsIgnored())
-        m_children.append(&row);
+    addChild(&row);
     appendedRows.add(&row);
 
     // store the maximum number of columns

Source/WebCore/accessibility/AccessibilityTableColumn.cpp

@@ -202,7 +202,7 @@ void AccessibilityTableColumn::addChildren()
         if (m_children.size() > 0 && m_children.last() == cell)
             continue;
 
-        m_children.append(cell);
+        addChild(cell);
     }
 }
 

Source/WebCore/accessibility/AccessibilityTableHeaderContainer.cpp

@@ -73,7 +73,8 @@ void AccessibilityTableHeaderContainer::addChildren()
     if (!parentTable.isExposable())
         return;
 
-    m_children = parentTable.columnHeaders();
+    for (auto& columnHeader : parentTable.columnHeaders())
+        addChild(columnHeader.get());
 
     for (const auto& child : m_children)
         m_headerRect.unite(child->elementRect());

Source/WebCore/accessibility/AccessibilityTableRow.cpp

@@ -109,7 +109,7 @@ AccessibilityTable* AccessibilityTableRow::parentTable() const
 
     return nullptr;
 }
-    
+
 AXCoreObject* AccessibilityTableRow::headerObject()
 {
     if (!m_renderer || !m_renderer->isTableRow())
@@ -150,10 +150,12 @@ AXCoreObject* AccessibilityTableRow::headerObject()
 void AccessibilityTableRow::addChildren()
 {
     // If the element specifies its cells through aria-owns, return that first.
-    AccessibilityChildrenVector ariaOwns;
-    ariaOwnsElements(ariaOwns);
-    if (ariaOwns.size())
-        m_children = WTFMove(ariaOwns);
+    AccessibilityChildrenVector ariaOwnedElements;
+    ariaOwnsElements(ariaOwnedElements);
+    if (ariaOwnedElements.size()) {
+        for (auto& ariaOwnedElement : ariaOwnedElements)
+            addChild(ariaOwnedElement.get());
+    }
     else
         AccessibilityRenderObject::addChildren();