Bug 241915 - AX ITM: AXIsolatedTree::updateNode doesn't update if the target node is a descendant of a barren object.
Summary: AX ITM: AXIsolatedTree::updateNode doesn't update if the target node is a des...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Accessibility (show other bugs)
Version: WebKit Nightly Build
Hardware: All All
: P2 Normal
Assignee: Andres Gonzalez
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2022-06-23 07:48 PDT by Andres Gonzalez
Modified: 2022-06-23 19:46 PDT (History)
10 users (show)

See Also:


Attachments
Patch (14.68 KB, patch)
2022-06-23 08:52 PDT, Andres Gonzalez
no flags Details | Formatted Diff | Diff
Patch (14.63 KB, patch)
2022-06-23 12:05 PDT, Andres Gonzalez
no flags Details | Formatted Diff | Diff
Patch (14.63 KB, patch)
2022-06-23 18:09 PDT, Andres Gonzalez
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Andres Gonzalez 2022-06-23 07:48:39 PDT
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";
Comment 1 Radar WebKit Bug Importer 2022-06-23 07:48:48 PDT
<rdar://problem/95776167>
Comment 2 Andres Gonzalez 2022-06-23 08:52:12 PDT
Created attachment 460444 [details]
Patch
Comment 3 chris fleizach 2022-06-23 09:21:36 PDT
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
Comment 4 chris fleizach 2022-06-23 09:22:52 PDT
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
Comment 5 Andres Gonzalez 2022-06-23 09:31:42 PDT
(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.
Comment 6 chris fleizach 2022-06-23 09:44:10 PDT
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
Comment 7 Andres Gonzalez 2022-06-23 12:05:33 PDT
Created attachment 460452 [details]
Patch
Comment 8 Andres Gonzalez 2022-06-23 12:07:20 PDT
(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.
Comment 9 Andres Gonzalez 2022-06-23 12:09:06 PDT
(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.
Comment 10 Andres Gonzalez 2022-06-23 18:09:22 PDT
Created attachment 460471 [details]
Patch
Comment 11 EWS 2022-06-23 19:46:26 PDT
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].