Bug 271674 – AX: Mac layout tests shouldn't access attributes that aren't exposed for that element

• Component: Accessibility
• Status: RESOLVED FIXED

[Dominic Mazzoni]

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.

[Radar WebKit Bug Importer]

<rdar://problem/125373694>

[Andres Gonzalez]

(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.

[Dominic Mazzoni]

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.

[Dominic Mazzoni]

Created attachment 470607 [details]
Patch

[Andres Gonzalez]

(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?

[Andres Gonzalez]

(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();

[Andres Gonzalez]

(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];
 }

[Andres Gonzalez]

(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?
'

[Andres Gonzalez]

(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`;
     }

[Dominic Mazzoni]

(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.

[Dominic Mazzoni]

Created attachment 470640 [details]
Patch

[Dominic Mazzoni]

Created attachment 470648 [details]
Patch

[Dominic Mazzoni]

Created attachment 470658 [details]
Patch

[Dominic Mazzoni]

OK, tests pass. Any final thoughts?

Remember, I want to remove the remaining exceptions in follow-ups.

[Dominic Mazzoni]

Created attachment 470662 [details]
Patch

[EWS]

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].