Bug 271674 - AX: Mac layout tests shouldn't access attributes that aren't exposed for that element
Summary: AX: Mac layout tests shouldn't access attributes that aren't exposed for that...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Accessibility (show other bugs)
Version: Safari Technology Preview
Hardware: All All
: P2 Normal
Assignee: Dominic Mazzoni
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2024-03-25 14:19 PDT by Dominic Mazzoni
Modified: 2024-04-02 12:27 PDT (History)
9 users (show)

See Also:


Attachments
Patch (22.78 KB, patch)
2024-03-26 09:34 PDT, Dominic Mazzoni
no flags Details | Formatted Diff | Diff
Patch (22.88 KB, patch)
2024-03-28 10:38 PDT, Dominic Mazzoni
no flags Details | Formatted Diff | Diff
Patch (22.97 KB, patch)
2024-03-28 14:44 PDT, Dominic Mazzoni
no flags Details | Formatted Diff | Diff
Patch (22.42 KB, patch)
2024-03-28 20:51 PDT, Dominic Mazzoni
no flags Details | Formatted Diff | Diff
Patch (22.44 KB, patch)
2024-03-29 08:49 PDT, Dominic Mazzoni
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Dominic Mazzoni 2024-03-25 14:19:44 PDT
AccessibilityUIElementMac accesses attributes by calling accessibilityAttributeValue on the underlying element. However, this doesn't mimic how VoiceOver and most other Mac AT actually work, they all check accessibilityAttributeNames first and don't bother querying for any attribute not contained in that list. So that leads to WebKit tests passing even though the attributes it's testing being effectively hidden from AT in practice.

By checking accessibilityAttributeNames in AccessibilityUIElementMac, we can catch these potential bugs sooner.
Comment 1 Radar WebKit Bug Importer 2024-03-25 14:19:51 PDT
<rdar://problem/125373694>
Comment 2 Andres Gonzalez 2024-03-25 14:24:40 PDT
(In reply to Radar WebKit Bug Importer from comment #1)
> <rdar://problem/125373694>

We expose a growing number of API attributes that are not to be used by AT, but instead for unit test purpose.
Comment 3 Dominic Mazzoni 2024-03-26 08:49:51 PDT
Yes, this could be a good opportunity to clean up these attributes too, and ensure that the ones that are intended to be internal-only really aren't exposed.

Right now some of them have a check to ensure they're only used for testing, and others appear to me to be testing-only but it isn't enforced.

There's at least one case I found of a testing-only attribute that VoiceOver is using, probably a misunderstanding.
Comment 4 Dominic Mazzoni 2024-03-26 09:34:52 PDT
Created attachment 470607 [details]
Patch
Comment 5 Andres Gonzalez 2024-03-27 07:28:44 PDT
(In reply to Dominic Mazzoni from comment #4)
> Created attachment 470607 [details]
> Patch

diff --git a/Source/WebCore/accessibility/AccessibilityObject.cpp b/Source/WebCore/accessibility/AccessibilityObject.cpp
index e98934f55a18..d6115806bd71 100644
--- a/Source/WebCore/accessibility/AccessibilityObject.cpp
+++ b/Source/WebCore/accessibility/AccessibilityObject.cpp
@@ -3449,6 +3449,9 @@ bool AccessibilityObject::supportsExpanded() const
     if (popoverTargetElement())
         return true;

+    if (is<HTMLDetailsElement>(node()))
+        return true;
+
     switch (roleValue()) {
     case AccessibilityRole::Details:
         return true;

AG: wouldn't an HTMLDetailsElement have AccessibilityRole::Details and thus be covered by the above case?
Comment 6 Andres Gonzalez 2024-03-27 07:51:47 PDT
(In reply to Dominic Mazzoni from comment #4)
> Created attachment 470607 [details]
> Patch

diff --git a/Source/WebCore/accessibility/mac/WebAccessibilityObjectWrapperMac.mm b/Source/WebCore/accessibility/mac/WebAccessibilityObjectWrapperMac.mm
index 0dc006087fe6..78674c421d2f 100644
--- a/Source/WebCore/accessibility/mac/WebAccessibilityObjectWrapperMac.mm
+++ b/Source/WebCore/accessibility/mac/WebAccessibilityObjectWrapperMac.mm
@@ -823,7 +823,8 @@ - (NSArray*)_additionalAccessibilityAttributeNames:(const RefPtr<AXCoreObject>&)
     if (backingObject->isScrollbar()
         || backingObject->isRadioGroup()
         || backingObject->isSplitter()
-        || backingObject->isToolbar())
+        || backingObject->isToolbar()
+        || backingObject->roleValue() == AccessibilityRole::HorizontalRule)
         [additional addObject:NSAccessibilityOrientationAttribute];

     if (backingObject->supportsDragging())
@@ -905,6 +906,24 @@ - (NSArray*)_additionalAccessibilityAttributeNames:(const RefPtr<AXCoreObject>&)
     if (backingObject->supportsExpandedTextValue())
         [additional addObject:NSAccessibilityExpandedTextValueAttribute];

+    if (!backingObject->brailleLabel().isEmpty())
+        [additional addObject:NSAccessibilityBrailleLabelAttribute];
+
+    if (!backingObject->brailleRoleDescription().isEmpty())
+        [additional addObject:NSAccessibilityBrailleRoleDescriptionAttribute];
+
+    if (backingObject->detailedByObjects().size())
+        [additional addObject:@"AXDetailsElements"];
+
+    if (backingObject->errorMessageObjects().size())
+        [additional addObject:@"AXErrorMessageElements"];
+
+    if (!backingObject->keyShortcuts().isEmpty())
+        [additional addObject:NSAccessibilityKeyShortcutsAttribute];
+
+    if (backingObject->titleUIElement())
+        [additional addObject:NSAccessibilityTitleUIElementAttribute];
+
 #if ENABLE(ACCESSIBILITY_ISOLATED_TREE)
     if (AXObjectCache::isIsolatedTreeEnabled())
         [additional addObject:NSAccessibilityRelativeFrameAttribute];
@@ -979,15 +998,6 @@ ALLOW_DEPRECATED_IMPLEMENTATIONS_END
         [tempArray addObject:NSAccessibilityLinkRelationshipTypeAttribute];
         return tempArray;
     }();
-    static NeverDestroyed<RetainPtr<NSArray>> commonMenuAttrs = @[
-        NSAccessibilityRoleAttribute,
-        NSAccessibilityRoleDescriptionAttribute,
-        NSAccessibilityChildrenAttribute,
-        NSAccessibilityParentAttribute,
-        NSAccessibilityEnabledAttribute,
-        NSAccessibilityPositionAttribute,
-        NSAccessibilitySizeAttribute,
-    ];
     static NeverDestroyed webAreaAttrs = [] {
         auto tempArray = adoptNS([[NSMutableArray alloc] initWithArray:attributes.get().get()]);
         // WebAreas should not expose AXSubrole.
@@ -1012,11 +1022,11 @@ ALLOW_DEPRECATED_IMPLEMENTATIONS_END
         [tempArray addObject:NSAccessibilitySelectedTextRangeAttribute];
         [tempArray addObject:NSAccessibilityVisibleCharacterRangeAttribute];
         [tempArray addObject:NSAccessibilityInsertionPointLineNumberAttribute];
-        [tempArray addObject:NSAccessibilityTitleUIElementAttribute];
         [tempArray addObject:NSAccessibilityAccessKeyAttribute];
         [tempArray addObject:NSAccessibilityRequiredAttribute];
         [tempArray addObject:NSAccessibilityInvalidAttribute];
         [tempArray addObject:NSAccessibilityPlaceholderValueAttribute];
+        [tempArray addObject:NSAccessibilityValueAutofillAvailableAttribute];
         return tempArray;
     }();
     static NeverDestroyed listAttrs = [] {
@@ -1024,7 +1034,6 @@ ALLOW_DEPRECATED_IMPLEMENTATIONS_END
         [tempArray addObject:NSAccessibilitySelectedChildrenAttribute];
         [tempArray addObject:NSAccessibilityVisibleChildrenAttribute];
         [tempArray addObject:NSAccessibilityOrientationAttribute];
-        [tempArray addObject:NSAccessibilityTitleUIElementAttribute];
         return tempArray;
     }();
     static NeverDestroyed listBoxAttrs = [] {
@@ -1041,25 +1050,22 @@ ALLOW_DEPRECATED_IMPLEMENTATIONS_END
         [tempArray addObject:NSAccessibilityMaxValueAttribute];
         [tempArray addObject:NSAccessibilityOrientationAttribute];
         [tempArray addObject:NSAccessibilityValueDescriptionAttribute];
-        [tempArray addObject:NSAccessibilityTitleUIElementAttribute];
         return tempArray;
     }();
     static NeverDestroyed menuBarAttrs = [] {
-        auto tempArray = adoptNS([[NSMutableArray alloc] initWithArray:commonMenuAttrs.get().get()]);
+        auto tempArray = adoptNS([[NSMutableArray alloc] initWithArray:attributes.get().get()]);
         [tempArray addObject:NSAccessibilitySelectedChildrenAttribute];
-        [tempArray addObject:NSAccessibilityTitleUIElementAttribute];
         [tempArray addObject:NSAccessibilityOrientationAttribute];
         return tempArray;
     }();
     static NeverDestroyed menuAttrs = [] {
-        auto tempArray = adoptNS([[NSMutableArray alloc] initWithArray:commonMenuAttrs.get().get()]);
+        auto tempArray = adoptNS([[NSMutableArray alloc] initWithArray:attributes.get().get()]);
         [tempArray addObject:NSAccessibilitySelectedChildrenAttribute];
-        [tempArray addObject:NSAccessibilityTitleUIElementAttribute];
         [tempArray addObject:NSAccessibilityOrientationAttribute];
         return tempArray;
     }();
     static NeverDestroyed menuItemAttrs = [] {
-        auto tempArray = adoptNS([[NSMutableArray alloc] initWithArray:commonMenuAttrs.get().get()]);
+        auto tempArray = adoptNS([[NSMutableArray alloc] initWithArray:attributes.get().get()]);
         [tempArray addObject:NSAccessibilityTitleAttribute];
         [tempArray addObject:NSAccessibilityDescriptionAttribute];
         [tempArray addObject:NSAccessibilityHelpAttribute];
@@ -1088,7 +1094,6 @@ ALLOW_DEPRECATED_IMPLEMENTATIONS_END
         NSAccessibilityChildrenAttribute,
     ];
     static NeverDestroyed<RetainPtr<NSArray>> sharedControlAttrs = @[
-        NSAccessibilityTitleUIElementAttribute,
         NSAccessibilityAccessKeyAttribute,
         NSAccessibilityRequiredAttribute,
         NSAccessibilityInvalidAttribute,
@@ -1107,8 +1112,8 @@ ALLOW_DEPRECATED_IMPLEMENTATIONS_END
         auto tempArray = adoptNS([[NSMutableArray alloc] initWithArray:attributes.get().get()]);
         // Buttons should not expose AXValue.
         [tempArray removeObject:NSAccessibilityValueAttribute];
-        [tempArray addObject:NSAccessibilityTitleUIElementAttribute];
         [tempArray addObject:NSAccessibilityAccessKeyAttribute];
+        [tempArray addObject:NSAccessibilityInvalidAttribute];
         return tempArray;
     }();
     static NeverDestroyed comboBoxAttrs = [] {
@@ -1137,6 +1142,7 @@ ALLOW_DEPRECATED_IMPLEMENTATIONS_END
         [tempArray addObject:NSAccessibilityARIAColumnCountAttribute];
         [tempArray addObject:NSAccessibilityARIARowCountAttribute];
         [tempArray addObject:NSAccessibilitySelectedCellsAttribute];
+        [tempArray addObject:@"AXTableLevel"];
         return tempArray;
     }();
     static NeverDestroyed tableRowAttrs = [] {
@@ -1164,7 +1170,7 @@ ALLOW_DEPRECATED_IMPLEMENTATIONS_END
     }();
     static NeverDestroyed groupAttrs = [] {
         auto tempArray = adoptNS([[NSMutableArray alloc] initWithArray:attributes.get().get()]);
-        [tempArray addObject:NSAccessibilityTitleUIElementAttribute];
+        [tempArray addObject:NSAccessibilityInlineTextAttribute];
         return tempArray;
     }();
     static NeverDestroyed inputImageAttrs = [] {
@@ -1174,7 +1180,6 @@ ALLOW_DEPRECATED_IMPLEMENTATIONS_END
     }();
     static NeverDestroyed secureFieldAttributes = [] {
         auto tempArray = adoptNS([[NSMutableArray alloc] initWithArray:attributes.get().get()]);
-        [tempArray addObject:NSAccessibilityTitleUIElementAttribute];
         [tempArray addObject:NSAccessibilityRequiredAttribute];
         [tempArray addObject:NSAccessibilityInvalidAttribute];
         [tempArray addObject:NSAccessibilityPlaceholderValueAttribute];
@@ -1185,6 +1190,7 @@ ALLOW_DEPRECATED_IMPLEMENTATIONS_END
         [tempArray addObject:NSAccessibilityTabsAttribute];
         [tempArray addObject:NSAccessibilityContentsAttribute];
         [tempArray addObject:NSAccessibilityOrientationAttribute];
+        [tempArray addObject:NSAccessibilitySelectedChildrenAttribute];
         return tempArray;
     }();
     static NeverDestroyed outlineAttrs = [] {
@@ -1193,6 +1199,7 @@ ALLOW_DEPRECATED_IMPLEMENTATIONS_END
         [tempArray addObject:NSAccessibilityRowsAttribute];
         [tempArray addObject:NSAccessibilityColumnsAttribute];
         [tempArray addObject:NSAccessibilityOrientationAttribute];
+        [tempArray addObject:NSAccessibilitySelectedChildrenAttribute];
         return tempArray;
     }();
     static NeverDestroyed outlineRowAttrs = [] {
@@ -1227,6 +1234,7 @@ ALLOW_DEPRECATED_IMPLEMENTATIONS_END
     static NeverDestroyed staticTextAttrs = [] {
         auto tempArray = adoptNS([[NSMutableArray alloc] initWithArray:attributes.get().get()]);
         [tempArray addObject:NSAccessibilityIntersectionWithSelectionRangeAttribute];
+        [tempArray addObject:NSAccessibilityVisibleCharacterRangeAttribute];
         return tempArray;
     }();

@@ -1248,7 +1256,7 @@ ALLOW_DEPRECATED_IMPLEMENTATIONS_END
         objectAttributes = anchorAttrs.get().get();
     else if (backingObject->isImage())
         objectAttributes = imageAttrs.get().get();
-    else if (backingObject->isTable() && backingObject->isExposable())
+    else if ((backingObject->isTable() && backingObject->isExposable()) || backingObject->isAccessibilityARIAGridInstance())

AG: isAccessibilityARIAGridInstance returns false for all isolated objects as it should. We cannot call is***Instance methods here, or equivalent is<T>.

         objectAttributes = tableAttrs.get().get();
     else if (backingObject->isTableColumn())
         objectAttributes = tableColAttrs.get().get();
@@ -1272,7 +1280,7 @@ ALLOW_DEPRECATED_IMPLEMENTATIONS_END
         objectAttributes = listBoxAttrs.get().get();
     else if (backingObject->isList())
         objectAttributes = listAttrs.get().get();
-    else if (backingObject->isProgressIndicator() || backingObject->isSlider())
+    else if (backingObject->isProgressIndicator() || backingObject->isSlider() || backingObject->isSplitter())
         objectAttributes = rangeAttrs.get().get();
     // These are processed in order because an input image is a button, and a button is a control.
     else if (backingObject->isInputImage())
@@ -1282,7 +1290,7 @@ ALLOW_DEPRECATED_IMPLEMENTATIONS_END
     else if (backingObject->isControl())
         objectAttributes = controlAttrs.get().get();

-    else if (backingObject->isGroup() || backingObject->isListItem())
+    else if (backingObject->isGroup() || backingObject->isListItem() || backingObject->roleValue() == AccessibilityRole::Figure)
         objectAttributes = groupAttrs.get().get();
     else if (backingObject->isTabList())
         objectAttributes = tabListAttrs.get().get();
Comment 7 Andres Gonzalez 2024-03-27 08:02:48 PDT
(In reply to Dominic Mazzoni from comment #4)
> Created attachment 470607 [details]
> Patch

diff --git a/Tools/WebKitTestRunner/InjectedBundle/mac/AccessibilityUIElementMac.mm b/Tools/WebKitTestRunner/InjectedBundle/mac/AccessibilityUIElementMac.mm
index 209dc473fd36..47af3b5dd84a 100644
--- a/Tools/WebKitTestRunner/InjectedBundle/mac/AccessibilityUIElementMac.mm
+++ b/Tools/WebKitTestRunner/InjectedBundle/mac/AccessibilityUIElementMac.mm
@@ -162,6 +162,51 @@ static id attributeValue(id element, NSString *attribute)
     if ([attribute isEqual:NSAccessibilityFocusedUIElementAttribute] && [element respondsToSelector:@selector(accessibilityFocusedUIElement)])
         return [element accessibilityFocusedUIElement];

+    // These are internal APIs used by DRT/WKTR; tests are allowed to use them but we don't want to advertise them.
+    static NeverDestroyed<RetainPtr<NSArray>> internalAttributes = @[
+        @"AXARIAPressedIsPresent",
+        @"AXARIARole",
+        @"AXAutocompleteValue",
+        @"AXClickPoint",
+        @"AXControllerFor",
+        @"AXControllers",
+        @"AXDRTSpeechAttribute",
+        @"AXDateTimeComponentsType",
+        @"AXDescribedBy",
+        @"AXDescriptionFor",
+        @"AXDetailsFor",
+        @"AXErrorMessageFor",
+        @"AXFlowFrom",
+        @"AXIsInCell",
+        @"AXIsInDescriptionListDetail",
+        @"AXIsInDescriptionListTerm",
+        @"AXIsIndeterminate",
+        @"AXIsMultiSelectable",
+        @"AXIsOnScreen",
+        @"AXLabelFor",
+        @"AXLabelledBy",
+        @"AXLineRectsAndText",
+        @"AXOwners",
+        @"AXStringValue",
+        @"AXValueAutofillType",
+
+        // FIXME: these shouldn't be here, but removing one of these causes tests to fail.
+        @"AXARIACurrent",
+        @"AXARIALive",
+        @"AXDescription",
+        @"AXKeyShortcutsValue",
+        @"AXOrientation",
+        @"AXPopupValue",
+        @"AXRequired",
+        @"AXSortDirection",
+        @"AXValue",
+        @"AXVisibleCharacterRange",
+    ];
+
+    NSArray<NSString *> *supportedAttributes = [element accessibilityAttributeNames];
+    if (![supportedAttributes containsObject:attribute] && ![internalAttributes.get() containsObject:attribute] && ![attribute isEqualToString:NSAccessibilityRoleAttribute])

AG: shouldn't NSAccessibilityRoleAttribute be a supported attribute?

+        return nil;
+
     return [element accessibilityAttributeValue:attribute];
 }
Comment 8 Andres Gonzalez 2024-03-27 08:07:34 PDT
(In reply to Dominic Mazzoni from comment #4)
> Created attachment 470607 [details]
> Patch

diff --git a/LayoutTests/accessibility/mac/aria-pressed-button-attributes-expected.txt b/LayoutTests/accessibility/mac/aria-pressed-button-attributes-expected.txt
index b6c2b6532c9d..a46f81d819b8 100644
--- a/LayoutTests/accessibility/mac/aria-pressed-button-attributes-expected.txt
+++ b/LayoutTests/accessibility/mac/aria-pressed-button-attributes-expected.txt
@@ -2,34 +2,34 @@ This test ensures a role=button element with aria-pressed has the right attribut

 #button1: AXRole: AXCheckBox
 #button1: AXSubrole: AXToggle
-#button1: AXValue: 1
+#button1: AXTitle: Foo
 #button1: AXARIAPressedIsPresent: true

 #button2: AXRole: AXCheckBox
 #button2: AXSubrole: AXToggle
-#button2: AXValue: 0
+#button2: AXTitle: Bar
 #button2: AXARIAPressedIsPresent: true

 #button-without-initial-press: AXRole: AXButton
 #button-without-initial-press: AXSubrole:
-#button-without-initial-press: AXValue:
+#button-without-initial-press: AXTitle: Baz
 #button-without-initial-press: AXARIAPressedIsPresent: false

 Toggling aria-pressed state on both buttons.

 #button1: AXRole: AXCheckBox
 #button1: AXSubrole: AXToggle
-#button1: AXValue: 0
+#button1: AXTitle: Foo
 #button1: AXARIAPressedIsPresent: true

 #button2: AXRole: AXCheckBox
 #button2: AXSubrole: AXToggle
-#button2: AXValue: 1
+#button2: AXTitle: Bar
 #button2: AXARIAPressedIsPresent: true

 #button-without-initial-press: AXRole: AXButton
 #button-without-initial-press: AXSubrole:
-#button-without-initial-press: AXValue:
+#button-without-initial-press: AXTitle: Baz
 #button-without-initial-press: AXARIAPressedIsPresent: true

AG: Don't we need to return AXValue for buttons any more? How does VO get the pressed state?
'
Comment 9 Andres Gonzalez 2024-03-27 08:10:56 PDT
(In reply to Dominic Mazzoni from comment #4)
> Created attachment 470607 [details]
> Patch

diff --git a/LayoutTests/accessibility/mac/aria-pressed-button-attributes.html b/LayoutTests/accessibility/mac/aria-pressed-button-attributes.html
index 0e847151071b..3490b0e003f2 100644
--- a/LayoutTests/accessibility/mac/aria-pressed-button-attributes.html
+++ b/LayoutTests/accessibility/mac/aria-pressed-button-attributes.html
@@ -18,7 +18,7 @@
         const element = accessibilityController.accessibleElementById(id);
         testOutput += `#${id}: ${element.role}\n`;
         testOutput += `#${id}: ${element.subrole}\n`;
-        testOutput += `#${id}: ${element.stringValue}\n`;
+        testOutput += `#${id}: ${element.title}\n`;

AG: assuming that we still want value for buttons, this should be element.value instead of element.stringValue?

         testOutput += `#${id}: AXARIAPressedIsPresent: ${element.boolAttributeValue("AXARIAPressedIsPresent")}\n\n`;
     }
Comment 10 Dominic Mazzoni 2024-03-28 09:25:20 PDT
(In reply to Andres Gonzalez from comment #5)
> +    if (is<HTMLDetailsElement>(node()))
> +        return true;
> 
> AG: wouldn't an HTMLDetailsElement have AccessibilityRole::Details and thus
> be covered by the above case?

No, because someone can add role="group" to it. It doesn't cease to be a details element.

(In reply to Andres Gonzalez from comment #7)
> +    if (![supportedAttributes containsObject:attribute] &&
> ![internalAttributes.get() containsObject:attribute] && ![attribute
> isEqualToString:NSAccessibilityRoleAttribute])
> 
> AG: shouldn't NSAccessibilityRoleAttribute be a supported attribute?

I'm having trouble with embedded PDFs, for some reason they're not exposing AXRole as an attribute name even though they obviously implement it.
Comment 11 Dominic Mazzoni 2024-03-28 10:38:13 PDT
Created attachment 470640 [details]
Patch
Comment 12 Dominic Mazzoni 2024-03-28 14:44:53 PDT
Created attachment 470648 [details]
Patch
Comment 13 Dominic Mazzoni 2024-03-28 20:51:10 PDT
Created attachment 470658 [details]
Patch
Comment 14 Dominic Mazzoni 2024-03-28 22:34:18 PDT
OK, tests pass. Any final thoughts?

Remember, I want to remove the remaining exceptions in follow-ups.
Comment 15 Dominic Mazzoni 2024-03-29 08:49:54 PDT
Created attachment 470662 [details]
Patch
Comment 16 EWS 2024-04-02 12:27:25 PDT
Committed 276955@main (b6b219bd567e): <https://commits.webkit.org/276955@main>

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