248572 – AX: Include AXKeyShortcutsValue in accessibilityAttributeNames when there is an aria-keyshortcuts attribute

RESOLVED FIXED 248572

AX: Include AXKeyShortcutsValue in accessibilityAttributeNames when there is an aria-keyshortcuts attribute

https://bugs.webkit.org/show_bug.cgi?id=248572

Summary AX: Include AXKeyShortcutsValue in accessibilityAttributeNames when there is ...

Tommy McHugh

Reported 2022-11-30 16:15:24 PST

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

Attachments
Patch (16.95 KB, patch) 2022-11-30 16:36 PST, Tommy McHugh	no flags	Details Formatted Diff Diff
Patch (19.70 KB, patch) 2022-12-01 11:08 PST, Tommy McHugh	no flags	Details Formatted Diff Diff
Patch (19.68 KB, patch) 2022-12-01 11:54 PST, Tommy McHugh	no flags	Details Formatted Diff Diff
Patch (20.31 KB, patch) 2022-12-01 15:58 PST, Tommy McHugh	no flags	Details Formatted Diff Diff
Show Obsolete (3) View All Add attachment proposed patch, testcase, etc.

Radar WebKit Bug Importer

Comment 1 2022-11-30 16:15:36 PST

<rdar://problem/102834471>

Tommy McHugh

Comment 2 2022-11-30 16:36:03 PST

Created attachment 463819 [details] Patch

Andres Gonzalez

Comment 3 2022-12-01 06:39:34 PST

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

Andres Gonzalez

Comment 4 2022-12-01 06:46:14 PST

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

Tommy McHugh

Comment 5 2022-12-01 11:08:54 PST

Created attachment 463839 [details] Patch

Tommy McHugh

Comment 6 2022-12-01 11:12:51 PST

(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

Darin Adler

Comment 7 2022-12-01 11:34:17 PST

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.

Tommy McHugh

Comment 8 2022-12-01 11:54:51 PST

Created attachment 463841 [details] Patch

Tommy McHugh

Comment 9 2022-12-01 15:58:47 PST

Created attachment 463846 [details] Patch

EWS

Comment 10 2022-12-01 23:28:15 PST

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

Note You need to log in before you can comment on or make changes to this bug.

Status RESOLVED

Resolution FIXED

Priority P2

Severity Normal

Classification Unclassified

Version Safari 16

Hardware All

OS All

Product WebKit

Component Accessibility

Assignee

Nobody

Reported

2022-11-30 16:15 PST

Modified

2022-12-01 23:28 PST History

CC List

11 users Show

URL

Keywords InRadar

Depends on

Blocks