Bug 272606 – AX: For children of display:contents elements, nextSibling() and previousSibling() can return non-siblings

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

[Tyler Wilcock]

This can cause the AX tree to be wrong, and for us to compute the wrong accessibility text, as computing it requires walking the DOM / render tree via nextSibling().

[Radar WebKit Bug Importer]

<rdar://problem/126376582>

[Tyler Wilcock]

Created attachment 470904 [details]
Patch

[Andres Gonzalez]

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

diff --git a/Source/WebCore/accessibility/AXObjectCache.cpp b/Source/WebCore/accessibility/AXObjectCache.cpp
index 8dca96f08b03..2cab8c06e799 100644
--- a/Source/WebCore/accessibility/AXObjectCache.cpp
+++ b/Source/WebCore/accessibility/AXObjectCache.cpp
@@ -36,6 +36,7 @@
 #include "AXLogger.h"
 #include "AXRemoteFrame.h"
 #include "AXTextMarker.h"
+#include "AXTreeIterator.h"
 #include "AccessibilityARIAGridCell.h"
 #include "AccessibilityARIAGridRow.h"
 #include "AccessibilityARIATable.h"
@@ -2227,16 +2228,13 @@ void AXObjectCache::liveRegionChangedNotificationPostTimerFired()
     m_liveRegionObjectsSet.clear();
 }

-static AccessibilityObject* firstFocusableChild(AccessibilityObject* obj)
+static AccessibilityObject* firstFocusableChild(AccessibilityObject& object)
 {
-    if (!obj)
-        return nullptr;
-
-    for (auto* child = obj->firstChild(); child; child = child->nextSibling()) {
-        if (child->canSetFocusAttribute())
-            return child;
-        if (AccessibilityObject* focusable = firstFocusableChild(child))
-            return focusable;
+    for (auto& child : AXChildIterator(object)) {
+        if (child.canSetFocusAttribute())
+            return &child;
+        if (auto* focusableDescendant = firstFocusableChild(child))
+            return focusableDescendant;
     }
     return nullptr;
 }
@@ -2260,7 +2258,7 @@ void AXObjectCache::focusCurrentModal()
         return;

     if (AccessibilityObject* currentModalNodeObject = getOrCreate(*m_currentModalElement)) {
-        if (AccessibilityObject* focusable = firstFocusableChild(currentModalNodeObject))
+        if (AccessibilityObject* focusable = firstFocusableChild(*currentModalNodeObject))

AG: auto* focusable

             focusable->setFocused(true);
     }
 }

[Andres Gonzalez]

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

diff --git a/Source/WebCore/accessibility/AXTreeIterator.h b/Source/WebCore/accessibility/AXTreeIterator.h
new file mode 100644
index 000000000000..8cd983437650
--- /dev/null
+++ b/Source/WebCore/accessibility/AXTreeIterator.h
@@ -0,0 +1,156 @@
+/*
+ * Copyright (C) 2024 Apple Inc. All rights reserved.
+ *
+ * Redistribution and use in source and binary forms, with or without
+ * modification, are permitted provided that the following conditions
+ * are met:
+ * 1. Redistributions of source code must retain the above copyright
+ *    notice, this list of conditions and the following disclaimer.
+ * 2. Redistributions in binary form must reproduce the above copyright
+ *    notice, this list of conditions and the following disclaimer in the
+ *    documentation and/or other materials provided with the distribution.
+ *
+ * THIS SOFTWARE IS PROVIDED BY APPLE INC. AND ITS CONTRIBUTORS ``AS IS''
+ * AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO,
+ * THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A PARTICULAR
+ * PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL APPLE INC. OR ITS CONTRIBUTORS
+ * BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL, SPECIAL, EXEMPLARY, OR
+ * CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT LIMITED TO, PROCUREMENT OF
+ * SUBSTITUTE GOODS OR SERVICES; LOSS OF USE, DATA, OR PROFITS; OR BUSINESS
+ * INTERRUPTION) HOWEVER CAUSED AND ON ANY THEORY OF LIABILITY, WHETHER IN
+ * CONTRACT, STRICT LIABILITY, OR TORT (INCLUDING NEGLIGENCE OR OTHERWISE)
+ * ARISING IN ANY WAY OUT OF THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF
+ * THE POSSIBILITY OF SUCH DAMAGE.
+ */
+
+#pragma once
+
+#include "AccessibilityObject.h"
+#include <iterator>
+
+namespace WebCore {
+
+namespace Accessibility {
+// When using the AccessibilityRenderObject and AccessibilityNodeObject previousSibling and nextSibling methods,
+// we can seamlessly alternate between walking the DOM and walking the render tree. There are complications with
+// this, especially introduced by display:contents, which removes the renderer for the given object, and moves it's

AG: it's -> its

+// render tree children up one level higher than they otherwise would've been on. This iterator abstracts over this
+// complexity, ensuring each object is actually a sibling of the last. Don't use this directly, instead use AXChildIterator
+// or AXSiblingIterator.
+class SiblingIterator {
+public:
+    using iterator_category = std::bidirectional_iterator_tag;
+    using difference_type = std::ptrdiff_t;
+    using element_type = AccessibilityObject;
+    using pointer = AccessibilityObject*;
+    using reference = AccessibilityObject&;
+
+    SiblingIterator() = default;
+    SiblingIterator(const AccessibilityObject& object)
+        : m_current(&object)
+        , m_displayContentsParent(object.displayContentsParent())
+    {
+        // This iterator relies on `nextSibling` and `previousSibling` which are only implemented
+        // on these two classes, so it won't do the right thing for other classes.

AG: all other subclass inherits the AccessibilityObject implementation, so I'm not sure what the meaning of this is.

+        ASSERT(object.isAccessibilityRenderObject() || object.isAccessibilityNodeObject());
+    }
+    SiblingIterator(const AccessibilityObject& object, const AccessibilityObject& parent)
+        : m_current(&object)
+        , m_displayContentsParent(parent.hasDisplayContents() ? &parent : nullptr)

AG: I think you can have just 1 constructor with a second param as a pointer or optional with a default value.

+    {
+        ASSERT(object.isAccessibilityRenderObject() || object.isAccessibilityNodeObject());
+    }
+
+    AccessibilityObject& operator*() const { return const_cast<AccessibilityObject&>(*m_current); }
+    AccessibilityObject* operator->() const { return const_cast<AccessibilityObject*>(m_current.get()); }
+    AccessibilityObject* ptr() const { return const_cast<AccessibilityObject*>(m_current.get()); }
+    bool operator==(const SiblingIterator& other) const { return other.m_current == m_current; }
+
+    // Prefix increment operator (++iterator).
+    SiblingIterator& operator++()
+    {
+        m_current = m_current->nextSibling();
+        ensureContentsParentValidity();
+        return *this;
+    }
+
+    // Postfix increment operator (iterator++).
+    SiblingIterator operator++(int)
+    {
+        auto copy = *this;
+        ++(*this);
+        return copy;
+    }
+
+    // --iterator
+    SiblingIterator& operator--()
+    {
+        m_current = m_current->previousSibling();
+        ensureContentsParentValidity();
+        return *this;
+    }
+
+    // iterator--
+    SiblingIterator operator--(int)
+    {
+        auto copy = *this;
+        --(*this);
+        return copy;
+    }
+private:
+    void ensureContentsParentValidity()
+    {
+        auto* contentsParent = m_current ? m_current->displayContentsParent() : nullptr;
+        if (contentsParent && m_displayContentsParent && contentsParent != m_displayContentsParent.get())
+            m_current = nullptr;
+    }
+
+    RefPtr<const AccessibilityObject> m_current;
+    // If the original object had a display:contents parent, it is stored here. This is nullptr otherwise.
+    RefPtr<const AccessibilityObject> m_displayContentsParent { nullptr };
+}; // class SiblingIterator
+static_assert(std::bidirectional_iterator<SiblingIterator>);
+
+} // namespace Accessibility
+
+class AXChildIterator {
+public:
+    AXChildIterator(const AccessibilityObject& parent)
+        : m_parent(parent)
+    { }
+
+    Accessibility::SiblingIterator begin()
+    {
+        if (auto* firstChild = m_parent->firstChild())
+            return Accessibility::SiblingIterator { *firstChild, m_parent.get() };
+        return end();
+    }
+    Accessibility::SiblingIterator end() { return Accessibility::SiblingIterator { }; }
+private:
+    Ref<const AccessibilityObject> m_parent;
+}; // class AXChildIterator
+
+class AXSiblingIterator {
+public:
+    AXSiblingIterator(const AccessibilityObject& object)
+        : m_object(object)
+    { }
+
+    Accessibility::SiblingIterator begin()
+    {
+        auto iterator = Accessibility::SiblingIterator { m_object.get() };
+        return ++iterator;
+    }
+    Accessibility::SiblingIterator end() { return Accessibility::SiblingIterator { }; }
+
+    Accessibility::SiblingIterator rbegin()
+    {
+        auto iterator = Accessibility::SiblingIterator { m_object.get() };
+        return --iterator;
+    }
+    Accessibility::SiblingIterator rend() { return end(); }
+private:
+    Ref<const AccessibilityObject> m_object;
+}; // class AXSiblingIterator

AG: AXSiblingIterator and AXChildIterator could be AccessibilityObject::SiblingIterator and ::ChildIterator.
AG: it seems that AXSiblingIterator::begin() assumes that the object passed to the constructor is before the first sibbling. If so, we should make that explicit.

+
+} // namespace WebCore

[Andres Gonzalez]

This would be clearer if we had a single AccessibilityObject::iterator that implements ++ and -- and you can instantiate it with *this to iterate siblings or with firstChild() to iterate children.

[Tyler Wilcock]

Created attachment 470959 [details]
Patch

[EWS]

Committed 277651@main (5b4dc91691b7): <https://commits.webkit.org/277651@main>

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