Bug 260174

<rdar://problem/113872525>

Created attachment 467272 [details]
Patch

Comment on attachment 467272 [details]
Patch

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

> Source/WebCore/accessibility/AXObjectCache.cpp:4091
> +            if (auto* axParent = notification.first->parentObject()) {
> +                if (axParent->isLabel() && axParent->correspondingControlForLabelElement())
> +                    updateNode(axParent->correspondingControlForLabelElement());

Let's use a RefPtr to protect usage of `axParent`, since we call axParent->correspondingControlForLabelElement() which does non-trivial work that could potentially destroy it. We can also use C++17-style multi-condition if-statements to make this more concise:

if (RefPtr axParent = notification.first->parentObject(); axParent && axParent->isLabel()) {
    if (RefPtr correspondingControl = axParent->correspondingControlForLabelElement())
        updateNode(axParent->correspondingControl.get());
}
break;

> Source/WebCore/accessibility/isolatedtree/AXIsolatedTree.cpp:615
> +    if (axObject.isLabel() && axObject.correspondingControlForLabelElement())
> +        updateNode(*axObject.correspondingControlForLabelElement());

correspondingControlForLabelElement does a bit of work, so let's only call it once. Something like this might work:

if (RefPtr correspondingControl = axObject.isLabel() ? axObject.correspondingControlForLabelElement() : nullptr)
    updateNode(*correspondingControl);

Created attachment 467276 [details]
Patch

(In reply to Joshua Hoffman from comment #4)
> Created attachment 467276 [details]
> Patch

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

+        case AXTextChanged:
+            updateNode(notification.first);
+            if (RefPtr axParent = notification.first->parentObject(); axParent && axParent->isLabel()) {
+                if (RefPtr correspondingControl = axParent->correspondingControlForLabelElement())
+                    updateNode(correspondingControl.get());
+            }
+            break;

The update of the related nodes and properties should happen in AXIsolatedTree::updateNode. So I would leave this as before and move this additional logic to AXIsolatedTree.

--- a/Source/WebCore/accessibility/isolatedtree/AXIsolatedTree.cpp
+++ b/Source/WebCore/accessibility/isolatedtree/AXIsolatedTree.cpp

+    if (RefPtr correspondingControl = axObject.isLabel() ? axObject.correspondingControlForLabelElement() : nullptr)
+        updateNode(*correspondingControl);

I see that you are already doing part of it in in here, so do the whole thing here.

(In reply to Andres Gonzalez from comment #5)
> (In reply to Joshua Hoffman from comment #4)
> > Created attachment 467276 [details]
> > Patch
> 
> 
> The update of the related nodes and properties should happen in
> AXIsolatedTree::updateNode. So I would leave this as before and move this
> additional logic to AXIsolatedTree.

We want to do this update when AXTextChanged, but AXIsolatedTree::updateNode doesn't know which notification was sent—so this allows us to conditionally do the update.

Committed 266919@main (28bfc960f4a1): <https://commits.webkit.org/266919@main>

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