Summary: | AX: Using aria-activedescendant to set the active cell within a grid does not convey focus movement | ||||||||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Joyce Zhu <joyce> | ||||||||||||||||||||||||||
Component: | Accessibility | Assignee: | chris fleizach <cfleizach> | ||||||||||||||||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||||||||||||||||
Severity: | Normal | CC: | aboxhall, andresg_22, apinheiro, cfleizach, dmazzoni, ews-watchlist, jcraig, jdiggs, samuel_white, spanage, todd.kloots, tyler_w, webkit-bug-importer | ||||||||||||||||||||||||||
Priority: | P2 | Keywords: | InRadar | ||||||||||||||||||||||||||
Version: | Safari 15 | ||||||||||||||||||||||||||||
Hardware: | Mac (Intel) | ||||||||||||||||||||||||||||
OS: | macOS 11 | ||||||||||||||||||||||||||||
Attachments: |
|
Description
Joyce Zhu
2021-10-19 16:52:52 PDT
Still seeing this on 13.1 Beta (22C5044e). (In reply to Sommer Panage from comment #2) > Still seeing this on 13.1 Beta (22C5044e). In short, we don't support selected cells and selected cells changed for grids. Need to add that Created attachment 465853 [details]
Patch
Comment on attachment 465853 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=465853&action=review > Source/WebCore/accessibility/AccessibilityObject.cpp:2843 > + for (auto cell : cells()) { The type of cell is RefPtr<AXCoreObject>, and the codestyle we've been following recently uses RefPtr (no <>) over auto. > Source/WebCore/accessibility/AccessibilityObject.cpp:2848 > + if (auto activeDesc = activeDescendant()) { `activeDescendant` returns a pointer, so this should be auto*. Also, I think WebKit coding style prefers no abbreviations, so this line could maybe be: if (auto* activeDescendant = this->activeDescendant()) { > LayoutTests/accessibility/mac/grid-selected-cells.html:12 > + <tr ca="tr" role="row" aria-level="1" id="row1"> Is this `ca` attribute necessary? Same question for the other uses in this file. > LayoutTests/accessibility/mac/grid-selected-cells.html:41 > + output += "Received notification: " + notification + "\n"; You can use JS template literals here: output += `Received notification: ${notification}\n`; Created attachment 465858 [details]
Patch
Created attachment 465859 [details]
Patch
Created attachment 465860 [details]
Patch
(In reply to Tyler Wilcock from comment #5) > Comment on attachment 465853 [details] > Patch > Thanks - addressed feedback I'm concerned with the premise of this request and change. ActiveDescendant should not be equated to a selected item. The goal of ActiveDescendant is to signal that a descendant has gain a "pseudo focus", i.e., it is the point of attention, while the ancestor retains focus. The ActiveDescendant element can then be operated on from the input device (e.g., Keyboard), can be edited, selected or deselected, deleted, etc. So ActiveDescendant should not be used to automatically extend the selected items in any container. There are other means to signal that an element has been selected or deselected, and the propagation of the update in the AX tree should start from there. There are some parts of this change that are still valid, adding an exposing the selectedCells() method for tables. (In reply to chris fleizach from comment #8) > Created attachment 465860 [details] > Patch --- a/Source/WebCore/accessibility/AXObjectCache.cpp +++ b/Source/WebCore/accessibility/AXObjectCache.cpp @@ -1974,6 +1972,14 @@ void AXObjectCache::handleActiveDescendantChanged(Element& element) #endif postPlatformNotification(target, AXNotification::AXActiveDescendantChanged); + + // Table cell active descendant changes should trigger selected cell changes. + if (target->isTable() && activeDescendant->isTableCell()) { +#if ENABLE(ACCESSIBILITY_ISOLATED_TREE) + updateIsolatedTree(*object, AXNotification::AXSelectedCellsChanged); object or target? +#endif + postPlatformNotification(object, AXSelectedCellsChanged); Same as above. // HTML tables don't support these attributes. - if ([attributeName isEqualToString:NSAccessibilitySelectedColumnsAttribute] - || [attributeName isEqualToString:NSAccessibilitySelectedCellsAttribute]) + if ([attributeName isEqualToString:NSAccessibilitySelectedColumnsAttribute]) return nil; In the comment, these attributes -> this attribute. --- a/Tools/DumpRenderTree/AccessibilityUIElement.h +++ b/Tools/DumpRenderTree/AccessibilityUIElement.h + void selectedCells(Vector<AccessibilityUIElement>& elements) const; This should return a Vector instead of taking an out param. --- a/Tools/WebKitTestRunner/InjectedBundle/mac/AccessibilityUIElementMac.mm +++ b/Tools/WebKitTestRunner/InjectedBundle/mac/AccessibilityUIElementMac.mm +JSValueRef AccessibilityUIElement::selectedCells() const +{ + BEGIN_AX_OBJC_EXCEPTIONS + Vector<RefPtr<AccessibilityUIElement>> elements; + auto value = attributeValue(NSAccessibilitySelectedCellsAttribute); + if ([value isKindOfClass:[NSArray class]]) + elements = makeVector<RefPtr<AccessibilityUIElement>>(value.get()); + return makeJSArray(elements); + END_AX_OBJC_EXCEPTIONS +} Don't need the temp elements, can be written as: +JSValueRef AccessibilityUIElement::selectedCells() const +{ + BEGIN_AX_OBJC_EXCEPTIONS + auto value = attributeValue(NSAccessibilitySelectedCellsAttribute); + if ([value isKindOfClass:[NSArray class]]) + retrun makeJSArray(makeVector<RefPtr<AccessibilityUIElement>>(value.get())); + END_AX_OBJC_EXCEPTIONS + return nullptr; +} Created attachment 465900 [details]
Patch
Created attachment 465901 [details]
Patch
Created attachment 465918 [details]
Patch
Created attachment 465920 [details]
Patch
(In reply to chris fleizach from comment #14) > Created attachment 465920 [details] > Patch --- a/Source/WebCore/accessibility/AXObjectCache.cpp +++ b/Source/WebCore/accessibility/AXObjectCache.cpp + // Table cell active descendant changes should trigger selected cell changes. We should qualify this comment by saying that we only support the case where active descendant changes result in selection changes. In general, that assumption is not true, e.g., the case of a table or list that supports non-contiguous selection. + if (target->isTable() && activeDescendant->isTableCell()) { +#if ENABLE(ACCESSIBILITY_ISOLATED_TREE) + updateIsolatedTree(*object, AXNotification::AXSelectedCellsChanged); +#endif + postPlatformNotification(object, AXSelectedCellsChanged); + } Should pass target instead of object to updateIsolatedTree and postPlatformNotification ? --- a/Source/WebCore/accessibility/AccessibilityObject.cpp +++ b/Source/WebCore/accessibility/AccessibilityObject.cpp +AXCoreObject::AccessibilityChildrenVector AccessibilityObject::selectedCells() +{ ... + for (RefPtr cell : cells()) { I think it should be auto or auto& instead of RefPtr because RefPtr would create a new RefPtr to hold a pointer that is already held in a RefPtr, each item in the Vector cells() is already a RefPtr. + if (auto* activeDescendant = this->activeDescendant()) { + if (!selectedCells.contains(activeDescendant)) + selectedCells.append(activeDescendant); + } Should we add the active descendant to the selection without checking that is actually selected and a cell? And if it is a cell and selected, then we don't need this, it would be added by the for loop above. --- a/Source/WebCore/accessibility/mac/WebAccessibilityObjectWrapperMac.mm +++ b/Source/WebCore/accessibility/mac/WebAccessibilityObjectWrapperMac.mm // HTML tables don't support these attributes. - if ([attributeName isEqualToString:NSAccessibilitySelectedColumnsAttribute] - || [attributeName isEqualToString:NSAccessibilitySelectedCellsAttribute]) + if ([attributeName isEqualToString:NSAccessibilitySelectedColumnsAttribute]) return nil; in the comment, change "these attributes" to singular. --- a/Tools/DumpRenderTree/AccessibilityUIElement.h +++ b/Tools/DumpRenderTree/AccessibilityUIElement.h + void selectedCells(Vector<AccessibilityUIElement>& elements) const; Can we make it to return the Vector instead of taking an out parameter? -- a/Tools/WebKitTestRunner/InjectedBundle/mac/AccessibilityUIElementMac.mm +++ b/Tools/WebKitTestRunner/InjectedBundle/mac/AccessibilityUIElementMac.mm +JSValueRef AccessibilityUIElement::selectedCells() const +{ + BEGIN_AX_OBJC_EXCEPTIONS + Vector<RefPtr<AccessibilityUIElement>> elements; + auto value = attributeValue(NSAccessibilitySelectedCellsAttribute); + if ([value isKindOfClass:[NSArray class]]) + elements = makeVector<RefPtr<AccessibilityUIElement>>(value.get()); + return makeJSArray(elements); + END_AX_OBJC_EXCEPTIONS +} Instead of using the temp elements, can be written as: JSValueRef AccessibilityUIElement::selectedCells() const { BEGIN_AX_OBJC_EXCEPTIONS auto value = attributeValue(NSAccessibilitySelectedCellsAttribute); if ([value isKindOfClass:[NSArray class]]) return makeJSArray(makeVector<RefPtr<AccessibilityUIElement>>(value.get())); END_AX_OBJC_EXCEPTIONS return nullptr; } --- /dev/null +++ b/LayoutTests/accessibility/mac/grid-selected-cells.html +<body id="body"> Doesn't need the id. Created attachment 465987 [details]
Patch
Created attachment 465989 [details]
Patch
Created attachment 465991 [details]
Patch
Created attachment 466018 [details]
Patch
Committed 263189@main (0290a8bc9c69): <https://commits.webkit.org/263189@main> All reviewed patches have been landed. Closing bug and clearing flags on attachment 466018 [details]. Verified fixed. Tested using Mac OS 14.1.1 in both Chrome (Version 119.0.6045.199) and Safari (Version 17.1 (19616.2.9.11.7). |