Bug 247316

Summary: AX ITM: Fix for LayoutTests/accessibility/svg-remote-element.html in isolated tree mode.
Product: WebKit Reporter: Andres Gonzalez <andresg_22>
Component: AccessibilityAssignee: Andres Gonzalez <andresg_22>
Status: RESOLVED FIXED    
Severity: Normal CC: aboxhall, andresg_22, apinheiro, cfleizach, dmazzoni, ews-watchlist, jcraig, jdiggs, samuel_white, tyler_w, webkit-bug-importer
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: All   
OS: All   
Attachments:
Description Flags
Patch
none
Patch
none
Patch
none
Patch none

Description Andres Gonzalez 2022-11-01 06:59:13 PDT
Currently timing out in ITM.
Comment 1 Radar WebKit Bug Importer 2022-11-01 06:59:23 PDT
<rdar://problem/101804648>
Comment 2 Andres Gonzalez 2022-11-01 07:12:58 PDT
Created attachment 463343 [details]
Patch
Comment 3 Tyler Wilcock 2022-11-01 09:54:18 PDT
Comment on attachment 463343 [details]
Patch

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

Would objects outside the main-document for other reasons (e.g. they're in an iframe) have the same problem that SVG objects do?

> Source/WebCore/accessibility/isolatedtree/AXIsolatedTree.cpp:336
> +        AXLOG("Bail out here, we can't build an isolated tree branch rooted at a detached object.");

If we're changing this comment to a log, the "Bail out here" portion doesn't have the same context. Maybe it should instead saying something like:

"collectNodeChangesForSubtree bailing for object ID %s because it is detached"
Comment 4 Andres Gonzalez 2022-11-01 18:43:18 PDT
Created attachment 463356 [details]
Patch
Comment 5 Andres Gonzalez 2022-11-01 18:49:49 PDT
(In reply to Tyler Wilcock from comment #3)
> Comment on attachment 463343 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=463343&action=review
> 
> Would objects outside the main-document for other reasons (e.g. they're in
> an iframe) have the same problem that SVG objects do?

I think iframes documents share the same top document with the main document, so it wouldn't have the same problem.
> 
> > Source/WebCore/accessibility/isolatedtree/AXIsolatedTree.cpp:336
> > +        AXLOG("Bail out here, we can't build an isolated tree branch rooted at a detached object.");
> 
> If we're changing this comment to a log, the "Bail out here" portion doesn't
> have the same context. Maybe it should instead saying something like:
> 
> "collectNodeChangesForSubtree bailing for object ID %s because it is
> detached"

Removed that part of the log message. It is clear in the log that this message is in the scope of this method.
Comment 6 Andres Gonzalez 2022-11-02 05:09:22 PDT
Created attachment 463360 [details]
Patch
Comment 7 chris fleizach 2022-11-02 09:55:20 PDT
Comment on attachment 463360 [details]
Patch

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

> Source/WebCore/accessibility/AXObjectCache.cpp:799
> +    RefPtr object = createObjectFromRenderer(renderer);

we don't need the type of the RefPtr anymore?

> Source/WebCore/accessibility/AccessibilitySVGElement.cpp:51
> +    ASSERT(renderer);

can we ASSERT(cache) here too
Comment 8 Andres Gonzalez 2022-11-02 11:44:00 PDT
Created attachment 463367 [details]
Patch
Comment 9 Andres Gonzalez 2022-11-02 11:47:00 PDT
(In reply to chris fleizach from comment #7)
> Comment on attachment 463360 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=463360&action=review
> 
> > Source/WebCore/accessibility/AXObjectCache.cpp:799
> > +    RefPtr object = createObjectFromRenderer(renderer);
> 
> we don't need the type of the RefPtr anymore?

No, compiler magic inference.
> 
> > Source/WebCore/accessibility/AccessibilitySVGElement.cpp:51
> > +    ASSERT(renderer);
> 
> can we ASSERT(cache) here too

Done.
Comment 10 EWS 2022-11-02 17:17:13 PDT
Committed 256261@main (c98a2ded1d00): <https://commits.webkit.org/256261@main>

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