This can cause crashes.
<rdar://problem/95240529>
Created attachment 460262 [details] Patch
rdar://94895437
Comment on attachment 460262 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=460262&action=review > Source/WebCore/accessibility/AccessibilityObject.cpp:636 > + if (insertionIndex > m_children.size()) do we want to insert this at position 0 in this case? or discard completely?
(In reply to chris fleizach from comment #4) > Comment on attachment 460262 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=460262&action=review > > > Source/WebCore/accessibility/AccessibilityObject.cpp:636 > > + if (insertionIndex > m_children.size()) > > do we want to insert this at position 0 in this case? or discard completely? I think we want to discard this grandchild (and any following) entirely, since they may no longer be the right children after the layout that caused m_children to be cleared. Also, when this happens, m_childrenInitialized should always become false (it does in the crash I was chasing down), meaning we will add the actually-correct children in the next call to children(true).
(In reply to Tyler Wilcock from comment #2) > Created attachment 460262 [details] > Patch It seems that what we are doing in this whole block is to insert the grandchildren if the newChild is ignored. Would it make it clearer if add a method called insertchildren(const Vector&, size_t index), and then we could write in the body of this block: if (descendIfIgnored == DescendIfIgnored::Yes && child->accessibilityIsIgnored()) insertChildren(child->children(), index);
(In reply to Andres Gonzalez from comment #6) > (In reply to Tyler Wilcock from comment #2) > > Created attachment 460262 [details] > > Patch > > It seems that what we are doing in this whole block is to insert the > grandchildren if the newChild is ignored. Would it make it clearer if add a > method called insertchildren(const Vector&, size_t index), and then we could > write in the body of this block: > if (descendIfIgnored == DescendIfIgnored::Yes > && child->accessibilityIsIgnored()) > insertChildren(child->children(), index); I tried this out and unfortunately we'd also need to pass a third AXAncestorFlags parameter (to capture the flags of the ignored object), which IMO makes this refactor not worth it.
(In reply to Tyler Wilcock from comment #7) > (In reply to Andres Gonzalez from comment #6) > > (In reply to Tyler Wilcock from comment #2) > > > Created attachment 460262 [details] > > > Patch > > > > It seems that what we are doing in this whole block is to insert the > > grandchildren if the newChild is ignored. Would it make it clearer if add a > > method called insertchildren(const Vector&, size_t index), and then we could > > write in the body of this block: > > if (descendIfIgnored == DescendIfIgnored::Yes > > && child->accessibilityIsIgnored()) > > insertChildren(child->children(), index); > I tried this out and unfortunately we'd also need to pass a third > AXAncestorFlags parameter (to capture the flags of the ignored object), > which IMO makes this refactor not worth it. No, you can do auto ancestorFlags = computeAncestorFlags(); inside insertObjects(...), no need to pass it as a param. Changed the name from insertchildren to insertObjects because in this case you would be inserting the grandChildren. I believe it would make the code maintainable.
(In reply to Andres Gonzalez from comment #8) > (In reply to Tyler Wilcock from comment #7) > > (In reply to Andres Gonzalez from comment #6) > > > (In reply to Tyler Wilcock from comment #2) > > > > Created attachment 460262 [details] > > > > Patch > > > > > > It seems that what we are doing in this whole block is to insert the > > > grandchildren if the newChild is ignored. Would it make it clearer if add a > > > method called insertchildren(const Vector&, size_t index), and then we could > > > write in the body of this block: > > > if (descendIfIgnored == DescendIfIgnored::Yes > > > && child->accessibilityIsIgnored()) > > > insertChildren(child->children(), index); > > I tried this out and unfortunately we'd also need to pass a third > > AXAncestorFlags parameter (to capture the flags of the ignored object), > > which IMO makes this refactor not worth it. > > No, you can do > > auto ancestorFlags = computeAncestorFlags(); > > inside insertObjects(...), no need to pass it as a param. Changed the name > from insertchildren to insertObjects because in this case you would be > inserting the grandChildren. I believe it would make the code maintainable. The name insertChildren is fine, thought for a moment that insertObjects was better but it is not, since you are inserting children that in this case happen to be grandchildren.
(In reply to Andres Gonzalez from comment #8) > (In reply to Tyler Wilcock from comment #7) > > (In reply to Andres Gonzalez from comment #6) > > > (In reply to Tyler Wilcock from comment #2) > > > > Created attachment 460262 [details] > > > > Patch > > > > > > It seems that what we are doing in this whole block is to insert the > > > grandchildren if the newChild is ignored. Would it make it clearer if add a > > > method called insertchildren(const Vector&, size_t index), and then we could > > > write in the body of this block: > > > if (descendIfIgnored == DescendIfIgnored::Yes > > > && child->accessibilityIsIgnored()) > > > insertChildren(child->children(), index); > > I tried this out and unfortunately we'd also need to pass a third > > AXAncestorFlags parameter (to capture the flags of the ignored object), > > which IMO makes this refactor not worth it. > > No, you can do > > auto ancestorFlags = computeAncestorFlags(); > > inside insertObjects(...), no need to pass it as a param. Changed the name > from insertchildren to insertObjects because in this case you would be > inserting the grandChildren. I believe it would make the code maintainable. The current behavior is to use the ancestor flags of the ignored child rather than `this` as you suggest. Using `this` ancestor flags might be OK...but I don't want to make that behavior change in this patch. Let's address this refactor in a separate patch.
Committed r295618 (251623@main): <https://commits.webkit.org/251623@main> All reviewed patches have been landed. Closing bug and clearing flags on attachment 460262 [details].