Bug 238267 – AX: Remove firstChild, lastChild, previousSibling, nextSibling, nextSiblingUnignored, and previousSiblingUnignored from the AXCoreObject interface

• Status: RESOLVED
• Resolution: FIXED
• Component: Accessibility

[Tyler Wilcock]

All of these except nextSibling and previousSibling are ASSERT_NOT_REACHED no-ops in AXIsolatedObject. We do compute next and previous sibling for isolated objects, but they're only used in one place (appendChildrenToArray), and the usage doesn't make sense for isolated objects.

[Radar WebKit Bug Importer]

<rdar://problem/90705867>

[Tyler Wilcock]

Created attachment 455511 [details]
Patch

[Andres Gonzalez]

(In reply to Tyler Wilcock from comment #2)
> Created attachment 455511 [details]
> Patch

--- a/Source/WebCore/accessibility/isolatedtree/AXIsolatedObject.h
+++ a/Source/WebCore/accessibility/isolatedtree/AXIsolatedObject.h

-    bool accessibilityIsIgnored() const override { return boolAttributeValue(AXPropertyName::IsAccessibilityIgnored); }
+    // We should never create an isolated object from an ignored live object, so we can hardcode this to false.
+    bool accessibilityIsIgnored() const override { return false; }

What about if the object becomes ignored after being created in an update? Perhaps we should maintain the property for that case.

The rest looks good!

[Tyler Wilcock]

(In reply to Andres Gonzalez from comment #3)
> (In reply to Tyler Wilcock from comment #2)
> > Created attachment 455511 [details]
> > Patch
> 
> --- a/Source/WebCore/accessibility/isolatedtree/AXIsolatedObject.h
> +++ a/Source/WebCore/accessibility/isolatedtree/AXIsolatedObject.h
> 
> -    bool accessibilityIsIgnored() const override { return
> boolAttributeValue(AXPropertyName::IsAccessibilityIgnored); }
> +    // We should never create an isolated object from an ignored live
> object, so we can hardcode this to false.
> +    bool accessibilityIsIgnored() const override { return false; }
> 
> What about if the object becomes ignored after being created in an update?
> Perhaps we should maintain the property for that case.
> 
> The rest looks good!
I would expect this sequence:

  1. Object becomes ignored through some webpage change
  2. Because the object is ignored, its live object parent gets a child update that now excludes this newly ignored object (because we never allow insertion of ignored children into what is effectively the AX tree, AccessibilityObject::m_children -- see AccessibilityObject::insertChild)
  3. We also get an AXIsolatedTree::updateChildren for the parent, which finds that it no longer has the newly ignored object as a child, and thus the child gets deleted from the isolated tree entirely

Meaning a property wouldn't be necessary, because the object should be deleted from the isolated tree outright.  Step 2 is heavy in assumption, though...not sure that every change that would cause an object to become ignored also causes a live object to update its children.

And regardless of all of that, because we don't have any logic in place to update AXPropertyName::IsAccessibilityIgnored, my opinion is that we can add the property again later when we actually need to (if we ever do).

[Andres Gonzalez]

(In reply to Tyler Wilcock from comment #4)
> (In reply to Andres Gonzalez from comment #3)
> > (In reply to Tyler Wilcock from comment #2)
> > > Created attachment 455511 [details]
> > > Patch
> > 
> > --- a/Source/WebCore/accessibility/isolatedtree/AXIsolatedObject.h
> > +++ a/Source/WebCore/accessibility/isolatedtree/AXIsolatedObject.h
> > 
> > -    bool accessibilityIsIgnored() const override { return
> > boolAttributeValue(AXPropertyName::IsAccessibilityIgnored); }
> > +    // We should never create an isolated object from an ignored live
> > object, so we can hardcode this to false.
> > +    bool accessibilityIsIgnored() const override { return false; }
> > 
> > What about if the object becomes ignored after being created in an update?
> > Perhaps we should maintain the property for that case.
> > 
> > The rest looks good!
> I would expect this sequence:
> 
>   1. Object becomes ignored through some webpage change
>   2. Because the object is ignored, its live object parent gets a child
> update that now excludes this newly ignored object (because we never allow
> insertion of ignored children into what is effectively the AX tree,
> AccessibilityObject::m_children -- see AccessibilityObject::insertChild)
>   3. We also get an AXIsolatedTree::updateChildren for the parent, which
> finds that it no longer has the newly ignored object as a child, and thus
> the child gets deleted from the isolated tree entirely
> 
> Meaning a property wouldn't be necessary, because the object should be
> deleted from the isolated tree outright.  Step 2 is heavy in assumption,
> though...not sure that every change that would cause an object to become
> ignored also causes a live object to update its children.
> 
> And regardless of all of that, because we don't have any logic in place to
> update AXPropertyName::IsAccessibilityIgnored, my opinion is that we can add
> the property again later when we actually need to (if we ever do).

Agree. Thanks.

[Tyler Wilcock]

Thanks for the review!

[EWS]

Committed r291798 (248826@main): <https://commits.webkit.org/248826@main>

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