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