AX: Per spec, aria-controls should be exposed via AXLinkedUIElements and not AXARIAControls

twilco · twilco · commit 30687a5c5877 · 2023-01-14T16:09:50.000-08:00
https://bugs.webkit.org/show_bug.cgi?id=250625 rdar://problem/104265235 Reviewed by Chris Fleizach. Per spec (https://www.w3.org/TR/core-aam-1.2), aria-controls should be exposed via AXLinkedUIElements and not AXARIAControls (which is not a specced attribute). This patch implements that and removes AXARIAControls entirely. This patch also fixes a bug where AXPropertyName::IsSelected is not updated when aria-controls changes. New testcases are added to aria-controls-with-tabs.html to cover this. We still don't pass aria-controls-with-tabs.html in ITM because we need to update AXPropertyName::IsSelected when focus changes. That change will come in a later patch. * LayoutTests/accessibility-isolated-tree/TestExpectations: * LayoutTests/accessibility/aria-controls-with-tabs-expected.txt: * LayoutTests/accessibility/aria-controls-with-tabs.html: * LayoutTests/accessibility/mac/relationships-in-frames.html: Wait for dynamic change to aria-controls to propagate to the isolated tree. * Source/WebCore/accessibility/AXLogger.cpp: (WebCore::operator<<): * Source/WebCore/accessibility/AXObjectCache.cpp: (WebCore::AXObjectCache::handleAttributeChange): (WebCore::AXObjectCache::updateIsolatedTree): * Source/WebCore/accessibility/AXObjectCache.h: * Source/WebCore/accessibility/AccessibilityRenderObject.cpp: (WebCore::AccessibilityRenderObject::linkedObjects const): * Source/WebCore/accessibility/mac/WebAccessibilityObjectWrapperMac.mm: (-[WebAccessibilityObjectWrapper accessibilityAttributeValue:]): * Tools/DumpRenderTree/mac/AccessibilityUIElementMac.mm: (AccessibilityUIElement::ariaControlsElementAtIndex): * Tools/WebKitTestRunner/InjectedBundle/mac/AccessibilityUIElementMac.mm: (WTR::AccessibilityUIElement::ariaControlsElementAtIndex): Canonical link: https://commits.webkit.org/258922@main
diff --git a/LayoutTests/accessibility-isolated-tree/TestExpectations b/LayoutTests/accessibility-isolated-tree/TestExpectations
@@ -1,9 +1,9 @@
 # Timeouts
+accessibility/aria-controls-with-tabs.html [ Timeout ]
 accessibility/frame-disconnect-textmarker-cache-crash.html [ Timeout ]
 accessibility/slider-with-lost-renderer.html [ Timeout ]
 
 # Text failures
-accessibility/aria-controls-with-tabs.html [ Failure ]
 accessibility/aria-current.html [ Failure ]
 accessibility/aria-roles.html [ Failure ]
 accessibility/dialog-showModal.html [ Failure ]
diff --git a/LayoutTests/accessibility/aria-controls-with-tabs-expected.txt b/LayoutTests/accessibility/aria-controls-with-tabs-expected.txt
@@ -1,21 +1,23 @@
-Crust
-Veges
-Test
-
-Select Crust
-
-Select Crust
-
 This tests that the aria tab item becomes selected if either aria-selected is used, or if aria-controls points to an item that contains KB focus.
 
-On success, you will see a series of "PASS" messages, followed by "TEST COMPLETE".
+PASS: tab2.isSelected === true
 
+Removing aria-controls from #tab_2
+PASS: tab2.isSelected === false
+
+Resetting #tab_2 aria-controls to be '#panel_2'
+PASS: tab2.isSelected === true
+PASS: tab1.isSelected === false
+PASS: tab2.isSelected === false
+PASS: tab1.isSelected === true
 
-PASS tab2.isSelected is true
-PASS tab1.isSelected is false
-PASS tab2.isSelected is false
-PASS tab1.isSelected is true
 PASS successfullyParsed is true
 
 TEST COMPLETE
+Crust
+Veges
+Test
 
+Select Crust
+
+Select Crust
diff --git a/LayoutTests/accessibility/aria-controls-with-tabs.html b/LayoutTests/accessibility/aria-controls-with-tabs.html
@@ -1,58 +1,65 @@
 <!DOCTYPE HTML PUBLIC "-//IETF//DTD HTML//EN">
 <html>
 <head>
-<script src="../resources/js-test-pre.js"></script>
+<script src="../resources/js-test.js"></script>
+<script src="../resources/accessibility-helper.js"></script>
 </head>
-<body id="body">
+<body>
 
 <ul id="tablist_1" role="tablist">
-<li id="tab_1" role="tab" tabindex="-1" class="">Crust</li>
-<li id="tab_2" role="tab" tabindex="-1" aria-controls="panel_2" class="">Veges</li>
+    <li id="tab_1" role="tab" tabindex="-1" class="">Crust</li>
+    <li id="tab_2" role="tab" tabindex="-1" aria-controls="panel_2" class="">Veges</li>
 </ul>
 
 <h3 tabindex=0 id="elementOutsideTabs">Test</h3>
 
 <div id="panel_1" role="tabpanel" >
-<h3 tabindex=0>Select Crust</h3>
+    <h3 tabindex=0>Select Crust</h3>
 </div>
 
 <div id="panel_2" role="tabpanel" >
-<h2 id="itemInPanel2" tabindex=0>Select Crust</h2>
+    <h2 id="itemInPanel2" tabindex=0>Select Crust</h2>
 </div>
 
-
-<p id="description"></p>
-<div id="console"></div>
-
 <script>
-
-    description("This tests that the aria tab item becomes selected if either aria-selected is used, or if aria-controls points to an item that contains KB focus.");
-
-    if (window.accessibilityController) {
-
-          var tabList = accessibilityController.accessibleElementById('tablist_1');
-          var tab1 = tabList.childAtIndex(0);
-          var tab2 = tabList.childAtIndex(1);
-
-          // we set KB focus to something in panel_2, which means that tab2 should become selected
-          // because it aria-controls panel_2
-          var panel2Item = document.getElementById("itemInPanel2");
-          panel2Item.focus();
-
-          shouldBe("tab2.isSelected", "true");
-
-          // reset KB focus and verify that neither tab is selected
-          document.getElementById("elementOutsideTabs").focus();
-          shouldBe("tab1.isSelected", "false");
-          shouldBe("tab2.isSelected", "false");
-
-          // Now we set aria-selected to be true on tab1 so that it should become selected
-          document.getElementById("tab_1").setAttribute("aria-selected", "true");
-          shouldBe("tab1.isSelected", "true");          
-    }
-
+var output = "This tests that the aria tab item becomes selected if either aria-selected is used, or if aria-controls points to an item that contains KB focus.\n\n";
+
+if (window.accessibilityController) {
+    window.jsTestIsAsync = true;
+
+    var tabList = accessibilityController.accessibleElementById('tablist_1');
+    var tab1 = tabList.childAtIndex(0);
+    var tab2 = tabList.childAtIndex(1);
+
+    var panel2Item = document.getElementById("itemInPanel2");
+    panel2Item.focus();
+    setTimeout(async () => {
+        // Set KB focus to something in #panel_2, which means that #tab2 should become selected because it aria-controls #panel_2.
+        output += await expectAsync("tab2.isSelected", "true");
+
+        output += "\nRemoving aria-controls from #tab_2\n";
+        document.getElementById("tab_2").removeAttribute("aria-controls");
+        // #tab_2 no longer aria-controls something with KB focus, so it should become unselected.
+        output += await expectAsync("tab2.isSelected", "false");
+
+        output += "\nResetting #tab_2 aria-controls to be '#panel_2'\n";
+        document.getElementById("tab_2").setAttribute("aria-controls", "panel_2");
+        output += await expectAsync("tab2.isSelected", "true");
+
+        // Reset KB focus and verify that neither tab is selected.
+        document.getElementById("elementOutsideTabs").focus();
+        output += expect("tab1.isSelected", "false");
+        output += expect("tab2.isSelected", "false");
+
+        // Now we set aria-selected to be true on tab1 so that it should become selected
+        document.getElementById("tab_1").setAttribute("aria-selected", "true");
+        output += expect("tab1.isSelected", "true");
+
+        debug(output);
+        finishJSTest();
+    }, 0);
+}
 </script>
-
-<script src="../resources/js-test-post.js"></script>
 </body>
 </html>
+
diff --git a/LayoutTests/accessibility/mac/relationships-in-frames.html b/LayoutTests/accessibility/mac/relationships-in-frames.html
@@ -28,7 +28,10 @@
             output += `aria-controls textbox->listbox : ${controlled.isEqual(listbox)}\n`;
 
             frame1.contentDocument.getElementById("controller").setAttribute("aria-controls", "listbox2");
-            controlled = controller.ariaControlsElementAtIndex(0);
+            await waitFor(() => {
+                controlled = controller.ariaControlsElementAtIndex(0);
+                return controlled.domIdentifier === "listbox2";
+            });
             output += `controller controls role after change: ${controlled.domIdentifier}\n`;
             debug(output);
             finishJSTest(); 
diff --git a/Source/WebCore/accessibility/AXLogger.cpp b/Source/WebCore/accessibility/AXLogger.cpp
@@ -415,6 +415,9 @@ TextStream& operator<<(TextStream& stream, AXObjectCache::AXNotification notific
     case AXObjectCache::AXNotification::AXColumnSpanChanged:
         stream << "AXColumnSpanChanged";
         break;
+    case AXObjectCache::AXNotification::AXControlledObjectsChanged:
+        stream << "AXControlledObjectsChanged";
+        break;
     case AXObjectCache::AXNotification::AXCurrentStateChanged:
         stream << "AXCurrentStateChanged";
         break;
diff --git a/Source/WebCore/accessibility/AXObjectCache.cpp b/Source/WebCore/accessibility/AXObjectCache.cpp
@@ -2085,6 +2085,8 @@ void AXObjectCache::handleAttributeChange(Element* element, const QualifiedName&
         postNotification(element, AXIsAtomicChanged);
     else if (attrName == aria_busyAttr)
         postNotification(element, AXObjectCache::AXElementBusyChanged);
+    else if (attrName == aria_controlsAttr)
+        postNotification(element, AXControlledObjectsChanged);
     else if (attrName == aria_valuenowAttr || attrName == aria_valuetextAttr)
         postNotification(element, AXObjectCache::AXValueChanged);
     else if (attrName == aria_labelAttr || attrName == aria_labeledbyAttr || attrName == aria_labelledbyAttr)
@@ -3759,6 +3761,7 @@ void AXObjectCache::updateIsolatedTree(const Vector<std::pair<RefPtr<Accessibili
         case AXActiveDescendantChanged:
         case AXRoleChanged:
         case AXColumnSpanChanged:
+        case AXControlledObjectsChanged:
         case AXDescribedByChanged:
         case AXDropEffectChanged:
         case AXElementBusyChanged:
diff --git a/Source/WebCore/accessibility/AXObjectCache.h b/Source/WebCore/accessibility/AXObjectCache.h
@@ -288,6 +288,7 @@ class AXObjectCache : public CanMakeWeakPtr<AXObjectCache>
         AXColumnCountChanged,
         AXColumnIndexChanged,
         AXColumnSpanChanged,
+        AXControlledObjectsChanged,
         AXCurrentStateChanged,
         AXDescribedByChanged,
         AXDisabledStateChanged,
diff --git a/Source/WebCore/accessibility/AccessibilityRenderObject.cpp b/Source/WebCore/accessibility/AccessibilityRenderObject.cpp
@@ -1072,8 +1072,6 @@ void AccessibilityRenderObject::addRadioButtonGroupMembers(AccessibilityChildren
     }
 }
 
-// Linked ui elements could be all the related radio buttons in a group
-// or an internal anchor connection.
 AXCoreObject::AccessibilityChildrenVector AccessibilityRenderObject::linkedObjects() const
 {
     auto linkedObjects = flowToObjects();
@@ -1087,6 +1085,8 @@ AXCoreObject::AccessibilityChildrenVector AccessibilityRenderObject::linkedObjec
     if (roleValue() == AccessibilityRole::RadioButton)
         addRadioButtonGroupMembers(linkedObjects);
 
+    linkedObjects.appendVector(controlledObjects());
+
     return linkedObjects;
 }
 
diff --git a/Source/WebCore/accessibility/mac/WebAccessibilityObjectWrapperMac.mm b/Source/WebCore/accessibility/mac/WebAccessibilityObjectWrapperMac.mm
@@ -232,10 +232,6 @@
 #define NSAccessibilityDocumentEncodingAttribute @"AXDocumentEncoding"
 #endif
 
-#ifndef NSAccessibilityAriaControlsAttribute
-#define NSAccessibilityAriaControlsAttribute @"AXARIAControls"
-#endif
-
 #define NSAccessibilityDOMIdentifierAttribute @"AXDOMIdentifier"
 #define NSAccessibilityDOMClassListAttribute @"AXDOMClassList"
 
@@ -2647,10 +2643,6 @@ - (id)accessibilityAttributeValue:(NSString*)attributeName
     if ([attributeName isEqualToString:NSAccessibilityDocumentEncodingAttribute])
         return backingObject->documentEncoding();
 
-    // Aria controls element
-    if ([attributeName isEqualToString:NSAccessibilityAriaControlsAttribute])
-        return makeNSArray(backingObject->controlledObjects());
-
     if ([attributeName isEqualToString:NSAccessibilityFocusableAncestorAttribute]) {
         AXCoreObject* object = backingObject->focusableAncestor();
         return object ? object->wrapper() : nil;
diff --git a/Tools/DumpRenderTree/mac/AccessibilityUIElementMac.mm b/Tools/DumpRenderTree/mac/AccessibilityUIElementMac.mm
@@ -375,13 +375,10 @@ - (void)_accessibilitySetValue:(id)value forAttribute:(NSString*)attributeName;
 
 AccessibilityUIElement AccessibilityUIElement::ariaControlsElementAtIndex(unsigned index)
 {
-    BEGIN_AX_OBJC_EXCEPTIONS
-    NSArray* ariaControls = [m_element accessibilityAttributeValue:@"AXARIAControls"];
-    if (index < [ariaControls count])
-        return [ariaControls objectAtIndex:index];
-    END_AX_OBJC_EXCEPTIONS
-    
-    return nullptr;
+    // Per spec, aria-controls is exposed via AXLinkedUIElements on the Mac.
+    // Note that a few other things are exposed via AXLinkedUIElements (aria-flowto), so this function
+    // may provide unexpected results for tests that use a combination of these attributes.
+    return linkedUIElementAtIndex(index);
 }
 
 AccessibilityUIElement AccessibilityUIElement::disclosedRowAtIndex(unsigned index)
diff --git a/Tools/WebKitTestRunner/InjectedBundle/mac/AccessibilityUIElementMac.mm b/Tools/WebKitTestRunner/InjectedBundle/mac/AccessibilityUIElementMac.mm
@@ -528,7 +528,10 @@ void setAttributeValue(id element, NSString* attribute, id value, bool synchrono
 
 RefPtr<AccessibilityUIElement> AccessibilityUIElement::ariaControlsElementAtIndex(unsigned index)
 {
-    return elementForAttributeAtIndex(@"AXARIAControls", index);
+    // Per spec, aria-controls is exposed via AXLinkedUIElements on the Mac.
+    // Note that a few other things are exposed via AXLinkedUIElements (aria-flowto), so this function
+    // may provide unexpected results for tests that use a combination of these attributes.
+    return linkedUIElementAtIndex(index);
 }
 
 RefPtr<AccessibilityUIElement> AccessibilityUIElement::ariaDetailsElementAtIndex(unsigned index)

Original file line number	Diff line number	Diff line change
`@@ -1072,8 +1072,6 @@ void AccessibilityRenderObject::addRadioButtonGroupMembers(AccessibilityChildren`
`1072`	`1072`	`}`
`1073`	`1073`	`}`
`1074`	`1074`
`1075`		`-// Linked ui elements could be all the related radio buttons in a group`
`1076`		`-// or an internal anchor connection.`
`1077`	`1075`	`AXCoreObject::AccessibilityChildrenVector AccessibilityRenderObject::linkedObjects() const`
`1078`	`1076`	`{`
`1079`	`1077`	`auto linkedObjects = flowToObjects();`
`@@ -1087,6 +1085,8 @@ AXCoreObject::AccessibilityChildrenVector AccessibilityRenderObject::linkedObjec`
`1087`	`1085`	`if (roleValue() == AccessibilityRole::RadioButton)`
`1088`	`1086`	`addRadioButtonGroupMembers(linkedObjects);`
`1089`	`1087`
	`1088`	`+ linkedObjects.appendVector(controlledObjects());`
	`1089`	`+`
`1090`	`1090`	`return linkedObjects;`
`1091`	`1091`	`}`
`1092`	`1092`