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.
<rdar://problem/125373694>
(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.
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.
Created attachment 470607 [details] Patch
(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?
(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();
(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]; }
(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? '
(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`; }
(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.
Created attachment 470640 [details] Patch
Created attachment 470648 [details] Patch
Created attachment 470658 [details] Patch
OK, tests pass. Any final thoughts? Remember, I want to remove the remaining exceptions in follow-ups.
Created attachment 470662 [details] Patch
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].