Bug 239498 - Fix for accessibility/aria-grid-with-aria-owns-rows.html in isolated tree mode.
Summary: Fix for accessibility/aria-grid-with-aria-owns-rows.html in isolated tree mode.
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Accessibility (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Andres Gonzalez
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2022-04-19 08:23 PDT by Andres Gonzalez
Modified: 2022-04-22 06:30 PDT (History)
11 users (show)

See Also:


Attachments
Patch (12.38 KB, patch)
2022-04-19 08:48 PDT, Andres Gonzalez
no flags Details | Formatted Diff | Diff
Patch (22.40 KB, patch)
2022-04-21 07:28 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-04-19 08:23:43 PDT
Fix for accessibility/aria-grid-with-aria-owns-rows.html in isolated tree mode.
Comment 1 Radar WebKit Bug Importer 2022-04-19 08:23:58 PDT
<rdar://problem/91961398>
Comment 2 Andres Gonzalez 2022-04-19 08:48:29 PDT
Created attachment 457899 [details]
Patch
Comment 3 chris fleizach 2022-04-19 09:00:40 PDT
Comment on attachment 457899 [details]
Patch

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

> Source/WebCore/accessibility/isolatedtree/AXIsolatedObject.cpp:52
> +        initializeAttributeData(axObject, !m_parentID.isValid());

What is the second Boolean parameter here? Can we make it an enum or evaluate the property inside this method?
Comment 4 Tyler Wilcock 2022-04-19 09:04:34 PDT
Comment on attachment 457899 [details]
Patch

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

> Source/WebCore/accessibility/isolatedtree/AXIsolatedObject.cpp:52
> +        initializeAttributeData(axObject, !m_parentID.isValid());

Is it safe to use member variables inside the constructor of an object, even if we've initialized it above?

> Source/WebCore/accessibility/isolatedtree/AXIsolatedTree.cpp:189
> +    auto object = AXIsolatedObject::create(axObject, this);

Below in this method, we update the node map with the passed-in parentID, which can be different than what AXIsolatedObject::create computes for itself with this patch. Will that be a problem?
Comment 5 Tyler Wilcock 2022-04-19 09:52:24 PDT
In AXIsolatedTree, we have a member variable:

HashMap<AXID, std::pair<AXID, AttachWrapper>> m_unresolvedPendingAppends;

Where the first part of the pair is the parent ID that should be used to resolve the append (i.e. when we call nodeChangeForObject for the object).

With your change, do we still need this data in m_unresolvedPendingAppends?
Comment 6 Andres Gonzalez 2022-04-21 07:28:46 PDT
Created attachment 458060 [details]
Patch
Comment 7 Andres Gonzalez 2022-04-21 08:03:18 PDT
(In reply to chris fleizach from comment #3)
> Comment on attachment 457899 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=457899&action=review
> 
> > Source/WebCore/accessibility/isolatedtree/AXIsolatedObject.cpp:52
> > +        initializeAttributeData(axObject, !m_parentID.isValid());
> 
> What is the second Boolean parameter here? Can we make it an enum or
> evaluate the property inside this method?

Done.
Comment 8 Andres Gonzalez 2022-04-21 08:05:49 PDT
(In reply to Tyler Wilcock from comment #4)
> Comment on attachment 457899 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=457899&action=review
> 
> > Source/WebCore/accessibility/isolatedtree/AXIsolatedObject.cpp:52
> > +        initializeAttributeData(axObject, !m_parentID.isValid());
> 
> Is it safe to use member variables inside the constructor of an object, even
> if we've initialized it above?

Yes.
> 
> > Source/WebCore/accessibility/isolatedtree/AXIsolatedTree.cpp:189
> > +    auto object = AXIsolatedObject::create(axObject, this);
> 
> Below in this method, we update the node map with the passed-in parentID,
> which can be different than what AXIsolatedObject::create computes for
> itself with this patch. Will that be a problem?

Fixed, thanks for pointing it out. Cleaned up the whole thing of passing the parentIDs.
Comment 9 Andres Gonzalez 2022-04-21 08:07:57 PDT
(In reply to Tyler Wilcock from comment #5)
> In AXIsolatedTree, we have a member variable:
> 
> HashMap<AXID, std::pair<AXID, AttachWrapper>> m_unresolvedPendingAppends;
> 
> Where the first part of the pair is the parent ID that should be used to
> resolve the append (i.e. when we call nodeChangeForObject for the object).
> 
> With your change, do we still need this data in m_unresolvedPendingAppends?

No we don't need that any more, so I cleaned it up. Thanks for pointing that out.
Comment 10 EWS 2022-04-22 06:30:33 PDT
Committed r293219 (249888@main): <https://commits.webkit.org/249888@main>

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