Bug 240750 – AccessibilityTable::m_isExposable is never recomputed after AccessibilityTable::init

• Component: Accessibility
• Status: RESOLVED FIXED

[Tyler Wilcock]

This can cause it to become out of date after dynamic page changes (e.g. ARIA attribute changes).

[Radar WebKit Bug Importer]

<rdar://problem/93690148>

[Tyler Wilcock]

Created attachment 459632 [details]
Patch

[Tyler Wilcock]

Created attachment 459633 [details]
Patch

[chris fleizach]

Comment on attachment 459633 [details]
Patch

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

> Source/WebCore/accessibility/AXObjectCache.cpp:1302
> +    if (auto* axObject = get(node); is<AccessibilityTable>(axObject))

is this a style in WebKit now to have two conditions in one if block?

[Andres Gonzalez]

(In reply to chris fleizach from comment #4)
> Comment on attachment 459633 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=459633&action=review
> 
> > Source/WebCore/accessibility/AXObjectCache.cpp:1302
> > +    if (auto* axObject = get(node); is<AccessibilityTable>(axObject))
> 
> is this a style in WebKit now to have two conditions in one if block?

It is not two conditions, the first statement 

auto* axObject = get(node);

it is just an initialization. The is only one condition is<AccessibilityTable>(axObject). 

I personally don't like this because if you wrote

if (auto* axObject = get(node))

Then that would be  an initialization and a condition at the same time. But if you add a second expression like in the case above, the first one becomes just an initialization and the second one is the real condition. I've seen it used more in WebKit, but I'm not sure that is the recommended way in most cases.

[Andres Gonzalez]

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

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

+void AXObjectCache::handleRowCountChanged(Node* node)

+    postNotification(node, AXObjectCache::AXRowCountChanged);

Don't need to qualify AXNotifications with AXObjectCache:: inside the AXObjectCache implementation.

+void AXObjectCache::handleRowCountChanged(AXCoreObject* axObject, Document* document)

Why do we need two overloads for this? At least one should call the other one, but I don't see why we need two.

+    postNotification(axObject, document, AXObjectCache::AXRowCountChanged);

Don't need to qualify AXNotifications with AXObjectCache:: inside the AXObjectCache implementation.

@@ -3414,6 +3438,14 @@ void AXObjectCache::updateIsolatedTree(const Vector<std::pair<RefPtr<AXCoreObjec

+    auto updateNode = [&] (RefPtr<AXCoreObject> axObject) {
+        auto updatedFields = updatedObjects.get(axObject->objectID());
+        if (!updatedFields.node) {
+            updatedObjects.set(axObject->objectID(), UpdatedFields { updatedFields.children, true });
+            tree->updateNode(*axObject);
+        }
+    };
+
This whole thing should be done in AXIsolatedTree::updateNode, not here.

[Tyler Wilcock]

Created attachment 459687 [details]
Patch

[Tyler Wilcock]

> +void AXObjectCache::handleRowCountChanged(AXCoreObject* axObject, Document*
> document)
> 
> Why do we need two overloads for this? At least one should call the other
> one, but I don't see why we need two.
This mirrors the overloads for postNotification:

  * postNotification(Node*, AXNotification, PostTarget)
  * postNotification(AXCoreObject*, Document*, AXNotification, PostTarget)

The behavior for each is different enough that I think we need to match these overloads, unless you have a different idea.

> @@ -3414,6 +3438,14 @@ void AXObjectCache::updateIsolatedTree(const
> Vector<std::pair<RefPtr<AXCoreObjec
> 
> +    auto updateNode = [&] (RefPtr<AXCoreObject> axObject) {
> +        auto updatedFields = updatedObjects.get(axObject->objectID());
> +        if (!updatedFields.node) {
> +            updatedObjects.set(axObject->objectID(), UpdatedFields {
> updatedFields.children, true });
> +            tree->updateNode(*axObject);
> +        }
> +    };
> +
> This whole thing should be done in AXIsolatedTree::updateNode, not here.
The code here in AXObjectCache::updateIsolatedTree(const Vector<std::pair<RefPtr<AXCoreObject>, AXNotification>>& notifications) filters notifications such that we don't perform the same work over and over (e.g. multiple calls to AXIsolatedTree::updateNode for the same object). This feels different than what AXIsolatedTree::updateNode does and should do, doesn't it? But maybe I don't totally understand your suggestion.

[Tyler Wilcock]

Created attachment 459695 [details]
Patch

[Tyler Wilcock]

Created attachment 459696 [details]
Patch

[Tyler Wilcock]

Created attachment 459726 [details]
Patch

[Andres Gonzalez]

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

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

+    if (auto* tableSectionElement = dynamicDowncast<HTMLTableSectionElement>(object.element())) {
+        auto* parentTable = tableSectionElement->findParentTable().get();
+        if (auto* axTable = dynamicDowncast<AccessibilityTable>(getOrCreate(const_cast<HTMLTableElement*>(parentTable))))
+            axTable->recomputeIsExposable();

We shouldn't need to go back to the Element at this point in the cache. Can we instead schedule some event for the table when we actually get the notification from the DOM in 
childrenChanged(Node* node, Node* changedChild)
or the equivalent RenderObject overload.

void AXObjectCache::updateIsolatedTree(const Vector<std::pair<RefPtr<AXCoreObjec

+        case AXRowCountChanged:
+            if (notification.first->isTable())
+                updateNode(notification.first);
+            FALLTHROUGH;

Do we need to check if (notification.first->isTable()) here? I'd think we can just update if we receive that notification like with the others, e.g., CheckedStateChanged.

[Andres Gonzalez]

(In reply to Tyler Wilcock from comment #8)
> > +void AXObjectCache::handleRowCountChanged(AXCoreObject* axObject, Document*
> > document)
> > 
> > Why do we need two overloads for this? At least one should call the other
> > one, but I don't see why we need two.
> This mirrors the overloads for postNotification:
> 
>   * postNotification(Node*, AXNotification, PostTarget)
>   * postNotification(AXCoreObject*, Document*, AXNotification, PostTarget)
> 
> The behavior for each is different enough that I think we need to match
> these overloads, unless you have a different idea.

Can we do

+    else if (attrName == aria_rowcountAttr)
+        handleRowCountChanged(get(element), ...);

If so, we just need one. The Document parameter doesn't even need to be passed I believe, but you can get it from the cache when needed.

> 
> > @@ -3414,6 +3438,14 @@ void AXObjectCache::updateIsolatedTree(const
> > Vector<std::pair<RefPtr<AXCoreObjec
> > 
> > +    auto updateNode = [&] (RefPtr<AXCoreObject> axObject) {
> > +        auto updatedFields = updatedObjects.get(axObject->objectID());
> > +        if (!updatedFields.node) {
> > +            updatedObjects.set(axObject->objectID(), UpdatedFields {
> > updatedFields.children, true });
> > +            tree->updateNode(*axObject);
> > +        }
> > +    };
> > +
> > This whole thing should be done in AXIsolatedTree::updateNode, not here.
> The code here in AXObjectCache::updateIsolatedTree(const
> Vector<std::pair<RefPtr<AXCoreObject>, AXNotification>>& notifications)
> filters notifications such that we don't perform the same work over and over
> (e.g. multiple calls to AXIsolatedTree::updateNode for the same object).
> This feels different than what AXIsolatedTree::updateNode does and should
> do, doesn't it? But maybe I don't totally understand your suggestion.

oh right, I misread this change.

[Tyler Wilcock]

Created attachment 459744 [details]
Patch

[Tyler Wilcock]

> --- a/Source/WebCore/accessibility/AXObjectCache.cpp
> +++ a/Source/WebCore/accessibility/AXObjectCache.cpp
> 
> +    if (auto* tableSectionElement =
> dynamicDowncast<HTMLTableSectionElement>(object.element())) {
> +        auto* parentTable = tableSectionElement->findParentTable().get();
> +        if (auto* axTable =
> dynamicDowncast<AccessibilityTable>(getOrCreate(const_cast<HTMLTableElement*>
> (parentTable))))
> +            axTable->recomputeIsExposable();
> 
> We shouldn't need to go back to the Element at this point in the cache. Can
> we instead schedule some event for the table when we actually get the notification from the DOM in 
> childrenChanged(Node* node, Node* changedChild)
> or the equivalent RenderObject overload.
OK, fixed this by adding m_deferredRecomputeTableIsExposedList.

> void AXObjectCache::updateIsolatedTree(const
> Vector<std::pair<RefPtr<AXCoreObjec
> 
> +        case AXRowCountChanged:
> +            if (notification.first->isTable())
> +                updateNode(notification.first);
> +            FALLTHROUGH;
> 
> Do we need to check if (notification.first->isTable()) here? I'd think we
> can just update if we receive that notification like with the others, e.g.,
> CheckedStateChanged.
I did this because the only time AXRowCountChanged actually needs to update properties is for tables. This guards against authoring errors where they change aria-rowcount on non-tables, having no practical effect but causing us to `updateNode`, which is expensive. But I might be overthinking it, so I'd be fine removing the check if you'd prefer.

> > The behavior for each is different enough that I think we need to match
> > these overloads, unless you have a different idea.
> 
> Can we do
> 
> +    else if (attrName == aria_rowcountAttr)
> +        handleRowCountChanged(get(element), ...);
> 
> If so, we just need one. The Document parameter doesn't even need to be
> passed I believe, but you can get it from the cache when needed.
I considered this too. But the behavior of postNotification(Node*, AXNotification) is more complicated than that, doing traversal if necessary:

RefPtr<AccessibilityObject> object = get(node);
while (!object && node) {
    node = node->parentNode();
    object = get(node);
}

But maybe we can get away with not doing that in this case? Not sure...what do you think, better to match the behavior that other consumers of postNotification(Node*) uses, or do the simpler, one-less-overload approach?

[Andres Gonzalez]

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

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

@@ -1140,6 +1143,12 @@ void AXObjectCache::childrenChanged(AccessibilityObject* object)

+    if (auto* tableSectionElement = dynamicDowncast<HTMLTableSectionElement>(object->element())) {
+        if (auto* parentTable = tableSectionElement->findParentTable().get())
+            m_deferredRecomputeTableIsExposedList.add(*parentTable);
+    }

Can we do this in childrenChanged(Node* node, Node* changedChild) or childrenChanged(RenderObject* renderer, RenderObject* changedChild), whichever is called in this case. That way we don't have to go from AXObject back to Element. This would make the path more straightforward, i.e., we receive a notification with a Node or a RenderObject, then for some special case like being a TableSection, we storage the containing table Element and process the update of the corresponding AX table update later.

[Andres Gonzalez]

(In reply to Tyler Wilcock from comment #15)
> > --- a/Source/WebCore/accessibility/AXObjectCache.cpp
> > +++ a/Source/WebCore/accessibility/AXObjectCache.cpp
> > 
> > +    if (auto* tableSectionElement =
> > dynamicDowncast<HTMLTableSectionElement>(object.element())) {
> > +        auto* parentTable = tableSectionElement->findParentTable().get();
> > +        if (auto* axTable =
> > dynamicDowncast<AccessibilityTable>(getOrCreate(const_cast<HTMLTableElement*>
> > (parentTable))))
> > +            axTable->recomputeIsExposable();
> > 
> > We shouldn't need to go back to the Element at this point in the cache. Can
> > we instead schedule some event for the table when we actually get the notification from the DOM in 
> > childrenChanged(Node* node, Node* changedChild)
> > or the equivalent RenderObject overload.
> OK, fixed this by adding m_deferredRecomputeTableIsExposedList.

See my comment above concerning this.

> 
> > void AXObjectCache::updateIsolatedTree(const
> > Vector<std::pair<RefPtr<AXCoreObjec
> > 
> > +        case AXRowCountChanged:
> > +            if (notification.first->isTable())
> > +                updateNode(notification.first);
> > +            FALLTHROUGH;
> > 
> > Do we need to check if (notification.first->isTable()) here? I'd think we
> > can just update if we receive that notification like with the others, e.g.,
> > CheckedStateChanged.
> I did this because the only time AXRowCountChanged actually needs to update
> properties is for tables. This guards against authoring errors where they
> change aria-rowcount on non-tables, having no practical effect but causing
> us to `updateNode`, which is expensive. But I might be overthinking it, so
> I'd be fine removing the check if you'd prefer.

Maybe doing the check in the IsolatedTree::updateNode would be more appropriate, i.e., the cache doesn't need to know all of this, but IsolatedTree::updateNode should know whether it needs to update something or not.
> 
> > > The behavior for each is different enough that I think we need to match
> > > these overloads, unless you have a different idea.
> > 
> > Can we do
> > 
> > +    else if (attrName == aria_rowcountAttr)
> > +        handleRowCountChanged(get(element), ...);
> > 
> > If so, we just need one. The Document parameter doesn't even need to be
> > passed I believe, but you can get it from the cache when needed.
> I considered this too. But the behavior of postNotification(Node*,
> AXNotification) is more complicated than that, doing traversal if necessary:
> 
> RefPtr<AccessibilityObject> object = get(node);
> while (!object && node) {
>     node = node->parentNode();
>     object = get(node);
> }
> 
> But maybe we can get away with not doing that in this case? Not sure...what
> do you think, better to match the behavior that other consumers of
> postNotification(Node*) uses, or do the simpler, one-less-overload approach?

I think the walk up is done for the case that we don't have an AX object for which to post a platform notification. I don't think that applies in this case.

[Tyler Wilcock]

Created attachment 459762 [details]
Patch

[EWS]

Committed r294875 (251005@main): <https://commits.webkit.org/251005@main>

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