Bug 238831 – AX: Incorrect role on dynamic lists

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

[Darin Senneff]

Created attachment 456741 [details]
Screen recording of static and dynamic lists and their roles

When using a traditional (ul/ol element) or ARIA (role="list") with children present on page load, Safari correctly gives these elements a role of "list" in the accessibility tree. 

However when these elements are empty on page load, Safari is giving different behavior: The ol element appears to be removed from the accessibility tree if it has no children, while the ARIA list is given a role of "group". That kind of makes sense since there's no children, thus it is not a list. But when children are added dynamically the roles do not update. 

Adding a dynamic list item to the ul does not update it and add it to the accessibility tree, and thus VoiceOver will not announce a list while treating any list items as regular text nodes.

Adding a dynamic list item to the ARIA list does not update its role either, it remains as "group" and thus VoiceOver will not announce it as a list. Its list item children are announced as regular text nodes.

But when you force a refresh of the virtual buffer (in the screen recording attached I move an element in the inspector), the roles of the dynamic lists get updated. The ul and ARIA list now have a "list" role and are correctly announced by VoiceOver as lists while their children are announced correctly as list items.

Demo CodePen page to test: https://codepen.io/dsenneff/pen/vYpdegY/ee39df80e6f4b0abeab3a49de7052290

Also attaching a screen recording showing the above in action.

[Radar WebKit Bug Importer]

<rdar://problem/91313663>

[Tyler Wilcock]

Created attachment 457125 [details]
Patch

[chris fleizach]

Comment on attachment 457125 [details]
Patch

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

> Source/WebCore/accessibility/AXObjectCache.cpp:1081
> +        object.updateAccessibilityRole();

will this still be an AccessibilityList if the role is changed?

[Tyler Wilcock]

Created attachment 457130 [details]
Patch

[Tyler Wilcock]

> > Source/WebCore/accessibility/AXObjectCache.cpp:1081
> > +        object.updateAccessibilityRole();
> 
> will this still be an AccessibilityList if the role is changed?
It will, yes. The only role it's allowed to compute is controlled by the logic in AccessibilityList::determineAccessibilityRole(), so it will either be a group or a list (both of which are valid for an AccessibilityList object, depending on the children).

[chris fleizach]

Comment on attachment 457130 [details]
Patch

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

> Source/WebCore/accessibility/AXObjectCache.cpp:1084
> +            updateIsolatedTree(object, AXAriaRoleChanged);

should we remove this role check here and put it in the isolated tree code so isolated tree checks its role to see if it's changed?
that could allow us to simplify this code a wee bit

[Tyler Wilcock]

Created attachment 457208 [details]
Patch

[Tyler Wilcock]

> > Source/WebCore/accessibility/AXObjectCache.cpp:1084
> > +            updateIsolatedTree(object, AXAriaRoleChanged);
> 
> should we remove this role check here and put it in the isolated tree code
> so isolated tree checks its role to see if it's changed?
> that could allow us to simplify this code a wee bit
I did some experimentation with this but haven't found anything I liked yet.

If I understand your suggestion correctly, we'd be updating both the live object role, and associated isolated object role in AXObjectCache::updateIsolatedTree, or AXIsolatedTree::updateChildren. However, we need to call object.updateAccessibilityRole() with and without ENABLE(ACCESSIBILITY_ISOLATED_TREE), so doing it that way would still require:

#if !ENABLE(ACCESSIBILITY_ISOLATED_TREE)
    if (is<AccessibilityList>(object))
        object.updateAccessibilityRole();
#endif

In AXObjectCache::handleChildrenChanged. I think this is confusing for a few reasons:

  1. We've now duplicated "update role after list children change" logic in non-obvious ways due to the distance these blocks will be from each other
  2. It seems odd that we'd be relying on the isolated tree updating to also update a live object. The right place to update a live object like this does seem like the AXObjectCache

But maybe I'm missing something, so if you have other ideas I'm open.

[Andres Gonzalez]

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

--- a/Source/WebCore/accessibility/AXObjectCache.cpp
+++ a/Source/WebCore/accessibility/AXObjectCache.cpp
@@ -1075,6 +1075,18 @@ void AXObjectCache::handleChildrenChanged(AccessibilityObject& object)

One way of simplifying this would be:

+    if (is<AccessibilityList>(object)) {
+        auto oldRole = object.updateRole();
+#if ENABLE(ACCESSIBILITY_ISOLATED_TREE)
+        if (oldRole != object.roleValue())
+            updateIsolatedTree(object, AXAriaRoleChanged);
+#endif
+    }

1. Rename the method to just updateRole() and make it return the old role.
2. Remove it from the AXCoreObject class if it is there.

Where were we doing this before that it worked for live objects?

[Andres Gonzalez]

a More basic question, why the role of an empty list has to be group? What's wrong with just being an empty list with role list? We have empty tables... I would get rid of this whole thing.

[Tyler Wilcock]

Created attachment 457283 [details]
Patch

[Tyler Wilcock]

> 1. Rename the method to just updateRole() and make it return the old role.
Renamed.

> 2. Remove it from the AXCoreObject class if it is there.
It's a virtual method on AccessibilityObject, so all good.

> Where were we doing this before that it worked for live objects?
Nowhere. We never updated live-object list roles after creation (which is this bug), and AccessibilityNodeObject::updateRole never resulted in a corresponding isolated tree update (fixed with the latest version of my patch).

> a More basic question, why the role of an empty list has to be group? What's wrong with just being an empty list with role list? We have empty tables... I would get rid of this whole thing.
Not entirely sure. I'm guessing the original idea was that in order for something to be truly considered a list, it must have list items -- otherwise, we consider the list a layout list (i.e. not something that should have list semantics). From AccessibilityList::determineAccessibilityRole():

    // Heuristic to determine if this list is being used for layout or for content.
    //   1. If it's a named list, like ol or aria=list, then it's a list.
    //      1a. Unless the list has no children, then it's not a list.
    //   2. If it displays visible list markers, it's a list.
    //   3. If it does not display list markers and has only one child, it's not a list.
    //   4. If it does not have any listitem children, it's not a list.
    //   5. Otherwise it's a list (for now).


I don't think we can eliminate this outright, since it is built this way to support AX for layout lists.

[EWS]

Committed r292774 (249558@main): <https://commits.webkit.org/249558@main>

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