Bug 231914 – AX: Any addition of children should funnel through AccessibilityObject::addChild

• Status: RESOLVED
• Resolution: FIXED
• Component: Accessibility

[Tyler Wilcock]

Centralizing this logic in one place unlocks the ability to reliably set child state based on parent state (e.g. "Do I have any ancestors with a subrole of document?").

[Radar WebKit Bug Importer]

<rdar://problem/84391370>

[Tyler Wilcock]

Created attachment 441670 [details]
Patch

[Tyler Wilcock]

Created attachment 441675 [details]
Patch

[Andres Gonzalez]

(In reply to Tyler Wilcock from comment #3)
> Created attachment 441675 [details]
> Patch

--- a/Source/WebCore/accessibility/AccessibilityObject.cpp
+++ a/Source/WebCore/accessibility/AccessibilityObject.cpp

-        ASSERT(child->parentObject() == this);
+#ifndef NDEBUG
+        // Autofill buttons are dynamically created elements by WebKit, and aren't assigned as a child
+        // to their proper DOM parent (see TextFieldInputType::createAutoFillButton), so exclude them from this ASSERT.
+        // Table component child-parent relationships also often doesn't line up properly, hence the need for methods
+        // like parentTable() and parentRow(). Exclude them from this ASSERT too.
+        if (!isAutofillButton(*this) && !isTableComponent(*child) && !isTableComponent(*this))
+            ASSERT(child->parentObject() == this);
+#endif

I'm not convinced we should special case this sanity check. The child-parent relationship of the corresponding DOM elements may not match the AX hierarchy, but the AX hierarchy should be consistent with itself in all cases. I.e., if we expose child as a child of parent, then child->parentObject() should be parent.

[Andres Gonzalez]

(In reply to Andres Gonzalez from comment #4)
> (In reply to Tyler Wilcock from comment #3)
> > Created attachment 441675 [details]
> > Patch
> 
> --- a/Source/WebCore/accessibility/AccessibilityObject.cpp
> +++ a/Source/WebCore/accessibility/AccessibilityObject.cpp
> 
> -        ASSERT(child->parentObject() == this);
> +#ifndef NDEBUG
> +        // Autofill buttons are dynamically created elements by WebKit, and
> aren't assigned as a child
> +        // to their proper DOM parent (see
> TextFieldInputType::createAutoFillButton), so exclude them from this ASSERT.
> +        // Table component child-parent relationships also often doesn't
> line up properly, hence the need for methods
> +        // like parentTable() and parentRow(). Exclude them from this
> ASSERT too.
> +        if (!isAutofillButton(*this) && !isTableComponent(*child) &&
> !isTableComponent(*this))
> +            ASSERT(child->parentObject() == this);
> +#endif
> 
> I'm not convinced we should special case this sanity check. The child-parent
> relationship of the corresponding DOM elements may not match the AX
> hierarchy, but the AX hierarchy should be consistent with itself in all
> cases. I.e., if we expose child as a child of parent, then
> child->parentObject() should be parent.

Perhaps part of the problem is that parentObject() is returning the parent based on the DOM hierarchy and not on the AX hierarchy?

[Andres Gonzalez]

(In reply to Tyler Wilcock from comment #3)
> Created attachment 441675 [details]
> Patch

--- a/Source/WebCore/accessibility/AccessibilityRenderObject.cpp
+++ a/Source/WebCore/accessibility/AccessibilityRenderObject.cpp

-    if (HTMLElement* autoFillElement = input.autoFillButtonElement()) {
-        if (AccessibilityObject* axAutoFill = axObjectCache()->getOrCreate(autoFillElement))
-            m_children.append(axAutoFill);
-    }
+    if (HTMLElement* autoFillElement = input.autoFillButtonElement())

if (auto* autoFillElement = input.autoFillButtonElement())

+        addChild(axObjectCache()->getOrCreate(autoFillElement));

We were checking for nullity of axObjectCache() before. I wouldn't remove that check yet, until we solidify the life times of the objects respect to the cache.

[Andres Gonzalez]

(In reply to Tyler Wilcock from comment #3)
> Created attachment 441675 [details]
> Patch

@@ -3381,12 +3379,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);

addChild would need to do the same type of deep append, i.e., if the root is ignored, try to add its children.

[Andres Gonzalez]

(In reply to Tyler Wilcock from comment #3)
> Created attachment 441675 [details]
> Patch

--- a/Source/WebCore/accessibility/AccessibilityTable.cpp
+++ a/Source/WebCore/accessibility/AccessibilityTable.cpp

+            // 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);

Considering this and the SVG case, perhaps it is cleaner to add a bool recursive param to addChild.

[Tyler Wilcock]

(In reply to Andres Gonzalez from comment #5)
> (In reply to Andres Gonzalez from comment #4)
> > (In reply to Tyler Wilcock from comment #3)
> > > Created attachment 441675 [details]
> > > Patch
> > 
> > --- a/Source/WebCore/accessibility/AccessibilityObject.cpp
> > +++ a/Source/WebCore/accessibility/AccessibilityObject.cpp
> > 
> > -        ASSERT(child->parentObject() == this);
> > +#ifndef NDEBUG
> > +        // Autofill buttons are dynamically created elements by WebKit, and
> > aren't assigned as a child
> > +        // to their proper DOM parent (see
> > TextFieldInputType::createAutoFillButton), so exclude them from this ASSERT.
> > +        // Table component child-parent relationships also often doesn't
> > line up properly, hence the need for methods
> > +        // like parentTable() and parentRow(). Exclude them from this
> > ASSERT too.
> > +        if (!isAutofillButton(*this) && !isTableComponent(*child) &&
> > !isTableComponent(*this))
> > +            ASSERT(child->parentObject() == this);
> > +#endif
> > 
> > I'm not convinced we should special case this sanity check. The child-parent
> > relationship of the corresponding DOM elements may not match the AX
> > hierarchy, but the AX hierarchy should be consistent with itself in all
> > cases. I.e., if we expose child as a child of parent, then
> > child->parentObject() should be parent.
> 
> Perhaps part of the problem is that parentObject() is returning the parent
> based on the DOM hierarchy and not on the AX hierarchy?

Yes, that's exactly the problem. This patch doesn't change any of these "faulty" relationships with autofill buttons and tables, just documents them more clearly in this assert.

[Tyler Wilcock]

(In reply to Andres Gonzalez from comment #7)
> (In reply to Tyler Wilcock from comment #3)
> > Created attachment 441675 [details]
> > Patch
> 
> @@ -3381,12 +3379,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);
> 
> addChild would need to do the same type of deep append, i.e., if the root is
> ignored, try to add its children.

`AccessibilityObject::addChild` calls `AccessibilityObject::insertChild`, which does exactly this :)

[Tyler Wilcock]

(In reply to Andres Gonzalez from comment #6)
> (In reply to Tyler Wilcock from comment #3)
> > Created attachment 441675 [details]
> > Patch
> 
> --- a/Source/WebCore/accessibility/AccessibilityRenderObject.cpp
> +++ a/Source/WebCore/accessibility/AccessibilityRenderObject.cpp
> 
> -    if (HTMLElement* autoFillElement = input.autoFillButtonElement()) {
> -        if (AccessibilityObject* axAutoFill =
> axObjectCache()->getOrCreate(autoFillElement))
> -            m_children.append(axAutoFill);
> -    }
> +    if (HTMLElement* autoFillElement = input.autoFillButtonElement())
> 
> if (auto* autoFillElement = input.autoFillButtonElement())
> 
> +        addChild(axObjectCache()->getOrCreate(autoFillElement));
> 
> We were checking for nullity of axObjectCache() before. I wouldn't remove
> that check yet, until we solidify the life times of the objects respect to
> the cache.

I might be mis-reading it, but I don't think the code before checked the nullity of the cache before dereferencing it.

Old:

if (HTMLElement* autoFillElement = input.autoFillButtonElement()) {
    if (AccessibilityObject* axAutoFill = axObjectCache()->getOrCreate(autoFillElement))
        m_children.append(axAutoFill);
}

New:

if (HTMLElement* autoFillElement = input.autoFillButtonElement())
    addChild(axObjectCache()->getOrCreate(autoFillElement));

If the new object the cache returns is null, `addChild` does nothing.

I can add a null check of the cache, though. Can also use an auto* here.

[chris fleizach]

Comment on attachment 441675 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=441675&action=review

> Source/WebCore/accessibility/AccessibilityObject.cpp:-518
> -        ASSERT(child->parentObject() == this);

do we still want this parent object assert too?

> Source/WebCore/accessibility/AccessibilityTableRow.cpp:155
> +    if (ariaOwnedElements.size()) {

this size check seems unnecessary now

[Tyler Wilcock]

(In reply to chris fleizach from comment #12)
> Comment on attachment 441675 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=441675&action=review
> 
> > Source/WebCore/accessibility/AccessibilityObject.cpp:-518
> > -        ASSERT(child->parentObject() == this);
> 
> do we still want this parent object assert too?
Yeah, spoke to Andres and we think this ASSERT might not be that useful in its current form, since it is often verifying DOM parent-child relationship rather than AX tree parent-child relationships. There are several cases where DOM parent-child relationships are validly not how the AX tree is structured (i.e. aria-owns, tables, autofill buttons). I'm just going to remove it.

> > Source/WebCore/accessibility/AccessibilityTableRow.cpp:155
> > +    if (ariaOwnedElements.size()) {
> 
> this size check seems unnecessary now
That would be a change in behavior from the current:

Either add "aria-owns" elements as m_children, _or_ AccessibilityRenderObject::addChildren() (aka DOM children) as m_children.

Your suggestion would be unconditionally adding both aria-owns children and DOM children to m_children.

Based on the definition of aria-owns (https://www.w3.org/TR/2017/REC-wai-aria-1.1-20171214/#aria-owns), your suggestion seems right.

> If an element has both aria-owns and DOM children then the order of the child elements with respect to the parent/child relationship is the DOM children first, then the elements referenced in aria-owns.
But I think I'd rather make that behavior change in a separate patch, since other classes do the same thing.

[chris fleizach]

Comment on attachment 441675 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=441675&action=review

>>> Source/WebCore/accessibility/AccessibilityObject.cpp:-518
>>> -        ASSERT(child->parentObject() == this);
>> 
>> do we still want this parent object assert too?
> 
> Yeah, spoke to Andres and we think this ASSERT might not be that useful in its current form, since it is often verifying DOM parent-child relationship rather than AX tree parent-child relationships. There are several cases where DOM parent-child relationships are validly not how the AX tree is structured (i.e. aria-owns, tables, autofill buttons). I'm just going to remove it.

I’ve seen this assert to be useful in the past when we mess up the parent child hierarchy for mock elements or other kinds of custom sub classes
Is it asserting now in your tests?

>>> Source/WebCore/accessibility/AccessibilityTableRow.cpp:155
>>> +    if (ariaOwnedElements.size()) {
>> 
>> this size check seems unnecessary now
> 
> That would be a change in behavior from the current:
> 
> Either add "aria-owns" elements as m_children, _or_ AccessibilityRenderObject::addChildren() (aka DOM children) as m_children.
> 
> Your suggestion would be unconditionally adding both aria-owns children and DOM children to m_children.
> 
> Based on the definition of aria-owns (https://www.w3.org/TR/2017/REC-wai-aria-1.1-20171214/#aria-owns), your suggestion seems right.

Agreed based on that reading. We need to make that change. And we’ll need a test for it. Could be separate patch

[Tyler Wilcock]

Created attachment 441752 [details]
Patch

[Tyler Wilcock]

> I’ve seen this assert to be useful in the past when we mess up the parent
> child hierarchy for mock elements or other kinds of custom sub classes
> Is it asserting now in your tests?

It is, yeah — specifically for anything table related (parent or child), and for autofill buttons. If it's been useful in the context of mock objects, maybe we can keep it, but would need the conditionals around the ASSERT I added in https://bugs.webkit.org/attachment.cgi?id=441675&action=diff.

It seems we've experienced issues in the past with table parent-child relationships — see AccessibiltyTableRow::parentTable():

AccessibilityTable* AccessibilityTableRow::parentTable() const
{
    // The parent table might not be the direct ancestor of the row unfortunately. ARIA states that role="grid" should
    // only have "row" elements, but if not, we still should handle it gracefully by finding the right table.
    for (AccessibilityObject* parent = parentObject(); parent; parent = parent->parentObject()) {
    ...truncated...
}

[chris fleizach]

(In reply to Tyler Wilcock from comment #16)
> > I’ve seen this assert to be useful in the past when we mess up the parent
> > child hierarchy for mock elements or other kinds of custom sub classes
> > Is it asserting now in your tests?
> 
> It is, yeah — specifically for anything table related (parent or child), and
> for autofill buttons. If it's been useful in the context of mock objects,
> maybe we can keep it, but would need the conditionals around the ASSERT I
> added in https://bugs.webkit.org/attachment.cgi?id=441675&action=diff.
> 

It worries me in those cases. it sounds like auto fill buttons parent/child hierarchy might be broken actually

> It seems we've experienced issues in the past with table parent-child
> relationships — see AccessibiltyTableRow::parentTable():
> 
> AccessibilityTable* AccessibilityTableRow::parentTable() const
> {
>     // The parent table might not be the direct ancestor of the row
> unfortunately. ARIA states that role="grid" should
>     // only have "row" elements, but if not, we still should handle it
> gracefully by finding the right table.
>     for (AccessibilityObject* parent = parentObject(); parent; parent =
> parent->parentObject()) {
>     ...truncated...
> }

[Tyler Wilcock]

I tested it out with a login form at discover.com and the autofill button is focusable and clickable via VoiceOver. Though navigation to the autofill button is not easy — required a lot of VO-right, VO-left, VO-Shift-Down into the text field, and VO-Shift-Right to get to. Activating the button isn't overly useful, either — it just re-opened the autofill menu that was already open.

[Tyler Wilcock]

Created attachment 441933 [details]
Patch

[Tyler Wilcock]

Created attachment 441940 [details]
Patch

[EWS]

Committed r284606 (243335@main): <https://commits.webkit.org/243335@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 441940 [details].

Reverted r284606 for reason:

Reverting because this commit caused accessibility/ios-simulator/accessibility-aria-table-children.html to fail

Committed r284687 (243407@main): <https://commits.webkit.org/243407@main>

[Tyler Wilcock]

Created attachment 442292 [details]
Patch

[Tyler Wilcock]

The previous patch was reverted because it caused a failure in three iOS tests.

The first two (accessibility/ios-simulator/disabled-states.html and accessibility/ios-simulator/popup-button-value-label.html) triggered ASSERT(child->parentObject() == this) in insertChild. This was because AccessibilityMenuListOption didn’t have a parentObject() method (therefore falling back to the default AccessibilityObject::parentObject implementation, which returns nullptr). I fixed this.

The other failing test was accessibility/ios-simulator/accessibility-aria-table-children.html. This test exposed a bug which indicates that some of our classes rely on addChild not descending into the child’s children when said child is ignored. As suggested by Andres above, I added a parameter to addChild and insertChild to control this behavior on a case-by-case basis.

Link showing failed test results: https://build.webkit.org/results/Apple-iOS-15-Simulator-Debug-WK2-Tests/r284606%20(253)/results.html

[EWS]

Committed r284760 (243469@main): <https://commits.webkit.org/243469@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 442292 [details].