Bug 238831

<rdar://problem/91313663>

Created attachment 457125 [details]
Patch

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?

Created attachment 457130 [details]
Patch

> > 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).

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

Created attachment 457208 [details]
Patch

> > 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.

(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?

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.

Created attachment 457283 [details]
Patch

> 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.

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].