Diff
Modified: trunk/LayoutTests/ChangeLog (293218 => 293219)
--- trunk/LayoutTests/ChangeLog 2022-04-22 13:27:43 UTC (rev 293218)
+++ trunk/LayoutTests/ChangeLog 2022-04-22 13:29:23 UTC (rev 293219)
@@ -1,3 +1,14 @@
+2022-04-22 Andres Gonzalez <[email protected]>
+
+ Fix for accessibility/aria-grid-with-aria-owns-rows.html in isolated tree mode.
+ https://bugs.webkit.org/show_bug.cgi?id=239498
+ <rdar://problem/91961398>
+
+ Reviewed by Chris Fleizach and Tyler Wilcock.
+
+ * accessibility/aria-grid-with-aria-owns-rows-expected.txt:
+ * accessibility/aria-grid-with-aria-owns-rows.html:
+
2022-04-22 Alan Bujtas <[email protected]>
Book content is clipped at page boundary when displaying list with bullet points
Modified: trunk/LayoutTests/accessibility/aria-grid-with-aria-owns-rows-expected.txt (293218 => 293219)
--- trunk/LayoutTests/accessibility/aria-grid-with-aria-owns-rows-expected.txt 2022-04-22 13:27:43 UTC (rev 293218)
+++ trunk/LayoutTests/accessibility/aria-grid-with-aria-owns-rows-expected.txt 2022-04-22 13:29:23 UTC (rev 293219)
@@ -1,15 +1,11 @@
-Foo
-Bar
This tests that an ARIA table can use aria-owns for its cells.
+PASS: row1.childAtIndex(0).isEqual(table.cellForColumnAndRow(0, 0)) === true
+PASS: row1.childAtIndex(0).isEqual(accessibilityController.accessibleElementById('row1-cell-1')) === true
+PASS: cell1.parentElement().isEqual(row1) === true
+PASS: row1.parentElement().isEqual(table) === true
-On success, you will see a series of "PASS" messages, followed by "TEST COMPLETE".
-
-
-PASS row1.childAtIndex(0).isEqual(table.cellForColumnAndRow(0, 0)) is true
-PASS row1.childAtIndex(0).isEqual(accessibilityController.accessibleElementById('row1-cell-1')) is true
-PASS cell1.parentElement().isEqual(row1) is true
-PASS row1.parentElement().isEqual(table) is true
PASS successfullyParsed is true
TEST COMPLETE
-
+Foo
+Bar
Modified: trunk/LayoutTests/accessibility/aria-grid-with-aria-owns-rows.html (293218 => 293219)
--- trunk/LayoutTests/accessibility/aria-grid-with-aria-owns-rows.html 2022-04-22 13:27:43 UTC (rev 293218)
+++ trunk/LayoutTests/accessibility/aria-grid-with-aria-owns-rows.html 2022-04-22 13:29:23 UTC (rev 293219)
@@ -1,33 +1,31 @@
<!DOCTYPE html>
<html>
<head>
-<script src=""
-<title>aria-sort</title>
+<script src=""
+<script src=""
</head>
<body>
<div aria-label="Files" role="grid" tabindex="0" aria-rowcount="2" aria-colcount="2" id="table">
- <div role="row" aria-owns="row1-cell-1 row1-cell-2" id="row1"></div>
- <div id="row1-cell-1" aria-rowindex="1" aria-colindex="1" role="gridcell" tabindex="0">Foo</div>
- <div id="row1-cell-2" aria-rowindex="1" aria-colindex="2" role="gridcell" tabindex="0">Bar</div>
+ <div role="row" aria-owns="row1-cell-1 row1-cell-2" id="row1"></div>
+ <div id="row1-cell-1" aria-rowindex="1" aria-colindex="1" role="gridcell" tabindex="0">Foo</div>
+ <div id="row1-cell-2" aria-rowindex="1" aria-colindex="2" role="gridcell" tabindex="0">Bar</div>
</div>
-<p id="description"></p>
-<div id="console"></div>
-
<script>
- description("This tests that an ARIA table can use aria-owns for its cells.");
+ let output = "This tests that an ARIA table can use aria-owns for its cells.\n";
if (window.accessibilityController) {
var row1 = accessibilityController.accessibleElementById("row1");
var table = accessibilityController.accessibleElementById("table")
var cell1 = table.cellForColumnAndRow(0, 0);
- shouldBeTrue("row1.childAtIndex(0).isEqual(table.cellForColumnAndRow(0, 0))");
- shouldBeTrue("row1.childAtIndex(0).isEqual(accessibilityController.accessibleElementById('row1-cell-1'))");
- shouldBeTrue("cell1.parentElement().isEqual(row1)");
- shouldBeTrue("row1.parentElement().isEqual(table)");
+ output += expect("row1.childAtIndex(0).isEqual(table.cellForColumnAndRow(0, 0))", "true");
+ output += expect("row1.childAtIndex(0).isEqual(accessibilityController.accessibleElementById('row1-cell-1'))", "true");
+ output += expect("cell1.parentElement().isEqual(row1)", "true");
+ output += expect("row1.parentElement().isEqual(table)", "true");
+
+ debug(output);
}
</script>
-<script src=""
</body>
</html>
Modified: trunk/Source/WebCore/ChangeLog (293218 => 293219)
--- trunk/Source/WebCore/ChangeLog 2022-04-22 13:27:43 UTC (rev 293218)
+++ trunk/Source/WebCore/ChangeLog 2022-04-22 13:29:23 UTC (rev 293219)
@@ -1,3 +1,46 @@
+2022-04-22 Andres Gonzalez <[email protected]>
+
+ Fix for accessibility/aria-grid-with-aria-owns-rows.html in isolated tree mode.
+ https://bugs.webkit.org/show_bug.cgi?id=239498
+ <rdar://problem/91961398>
+
+ Reviewed by Chris Fleizach and Tyler Wilcock.
+
+ Test: accessibility/aria-grid-with-aria-owns-rows.html.
+
+ When creating AXIsolatedObjects, we were passing the object ID of the
+ parent to the constructor in a parent to child traversal, and storing
+ that parent ID in the object. This assumed that all AX objects have a
+ parent-child reflexive relationship, i.e., if A is a child of B, B is
+ the parent of A. But that is not true in several cases in the
+ accessibility hierarchy.
+ One example where the parent-child relationship is not reflexive is for table cells, where a cell can be the child of an object with role column, but its parent is an object with role row.
+ Another case is when aria-owns is used.
+ This patch fixes the problem by letting the live object determine its
+ own parent, instead of having the traversal algorithm pass the parent ID
+ to the AXIsolatedObject constructor.
+ A previous attempt to fix this problem was made in
+ https://bugs.webkit.org/show_bug.cgi?id=236156.
+
+ * accessibility/isolatedtree/AXIsolatedObject.cpp:
+ (WebCore::AXIsolatedObject::AXIsolatedObject):
+ (WebCore::AXIsolatedObject::create):
+ (WebCore::AXIsolatedObject::initializeProperties):
+ (WebCore::AXIsolatedObject::initializeAttributeData): Renamed.
+ * accessibility/isolatedtree/AXIsolatedObject.h:
+ * accessibility/isolatedtree/AXIsolatedTree.cpp:
+ (WebCore::AXIsolatedTree::create):
+ (WebCore::AXIsolatedTree::generateSubtree):
+ (WebCore::AXIsolatedTree::nodeChangeForObject):
+ (WebCore::AXIsolatedTree::queueRemovalsAndUnresolvedChanges):
+ (WebCore::AXIsolatedTree::collectNodeChangesForSubtree):
+ (WebCore::AXIsolatedTree::updateNode):
+ (WebCore::AXIsolatedTree::updateChildren):
+ (WebCore::AXIsolatedTree::parentIDForObject): Deleted.
+ * accessibility/isolatedtree/AXIsolatedTree.h:
+ * accessibility/isolatedtree/mac/AXIsolatedObjectMac.mm:
+ (WebCore::AXIsolatedObject::initializePlatformProperties):
+
2022-04-22 Alan Bujtas <[email protected]>
Book content is clipped at page boundary when displaying list with bullet points
Modified: trunk/Source/WebCore/accessibility/isolatedtree/AXIsolatedObject.cpp (293218 => 293219)
--- trunk/Source/WebCore/accessibility/isolatedtree/AXIsolatedObject.cpp 2022-04-22 13:27:43 UTC (rev 293218)
+++ trunk/Source/WebCore/accessibility/isolatedtree/AXIsolatedObject.cpp 2022-04-22 13:29:23 UTC (rev 293219)
@@ -38,23 +38,28 @@
namespace WebCore {
-AXIsolatedObject::AXIsolatedObject(AXCoreObject& object, AXIsolatedTree* tree, AXID parentID)
+AXIsolatedObject::AXIsolatedObject(AXCoreObject& axObject, AXIsolatedTree* tree)
: m_cachedTree(tree)
- , m_parentID(parentID)
- , m_id(object.objectID())
+ , m_id(axObject.objectID())
{
ASSERT(isMainThread());
- if (m_id.isValid())
- initializeAttributeData(object, !parentID.isValid());
- else {
+ ASSERT(is<AccessibilityObject>(axObject));
+
+ auto* axParent = axObject.parentObjectUnignored();
+ m_parentID = axParent ? axParent->objectID() : AXID();
+
+ if (m_id.isValid()) {
+ auto isRoot = !axParent && axObject.isScrollView() ? IsRoot::Yes : IsRoot::No;
+ initializeProperties(axObject, isRoot);
+ } else {
// Should never happen under normal circumstances.
ASSERT_NOT_REACHED();
}
}
-Ref<AXIsolatedObject> AXIsolatedObject::create(AXCoreObject& object, AXIsolatedTree* tree, AXID parentID)
+Ref<AXIsolatedObject> AXIsolatedObject::create(AXCoreObject& object, AXIsolatedTree* tree)
{
- return adoptRef(*new AXIsolatedObject(object, tree, parentID));
+ return adoptRef(*new AXIsolatedObject(object, tree));
}
AXIsolatedObject::~AXIsolatedObject()
@@ -67,7 +72,7 @@
ASSERT_NOT_REACHED();
}
-void AXIsolatedObject::initializeAttributeData(AXCoreObject& coreObject, bool isRoot)
+void AXIsolatedObject::initializeProperties(AXCoreObject& coreObject, IsRoot isRoot)
{
ASSERT(is<AccessibilityObject>(coreObject));
auto& object = downcast<AccessibilityObject>(coreObject);
@@ -408,7 +413,7 @@
setMathscripts(AXPropertyName::MathPostscripts, object);
}
- if (isRoot) {
+ if (isRoot == IsRoot::Yes) {
setObjectProperty(AXPropertyName::WebArea, object.webAreaObject());
setProperty(AXPropertyName::SessionID, object.sessionID().isolatedCopy());
setProperty(AXPropertyName::DocumentURI, object.documentURI().isolatedCopy());
Modified: trunk/Source/WebCore/accessibility/isolatedtree/AXIsolatedObject.h (293218 => 293219)
--- trunk/Source/WebCore/accessibility/isolatedtree/AXIsolatedObject.h 2022-04-22 13:27:43 UTC (rev 293218)
+++ trunk/Source/WebCore/accessibility/isolatedtree/AXIsolatedObject.h 2022-04-22 13:29:23 UTC (rev 293219)
@@ -47,7 +47,7 @@
class AXIsolatedObject final : public AXCoreObject {
friend class AXIsolatedTree;
public:
- static Ref<AXIsolatedObject> create(AXCoreObject&, AXIsolatedTree*, AXID parentID);
+ static Ref<AXIsolatedObject> create(AXCoreObject&, AXIsolatedTree*);
~AXIsolatedObject();
void setObjectID(AXID id) override { m_id = id; }
@@ -66,12 +66,14 @@
AXIsolatedTree* tree() const { return m_cachedTree.get(); }
AXIsolatedObject() = default;
- AXIsolatedObject(AXCoreObject&, AXIsolatedTree*, AXID parentID);
+ AXIsolatedObject(AXCoreObject&, AXIsolatedTree*);
bool isAXIsolatedObjectInstance() const override { return true; }
- void initializeAttributeData(AXCoreObject&, bool isRoot);
- void initializePlatformProperties(const AXCoreObject&, bool isRoot);
AXCoreObject* associatedAXObject() const;
+ enum class IsRoot : bool { Yes, No };
+ void initializeProperties(AXCoreObject&, IsRoot);
+ void initializePlatformProperties(const AXCoreObject&, IsRoot);
+
void setProperty(AXPropertyName, AXPropertyValueVariant&&, bool shouldRemove = false);
void setObjectProperty(AXPropertyName, AXCoreObject*);
void setObjectVectorProperty(AXPropertyName, const AccessibilityChildrenVector&);
Modified: trunk/Source/WebCore/accessibility/isolatedtree/AXIsolatedTree.cpp (293218 => 293219)
--- trunk/Source/WebCore/accessibility/isolatedtree/AXIsolatedTree.cpp 2022-04-22 13:27:43 UTC (rev 293218)
+++ trunk/Source/WebCore/accessibility/isolatedtree/AXIsolatedTree.cpp 2022-04-22 13:29:23 UTC (rev 293219)
@@ -106,7 +106,7 @@
// For this, we need the root and focused objects of the AXObject tree.
auto* axRoot = axObjectCache->getOrCreate(axObjectCache->document().view());
if (axRoot)
- tree->generateSubtree(*axRoot, nullptr);
+ tree->generateSubtree(*axRoot);
auto* axFocus = axObjectCache->focusedObjectForPage(axObjectCache->document().page());
if (axFocus)
tree->setFocusedNodeID(axFocus->objectID());
@@ -169,7 +169,7 @@
return result;
}
-void AXIsolatedTree::generateSubtree(AXCoreObject& axObject, AXCoreObject* axParent)
+void AXIsolatedTree::generateSubtree(AXCoreObject& axObject)
{
AXTRACE("AXIsolatedTree::generateSubtree"_s);
ASSERT(isMainThread());
@@ -177,36 +177,15 @@
if (!axObject.objectID().isValid())
return;
- AXID parentID = axParent ? axParent->objectID() : AXID();
- collectNodeChangesForSubtree(axObject, parentID);
+ collectNodeChangesForSubtree(axObject);
queueRemovalsAndUnresolvedChanges({ });
}
-AXID AXIsolatedTree::parentIDForObject(AXCoreObject& axObject, AXID assumedParentID)
+AXIsolatedTree::NodeChange AXIsolatedTree::nodeChangeForObject(AXCoreObject& axObject, AttachWrapper attachWrapper)
{
ASSERT(isMainThread());
- auto role = axObject.roleValue();
- if (role == AccessibilityRole::Cell || role == AccessibilityRole::RowHeader || role == AccessibilityRole::ColumnHeader) {
- // Unfortunately, table relationships don't always follow the usual model we build the isolated tree with (a simple
- // live-tree walk, calling children() and setting those children to have a parent of the caller of children()).
- // Overwrite the parentID to be that of the actual parent.
- if (auto* actualParent = axObject.parentObjectUnignored()) {
- // Expect that the parent row has been created by now.
- // If we hit this m_nodeMap.contains ASSERT, we may need to create an isolated object for `actualParent` here or elsewhere.
- ASSERT(m_nodeMap.contains(actualParent->objectID()));
- ASSERT(actualParent->roleValue() == AccessibilityRole::Row);
- return actualParent->objectID();
- }
- }
- return assumedParentID;
-}
-
-AXIsolatedTree::NodeChange AXIsolatedTree::nodeChangeForObject(AXCoreObject& axObject, AXID parentID, AttachWrapper attachWrapper)
-{
- ASSERT(isMainThread());
-
- auto object = AXIsolatedObject::create(axObject, this, parentIDForObject(axObject, parentID));
+ auto object = AXIsolatedObject::create(axObject, this);
NodeChange nodeChange { object, nullptr };
if (!object->objectID().isValid()) {
@@ -223,9 +202,9 @@
nodeChange.wrapper = axObject.wrapper();
}
- m_nodeMap.set(axObject.objectID(), ParentChildrenIDs { parentID, axObject.childrenIDs() });
+ m_nodeMap.set(axObject.objectID(), ParentChildrenIDs { nodeChange.isolatedObject->parent(), axObject.childrenIDs() });
- if (!parentID.isValid()) {
+ if (!nodeChange.isolatedObject->parent().isValid()) {
Locker locker { m_changeLogLock };
setRootNode(nodeChange.isolatedObject.ptr());
}
@@ -281,7 +260,7 @@
resolvedAppends.reserveInitialCapacity(m_unresolvedPendingAppends.size());
for (const auto& unresolvedAppend : m_unresolvedPendingAppends) {
if (auto* axObject = cache->objectFromAXID(unresolvedAppend.key))
- resolvedAppends.uncheckedAppend(nodeChangeForObject(*axObject, unresolvedAppend.value.first, unresolvedAppend.value.second));
+ resolvedAppends.uncheckedAppend(nodeChangeForObject(*axObject, unresolvedAppend.value));
}
m_unresolvedPendingAppends.clear();
}
@@ -293,19 +272,22 @@
queueRemovalsLocked(subtreeRemovals);
}
-void AXIsolatedTree::collectNodeChangesForSubtree(AXCoreObject& axObject, AXID parentID)
+void AXIsolatedTree::collectNodeChangesForSubtree(AXCoreObject& axObject)
{
AXTRACE("AXIsolatedTree::collectNodeChangesForSubtree"_s);
ASSERT(isMainThread());
SetForScope collectingNodeChanges(m_isCollectingNodeChanges, true);
- m_unresolvedPendingAppends.set(axObject.objectID(), std::make_pair(parentID, AttachWrapper::OnMainThread));
+ m_unresolvedPendingAppends.set(axObject.objectID(), AttachWrapper::OnMainThread);
auto axChildrenCopy = axObject.children();
auto axChildrenIDs = axChildrenCopy.map([&](auto& axChild) {
- collectNodeChangesForSubtree(*axChild, axObject.objectID());
+ collectNodeChangesForSubtree(*axChild);
return axChild->objectID();
});
- m_nodeMap.set(axObject.objectID(), ParentChildrenIDs { parentID, WTFMove(axChildrenIDs) });
+
+ // Update the m_nodeMap.
+ auto* axParent = axObject.parentObjectUnignored();
+ m_nodeMap.set(axObject.objectID(), ParentChildrenIDs { axParent ? axParent->objectID() : AXID(), WTFMove(axChildrenIDs) });
}
void AXIsolatedTree::updateNode(AXCoreObject& axObject)
@@ -314,19 +296,17 @@
AXLOG(&axObject);
ASSERT(isMainThread());
- auto* axParent = axObject.parentObjectUnignored();
- AXID parentID = axParent ? axParent->objectID() : AXID();
// If we update a node as the result of some side effect while collecting node changes (e.g. a role change from
// AccessibilityRenderObject::updateRoleAfterChildrenCreation), queue the append up to be resolved with the rest
// of the collected changes. This prevents us from creating two node changes for the same object.
if (m_isCollectingNodeChanges) {
- m_unresolvedPendingAppends.set(axObject.objectID(), std::make_pair(WTFMove(parentID), AttachWrapper::OnAXThread));
+ m_unresolvedPendingAppends.set(axObject.objectID(), AttachWrapper::OnAXThread);
return;
}
// Otherwise, resolve the change immediately and queue it up.
// In both cases, we can't attach the wrapper immediately on the main thread, since the wrapper could be in use
// on the AX thread (because this function updates an existing node).
- auto change = nodeChangeForObject(axObject, parentID, AttachWrapper::OnAXThread);
+ auto change = nodeChangeForObject(axObject, AttachWrapper::OnAXThread);
Locker locker { m_changeLogLock };
queueChange(change);
}
@@ -499,7 +479,7 @@
// This is a new child, add it to the tree.
AXLOG(makeString("AXID ", axAncestor->objectID().loggingString(), " gaining new subtree, starting at ID ", newChildren[i]->objectID().loggingString(), ":"));
AXLOG(newChildren[i]);
- collectNodeChangesForSubtree(*newChildren[i], axAncestor->objectID());
+ collectNodeChangesForSubtree(*newChildren[i]);
}
}
m_nodeMap.set(axAncestor->objectID(), ParentChildrenIDs { oldIDs.parentID, WTFMove(newChildrenIDs) });
Modified: trunk/Source/WebCore/accessibility/isolatedtree/AXIsolatedTree.h (293218 => 293219)
--- trunk/Source/WebCore/accessibility/isolatedtree/AXIsolatedTree.h 2022-04-22 13:27:43 UTC (rev 293218)
+++ trunk/Source/WebCore/accessibility/isolatedtree/AXIsolatedTree.h 2022-04-22 13:29:23 UTC (rev 293219)
@@ -349,7 +349,7 @@
#endif
};
- void generateSubtree(AXCoreObject&, AXCoreObject*);
+ void generateSubtree(AXCoreObject&);
void updateNode(AXCoreObject&);
void updateChildren(AXCoreObject&);
void updateNodeProperty(AXCoreObject&, AXPropertyName);
@@ -385,12 +385,9 @@
static HashMap<AXIsolatedTreeID, Ref<AXIsolatedTree>>& treeIDCache() WTF_REQUIRES_LOCK(s_cacheLock);
static HashMap<PageIdentifier, Ref<AXIsolatedTree>>& treePageCache() WTF_REQUIRES_LOCK(s_cacheLock);
- // Methods in this block are called on the main thread.
- // Computes the parent ID of the given object, which is generally the "assumed" parent ID (but not always, like in the case of tables).
- AXID parentIDForObject(AXCoreObject&, AXID assumedParentID);
enum class AttachWrapper : bool { OnMainThread, OnAXThread };
- NodeChange nodeChangeForObject(AXCoreObject&, AXID parentID, AttachWrapper = AttachWrapper::OnMainThread);
- void collectNodeChangesForSubtree(AXCoreObject&, AXID parentID);
+ NodeChange nodeChangeForObject(AXCoreObject&, AttachWrapper = AttachWrapper::OnMainThread);
+ void collectNodeChangesForSubtree(AXCoreObject&);
void queueChange(const NodeChange&) WTF_REQUIRES_LOCK(m_changeLogLock);
void queueRemovals(const Vector<AXID>&);
void queueRemovalsLocked(const Vector<AXID>&) WTF_REQUIRES_LOCK(m_changeLogLock);
@@ -413,8 +410,8 @@
// Only accessed on the main thread.
// The key is the ID of the object that will be resolved into an m_pendingAppends NodeChange.
- // The value represents the parent ID of the object, and whether the wrapper should be attached on the main thread or the AX thread.
- HashMap<AXID, std::pair<AXID, AttachWrapper>> m_unresolvedPendingAppends;
+ // The value is whether the wrapper should be attached on the main thread or the AX thread.
+ HashMap<AXID, AttachWrapper> m_unresolvedPendingAppends;
// Only accessed on the main thread.
bool m_isCollectingNodeChanges { false };
Modified: trunk/Source/WebCore/accessibility/isolatedtree/mac/AXIsolatedObjectMac.mm (293218 => 293219)
--- trunk/Source/WebCore/accessibility/isolatedtree/mac/AXIsolatedObjectMac.mm 2022-04-22 13:27:43 UTC (rev 293218)
+++ trunk/Source/WebCore/accessibility/isolatedtree/mac/AXIsolatedObjectMac.mm 2022-04-22 13:29:23 UTC (rev 293219)
@@ -32,13 +32,13 @@
namespace WebCore {
-void AXIsolatedObject::initializePlatformProperties(const AXCoreObject& object, bool isRoot)
+void AXIsolatedObject::initializePlatformProperties(const AXCoreObject& object, IsRoot isRoot)
{
setProperty(AXPropertyName::HasApplePDFAnnotationAttribute, object.hasApplePDFAnnotationAttribute());
setProperty(AXPropertyName::SpeechHint, object.speechHintAttributeValue().isolatedCopy());
setProperty(AXPropertyName::CaretBrowsingEnabled, object.caretBrowsingEnabled());
- if (isRoot)
+ if (isRoot == IsRoot::Yes)
setProperty(AXPropertyName::PreventKeyboardDOMEventDispatch, object.preventKeyboardDOMEventDispatch());
if (object.isScrollView()) {
