Bug 151498 – AX: aria-owns attribute does not work as expected for role radio

• Component: Accessibility
• Status: RESOLVED FIXED

[Jonathan Dallas]

Created attachment 265962 [details]
Demo of the test case in the problem description

When using the aria-owns attribute radio controls are not grouped as expected.

Here is a test case:
    <div role="radiogroup" aria-owns="r1 r2 r3">
       <div role="radio" id="r1">Alpha</div>
       <div role="radio" id="r2">Beta</div>
    </div>
    <!-- this one, too. -->
    <div role="radio" id="r3">Gamma</div>

In this case the 3rd radio button "Gamma" should be included in the radio group. However, when VoiceOver looks at this it only sees two radio buttons in the group, the 3rd is announced as a radio button without a group.

Spec for aria-owns: http://www.w3.org/TR/wai-aria-1.1/#aria-owns

[Radar WebKit Bug Importer]

<rdar://problem/23630121>

[Jonathan Dallas]

*** Bug 151429 has been marked as a duplicate of this bug. ***

[Jonathan Dallas]

Created attachment 265964 [details]
Screenshot of demo test case. (second item selected)

[Jonathan Dallas]

Created attachment 265965 [details]
Screenshot of demo test case. (third item selected)

[James Craig]

possibly forward dupe to bug 241694

[chris fleizach]

Created attachment 464300 [details]
Patch

[chris fleizach]

Created attachment 464316 [details]
Patch

[Andres Gonzalez]

(In reply to chris fleizach from comment #7)
> Created attachment 464316 [details]
> Patch

--- a/Source/WebCore/accessibility/AXObjectCache.cpp
+++ b/Source/WebCore/accessibility/AXObjectCache.cpp

+    if (relationType == AXRelationType::OwnerFor)
+        origin->setNeedsToUpdateChildren();
+    if (relationType == AXRelationType::OwnedBy)

else if

+        origin->parentObjectUnignored()->setNeedsToUpdateChildren();

origin->parentObjectUnignored() can be null.

--- a/Source/WebCore/accessibility/AccessibilityNodeObject.cpp
+++ b/Source/WebCore/accessibility/AccessibilityNodeObject.cpp

+void AccessibilityNodeObject::updatedOwnedChildren()
+{
+    // If we added any children who are owned by others, remove them from our list.
+    for (unsigned k = 0; k < m_children.size(); k++) {
+        auto child = m_children[k];
+        auto childOwners = child->owners();
+        if (childOwners.size() && !childOwners.contains(this)) {
+            m_children.remove(k);
+            k--;

You can do one liner:

+            m_children.remove(k--);

There is probably a more elegant way of doing this with reverse iterators or something like that.

What is this trying to do? To ensure that a child is owned only by one parent? If so, maybe we should do that check earlier as we add relations?

[Andres Gonzalez]

(In reply to chris fleizach from comment #7)
> Created attachment 464316 [details]
> Patch

--- a/Source/WebCore/accessibility/AccessibilityNodeObject.h
+++ b/Source/WebCore/accessibility/AccessibilityNodeObject.h


+    void updatedOwnedChildren();

Should this be final?

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

-        ASSERT(isTableComponent(*child) || isTableComponent(*this) || child->parentObject() == this);
+        ASSERT(isTableComponent(*child) || isTableComponent(*this) || child->parentObject() == this || child->owners().contains(this));

At this point, shouldn't parentObject already return the owner if the child is indeed owned?

--- a/Source/WebCore/accessibility/AccessibilityObjectInterface.h
+++ b/Source/WebCore/accessibility/AccessibilityObjectInterface.h
@@ -1518,14 +1518,23 @@ namespace Accessibility {
 template<typename T, typename F>
 T* findAncestor(const T& object, bool includeSelf, const F& matches)

parentObject() should return the owner if the object is owned, otherwise we are going to need to do this everywhere we get a parent. After the aria-owns is processed and the hierarchy modified accordingly, we should work with that hierarchy and should not have to check for owners again.

[chris fleizach]

(In reply to Andres Gonzalez from comment #9)
> (In reply to chris fleizach from comment #7)
> > Created attachment 464316 [details]
> > Patch
> 
> --- a/Source/WebCore/accessibility/AccessibilityNodeObject.h
> +++ b/Source/WebCore/accessibility/AccessibilityNodeObject.h
> 
> 
> +    void updatedOwnedChildren();
> 
> Should this be final?

These ones aren't virtual, so can't do it:
    error: only virtual member functions can be marked 'final'

[chris fleizach]

Created attachment 464778 [details]
Patch

[chris fleizach]

Created attachment 465412 [details]
Patch

[chris fleizach]

Created attachment 465414 [details]
Patch

[chris fleizach]

Created attachment 465419 [details]
Patch

[chris fleizach]

Created attachment 465515 [details]
Patch

[chris fleizach]

Created attachment 465521 [details]
Patch

[Andres Gonzalez]

(In reply to chris fleizach from comment #15)
> Created attachment 465515 [details]
> Patch

--- a/Source/WebCore/accessibility/AXObjectCache.cpp
+++ b/Source/WebCore/accessibility/AXObjectCache.cpp

+AccessibilityObject* AXObjectCache::get(AXID objectID)
+{
+    return objectID ? m_objects.get(objectID) : nullptr;
+}

We already have:
    AccessibilityObject* objectForID(const AXID id) const { return m_objects.get(id); }

Which matches the equivalent in AXIsolatedTree.

I'm fine with renaming to just get(...), but we should then rename the AXIsolatedTree method. We also have objectsForIDs(const Vector<AXID>& axIDs) const.

[chris fleizach]

> 
> Which matches the equivalent in AXIsolatedTree.
> 
> I'm fine with renaming to just get(...), but we should then rename the
> AXIsolatedTree method. We also have objectsForIDs(const Vector<AXID>& axIDs)
> const.

Changing to objectForID

[chris fleizach]

Created attachment 465522 [details]
Patch

[Andres Gonzalez]

(In reply to chris fleizach from comment #19)
> Created attachment 465522 [details]
> Patch

--- a/Source/WebCore/accessibility/AXObjectCache.cpp
+++ b/Source/WebCore/accessibility/AXObjectCache.cpp

-    if (AccessibilityObject* scrollViewObject = get(view)) {
+    if (AccessibilityObject* scrollViewObject = get(&view)) {

AccessibilityObject* --> auto*

--- a/Source/WebCore/accessibility/AXObjectCache.h
+++ b/Source/WebCore/accessibility/AXObjectCache.h


+    void deferScrollbarUpdate(ScrollView*);

We are favoring the onScrollbarUpdate naming convention instead of defer***() for event notification methods called from outside accessibility.

[chris fleizach]

Created attachment 465527 [details]
Patch

[chris fleizach]

Created attachment 465529 [details]
Patch

[Tyler Wilcock]

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

Do you know if we have any existing layout tests that verify aria-owns cycles don't cause us to crash? Some ideas:

A aria-owns B.
B aria-owns A.

Results: Neither B nor A have any children, their parent is the web area (or whatever)

Another:

A aria-owns B.
B DOM-owns C.
C aria-owns A.

Results: A has child B, B has child C, C has no children

> Source/WebCore/accessibility/AXObjectCache.cpp:4046
> +static bool validateOwnerCycleAbsence(AccessibilityObject* origin, AccessibilityObject* target, AXRelationType relationType)

I wonder if there is a more fitting method for this name considering it returns a bool. "relationCausesCycle"? "relationAbsentOfCycle"?

> Source/WebCore/accessibility/AXObjectCache.cpp:4098
> +            if (auto oldOwner = objectForID(oldOwnerIterator->key))

I believe AXObjectCache::objectForID returns an AccessibilityObject*, so this should be auto* rather than auto.

> Source/WebCore/accessibility/AXObjectCache.cpp:4099
> +                oldOwner->setNeedsToUpdateChildren();

Is there a reason you're calling `setNeedsToUpdateChildren` directly rather than using AXObjectCache::childrenChanged? There are hooks in that method to do things when children change that we probably want in this situation too (e.g. re-computing table exposure status if necessary).

> Source/WebCore/accessibility/AXObjectCache.cpp:4104
> +        origin->parentObjectUnignored()->setNeedsToUpdateChildren();

Is it safe not to null-check parentObjectUnignored() here?

> Source/WebCore/accessibility/AccessibilityNodeObject.cpp:193
> +    auto owners = this->owners();

Does it make sense to ASSERT(owners.size() <= 1)? That could only be caused by a logic bug, right?

> Source/WebCore/accessibility/AccessibilityNodeObject.cpp:479
> +    for (auto child : ownedObjects()) {

auto evalutes to RefPtr here. I think we've been moving towards not using auto for smart pointers because their semantics are important to understand, so it's helpful to not hide them with auto.

> Source/WebCore/accessibility/AccessibilityNodeObject.cpp:483
> +        if (m_children.contains(child))
> +            m_children.removeFirst(child);

This does two iterations of m_children. Would it be correct to just unconditionally `m_children.removeFirst(child)`?

> Source/WebCore/accessibility/AccessibilityRenderObject.cpp:3098
> +        if (object.owners().size() && !object.owners().contains(this))

AXCoreObject::owners returns a copy, and we call it twice in a row here. Hopefully the compiler would be smart enough to optimize that away, but might be better to put `object.owners()` into a temporary local just in case.

> LayoutTests/accessibility/aria-owns-hierarchy-remap.html:4
> +<script src="../resources/js-test-pre.js"></script>

This should be replaced with "js-test.js".

> LayoutTests/accessibility/aria-owns-hierarchy-remap.html:26
> +<p id="description"></p>
> +<div id="console"></div>

This can be removed after importing js-test.js.

> LayoutTests/accessibility/aria-owns-hierarchy-remap.html:47
> +    debug(output);

`debug(output)` should go right before finishJSTest();

> LayoutTests/accessibility/aria-owns-hierarchy-remap.html:56
> +        await expectAsyncExpression("ax.accessibleElementById('list3').childrenCount", 0);
> +        await expectAsyncExpression("ax.accessibleElementById('list1').childrenCount", 3);
> +
> +        await expectAsyncExpression("ax.accessibleElementById('list1').childAtIndex(1).isEqual(ax.accessibleElementById('item5'))", "true");

These uses of `await expectAsyncExpression` should be replaced by `output += await expectAsync(...);`

> LayoutTests/accessibility/aria-owns-hierarchy-remap.html:63
> +<script src="../resources/js-test-post.js"></script>

This can be removed if you import "js-test.js"

[Tyler Wilcock]

> A aria-owns B.
> B aria-owns A.
> 
> Results: Neither B nor A have any children, their parent is the web area (or
> whatever)
Ah, I take this back, A should have B as a child in this situation

[chris fleizach]

(In reply to Tyler Wilcock from comment #23)
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=465529&action=review
> 
> Do you know if we have any existing layout tests that verify aria-owns
> cycles don't cause us to crash? Some ideas:
> 
> A aria-owns B.
> B aria-owns A.
> 
> Results: Neither B nor A have any children, their parent is the web area (or
> whatever)
> 
> Another:
> 
> A aria-owns B.
> B DOM-owns C.
> C aria-owns A.
> 
> Results: A has child B, B has child C, C has no children
> 

Will add some more tests

> 
> > Source/WebCore/accessibility/AXObjectCache.cpp:4099
> > +                oldOwner->setNeedsToUpdateChildren();
> 
> Is there a reason you're calling `setNeedsToUpdateChildren` directly rather
> than using AXObjectCache::childrenChanged? There are hooks in that method to
> do things when children change that we probably want in this situation too
> (e.g. re-computing table exposure status if necessary).
> 

This naming seems unfortunate. From my usage, I don't know if the childrenChanged, but I do know that it should update the children. This is similar to [CALayer setNeedsDisplay] vs [CALayer display]

/* Reload the content of this layer. Calls the -drawInContext: method
 * then updates the `contents' property of the layer. Typically this is
 * not called directly. */
- (void)display;

[chris fleizach]

Created attachment 465532 [details]
Patch

[Tyler Wilcock]

Comment on attachment 465532 [details]
Patch

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

> LayoutTests/accessibility/aria-owns-hierarchy-remap.html:53
> +        output += await expectAsyncExpression("ax.accessibleElementById('list3').childrenCount", 0);
> +        output += await expectAsyncExpression("ax.accessibleElementById('list1').childrenCount", 3);
> +
> +        output += await expectAsyncExpression("ax.accessibleElementById('list1').childAtIndex(1).isEqual(ax.accessibleElementById('item5'))", "true");

Typo -- it's "expectAsync", not "expectAsyncExpression".

output += await expectAsync("ax.accessibleElementById('list3').childrenCount", 0);

As the person who introduced both of those functions — sorry for the confusion. It has been on my to-do list to remove "expectAsyncExpression" since it forces the output to be debugged, which is not good for the semantics of our tests.

[Tyler Wilcock]

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

> LayoutTests/accessibility/aria-owns-hierarchy-remap.html:53
> +        output += await expectAsyncExpression("ax.accessibleElementById('list3').childrenCount", 0);
> +        output += await expectAsyncExpression("ax.accessibleElementById('list1').childrenCount", 3);
> +
> +        output += await expectAsyncExpression("ax.accessibleElementById('list1').childAtIndex(1).isEqual(ax.accessibleElementById('item5'))", "true");

Typo -- it's "expectAsync", not "expectAsyncExpression".

output += await expectAsync("ax.accessibleElementById('list3').childrenCount", 0);

As the person who introduced both of those functions — sorry for the confusion. It has been on my to-do list to remove "expectAsyncExpression" since it forces the output to be debugged, which is not good for the semantics of our tests.

[chris fleizach]

Created attachment 465603 [details]
Patch

[chris fleizach]

Created attachment 465675 [details]
Patch

[chris fleizach]

Created attachment 465723 [details]
Patch

[chris fleizach]

Created attachment 465724 [details]
Patch

[chris fleizach]

Created attachment 465732 [details]
Patch

[chris fleizach]

Created attachment 465741 [details]
Patch

[chris fleizach]

Created attachment 465744 [details]
Patch

[chris fleizach]

Created attachment 465745 [details]
Patch

[Tyler Wilcock]

Comment on attachment 465745 [details]
Patch

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

> Source/WebCore/accessibility/AXObjectCache.cpp:4101
> +        origin->setNeedsToUpdateChildren();

Here's another use of `setNeedsToUpdateChildren` over `AXObjectCache::childrenChanged`. Is this intentional? I agree the naming and relationship between these methods is confusing and needs to be fixed in the future.

> LayoutTests/accessibility/aria-checkbox-sends-notification-expected.txt:11
> +FAIL: Timed out waiting for notifyDone to be called
> +
> +Test Checkbox
> +This tests that checking of an aria checkbox sends a notification.
> +
> +On success, you will see a series of "PASS" messages, followed by "TEST COMPLETE".
> +
> +
> +Got notification: AXValueChanged
> +Got notification: AXValueChanged
> +

Seems like this test is failing.

> LayoutTests/accessibility/element-reflection-ariaactivedescendant.html:29
> +       	output += expect("axTarget1.selectedChildAtIndex(0).isEqual(axDescendant1)", "true");

Seems like this indentation might not match the sibling statements.

> LayoutTests/accessibility/element-reflection-ariacontrols.html:72
> +setTimeout(function() {

This is nested within the other setTimeouts, right? If so, seems like the indentation is wrong — should be three levels indented.

> LayoutTests/accessibility/element-reflection-ariacontrols.html:75
> +         axPanel3 = accessibilityController.accessibleElementById("panel3");

Extra space preceding "axPanel3".

> LayoutTests/accessibility/element-reflection-ariacontrols.html:95
> +       }, 0);

This indentation doesn't seem right. Each "}, 0);" and the contents within should be indented one level deeper than the one encapsulating it.

> LayoutTests/accessibility/element-reflection-ariaowns.html:69
> +        if (accessibilityController.platformName == "mac") {

The braces for this if-else aren't necessary.

[chris fleizach]

(In reply to Tyler Wilcock from comment #37)
> Comment on attachment 465745 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=465745&action=review
> 
> > Source/WebCore/accessibility/AXObjectCache.cpp:4101
> > +        origin->setNeedsToUpdateChildren();
> 
> Here's another use of `setNeedsToUpdateChildren` over
> `AXObjectCache::childrenChanged`. Is this intentional? I agree the naming
> and relationship between these methods is confusing and needs to be fixed in
> the future.

Should be able to change. Just missed this from last review
> 
> > LayoutTests/accessibility/aria-checkbox-sends-notification-expected.txt:11
> > +FAIL: Timed out waiting for notifyDone to be called
> > +
> > +Test Checkbox
> > +This tests that checking of an aria checkbox sends a notification.
> > +
> > +On success, you will see a series of "PASS" messages, followed by "TEST COMPLETE".
> > +
> > +
> > +Got notification: AXValueChanged
> > +Got notification: AXValueChanged
> > +
> 
> Seems like this test is failing.
> 
> > LayoutTests/accessibility/element-reflection-ariaactivedescendant.html:29
> > +       	output += expect("axTarget1.selectedChildAtIndex(0).isEqual(axDescendant1)", "true");
> 
> Seems like this indentation might not match the sibling statements.
> 
> > LayoutTests/accessibility/element-reflection-ariacontrols.html:72
> > +setTimeout(function() {
> 
> This is nested within the other setTimeouts, right? If so, seems like the
> indentation is wrong — should be three levels indented.
> 
> > LayoutTests/accessibility/element-reflection-ariacontrols.html:75
> > +         axPanel3 = accessibilityController.accessibleElementById("panel3");
> 
> Extra space preceding "axPanel3".
> 
> > LayoutTests/accessibility/element-reflection-ariacontrols.html:95
> > +       }, 0);
> 
> This indentation doesn't seem right. Each "}, 0);" and the contents within
> should be indented one level deeper than the one encapsulating it.
> 
> > LayoutTests/accessibility/element-reflection-ariaowns.html:69
> > +        if (accessibilityController.platformName == "mac") {
> 
> The braces for this if-else aren't necessary.

[chris fleizach]

Created attachment 465755 [details]
Patch

[EWS]

Committed 262566@main (ca304bdacc08): <https://commits.webkit.org/262566@main>

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

[Ross Kirsling]

Landed build fix: https://github.com/WebKit/WebKit/pull/12368