AX: Missing nullptr check for parentObjectUnignored in AccessibilityO…

…bject::isSelected() https://bugs.webkit.org/show_bug.cgi?id=268509 rdar://121945437 Reviewed by Chris Fleizach and Andres Gonzalez. parentObjectUnignored() can return nullptr, we need to check it before deferencing to avoid a crash. * LayoutTests/accessibility-isolated-tree/TestExpectations: Skip new test, as it exposes a bug that affects ITM. * LayoutTests/accessibility/menuitem-is-selected-crash-expected.txt: Added. * LayoutTests/accessibility/menuitem-is-selected-crash.html: Added. * LayoutTests/platform/glib/TestExpectations: Skip new test. * Source/WebCore/accessibility/AccessibilityNodeObject.cpp: (WebCore::AccessibilityNodeObject::computeAccessibilityIsIgnored const): * Source/WebCore/accessibility/AccessibilityObject.cpp: (WebCore::AccessibilityObject::isSelected const): * Source/WebCore/accessibility/isolatedtree/AXIsolatedTree.cpp: (WebCore::AXIsolatedTree::processQueuedNodeUpdates): Drive-by fix to remove unnecessary HashMap::contains check, HashMap::ensure inherently does this so the contains check was wasted work. Canonical link: https://commits.webkit.org/273971@main
WebKit · Feb 2, 2024 · 870d68c · 870d68c
1 parent b1dc028
commit 870d68c
Show file tree

Hide file tree

Showing 8 changed files with 60 additions and 7 deletions.
diff --git a/LayoutTests/accessibility-isolated-tree/TestExpectations b/LayoutTests/accessibility-isolated-tree/TestExpectations
@@ -34,6 +34,11 @@ accessibility/mac/set-value-editable-types.html [ Failure ]
 accessibility/mac/setting-attributes-is-asynchronous.html [ Failure ]
 accessibility/mac/attributed-string/attributed-string-has-completion-annotation.html [ Failure ]
 
+# Fails because we don't compute node-only AX objects as ignored when they're within a display:none container.
+# This causes us to fail to call AXIsolatedTree::addUnconnectedNode in AXObjectCache::addRelation, meaning
+# we never create an isolated object for #menu-item-one, causing ariaLabelledByElementAtIndex(0) to return null.
+accessibility/menuitem-is-selected.html [ Failure ]
+
 # Frames are off by 1px vs. non-ITM mode.
 accessibility/table-cells.html [ Failure ]
 accessibility/mac/document-links.html [ Failure ]

diff --git a/LayoutTests/accessibility/menuitem-is-selected-expected.txt b/LayoutTests/accessibility/menuitem-is-selected-expected.txt
@@ -0,0 +1,14 @@
+This test ensures that calling isSelected on a menuitem within an unrendered container doesn't cause a crash.
+
+
+{AXRole: AXWebArea}
+
+{AXRole: AXGroup}
+
+{#group AXRole: AXGroup}
+PASS: accessibilityController.accessibleElementById('group').ariaLabelledByElementAtIndex(0).isSelected === false
+
+PASS successfullyParsed is true
+
+TEST COMPLETE
+
diff --git a/LayoutTests/accessibility/menuitem-is-selected.html b/LayoutTests/accessibility/menuitem-is-selected.html
@@ -0,0 +1,29 @@
+<!DOCTYPE HTML PUBLIC "-//IETF//DTD HTML//EN">
+<html>
+<head>
+<script src="../resources/accessibility-helper.js"></script>
+<script src="../resources/js-test.js"></script>
+</head>
+<body>
+
+<div role="application">
+    <ul role="menu" style="display:none">
+        <li style="list-style: none" tabindex="-2" aria-posinset="1" aria-setsize="2" role="menuitem" id="menu-item-one">Menuitem one</li>
+        <li style="list-style: none" tabindex="-2" aria-posinset="2" aria-setsize="2" role="menuitem">Menuitem two</li>
+    </ul>
+    <div role="group" id="group" aria-labelledby="menu-item-one"></div>
+</div>
+
+<script>
+var output = "This test ensures that calling isSelected on a menuitem within an unrendered container doesn't cause a crash.\n\n";
+
+if (window.accessibilityController) {
+    output += dumpAXSearchTraversal(accessibilityController.rootElement, { excludeRoles: "scrollbar" });
+    // Accessing isSelected() should not cause a crash.
+    output += expect("accessibilityController.accessibleElementById('group').ariaLabelledByElementAtIndex(0).isSelected", "false");
+    debug(output);
+}
+</script>
+</body>
+</html>
+
diff --git a/LayoutTests/platform/glib/TestExpectations b/LayoutTests/platform/glib/TestExpectations
@@ -514,6 +514,7 @@ accessibility/display-contents/dynamically-added-children.html [ Skip ]
 accessibility/display-contents/object-ordering.html [ Skip ]
 accessibility/display-contents/search-traversal.html [ Skip ]
 accessibility/empty-text-under-element-cached.html [ Skip ]
+accessibility/menuitem-is-selected.html [ Skip ]
 accessibility/search-traversal-after-role-change.html [ Skip ]
 accessibility/table-search-traversal.html [ Skip ]
 accessibility/tree-update-with-dirty-layout.html [ Skip ]

diff --git a/LayoutTests/platform/mac-wk1/TestExpectations b/LayoutTests/platform/mac-wk1/TestExpectations
@@ -817,6 +817,7 @@ fast/speechrecognition/start-recognition-after-denied-gum.html [ Skip ]
 fast/speechrecognition/start-recognition-without-devices.html [ Skip ]
 
 accessibility/announcement-notification.html [ WontFix ]
+accessibility/menuitem-is-selected.html [ WontFix ]
 accessibility/relationships.html [ WontFix ]
 # Datalist is unsupported in WK1
 accessibility/datalist.html [ WontFix ]

diff --git a/Source/WebCore/accessibility/AccessibilityNodeObject.cpp b/Source/WebCore/accessibility/AccessibilityNodeObject.cpp
@@ -647,6 +647,7 @@ bool AccessibilityNodeObject::computeAccessibilityIsIgnored() const
     if (decision == AccessibilityObjectInclusion::IgnoreObject)
         return true;
 
+    // FIXME: We should return true for node-only objects within display:none containers, but we don't.
     auto role = roleValue();
     return role == AccessibilityRole::Ignored || role == AccessibilityRole::Unknown;
 }

diff --git a/Source/WebCore/accessibility/AccessibilityObject.cpp b/Source/WebCore/accessibility/AccessibilityObject.cpp
@@ -3040,8 +3040,12 @@ bool AccessibilityObject::isSelected() const
         return true;
 
     // Menu items are considered selectable by assistive technologies
-    if (isMenuItem())
-        return isFocused() || parentObjectUnignored()->activeDescendant() == this;
+    if (isMenuItem()) {
+        if (isFocused())
+            return true;
+        WeakPtr parent = parentObjectUnignored();
+        return parent && parent->activeDescendant() == this;
+    }
 
     return false;
 }

diff --git a/Source/WebCore/accessibility/isolatedtree/AXIsolatedTree.cpp b/Source/WebCore/accessibility/isolatedtree/AXIsolatedTree.cpp
@@ -1291,11 +1291,9 @@ void AXIsolatedTree::processQueuedNodeUpdates()
     m_needsUpdateChildren.clear();
 
     for (AXID objectID : m_needsUpdateNode) {
-        if (!m_unresolvedPendingAppends.contains(objectID)) {
-            m_unresolvedPendingAppends.ensure(objectID, [] {
-                return AttachWrapper::OnAXThread;
-            });
-        }
+        m_unresolvedPendingAppends.ensure(objectID, [] {
+            return AttachWrapper::OnAXThread;
+        });
     }
     m_needsUpdateNode.clear();