Bug 231993 – AX: Using aria-activedescendant to set the active cell within a grid does not convey focus movement

• Component: Accessibility
• Status: RESOLVED FIXED

[Joyce Zhu]

When navigating within a grid via non-VoiceOver keys (i.e. those provided by the web page), the change in active cell is not announced by VoiceOver if the element with `role="grid"` uses `aria-activedescendant` instead of explicit `element.focus()` calls on individual cell DOM nodes.

Steps to Reproduce:

1. Open the sample page at https://jscholes.github.io/basic-grid-activedescendant.html with Safari. Focus will be automatically placed on a link before the grid.
2. Activate VoiceOver.
3. Disable VoiceOver Quick Nav if necessary by pressing Left and Right Arrow together until VO announces: "Quick Nav off".
4. Move to the grid, by pressing either Tab or VO+Right Arrow. VO will announce the grid name, role and size, as well as the value of the first cell: "Hello".
5. Move to the second cell on the first and only row, by pressing the Right Arrow key without modifiers.

Expected Behavior:

Since `aria-activedescendant` can be used to manage focus (https://www.w3.org/TR/wai-aria-1.2/#aria-activedescendant), VoiceOver should announce the change in active cell, specifically by conveying the text of the new active cell ("World") and the coordinates of the new active cell ("Column 2 of 2").

Actual Behavior:

VoiceOver doesn't speak or otherwise indicate the change.

------------------------------------------------------------

Other notes which may be helpful:

* Following the same repro steps in Google Chrome 94.0.4606.81, with the same version of macOS (11.6) results in the output listed in the "Expected Behavior" section. That is, users will hear: "World. Column 2 of 2".

* While users can navigate the grid using VoiceOver-specific keys (e.g. VO+Right Arrow to move to the next column), this is not adequate for web applications which rely on users operating the grid with key handlers provided by the webpage, e.g. to load data on demand in large grids.

* While navigating the grid using VoiceOver-specific keys in Safari or Chrome, the VoiceOver hint text consistently indicates that the user is "currently on a text element inside a cell". However, when the grid initially receives focus, or when navigating via page-provided keys in Google Chrome rather than VoiceOver-specific ones, the hint text indicates the role of the current element as a "cell" directly. This may or may not have something to do with the bug being reported.

[Radar WebKit Bug Importer]

<rdar://problem/84439987>

[Sommer Panage]

Still seeing this on 13.1 Beta (22C5044e).

[chris fleizach]

(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

[chris fleizach]

Created attachment 465853 [details]
Patch

[Tyler Wilcock]

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

[chris fleizach]

Created attachment 465858 [details]
Patch

[chris fleizach]

Created attachment 465859 [details]
Patch

[chris fleizach]

Created attachment 465860 [details]
Patch

[chris fleizach]

(In reply to Tyler Wilcock from comment #5)
> Comment on attachment 465853 [details]
> Patch
> 
Thanks - addressed feedback

[Andres Gonzalez]

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;
+}

[chris fleizach]

Created attachment 465900 [details]
Patch

[chris fleizach]

Created attachment 465901 [details]
Patch

[chris fleizach]

Created attachment 465918 [details]
Patch

[chris fleizach]

Created attachment 465920 [details]
Patch

[Andres Gonzalez]

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

[chris fleizach]

Created attachment 465987 [details]
Patch

[chris fleizach]

Created attachment 465989 [details]
Patch

[chris fleizach]

Created attachment 465991 [details]
Patch

[chris fleizach]

Created attachment 466018 [details]
Patch

[EWS]

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

[Todd Kloots]

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