AX ITM: AXIsolatedTree::updateNode doesn't update if the target node is a descendant of a barren object.

• Component: Accessibility
• Status: RESOLVED FIXED

[Andres Gonzalez]

This causes, for instance, that a button title is not updated if it is modified as follows:

<button id="button">
    <span id="counter">0<span>
</button>

document.getElementById("counter").innerText = "3";

[Radar WebKit Bug Importer]

<rdar://problem/95776167>

[Andres Gonzalez]

Created attachment 460444 [details]
Patch

[chris fleizach]

Comment on attachment 460444 [details]
Patch

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

> Source/WebCore/accessibility/isolatedtree/AXIsolatedTree.cpp:367
> +    ASSERT(is<AccessibilityObject>(axObject));

I don't know if we need to assert here, because it will crash on the next line anyway

[chris fleizach]

Comment on attachment 460444 [details]
Patch

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

> Source/WebCore/accessibility/AXObjectCache.cpp:1913
> +        handleTextChanged(getOrCreate(element));

I'm worried about the getOrCreate() here. this might get fired during layout and creating ax objects can be problematic. it looks like the rest of the versions here only use get() or defer changes

[Andres Gonzalez]

(In reply to chris fleizach from comment #4)
> Comment on attachment 460444 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=460444&action=review
> 
> > Source/WebCore/accessibility/AXObjectCache.cpp:1913
> > +        handleTextChanged(getOrCreate(element));
> 
> I'm worried about the getOrCreate() here. this might get fired during layout
> and creating ax objects can be problematic. it looks like the rest of the
> versions here only use get() or defer changes

That's what we had before for textChanged:

-void AXObjectCache::textChanged(Node* node)
 {
-    textChanged(getOrCreate(node));
-}

I could change that, but didn't want to change a long standing behavior this late.

[chris fleizach]

Comment on attachment 460444 [details]
Patch

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

>>> Source/WebCore/accessibility/AXObjectCache.cpp:1913
>>> +        handleTextChanged(getOrCreate(element));
>> 
>> I'm worried about the getOrCreate() here. this might get fired during layout and creating ax objects can be problematic. it looks like the rest of the versions here only use get() or defer changes
> 
> That's what we had before for textChanged:
> 
> -void AXObjectCache::textChanged(Node* node)
>  {
> -    textChanged(getOrCreate(node));
> -}
> 
> I could change that, but didn't want to change a long standing behavior this late.

if its the same behavior we can probably keep as is but could be problematic in the future

[Andres Gonzalez]

Created attachment 460452 [details]
Patch

[Andres Gonzalez]

(In reply to chris fleizach from comment #3)
> Comment on attachment 460444 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=460444&action=review
> 
> > Source/WebCore/accessibility/isolatedtree/AXIsolatedTree.cpp:367
> > +    ASSERT(is<AccessibilityObject>(axObject));
> 
> I don't know if we need to assert here, because it will crash on the next
> line anyway

Removed assert.

[Andres Gonzalez]

(In reply to chris fleizach from comment #6)
> Comment on attachment 460444 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=460444&action=review
> 
> >>> Source/WebCore/accessibility/AXObjectCache.cpp:1913
> >>> +        handleTextChanged(getOrCreate(element));
> >> 
> >> I'm worried about the getOrCreate() here. this might get fired during layout and creating ax objects can be problematic. it looks like the rest of the versions here only use get() or defer changes
> > 
> > That's what we had before for textChanged:
> > 
> > -void AXObjectCache::textChanged(Node* node)
> >  {
> > -    textChanged(getOrCreate(node));
> > -}
> > 
> > I could change that, but didn't want to change a long standing behavior this late.
> 
> if its the same behavior we can probably keep as is but could be problematic
> in the future

Agree.

[Andres Gonzalez]

Created attachment 460471 [details]
Patch

[EWS]

Committed 251817@main (6dfa4f8dfed6): <https://commits.webkit.org/251817@main>

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