Bug 261643 – AX: Accessibility relations become stale for objects that have a renderer attached after their creation

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

[Tyler Wilcock]

...

[Radar WebKit Bug Importer]

<rdar://problem/115604027>

[Tyler Wilcock]

Created attachment 467706 [details]
Patch

[Tyler Wilcock]

Created attachment 467723 [details]
Patch

[Andres Gonzalez]

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

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

+    for (AXID axID : m_deferredRemoveReplacedObjects) {
+        if (!markedRelationsDirty) {
+            // If the replaced object was part of any relation, we need to make sure the relations are updated.
+            if (m_relations.contains(axID))
+                markRelationsDirty();
+            else if (auto* axObject = objectForID(axID); axObject && !axObject->identifierAttribute().isEmpty())
+                markRelationsDirty();

Do we need to update the relations even if the object being removed wasn't previously related?

Wouldn't it be simpler to do this in AXObjectCache::onRendererCreated:

if (m_relations.contains(axID))
    relationsNeedUpdate(true);

+        // Contrary to the name "AXSelectedCellsChanged", this notification can be posted on a cell who has changed
+        // selected state, not just on table or grid who has changed its selected cells.
         case AXSelectedCellsChanged:

That actually seems a bug to me, we already have a selected state that applies to several types of objects, so I don't see why we would need SelectedCellsChange with a target cell, the target should be a table/grid container. Could this comment be a FIXME then?

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

-    HashSet<AXID> m_deferredRemovedObjects;
+    // An object can be "replaced" when we create an AX object from the backing element before it has
+    // attached a renderer, but then want to replace it with a new AX object after the renderer has been attached.
+    HashSet<AXID> m_deferredRemoveReplacedObjects;

Can we call this just m_replacedObjects?

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

+inline AXCoreObject* AXCoreObject::activeDescendant() const
+{
+    auto activeDescendants = relatedObjects(AXRelationType::ActiveDescendant);
+    ASSERT(activeDescendants.size() <= 1);
+    if (!activeDescendants.isEmpty())
+        return activeDescendants[0].get();
+    return nullptr;

I'd prefer the more succinct:

return activeDescendants.size() ? activeDescendants[0].get() : nullptr;

+inline AXCoreObject::AccessibilityChildrenVector AXCoreObject::selectedCells()
+{
...

We need to add an AXCoreObjectImpl.cpp to keep adding these to the header file.

[Tyler Wilcock]

Created attachment 467740 [details]
Patch

[Tyler Wilcock]

> +    for (AXID axID : m_deferredRemoveReplacedObjects) {
> +        if (!markedRelationsDirty) {
> +            // If the replaced object was part of any relation, we need to
> make sure the relations are updated.
> +            if (m_relations.contains(axID))
> +                markRelationsDirty();
> +            else if (auto* axObject = objectForID(axID); axObject &&
> !axObject->identifierAttribute().isEmpty())
> +                markRelationsDirty();
> 
> Do we need to update the relations even if the object being removed wasn't
> previously related?
Fixed!

> Wouldn't it be simpler to do this in AXObjectCache::onRendererCreated:
> 
> if (m_relations.contains(axID))
>     relationsNeedUpdate(true);
We discussed this morning, but the reason I don't dirty relations directly in onRendererCreated is in case it's called multiple times in a row. Doing it in performDeferredCacheUpdate allows us to dirty relations as few times as possible.

> +        // Contrary to the name "AXSelectedCellsChanged", this notification
> can be posted on a cell who has changed
> +        // selected state, not just on table or grid who has changed its
> selected cells.
>          case AXSelectedCellsChanged:
> 
> That actually seems a bug to me, we already have a selected state that
> applies to several types of objects, so I don't see why we would need
> SelectedCellsChange with a target cell, the target should be a table/grid
> container. Could this comment be a FIXME then?
I agree that this behavior is not ideal. Changed to a FIXME.

> +    HashSet<AXID> m_deferredRemoveReplacedObjects;
> 
> Can we call this just m_replacedObjects?
Fixed.
 
> I'd prefer the more succinct:
> 
> return activeDescendants.size() ? activeDescendants[0].get() : nullptr;
Fixed.

> +inline AXCoreObject::AccessibilityChildrenVector
> AXCoreObject::selectedCells()
> +{
> ...
> 
> We need to add an AXCoreObjectImpl.cpp to keep adding these to the header
> file.
Definitely. Sounds like you may have already started on this, so let's move everything in a follow-up patch.

[EWS]

Committed 268122@main (7a0745034449): <https://commits.webkit.org/268122@main>

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