AX: AXPropertyName::{ColumnIndexRange, RowIndexRange} properties are …

…never updated https://bugs.webkit.org/show_bug.cgi?id=270496 rdar://problem/124046227 Reviewed by Chris Fleizach. This can cause ATs to miss table content if they use the rowIndexRange and columnIndexRange APIs as inputs to the cellForColumnAndRow API. Fix this by posting a notification when table cells modify their row / column index. This patch also adds a new postNotification overload to avoid unnecessary null-checking and verbosity of passing a hardcoded `nullptr Document*` argument to postNotification(AccessibilityObject*, Document*, AXNotification, PostTarget = PostTarget::Element). Existing notifications AXColumnIndexChanged and AXRowIndexChanged are repurposed to report this dynamic change, and two new notifications (AXARIAColumnIndexChanged and AXARIARowIndexChanged) are added to support what the former two notifications used to do (as they were only fired when aria-colindex and aria-rowindex changed). * LayoutTests/accessibility/dynamic-aria-hidden-cell-expected.txt: Added. * LayoutTests/accessibility/dynamic-aria-hidden-cell.html: Added. * Source/WebCore/accessibility/AXLogger.cpp: (WebCore::operator<<): * Source/WebCore/accessibility/AXObjectCache.cpp: (WebCore::AXObjectCache::valueChanged): (WebCore::AXObjectCache::columnIndexChanged): (WebCore::AXObjectCache::rowIndexChanged): (WebCore::AXObjectCache::postNotification): (WebCore::AXObjectCache::onTextSecurityChanged): (WebCore::AXObjectCache::onTitleChange): (WebCore::AXObjectCache::onValidityChange): (WebCore::AXObjectCache::handleRoleChanged): Take a reference rather than a pointer as all callsites have access to a reference. (WebCore::AXObjectCache::handleAttributeChange): (WebCore::AXObjectCache::updateIsolatedTree): (WebCore::AXObjectCache::selectedTextRangeTimerFired): (WebCore::AXObjectCache::onWidgetVisibilityChanged): * Source/WebCore/accessibility/AXObjectCache.h: * Source/WebCore/accessibility/AccessibilityObject.cpp: (WebCore::AccessibilityObject::updateRole): * Source/WebCore/accessibility/AccessibilityRenderObject.cpp: (WebCore::AccessibilityRenderObject::updateRoleAfterChildrenCreation): * Source/WebCore/accessibility/AccessibilityTable.cpp: (WebCore::AccessibilityTable::recomputeIsExposable): (WebCore::AccessibilityTable::columnCount): * Source/WebCore/accessibility/AccessibilityTableCell.cpp: (WebCore::AccessibilityTableCell::setRowIndex): (WebCore::AccessibilityTableCell::setColumnIndex): * Source/WebCore/accessibility/AccessibilityTableCell.h: (WebCore::AccessibilityTableCell::setRowIndex): Deleted. (WebCore::AccessibilityTableCell::setColumnIndex): Deleted. Canonical link: https://commits.webkit.org/275710@main
WebKit · Mar 5, 2024 · 6e896e7 · 6e896e7
1 parent a1c9c2d
commit 6e896e7
Show file tree

Hide file tree

Showing 10 changed files with 198 additions and 24 deletions.
diff --git a/LayoutTests/accessibility/dynamic-aria-hidden-cell-expected.txt b/LayoutTests/accessibility/dynamic-aria-hidden-cell-expected.txt
@@ -0,0 +1,29 @@
+This test ensures we report the right information for tables with presentational cells.
+
+PASS: table.rowCount === 2
+PASS: table.columnCount === 2
+PASS: accessibilityController.accessibleElementById('th0-1').rowIndexRange() === '{0, 1}'
+PASS: accessibilityController.accessibleElementById('th0-1').columnIndexRange() === '{0, 1}'
+PASS: accessibilityController.accessibleElementById('th0-2').rowIndexRange() === '{0, 1}'
+PASS: accessibilityController.accessibleElementById('th0-2').columnIndexRange() === '{1, 1}'
+PASS: accessibilityController.accessibleElementById('td1-1').rowIndexRange() === '{1, 1}'
+PASS: accessibilityController.accessibleElementById('td1-1').columnIndexRange() === '{0, 1}'
+PASS: accessibilityController.accessibleElementById('td1-2').rowIndexRange() === '{1, 1}'
+PASS: accessibilityController.accessibleElementById('td1-2').columnIndexRange() === '{1, 1}'
+PASS: table.columnCount === 3
+PASS: accessibilityController.accessibleElementById('th0-0-hidden').rowIndexRange() === '{0, 1}'
+PASS: accessibilityController.accessibleElementById('th0-0-hidden').columnIndexRange() === '{0, 1}'
+PASS: accessibilityController.accessibleElementById('th0-1').rowIndexRange() === '{0, 1}'
+PASS: accessibilityController.accessibleElementById('th0-1').columnIndexRange() === '{1, 1}'
+PASS: accessibilityController.accessibleElementById('td1-0-hidden').rowIndexRange() === '{1, 1}'
+PASS: accessibilityController.accessibleElementById('td1-0-hidden').columnIndexRange() === '{0, 1}'
+PASS: accessibilityController.accessibleElementById('td1-1').rowIndexRange() === '{1, 1}'
+PASS: accessibilityController.accessibleElementById('td1-1').columnIndexRange() === '{1, 1}'
+PASS: accessibilityController.accessibleElementById('td1-2').rowIndexRange() === '{1, 1}'
+PASS: accessibilityController.accessibleElementById('td1-2').columnIndexRange() === '{2, 1}'
+
+PASS successfullyParsed is true
+
+TEST COMPLETE
+Header one	Header two	Header three
+A	B	C
diff --git a/LayoutTests/accessibility/dynamic-aria-hidden-cell.html b/LayoutTests/accessibility/dynamic-aria-hidden-cell.html
@@ -0,0 +1,75 @@
+<!DOCTYPE HTML PUBLIC "-//IETF//DTD HTML//EN">
+<html>
+<head>
+<script src="../resources/accessibility-helper.js"></script>
+<script src="../resources/js-test.js"></script>
+</head>
+<body>
+
+<table id="table" tabindex="0">
+    <thead id="thead">
+        <tr id="tr0">
+            <th id="th0-0-hidden" aria-hidden="true">Header one</th>
+            <th id="th0-1" tabindex="0" aria-colindex="1"><strong>Header two</strong></th>
+            <th id="th0-2" tabindex="0" aria-colindex="2"><strong>Header three</strong></th>
+        </tr>
+    </thead>
+    <tbody id="tbody">
+        <tr id="tr1">
+            <td id="td1-0-hidden" aria-hidden="true">A</td>
+            <td id="td1-1" tabindex="0" aria-colindex="1">B</td>
+            <td id="td1-2" tabindex="0" aria-colindex="2">C</td>
+        </tr>
+    </tbody>
+</table>
+
+<script>
+var output = "This test ensures we report the right information for tables with presentational cells.\n\n";
+
+if (window.accessibilityController) {
+    window.jsTestIsAsync = true;
+
+    var table = accessibilityController.accessibleElementById("table");
+    output += expect("table.rowCount", "2");
+    output += expect("table.columnCount", "2");
+
+    output += expect("accessibilityController.accessibleElementById('th0-1').rowIndexRange()", "'{0, 1}'");
+    output += expect("accessibilityController.accessibleElementById('th0-1').columnIndexRange()", "'{0, 1}'");
+
+    output += expect("accessibilityController.accessibleElementById('th0-2').rowIndexRange()", "'{0, 1}'");
+    output += expect("accessibilityController.accessibleElementById('th0-2').columnIndexRange()", "'{1, 1}'");
+
+    output += expect("accessibilityController.accessibleElementById('td1-1').rowIndexRange()", "'{1, 1}'");
+    output += expect("accessibilityController.accessibleElementById('td1-1').columnIndexRange()", "'{0, 1}'");
+
+    output += expect("accessibilityController.accessibleElementById('td1-2').rowIndexRange()", "'{1, 1}'");
+    output += expect("accessibilityController.accessibleElementById('td1-2').columnIndexRange()", "'{1, 1}'");
+
+    document.getElementById("th0-0-hidden").removeAttribute("aria-hidden");
+    document.getElementById("td1-0-hidden").removeAttribute("aria-hidden");
+    setTimeout(async function() {
+        output += await expectAsync("table.columnCount", "3");
+
+        output += await expectAsync("accessibilityController.accessibleElementById('th0-0-hidden').rowIndexRange()", "'{0, 1}'");
+        output += await expectAsync("accessibilityController.accessibleElementById('th0-0-hidden').columnIndexRange()", "'{0, 1}'");
+
+        output += await expectAsync("accessibilityController.accessibleElementById('th0-1').rowIndexRange()", "'{0, 1}'");
+        output += await expectAsync("accessibilityController.accessibleElementById('th0-1').columnIndexRange()", "'{1, 1}'");
+
+        output += await expectAsync("accessibilityController.accessibleElementById('td1-0-hidden').rowIndexRange()", "'{1, 1}'");
+        output += await expectAsync("accessibilityController.accessibleElementById('td1-0-hidden').columnIndexRange()", "'{0, 1}'");
+
+        output += expect("accessibilityController.accessibleElementById('td1-1').rowIndexRange()", "'{1, 1}'");
+        output += expect("accessibilityController.accessibleElementById('td1-1').columnIndexRange()", "'{1, 1}'");
+
+        output += expect("accessibilityController.accessibleElementById('td1-2').rowIndexRange()", "'{1, 1}'");
+        output += expect("accessibilityController.accessibleElementById('td1-2').columnIndexRange()", "'{2, 1}'");
+
+        debug(output);
+        finishJSTest();
+    }, 0);
+}
+</script>
+</body>
+</html>
+
diff --git a/Source/WebCore/accessibility/AXLogger.cpp b/Source/WebCore/accessibility/AXLogger.cpp
@@ -558,6 +558,12 @@ TextStream& operator<<(TextStream& stream, AXObjectCache::AXNotification notific
     case AXObjectCache::AXNotification::AXAutofillTypeChanged:
         stream << "AXAutofillTypeChanged";
         break;
+    case AXObjectCache::AXNotification::AXARIAColumnIndexChanged:
+        stream << "AXARIAColumnIndexChanged";
+        break;
+    case AXObjectCache::AXNotification::AXARIARowIndexChanged:
+        stream << "AXARIARowIndexChanged";
+        break;
     case AXObjectCache::AXNotification::AXBrailleLabelChanged:
         stream << "AXBrailleLabelChanged";
         break;

diff --git a/Source/WebCore/accessibility/AXObjectCache.cpp b/Source/WebCore/accessibility/AXObjectCache.cpp
@@ -1474,9 +1474,21 @@ void AXObjectCache::childrenChanged(AccessibilityObject* object)
 
 void AXObjectCache::valueChanged(Element* element)
 {
-    postNotification(element, AXNotification::AXValueChanged);
+    postNotification(element, AXValueChanged);
 }
 
+#if ENABLE(ACCESSIBILITY_ISOLATED_TREE)
+void AXObjectCache::columnIndexChanged(AccessibilityTableCell& cell)
+{
+    postNotification(cell, AXColumnIndexChanged);
+}
+
+void AXObjectCache::rowIndexChanged(AccessibilityTableCell& cell)
+{
+    postNotification(cell, AXRowIndexChanged);
+}
+#endif // ENABLE(ACCESSIBILITY_ISOLATED_TREE)
+
 void AXObjectCache::notificationPostTimerFired()
 {
     AXTRACE(makeString("AXObjectCache::notificationPostTimerFired 0x"_s, hex(reinterpret_cast<uintptr_t>(this))));
@@ -1608,6 +1620,19 @@ void AXObjectCache::postNotification(AccessibilityObject* object, Document* docu
         m_notificationPostTimer.startOneShot(0_s);
 }
 
+void AXObjectCache::postNotification(AccessibilityObject& object, AXNotification notification)
+{
+    AXTRACE(makeString("AXObjectCache::postNotification 0x"_s, hex(reinterpret_cast<uintptr_t>(this))));
+    AXLOG(std::make_pair(&object, notification));
+    ASSERT(isMainThread());
+
+    stopCachingComputedObjectAttributes();
+
+    m_notificationsToPost.append(std::make_pair(&object, notification));
+    if (!m_notificationPostTimer.isActive())
+        m_notificationPostTimer.startOneShot(0_s);
+}
+
 void AXObjectCache::checkedStateChanged(Node* node)
 {
     postNotification(node, AXCheckedStateChanged);
@@ -1810,20 +1835,20 @@ void AXObjectCache::onSelectedChanged(Node* node)
 void AXObjectCache::onTextSecurityChanged(HTMLInputElement& inputElement)
 {
 #if ENABLE(ACCESSIBILITY_ISOLATED_TREE)
-    postNotification(get(&inputElement), nullptr, AXTextSecurityChanged);
+    postNotification(get(&inputElement), AXTextSecurityChanged);
 #else
     UNUSED_PARAM(inputElement);
 #endif
 }
 
 void AXObjectCache::onTitleChange(Document& document)
 {
-    postNotification(get(&document), nullptr, AXTextChanged);
+    postNotification(get(&document), AXTextChanged);
 }
 
 void AXObjectCache::onValidityChange(Element& element)
 {
-    postNotification(get(&element), nullptr, AXInvalidStatusChanged);
+    postNotification(get(&element), AXInvalidStatusChanged);
 }
 
 void AXObjectCache::onTextCompositionChange(Node& node, CompositionState compositionState, bool valueChanged, const String& text, size_t position, bool handlingAcceptedCandidate)
@@ -2418,13 +2443,13 @@ void AXObjectCache::handleRoleChanged(Element* element, const AtomString& oldVal
     object->updateRole();
 }
 
-void AXObjectCache::handleRoleChanged(AccessibilityObject* axObject)
+void AXObjectCache::handleRoleChanged(AccessibilityObject& axObject)
 {
     stopCachingComputedObjectAttributes();
-    axObject->recomputeIsIgnored();
+    axObject.recomputeIsIgnored();
 
 #if ENABLE(ACCESSIBILITY_ISOLATED_TREE)
-    postNotification(axObject, nullptr, AXRoleChanged);
+    postNotification(axObject, AXRoleChanged);
 #endif
 }
 
@@ -2533,7 +2558,7 @@ void AXObjectCache::handleAttributeChange(Element* element, const QualifiedName&
     else if (attrName == langAttr)
         updateIsolatedTree(get(element), AXLanguageChanged);
     else if (attrName == nameAttr)
-        postNotification(get(element), nullptr, AXNameChanged);
+        postNotification(get(element), AXNameChanged);
     else if (attrName == placeholderAttr)
         postNotification(element, AXPlaceholderChanged);
     else if (attrName == hrefAttr || attrName == srcAttr)
@@ -2594,7 +2619,7 @@ void AXObjectCache::handleAttributeChange(Element* element, const QualifiedName&
         postNotification(element, AXColumnCountChanged);
         deferRecomputeTableIsExposed(dynamicDowncast<HTMLTableElement>(element));
     } else if (attrName == aria_colindexAttr) {
-        postNotification(element, AXColumnIndexChanged);
+        postNotification(element, AXARIAColumnIndexChanged);
         recomputeParentTableProperties(element, TableProperty::Exposed);
     } else if (attrName == aria_colspanAttr) {
         postNotification(element, AXColumnSpanChanged);
@@ -2619,7 +2644,7 @@ void AXObjectCache::handleAttributeChange(Element* element, const QualifiedName&
     else if (attrName == aria_placeholderAttr)
         postNotification(element, AXPlaceholderChanged);
     else if (attrName == aria_rowindexAttr) {
-        postNotification(element, AXRowIndexChanged);
+        postNotification(element, AXARIARowIndexChanged);
         recomputeParentTableProperties(element, { TableProperty::CellSlots, TableProperty::Exposed });
     }
     else if (attrName == aria_valuemaxAttr)
@@ -4320,6 +4345,12 @@ void AXObjectCache::updateIsolatedTree(const Vector<std::pair<RefPtr<Accessibili
         case AXAutofillTypeChanged:
             tree->queueNodeUpdate(notification.first->objectID(), { AXPropertyName::ValueAutofillButtonType });
             break;
+        case AXARIAColumnIndexChanged:
+            tree->queueNodeUpdate(notification.first->objectID(), { AXPropertyName::AXColumnIndex });
+            break;
+        case AXARIARowIndexChanged:
+            tree->queueNodeUpdate(notification.first->objectID(), { AXPropertyName::AXRowIndex });
+            break;
         case AXBrailleLabelChanged:
             tree->queueNodeUpdate(notification.first->objectID(), { AXPropertyName::BrailleLabel });
             break;
@@ -4340,8 +4371,6 @@ void AXObjectCache::updateIsolatedTree(const Vector<std::pair<RefPtr<Accessibili
             tree->queueNodeUpdate(notification.first->objectID(), { AXPropertyName::AXColumnCount });
             break;
         case AXColumnIndexChanged:
-            tree->queueNodeUpdate(notification.first->objectID(), { AXPropertyName::AXColumnIndex });
-            break;
         case AXColumnSpanChanged:
             tree->queueNodeUpdate(notification.first->objectID(), { AXPropertyName::ColumnIndexRange });
             break;
@@ -4407,8 +4436,6 @@ void AXObjectCache::updateIsolatedTree(const Vector<std::pair<RefPtr<Accessibili
             tree->queueNodeUpdate(notification.first->objectID(), { AXPropertyName::RoleDescription });
             break;
         case AXRowIndexChanged:
-            tree->queueNodeUpdate(notification.first->objectID(), { AXPropertyName::AXRowIndex });
-            break;
         case AXRowSpanChanged:
             tree->queueNodeUpdate(notification.first->objectID(), { AXPropertyName::RowIndexRange });
             break;
@@ -5182,7 +5209,7 @@ void AXObjectCache::selectedTextRangeTimerFired()
     if (m_lastDebouncedTextRangeObject.isValid()) {
         for (auto* axObject = objectForID(m_lastDebouncedTextRangeObject); axObject; axObject = axObject->parentObject()) {
             if (axObject->isTextControl())
-                postNotification(axObject, nullptr, AXSelectedTextChanged);
+                postNotification(*axObject, AXSelectedTextChanged);
         }
     }
 
@@ -5205,8 +5232,7 @@ void AXObjectCache::processQueuedIsolatedNodeUpdates()
 void AXObjectCache::onWidgetVisibilityChanged(RenderWidget* widget)
 {
 #if ENABLE(ACCESSIBILITY_ISOLATED_TREE)
-    if (auto* axWidget = get(widget))
-        postNotification(axWidget, nullptr, AXVisibilityChanged);
+    postNotification(get(widget), AXVisibilityChanged);
 #else
     UNUSED_PARAM(widget);
 #endif

diff --git a/Source/WebCore/accessibility/AXObjectCache.h b/Source/WebCore/accessibility/AXObjectCache.h
@@ -209,7 +209,13 @@ class AXObjectCache final : public CanMakeWeakPtr<AXObjectCache>, public CanMake
     void valueChanged(Element*);
     void checkedStateChanged(Node*);
     void autofillTypeChanged(Node*);
-    void handleRoleChanged(AccessibilityObject*);
+    void handleRoleChanged(AccessibilityObject&);
+
+#if ENABLE(ACCESSIBILITY_ISOLATED_TREE)
+    void columnIndexChanged(AccessibilityTableCell&);
+    void rowIndexChanged(AccessibilityTableCell&);
+#endif
+
     // Called when a RenderObject is created for an Element. Depending on the
     // presence of a RenderObject, we may have instatiated an AXRenderObject or
     // an AXNodeObject. This occurs when an Element with no renderer is
@@ -354,6 +360,8 @@ class AXObjectCache final : public CanMakeWeakPtr<AXObjectCache>, public CanMake
         AXAnnouncementRequested,
         AXAutocorrectionOccured,
         AXAutofillTypeChanged,
+        AXARIAColumnIndexChanged,
+        AXARIARowIndexChanged,
         AXBrailleLabelChanged,
         AXBrailleRoleDescriptionChanged,
         AXCellSlotsChanged,
@@ -448,6 +456,12 @@ class AXObjectCache final : public CanMakeWeakPtr<AXObjectCache>, public CanMake
     void postNotification(RenderObject*, AXNotification, PostTarget = PostTarget::Element);
     void postNotification(Node*, AXNotification, PostTarget = PostTarget::Element);
     void postNotification(AccessibilityObject*, Document*, AXNotification, PostTarget = PostTarget::Element);
+    void postNotification(AccessibilityObject* object, AXNotification notification)
+    {
+        if (object)
+            postNotification(*object, notification);
+    }
+    void postNotification(AccessibilityObject&, AXNotification);
     // Requests clients to announce to the user the given message in the way they deem appropriate.
     WEBCORE_EXPORT void announce(const String&);
 

diff --git a/Source/WebCore/accessibility/AccessibilityObject.cpp b/Source/WebCore/accessibility/AccessibilityObject.cpp
@@ -2868,7 +2868,7 @@ void AccessibilityObject::updateRole()
     m_role = determineAccessibilityRole();
     if (previousRole != m_role) {
         if (auto* cache = axObjectCache())
-            cache->handleRoleChanged(this);
+            cache->handleRoleChanged(*this);
     }
 }
 

diff --git a/Source/WebCore/accessibility/AccessibilityRenderObject.cpp b/Source/WebCore/accessibility/AccessibilityRenderObject.cpp
@@ -2564,7 +2564,7 @@ void AccessibilityRenderObject::updateRoleAfterChildrenCreation()
 
     if (role != m_role) {
         if (auto* cache = axObjectCache())
-            cache->handleRoleChanged(this);
+            cache->handleRoleChanged(*this);
     }
 }
 

diff --git a/Source/WebCore/accessibility/AccessibilityTable.cpp b/Source/WebCore/accessibility/AccessibilityTable.cpp
@@ -404,7 +404,7 @@ void AccessibilityTable::recomputeIsExposable()
     if (previouslyExposable != m_isExposable) {
         // A table's role value is dependent on whether it's exposed, so notify the cache this has changed.
         if (auto* cache = axObjectCache())
-            cache->handleRoleChanged(this);
+            cache->handleRoleChanged(*this);
 
         // Before resetting our existing children, possibly losing references to them, ensure we update their role (since a table cell's role is dependent on whether its parent table is exposable).
         updateChildrenRoles();
@@ -845,7 +845,7 @@ unsigned AccessibilityTable::columnCount()
 {
     updateChildrenIfNecessary();
 
-    return m_columns.size();    
+    return m_columns.size();
 }
 
 unsigned AccessibilityTable::rowCount()

diff --git a/Source/WebCore/accessibility/AccessibilityTableCell.cpp b/Source/WebCore/accessibility/AccessibilityTableCell.cpp
@@ -328,6 +328,30 @@ void AccessibilityTableCell::ensureIndexesUpToDate() const
         parentTable->ensureCellIndexesUpToDate();
 }
 
+void AccessibilityTableCell::setRowIndex(unsigned index)
+{
+    if (m_rowIndex == index)
+        return;
+    m_rowIndex = index;
+
+#if ENABLE(ACCESSIBILITY_ISOLATED_TREE)
+    if (auto* cache = axObjectCache())
+        cache->rowIndexChanged(*this);
+#endif
+}
+
+void AccessibilityTableCell::setColumnIndex(unsigned index)
+{
+    if (m_columnIndex == index)
+        return;
+    m_columnIndex = index;
+
+#if ENABLE(ACCESSIBILITY_ISOLATED_TREE)
+    if (auto* cache = axObjectCache())
+        cache->columnIndexChanged(*this);
+#endif
+}
+
 std::pair<unsigned, unsigned> AccessibilityTableCell::rowIndexRange() const
 {
     ensureIndexesUpToDate();

diff --git a/Source/WebCore/accessibility/AccessibilityTableCell.h b/Source/WebCore/accessibility/AccessibilityTableCell.h
@@ -67,8 +67,8 @@ class AccessibilityTableCell : public AccessibilityRenderObject {
     void resetEffectiveRowSpan() { m_effectiveRowSpan = 1; }
     void setAXColIndexFromRow(int index) { m_axColIndexFromRow = index; }
 
-    void setRowIndex(unsigned index) { m_rowIndex = index; }
-    void setColumnIndex(unsigned index) { m_columnIndex = index; }
+    void setRowIndex(unsigned);
+    void setColumnIndex(unsigned);
 
 #if USE(ATSPI)
     int axColumnSpan() const;