Bug 233558 – AX Isolated Tree: When focus changes, update AXPropertyName::IsSelected for any aria-controls controllers of tab panel ancestor objects

• Component: Accessibility
• Status: NEW

[Tyler Wilcock]

AccessibilityRenderObject::isTabItemSelected is dependent on object focus state, so isolated tree needs to re-compute AXPropertyName::IsSelected whenever focus changes.

This will fix test accessibility/aria-controls-with-tabs.html.

[Radar WebKit Bug Importer]

<rdar://problem/85815537>

[Tyler Wilcock]

Created attachment 445279 [details]
Patch

[chris fleizach]

Comment on attachment 445279 [details]
Patch

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

> Source/WebCore/accessibility/AXObjectCache.cpp:416
>      auto* focus = getOrCreate(focusedNode);

can we rename this variable to focusedObject or something along those lines. focus by itself sounds like a verb

> Source/WebCore/accessibility/isolatedtree/AXIsolatedTree.cpp:259
> +    updateNodePropertyLocked(axObject, property);

why do we want to hold the lock during the entire change set in updateNodeProperty? we only need to protected pending property changes right?

> Source/WebCore/accessibility/isolatedtree/AXIsolatedTree.cpp:422
> +        if (controlledTabPanelAncestor) {

controlledTabPanelAncestor should be non null coming out of ariaControlledTabPanelAncestors() right?

> Source/WebCore/accessibility/isolatedtree/AXIsolatedTree.cpp:424
> +            controlledTabPanelAncestor->ariaControlsReferencingElements(controllers);

can we make this method return the vector of children?

[Andres Gonzalez]

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

Too many different changes in this patch that makes it difficult to review and understand. I would split it up in at least three or four separate patches:
1. getting the ancestors that match a given condition.
2. the change that is specific to tab controls.
3. The isolated tree mode change concerning focus changes and having to update some additional properties.
4. The changes to the Tools AX objects.

[Tyler Wilcock]

Created attachment 445580 [details]
Patch

[Tyler Wilcock]

In this new patch, I removed the AXAncestorFlag::HasAriaControlledTabPanelAncestor that the previous patch had. It was only an optimization, i.e. not necessary for correctness, and after reviewing it further I'm not sure it was even that great of an optimization.

Also, the test refactoring was all moved to https://bugs.webkit.org/show_bug.cgi?id=233603, so this patch should be more atomically reviewable now.

> > Source/WebCore/accessibility/isolatedtree/AXIsolatedTree.cpp:259
> > +    updateNodePropertyLocked(axObject, property);
> 
> why do we want to hold the lock during the entire change set in
> updateNodeProperty? we only need to protected pending property changes right?
I did it this way because our locks aren't reentrant, meaning this call chain would cause a deadlock:

setFocusedNode -> handleFocusChange -> updateNodeProperty(*ariaController, AXPropertyName::IsSelected)

Because setFocusedNode acquires the lock, and then updateNodeProperty tries to acquire the lock.

updateNodePropertyLocked is a variant of the function for situations like this where we want to update a node property but already hold the lock.

If either of you don't like this approach, we could just not use updateNodeProperty in this scenario and update the m_propertyMap manually wherever needed.

I addressed all your other comments, Chris.

[Tyler Wilcock]

Created attachment 445584 [details]
Patch

[Tyler Wilcock]

Created attachment 445586 [details]
Patch

[Andres Gonzalez]

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

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

+bool AccessibilityObject::hasAriaElementsReferencedByAttribute(const QualifiedName& attribute) const
+{
+    AccessibilityChildrenVector referencingElements;
+    ariaElementsReferencedByAttribute(referencingElements, attribute, ARIAElementSearchBehavior::StopAtFirst);
+    return !referencingElements.isEmpty();
+}
+

Where is this used?

[Tyler Wilcock]

(In reply to Andres Gonzalez from comment #9)
> (In reply to Tyler Wilcock from comment #8)
> > Created attachment 445586 [details]
> > Patch
> 
> --- a/Source/WebCore/accessibility/AccessibilityObject.cpp
> +++ a/Source/WebCore/accessibility/AccessibilityObject.cpp
> 
> +bool AccessibilityObject::hasAriaElementsReferencedByAttribute(const
> QualifiedName& attribute) const
> +{
> +    AccessibilityChildrenVector referencingElements;
> +    ariaElementsReferencedByAttribute(referencingElements, attribute,
> ARIAElementSearchBehavior::StopAtFirst);
> +    return !referencingElements.isEmpty();
> +}
> +
> 
> Where is this used?
It's used by AccessibilityObject::hasAriaControlsReferencingElement (another new function), which in turn is used by AccessibilityObject::controlledTabPanelAncestors.

Using hasAriaControlsReferencingElement rather than !AccessibilityObject::ariaControlsReferencingElements().isEmpty() is more efficient because the former stops iterating when it finds the first referencing element, while the latter iterates through the entire tree no matter what.

[Andres Gonzalez]

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

--- a/Source/WebCore/accessibility/AccessibilityObjectInterface.h
+++ a/Source/WebCore/accessibility/AccessibilityObjectInterface.h

-    virtual void ariaControlsReferencingElements(AccessibilityChildrenVector&) const = 0;
+    virtual AccessibilityChildrenVector ariaControlsReferencingElements() const = 0;

Can we rename ariaControlsReferencingElements -> controllerElements ?

+template<typename T, typename F>
+AXCoreObject::AccessibilityChildrenVector findAncestors(const T& object, const F& matches)

The return should be Vector<T> or Vector<RefPtr<T>>.

+    AXCoreObject::AccessibilityChildrenVector ancestors;

Vector<T> or Vector<RefPtr<T>>.

--- a/Source/WebCore/accessibility/isolatedtree/AXIsolatedTree.h
+++ a/Source/WebCore/accessibility/isolatedtree/AXIsolatedTree.h

I'm concerned about these changes to AXIsolatedTree. I would kindly request we split the AXCore changes from the isolated tree changes, and put this in a separate patch.

+class AccessibilityObject;

The isolated tree should not need to know about AccessibilityObjects.

 enum class AXPropertyName : uint16_t {
     ARIAControlsElements,
+    ARIAControlsReferencingElements,

Rename ControllerElements.

+    ControlledTabPanelAncestors,

Does this mean ancestors of a tab panel that is controlled by another element? I find this whole naming quite confusing.

-    void setFocusedNodeID(AXID);
+    void setFocusedNode(AccessibilityObject*);

I don't think this is a good change. IsolatedTree shouldn't know about AccessibilityObject.

+    void updateNodePropertyLocked(const AXCoreObject&, AXPropertyName) WTF_REQUIRES_LOCK(m_changeLogLock);

I don't think we need this since we already have a mechanism to update the properties of a node. If IsSelected needs to be updated when the focus changes, we can call the updateNodeProperty for the corresponding selected object.

+    void handleFocusChange(AXCoreObject*) WTF_REQUIRES_LOCK(m_changeLogLock);

Can we call this updateXXX, making explicit what we are doing here? We give handleXXXChanges a totally different meaning in the AXObjectCache, handling notifications.