When we have an element that implements and provides AXKeyShortcutsValue through aria-keyshortcuts we should include AXKeyShortcutsValue in accessibilityAttributeNames so that VoiceOver knows whether this is actually a supported attribute
<rdar://problem/102834471>
Created attachment 463819 [details] Patch
(In reply to Tommy McHugh from comment #2) > Created attachment 463819 [details] > Patch --- a/Source/WebCore/accessibility/mac/WebAccessibilityObjectWrapperMac.mm +++ b/Source/WebCore/accessibility/mac/WebAccessibilityObjectWrapperMac.mm +#ifndef NSAccessibilityKeyShortcutsValue +#define NSAccessibilityKeyShortcutsValue @"AXKeyShortcutsValue" +#endif While naming these constants, please drop the "Value", it doesn't add anything to the name, just makes it longer. Also The NSAccessibility names have traditionally ended in "Attribute", so if we keep tradition this should be: +#ifndef NSAccessibilityKeyShortcutsAttribute +#define NSAccessibilityKeyShortcutsAttribute @"AXKeyShortcuts" +#endif --- a/Source/WebCore/accessibility/AXObjectCache.cpp +++ b/Source/WebCore/accessibility/AXObjectCache.cpp @@ -3747,6 +3749,9 @@ void AXObjectCache::updateIsolatedTree(const Vector<std::pair<RefPtr<Accessibili + case AXKeyShortcutsChanged: + tree->updateNodeProperties(*notification.first, { AXPropertyName::KeyShortcutsValue, AXPropertyName::SupportsKeyShortcutsValue }); + break; Does this compile? It should be: + tree->updateNodeProperties(*notification.first, AXPropertyName::KeyShortcutsValue); --- a/Source/WebCore/accessibility/AccessibilityObjectInterface.h +++ b/Source/WebCore/accessibility/AccessibilityObjectInterface.h @@ -321,6 +321,7 @@ public: + virtual bool supportsKeyShortcutsValue() const = 0; Drop Value from the name: supportsKeyShortcuts() --- a/Source/WebCore/accessibility/isolatedtree/AXIsolatedTree.cpp +++ b/Source/WebCore/accessibility/isolatedtree/AXIsolatedTree.cpp @@ -498,6 +498,9 @@ void AXIsolatedTree::updateNodeProperties(AXCoreObject& axObject, const Vector<A + case AXPropertyName::SupportsKeyShortcutsValue: + propertyMap.set(AXPropertyName::SupportsKeyShortcutsValue, axObject.supportsKeyShortcutsValue()); + break; It should be: + case AXPropertyName::KeyShortcuts: + propertyMap.set(AXPropertyName::SupportsKeyShortcutsValue, axObject.supportsKeyShortcutsValue()); and + propertyMap.set(AXPropertyName::KeyShortcuts, axObject.keyShortcutsValue().isolatedCopy()); + break; --- a/Source/WebCore/accessibility/isolatedtree/AXIsolatedTree.h +++ b/Source/WebCore/accessibility/isolatedtree/AXIsolatedTree.h @@ -245,6 +245,7 @@ enum class AXPropertyName : uint16_t { + SupportsKeyShortcutsValue, Drop Value from the name. --- /dev/null +++ b/LayoutTests/accessibility/aria-keyshortcuts.html +<body id="body"> No need for id in <body>. + <div id="test1" role="button" tabindex="0">X</div> + <div id="test2" role="button" tabindex="0" aria-keyshortcuts="Shift+2">X</div> + <div id="test3" role="button" tabindex="0" aria-keyshortcuts="Shift+3 Option+4">X</div> No need for tabindex in any of these elements. + + output += expect("axItem2.stringAttributeValue('AXKeyShortcutsValue')", "'Shift+2'"); + output += expect("axItem3.stringAttributeValue('AXKeyShortcutsValue')", "'Shift+3 Option+4'"); + Should we test retrieving the shortcuts for test1 even when it is not supported to make sure we fail gracefully?
(In reply to Tommy McHugh from comment #2) > Created attachment 463819 [details] > Patch Also in the test: + output += "Update aria-keyshortcuts to Shift+Command+1 for #test3\n"; + document.getElementById("test3").setAttribute("aria-keyshortcuts", "Shift+Command+1"); + await waitFor(() => axItem3.isAttributeSupported('AXKeyShortcutsValue') === true); I don't think this would wait for anything since test3 already supported the attribute before the change.
Created attachment 463839 [details] Patch
(In reply to Andres Gonzalez from comment #3) > (In reply to Tommy McHugh from comment #2) > > Created attachment 463819 [details] > > Patch > > --- a/Source/WebCore/accessibility/mac/WebAccessibilityObjectWrapperMac.mm > +++ b/Source/WebCore/accessibility/mac/WebAccessibilityObjectWrapperMac.mm > > +#ifndef NSAccessibilityKeyShortcutsValue > +#define NSAccessibilityKeyShortcutsValue @"AXKeyShortcutsValue" > +#endif > > While naming these constants, please drop the "Value", it doesn't add > anything to the name, just makes it longer. Also The NSAccessibility names > have traditionally ended in "Attribute", so if we keep tradition this should > be: > > +#ifndef NSAccessibilityKeyShortcutsAttribute > +#define NSAccessibilityKeyShortcutsAttribute @"AXKeyShortcuts" > +#endif Sounds good! Updated these to remove value. I don't think we can change the actual attribute name though, its seems to be specced that way https://github.com/web-platform-tests/wpt/pull/36082/files
Comment on attachment 463839 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=463839&action=review > Source/WebCore/accessibility/AccessibilityObjectInterface.h:1071 > + virtual const String keyShortcuts() const = 0; Not new in this patch: The use of "const String" for the return type here does not make much sense. It’s like using "const int" for a function that returns an integer.
Created attachment 463841 [details] Patch
Created attachment 463846 [details] Patch
Committed 257274@main (f7a887f4c257): <https://commits.webkit.org/257274@main> All reviewed patches have been landed. Closing bug and clearing flags on attachment 463846 [details].