Bug 263162 – AX: Submit Bug button on Bugzilla is not accessible via VoiceOver VO-Right navigation

• Component: Accessibility
• Status: RESOLVED FIXED

[Tyler Wilcock]

...

[Radar WebKit Bug Importer]

<rdar://problem/116960271>

[Tyler Wilcock]

rdar://115783249

[Tyler Wilcock]

Created attachment 468212 [details]
Patch

[Tyler Wilcock]

Created attachment 468213 [details]
Patch

[chris fleizach]

Comment on attachment 468213 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=468213&action=review

> Source/WebCore/accessibility/AccessibilityTableCell.cpp:443
> +        unsigned rowsUntilEnd = 0;

If we can't find either row or section should we limit row span to just 1?

> Source/WebCore/accessibility/AccessibilityTableCell.cpp:448
> +                if (auto* style = row.computedStyle(); style && style->display() != DisplayType::None)

What about aria-hidden here

[Tyler Wilcock]

> > Source/WebCore/accessibility/AccessibilityTableCell.cpp:448
> > +                if (auto* style = row.computedStyle(); style && style->display() != DisplayType::None)
> 
> What about aria-hidden here
Ah yes, that's a flaw in this approach, and role="presentation" rows, and probably other things. Let me explore a different approach.

[Tyler Wilcock]

Created attachment 468240 [details]
Patch

[Tyler Wilcock]

Created attachment 468241 [details]
Patch

[Tyler Wilcock]

Created attachment 468242 [details]
Patch

[Tyler Wilcock]

Created attachment 468243 [details]
Patch

[chris fleizach]

Comment on attachment 468243 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=468243&action=review

> Source/WebCore/accessibility/AXObjectCache.cpp:4352
> +#if ENABLE(ACCESSIBILITY_ISOLATED_TREE)

do we need to do the same thing for colspan changes?

[Tyler Wilcock]

(In reply to chris fleizach from comment #11)
> Comment on attachment 468243 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=468243&action=review
> 
> > Source/WebCore/accessibility/AXObjectCache.cpp:4352
> > +#if ENABLE(ACCESSIBILITY_ISOLATED_TREE)
> 
> do we need to do the same thing for colspan changes?
We don't. The reason I have to move rowspan change handling around to this new deferRowspanChange function is because after this patch, AXPropertyName::RowIndexRange depends on the final layout of the table, which is not dirtied until performDeferredCacheUpdate is called. Now we perform rowspan isolated tree updates after the table has been re-laid out, ensuring a correct result.

Colspan does not have the same problem because it is entirely local to the row, and thus does not depend on the rest of table to be laid out.

[Tyler Wilcock]

Created attachment 468253 [details]
Patch

[Tyler Wilcock]

Created attachment 468255 [details]
Patch

[Andres Gonzalez]

(In reply to Tyler Wilcock from comment #14)
> Created attachment 468255 [details]
> Patch

diff --git a/Source/WebCore/accessibility/AXLogger.cpp b/Source/WebCore/accessibility/AXLogger.cpp
index 2374c4cd6b60..9866fc7c2473 100644
--- a/Source/WebCore/accessibility/AXLogger.cpp
+++ b/Source/WebCore/accessibility/AXLogger.cpp
@@ -209,6 +209,7 @@ void AXLogger::log(const String& collectionName, const AXObjectCache::DeferredCo
         [&size] (const WeakHashSet<Element, WeakPtrImplWithEventTargetData>& typedCollection) { size = typedCollection.computeSize(); },
         [&size] (const WeakHashSet<HTMLTableElement, WeakPtrImplWithEventTargetData>& typedCollection) { size = typedCollection.computeSize(); },
         [&size] (const WeakHashSet<AccessibilityTable>& typedCollection) { size = typedCollection.computeSize(); },
+        [&size] (const WeakHashSet<AccessibilityTableCell>& typedCollection) { size = typedCollection.computeSize(); },
         [&size] (const WeakListHashSet<Node, WeakPtrImplWithEventTargetData>& typedCollection) { size = typedCollection.computeSize(); },
         [&size] (const WeakHashMap<Element, String, WeakPtrImplWithEventTargetData>& typedCollection) { size = typedCollection.computeSize(); },
         [] (auto&) {
diff --git a/Source/WebCore/accessibility/AXObjectCache.cpp b/Source/WebCore/accessibility/AXObjectCache.cpp
index 1892c12432b4..6a10e365edb7 100644
--- a/Source/WebCore/accessibility/AXObjectCache.cpp
+++ b/Source/WebCore/accessibility/AXObjectCache.cpp
@@ -1311,6 +1311,13 @@ void AXObjectCache::handleRecomputeCellSlots(AccessibilityTable& axTable)
 #endif
 }

+#if ENABLE(ACCESSIBILITY_ISOLATED_TREE)
+void AXObjectCache::handleRowspanChanged(AccessibilityTableCell& axCell)
+{
+    updateIsolatedTree(axCell, AXRowSpanChanged);
+}
+#endif
+
 void AXObjectCache::handleMenuOpened(Node* node)
 {
     if (!node || !node->renderer() || !nodeHasRole(node, "menu"_s))
@@ -2456,7 +2463,7 @@ void AXObjectCache::handleAttributeChange(Element* element, const QualifiedName&
         deferModalChange(element);
         recomputeIsIgnored(element->parentNode());
     } else if (attrName == rowspanAttr) {
-        postNotification(element, AXRowSpanChanged);
+        deferRowspanChange(get(element));
         recomputeParentTableProperties(element, TableProperty::CellSlots);
     } else if (attrName == colspanAttr) {
         postNotification(element, AXColumnSpanChanged);
@@ -2582,7 +2589,7 @@ void AXObjectCache::handleAttributeChange(Element* element, const QualifiedName&
     else if (attrName == aria_rowcountAttr)
         handleRowCountChanged(get(element), element ? &element->document() : nullptr);
     else if (attrName == aria_rowspanAttr) {
-        postNotification(element, AXRowSpanChanged);
+        deferRowspanChange(get(element));
         recomputeParentTableProperties(element, { TableProperty::CellSlots, TableProperty::Exposed });
     } else if (attrName == aria_sortAttr)
         postNotification(element, AXObjectCache::AXSortDirectionChanged);
@@ -3939,6 +3946,14 @@ void AXObjectCache::performDeferredCacheUpdate()
     });
     m_deferredRecomputeTableCellSlotsList.clear();

+#if ENABLE(ACCESSIBILITY_ISOLATED_TREE)
+    AXLOGDeferredCollection("RowspanChangesList"_s, m_deferredRowspanChanges);
+    m_deferredRowspanChanges.forEach([this] (auto& axCell) {
+        handleRowspanChanged(axCell);
+    });
+    m_deferredRowspanChanges.clear();
+#endif
+
     AXLOGDeferredCollection("TextChangedList"_s, m_deferredTextChangedList);
     for (auto& node : m_deferredTextChangedList)
         handleTextChanged(getOrCreate(&node));
@@ -4133,6 +4148,9 @@ void AXObjectCache::updateIsolatedTree(const Vector<std::pair<RefPtr<Accessibili
         case AXColumnIndexChanged:
             tree->updateNodeProperty(*notification.first, AXPropertyName::AXColumnIndex);
             break;
+        case AXColumnSpanChanged:
+            tree->updateNodeProperty(*notification.first, AXPropertyName::ColumnIndexRange);
+            break;
         case AXDisabledStateChanged:
             tree->updatePropertiesForSelfAndDescendants(*notification.first, { AXPropertyName::CanSetFocusAttribute, AXPropertyName::IsEnabled });
             break;
@@ -4188,6 +4206,9 @@ void AXObjectCache::updateIsolatedTree(const Vector<std::pair<RefPtr<Accessibili
         case AXRowIndexChanged:
             tree->updateNodeProperty(*notification.first, AXPropertyName::AXRowIndex);
             break;
+        case AXRowSpanChanged:
+            tree->updateNodeProperty(*notification.first, AXPropertyName::RowIndexRange);
+            break;
         case AXCellScopeChanged:
             tree->updateNodeProperties(*notification.first, { AXPropertyName::CellScope, AXPropertyName::IsColumnHeader, AXPropertyName::IsRowHeader });
             break;
@@ -4214,7 +4235,6 @@ void AXObjectCache::updateIsolatedTree(const Vector<std::pair<RefPtr<Accessibili
             break;
         case AXActiveDescendantChanged:
         case AXRoleChanged:
-        case AXColumnSpanChanged:
         case AXControlledObjectsChanged:
         case AXDescribedByChanged:
         case AXDropEffectChanged:
@@ -4231,7 +4251,6 @@ void AXObjectCache::updateIsolatedTree(const Vector<std::pair<RefPtr<Accessibili
         case AXMenuListValueChanged:
         case AXMultiSelectableStateChanged:
         case AXPressedStateChanged:
-        case AXRowSpanChanged:
         case AXSelectedChildrenChanged:
         case AXTextSecurityChanged:
         case AXValueChanged:
@@ -4328,6 +4347,21 @@ void AXObjectCache::deferRecomputeTableCellSlots(AccessibilityTable& axTable)
         m_performCacheUpdateTimer.startOneShot(0_s);
 }

+void AXObjectCache::deferRowspanChange(AccessibilityObject* axObject)
+{
+#if ENABLE(ACCESSIBILITY_ISOLATED_TREE)
+    auto* axTableCell = dynamicDowncast<AccessibilityTableCell>(axObject);
+    if (!axTableCell)
+        return;

AG: AccessibilityTableCell are created only for RenderTableCell and HTMLTableCellElement. Do we need to consider also role="cell"?

+
+    m_deferredRowspanChanges.add(*axTableCell);
+    if (!m_performCacheUpdateTimer.isActive())
+        m_performCacheUpdateTimer.startOneShot(0_s);
+#else
+    UNUSED_PARAM(axObject);
+#endif
+}
+
 void AXObjectCache::deferTextChangedIfNeeded(Node* node)
 {
     if (!nodeAndRendererAreValid(node))
diff --git a/Source/WebCore/accessibility/AXObjectCache.h b/Source/WebCore/accessibility/AXObjectCache.h
index 4893f6aa7c20..4378f5e49faf 100644
--- a/Source/WebCore/accessibility/AXObjectCache.h
+++ b/Source/WebCore/accessibility/AXObjectCache.h
@@ -49,6 +49,7 @@ class TextStream;
 namespace WebCore {

 class AccessibilityTable;
+class AccessibilityTableCell;
 class Document;
 class HTMLAreaElement;
 class HTMLTableElement;
@@ -234,6 +235,7 @@ public:
         , WeakHashSet<Element, WeakPtrImplWithEventTargetData>
         , WeakHashSet<HTMLTableElement, WeakPtrImplWithEventTargetData>
         , WeakHashSet<AccessibilityTable>
+        , WeakHashSet<AccessibilityTableCell>
         , WeakListHashSet<Node, WeakPtrImplWithEventTargetData>
         , WeakHashMap<Element, String, WeakPtrImplWithEventTargetData>>;
     void deferFocusedUIElementChangeIfNeeded(Node* oldFocusedNode, Node* newFocusedNode);
@@ -461,6 +463,7 @@ public:
     void deferRecomputeIsIgnored(Element*);
     void deferRecomputeTableIsExposed(Element*);
     void deferRecomputeTableCellSlots(AccessibilityTable&);
+    void deferRowspanChange(AccessibilityObject*);

AG: Do wee need it to be public?

     void deferTextChangedIfNeeded(Node*);
     void deferSelectedChildrenChangedIfNeeded(Element&);
     void performDeferredCacheUpdate();
@@ -595,6 +598,9 @@ private:
     void handleMenuListValueChanged(Element&);
     void handleTextChanged(AccessibilityObject*);
     void handleRecomputeCellSlots(AccessibilityTable&);
+#if ENABLE(ACCESSIBILITY_ISOLATED_TREE)
+    void handleRowspanChanged(AccessibilityTableCell&);
+#endif

     // aria-modal or modal <dialog> related
     bool isModalElement(Element&) const;
@@ -674,6 +680,7 @@ private:
     WeakHashSet<Element, WeakPtrImplWithEventTargetData> m_deferredRecomputeIsIgnoredList;
     WeakHashSet<HTMLTableElement, WeakPtrImplWithEventTargetData> m_deferredRecomputeTableIsExposedList;
     WeakHashSet<AccessibilityTable> m_deferredRecomputeTableCellSlotsList;
+    WeakHashSet<AccessibilityTableCell> m_deferredRowspanChanges;
     WeakListHashSet<Node, WeakPtrImplWithEventTargetData> m_deferredTextChangedList;
     WeakHashSet<Element, WeakPtrImplWithEventTargetData> m_deferredSelectedChildredChangedList;
     ListHashSet<RefPtr<AccessibilityObject>> m_deferredChildrenChangedList;
@@ -774,6 +781,7 @@ inline void AXObjectCache::deferTextChangedIfNeeded(Node*) { }
 inline void AXObjectCache::deferSelectedChildrenChangedIfNeeded(Element&) { }
 inline void AXObjectCache::deferTextReplacementNotificationForTextControl(HTMLTextFormControlElement&, const String&) { }
 inline void AXObjectCache::deferRecomputeTableCellSlots(AccessibilityTable&) { }
+inline void AXObjectCache::deferRowspanChange(AccessibilityObject*) { }
 #if !PLATFORM(COCOA) && !USE(ATSPI)
 inline void AXObjectCache::detachWrapper(AXCoreObject*, AccessibilityDetachmentType) { }
 #endif
diff --git a/Source/WebCore/accessibility/AccessibilityObject.cpp b/Source/WebCore/accessibility/AccessibilityObject.cpp
index e70d67a18c69..f26daf3c54ad 100644
--- a/Source/WebCore/accessibility/AccessibilityObject.cpp
+++ b/Source/WebCore/accessibility/AccessibilityObject.cpp
@@ -3873,6 +3873,11 @@ void AccessibilityObject::setLastKnownIsIgnoredValue(bool isIgnored)
     m_lastKnownIsIgnoredValue = isIgnored ? AccessibilityObjectInclusion::IgnoreObject : AccessibilityObjectInclusion::IncludeObject;
 }

+bool AccessibilityObject::ignoredFromPresentationalRole() const
+{
+    return roleValue() == AccessibilityRole::Presentational || inheritsPresentationalRole();
+}
+
 bool AccessibilityObject::pressedIsPresent() const
 {
     return !getAttribute(aria_pressedAttr).isEmpty();
diff --git a/Source/WebCore/accessibility/AccessibilityObject.h b/Source/WebCore/accessibility/AccessibilityObject.h
index b2a521562a5b..f0d09e287288 100644
--- a/Source/WebCore/accessibility/AccessibilityObject.h
+++ b/Source/WebCore/accessibility/AccessibilityObject.h
@@ -69,7 +69,7 @@ struct AccessibilityText {

 bool nodeHasPresentationRole(Node*);

-class AccessibilityObject : public AXCoreObject, public CanMakeWeakPtr<AccessibilityObject> {
+class AccessibilityObject : public AXCoreObject, public CanMakeWeakPtr<AccessibilityObject>, public CanMakeCheckedPtr {
 public:
     virtual ~AccessibilityObject();

@@ -787,6 +787,7 @@ protected:
     void detachPlatformWrapper(AccessibilityDetachmentType) override;

     void setIsIgnoredFromParentData(AccessibilityIsIgnoredFromParentData& data) { m_isIgnoredFromParentData = data; }
+    bool ignoredFromPresentationalRole() const;

     bool isAccessibilityObject() const override { return true; }

diff --git a/Source/WebCore/accessibility/AccessibilityRenderObject.cpp b/Source/WebCore/accessibility/AccessibilityRenderObject.cpp
index cc75cf754d70..f92933880e5c 100644
--- a/Source/WebCore/accessibility/AccessibilityRenderObject.cpp
+++ b/Source/WebCore/accessibility/AccessibilityRenderObject.cpp
@@ -1069,7 +1069,7 @@ bool AccessibilityRenderObject::computeAccessibilityIsIgnored() const
     if (roleValue() == AccessibilityRole::Ignored)
         return true;

-    if (roleValue() == AccessibilityRole::Presentational || inheritsPresentationalRole())
+    if (ignoredFromPresentationalRole())
         return true;

     // WebAreas should be ignored if their iframe container is marked as presentational.
diff --git a/Source/WebCore/accessibility/AccessibilitySVGElement.cpp b/Source/WebCore/accessibility/AccessibilitySVGElement.cpp
index 7e1ffe17eb3d..db0e067a4d47 100644
--- a/Source/WebCore/accessibility/AccessibilitySVGElement.cpp
+++ b/Source/WebCore/accessibility/AccessibilitySVGElement.cpp
@@ -220,7 +220,7 @@ bool AccessibilitySVGElement::computeAccessibilityIsIgnored() const
             return false;
     }

-    if (roleValue() == AccessibilityRole::Presentational || inheritsPresentationalRole())
+    if (ignoredFromPresentationalRole())
         return true;

     if (ariaRoleAttribute() != AccessibilityRole::Unknown)
diff --git a/Source/WebCore/accessibility/AccessibilityTable.cpp b/Source/WebCore/accessibility/AccessibilityTable.cpp
index 7f1d08781e3d..1cc8b8c15ea7 100644
--- a/Source/WebCore/accessibility/AccessibilityTable.cpp
+++ b/Source/WebCore/accessibility/AccessibilityTable.cpp
@@ -482,25 +482,64 @@ void AccessibilityTable::addChildren()
     unsigned yCurrent = 0;
     RefPtr<HTMLTableCaptionElement> captionElement;

-    HashSet<AccessibilityObject*> appendedRows;
+    struct DownwardGrowingCell {
+        CheckedRef<AccessibilityTableCell> axObject;
+        // The column the cell starts in.
+        unsigned x;
+        // The number of columns the cell spans (called "width" in the spec).
+        unsigned colSpan;
+        unsigned remainingRowsToSpan;
+    };
+    Vector<DownwardGrowingCell> downwardGrowingCells;
+
+    // https://html.spec.whatwg.org/multipage/tables.html#algorithm-for-growing-downward-growing-cells
+    auto growDownwardsCells = [&] () {
+        // ...for growing downward-growing cells, the user agent must, for each {cell, cellX, width} tuple in the list
+        // of downward-growing cells, extend the cell so that it also covers the slots with coordinates (x, yCurrent), where cellX ≤ x < cellX+width.
+        for (auto& cell : downwardGrowingCells) {
+            if (!cell.remainingRowsToSpan)
+                continue;
+            cell.remainingRowsToSpan -= 1;

AG: --cell.remainingRowsToSpan;

+            cell.axObject->incrementEffectiveRowSpan();
+
+            auto axID = cell.axObject->objectID();
+            for (unsigned c = 0; c < cell.colSpan; c++) {
+                unsigned computedColumn = cell.x + c;

AG: computedColumn -> index?

+                ensureRowAndColumn(yCurrent, computedColumn);
+                m_cellSlots[yCurrent][computedColumn] = axID;

AG: You don't need the temp axID.

+            }
+        }
+    };
+
+    HashSet<AccessibilityObject*> processedRows;
     // https://html.spec.whatwg.org/multipage/tables.html#algorithm-for-processing-rows
     auto processRow = [&, this] (AccessibilityTableRow* row) {

AG: [&] captures this too, doesn't it?

-        if (!row || appendedRows.contains(row))
+        if (!row || processedRows.contains(row))
+            return;
+        processedRows.add(row);
+
+        if (row->roleValue() != AccessibilityRole::Unknown && row->accessibilityIsIgnored()) {
+            // Skip ignored rows (except for those ignored because they have an unknown role, which will happen after a table has become un-exposed but is potentially becoming re-exposed).
+            // This is an addition on top of the HTML algorithm because the computed AX table has extra restrictions (e.g. cannot contain aria-hidden or role="presentation" rows).
             return;
-        appendedRows.add(row);
+        }
+
         // Step 1: If yheight is equal to ycurrent, then increase yheight by 1. (ycurrent must never be greater than yheight.)
         if (yHeight <= yCurrent)
             yHeight = yCurrent + 1;

         // Step 2.
         unsigned xCurrent = 0;
-        // Step 3: Run the algorithm for growing downward-growing cells (not implemented).
+        // Step 3: Run the algorithm for growing downward-growing cells.
+        growDownwardsCells();

         // Step 4: If the tr element being processed has no td or th element children, then increase ycurrent by 1, abort this set of steps, and return to the algorithm above.
         for (const auto& child : row->children()) {
             auto* currentCell = dynamicDowncast<AccessibilityTableCell>(child.get());
             if (!currentCell)
                 continue;
+            // (Not specified): As part of beginning to process this cell, reset its effective rowspan in case it had a non-default value set from a previous call to AccessibilityTable::addChildren().
+            currentCell->resetEffectiveRowSpan();

             // Step 6: While the slot with coordinate (xcurrent, ycurrent) already has a cell assigned to it, increase xcurrent by 1.
             ensureRowAndColumn(yCurrent, xCurrent);
@@ -526,23 +565,36 @@ void AccessibilityTable::addChildren()
                 xWidth = xCurrent + colSpan;

             // Step 12: If yheight < ycurrent+rowspan, then let yheight be ycurrent+rowspan.
-            if (yHeight < yCurrent + rowSpan)
-                yHeight = yCurrent + rowSpan;
+            // NOTE: An explicit choice is made not to follow this part of the spec, because rowspan
+            // can be some arbitrarily large number (up to 65535) that will not actually reflect how
+            // many rows the cell spans in the final table. Taking it as-provided will cause incorrect
+            // results in many scenarios. Instead, only check for yHeight < yCurrent.
+            if (yHeight < yCurrent)
+                yHeight = yCurrent;

             // Step 13: Let the slots with coordinates (x, y) such that xcurrent ≤ x < xcurrent+colspan and
             // ycurrent ≤ y < ycurrent+rowspan be covered by a new cell c, anchored at (xcurrent, ycurrent),
             // which has width colspan and height rowspan, corresponding to the current cell element.
+            // NOTE: We don't implement this exactly, instead using the downward-growing cell algorithm to accurately
+            // handle rowspan cells. This makes it easy to avoid extending cells outside their rowgroup.
             currentCell->setRowIndex(yCurrent);
             currentCell->setColumnIndex(xCurrent);
-            for (unsigned y = yCurrent; y < yCurrent + rowSpan; y++) {
-                for (unsigned x = xCurrent; x < xCurrent + colSpan; x++) {
-                    ensureRowAndColumn(y, x);
-                    m_cellSlots[y][x] = currentCell->objectID();
-                }
+            for (unsigned x = xCurrent; x < xCurrent + colSpan; x++) {
+                ensureRowAndColumn(yCurrent, x);
+                m_cellSlots[yCurrent][x] = currentCell->objectID();
             }

             // Step 14: If cell grows downward is true, then add the tuple {c, xcurrent, colspan} to the
-            // list of downward-growing cells. Not implemented.
+            // list of downward-growing cells.
+            // NOTE: We use the downward-growing cell algorithm to expand rowspanned cells.
+            if (rowSpan > 1) {
+                downwardGrowingCells.append({
+                    *currentCell,
+                    xCurrent,
+                    colSpan,
+                    rowSpan - 1
+                });
+            }

             // Step 15.
             xCurrent += colSpan;
@@ -559,9 +611,9 @@ void AccessibilityTable::addChildren()
         // Step 16: If current cell is the last td or th element child in the tr element being processed, then increase ycurrent by 1, abort this set of steps, and return to the algorithm above.
         yCurrent += 1;
     };
-    auto needsToDescend = [&appendedRows] (AXCoreObject& axObject) {
+    auto needsToDescend = [&processedRows] (AXCoreObject& axObject) {
         return (!axObject.isTableRow() || !axObject.node())
-            && !appendedRows.contains(dynamicDowncast<AccessibilityObject>(axObject));
+            && !processedRows.contains(dynamicDowncast<AccessibilityObject>(axObject));
     };
     std::function<void(AXCoreObject*)> processRowDescendingIfNeeded = [&] (AXCoreObject* axObject) {
         if (!axObject)
@@ -575,8 +627,15 @@ void AccessibilityTable::addChildren()
     };
     // https://html.spec.whatwg.org/multipage/tables.html#algorithm-for-ending-a-row-group
     auto endRowGroup = [&] () {
-        if (yCurrent < yHeight)
-            yCurrent = yHeight;
+        // 1. While yCurrent is less than yHeight, follow these steps:
+        while (yCurrent < yHeight) {
+            // 1a. Run the algorithm for growing downward-growing cells.
+            growDownwardsCells();
+            // 1b. Increase yCurrent by 1.
+            yCurrent += 1;

AG: ++yCurrent;

+        }
+        // 2. Empty the list of downward-growing cells.
+        downwardGrowingCells.clear();
     };
     // https://html.spec.whatwg.org/multipage/tables.html#algorithm-for-processing-row-groups
     auto processRowGroup = [&] (Element& sectionElement) {
@@ -608,6 +667,7 @@ void AccessibilityTable::addChildren()
     if (!is<HTMLTableElement>(tableElement.get()) && !isAriaTable())
         return;

+    bool withinImplicitRowGroup = false;
     std::function<void(Node*)> processTableDescendant = [&, this] (Node* node) {

AG: auto processTableDescendant = [&] ...

         // Step 8: While the current element is not one of the following elements, advance the
         // current element to the next child of the table.
@@ -622,9 +682,19 @@ void AccessibilityTable::addChildren()
         bool descendantIsRow = element && (element->hasTagName(trTag) || nodeHasRole(element, "row"_s));
         bool descendantIsRowGroup = element && !descendantIsRow && (element->hasTagName(theadTag) || element->hasTagName(tbodyTag) || element->hasTagName(tfootTag) || nodeHasRole(element, "rowgroup"_s));

+        if (descendantIsRowGroup)
+            withinImplicitRowGroup = false;
+        else {
+            // (Not specified): For ARIA tables, we need to track implicit rowgroups (allowed by the ARIA spec)
+            // in order to properly perform the downward-growing cell algorithm.
+            withinImplicitRowGroup = isAriaTable();
+        }
+
         // Step 9: Handle the colgroup element. Not implemented.
         // Step 10: Handled above.
-        // Step 11: Skipped. Not currently implementing "downward-growing cells" algorithm.
+        // Step 11: Let the list of downward-growing cells be an empty list.
+        if (!withinImplicitRowGroup)
+            downwardGrowingCells.clear();
         // Step 12: While the current element is not one of the following elements, advance the current element to the next child of the table
         if (!descendantIsRow && !descendantIsRowGroup) {
             if (isAriaTable()) {
@@ -643,7 +713,8 @@ void AccessibilityTable::addChildren()
             processRow(dynamicDowncast<AccessibilityTableRow>(cache->getOrCreate(element)));

         // Step 14: Run the algorithm for ending a row group.
-        endRowGroup();
+        if (!withinImplicitRowGroup)
+            endRowGroup();

         // Step 15: If the current element is a tfoot...
         if (element->hasTagName(tfootTag)) {
diff --git a/Source/WebCore/accessibility/AccessibilityTableCell.cpp b/Source/WebCore/accessibility/AccessibilityTableCell.cpp
index 7814b6ef828b..384159e472ff 100644
--- a/Source/WebCore/accessibility/AccessibilityTableCell.cpp
+++ b/Source/WebCore/accessibility/AccessibilityTableCell.cpp
@@ -333,7 +333,7 @@ void AccessibilityTableCell::ensureIndexesUpToDate() const
 std::pair<unsigned, unsigned> AccessibilityTableCell::rowIndexRange() const
 {
     ensureIndexesUpToDate();
-    return { m_rowIndex, rowSpan() };
+    return { m_rowIndex, m_effectiveRowSpan };
 }

 std::pair<unsigned, unsigned> AccessibilityTableCell::columnIndexRange() const
diff --git a/Source/WebCore/accessibility/AccessibilityTableCell.h b/Source/WebCore/accessibility/AccessibilityTableCell.h
index c34db92e53db..6a8a0de35c45 100644
--- a/Source/WebCore/accessibility/AccessibilityTableCell.h
+++ b/Source/WebCore/accessibility/AccessibilityTableCell.h
@@ -63,6 +63,8 @@ public:
     int axRowIndex() const override;
     unsigned colSpan() const;
     unsigned rowSpan() const;
+    void incrementEffectiveRowSpan() { m_effectiveRowSpan += 1; }

AG: ++m_effectiveRowSpan;

+    void resetEffectiveRowSpan() { m_effectiveRowSpan = 1; }
     void setAXColIndexFromRow(int index) { m_axColIndexFromRow = index; }

     void setRowIndex(unsigned index) { m_rowIndex = index; }
@@ -81,10 +83,6 @@ protected:
     AccessibilityRole determineAccessibilityRole() final;
     AccessibilityObject* parentObjectUnignored() const override;

-    unsigned m_rowIndex { 0 };
-    unsigned m_columnIndex { 0 };
-    int m_axColIndexFromRow { -1 };
-
 private:
     // If a table cell is not exposed as a table cell, a TH element can serve as its title UI element.
     AccessibilityObject* titleUIElement() const final;
@@ -94,6 +92,15 @@ private:
     AccessibilityTableRow* ariaOwnedByParent() const;
     void ensureIndexesUpToDate() const;

+    unsigned m_rowIndex { 0 };
+    unsigned m_columnIndex { 0 };
+    int m_axColIndexFromRow { -1 };
+
+    // How many rows does this cell actually span?
+    // This differs from rowSpan(), which can be an author-specified number all the way up 65535 that doesn't actually
+    // reflect how many rows the cell spans in the rendered table.
+    // Default to 1, as the cell should span at least the row it starts in.
+    unsigned m_effectiveRowSpan { 1 };
 };

 } // namespace WebCore
diff --git a/Source/WebCore/accessibility/AccessibilityTableRow.cpp b/Source/WebCore/accessibility/AccessibilityTableRow.cpp
index e04276c49648..261bc3ed92d8 100644
--- a/Source/WebCore/accessibility/AccessibilityTableRow.cpp
+++ b/Source/WebCore/accessibility/AccessibilityTableRow.cpp
@@ -94,6 +94,9 @@ bool AccessibilityTableRow::computeAccessibilityIsIgnored() const
     if (!isTableRow())
         return AccessibilityRenderObject::computeAccessibilityIsIgnored();

+    if (ignoredFromPresentationalRole())
+        return true;
+
     return false;
 }

diff --git a/Source/WebCore/accessibility/isolatedtree/AXIsolatedTree.cpp b/Source/WebCore/accessibility/isolatedtree/AXIsolatedTree.cpp
index b19ac2856136..480658907486 100644
--- a/Source/WebCore/accessibility/isolatedtree/AXIsolatedTree.cpp
+++ b/Source/WebCore/accessibility/isolatedtree/AXIsolatedTree.cpp
@@ -549,6 +549,9 @@ void AXIsolatedTree::updateNodeProperties(AXCoreObject& axObject, const Vector<A
         case AXPropertyName::CellSlots:
             propertyMap.set(AXPropertyName::CellSlots, dynamicDowncast<AccessibilityObject>(axObject)->cellSlots());
             break;
+        case AXPropertyName::ColumnIndexRange:
+            propertyMap.set(AXPropertyName::ColumnIndexRange, axObject.columnIndexRange());
+            break;
         case AXPropertyName::CurrentState:
             propertyMap.set(AXPropertyName::CurrentState, static_cast<int>(axObject.currentState()));
             break;
@@ -596,6 +599,9 @@ void AXIsolatedTree::updateNodeProperties(AXCoreObject& axObject, const Vector<A
         case AXPropertyName::RoleDescription:
             propertyMap.set(AXPropertyName::RoleDescription, axObject.roleDescription().isolatedCopy());
             break;
+        case AXPropertyName::RowIndexRange:
+            propertyMap.set(AXPropertyName::RowIndexRange, axObject.rowIndexRange());
+            break;
         case AXPropertyName::AXRowIndex:
             propertyMap.set(AXPropertyName::AXRowIndex, axObject.axRowIndex());
             break;

[Tyler Wilcock]

Created attachment 468264 [details]
Patch

[Tyler Wilcock]

> +void AXObjectCache::deferRowspanChange(AccessibilityObject* axObject)
> +{
> +#if ENABLE(ACCESSIBILITY_ISOLATED_TREE)
> +    auto* axTableCell = dynamicDowncast<AccessibilityTableCell>(axObject);
> +    if (!axTableCell)
> +        return;
> 
> AG: AccessibilityTableCell are created only for RenderTableCell and
> HTMLTableCellElement. Do we need to consider also role="cell"?
For role="cell", role="grid", role="columnheader", and role="rowheader", we create AccessibilityARIAGridCell instances, so that should be OK here.

> +            cell.axObject->incrementEffectiveRowSpan();
> +
> +            auto axID = cell.axObject->objectID();
> +            for (unsigned c = 0; c < cell.colSpan; c++) {
> +                unsigned computedColumn = cell.x + c;
> 
> AG: computedColumn -> index?
Hmm, I feel like that's less descriptive considering there's also a row "index" in this scope. Made a change to remove this temporary entirely, instead using the for-loop to compute the column index.

> +            }
> +        }
> +    };
> +
> +    HashSet<AccessibilityObject*> processedRows;
>      //
> https://html.spec.whatwg.org/multipage/tables.html#algorithm-for-processing-
> rows
>      auto processRow = [&, this] (AccessibilityTableRow* row) {
> 
> AG: [&] captures this too, doesn't it?
Wow, I always thought you had to explicitly specify a this capture, even with &! Swear I had gotten compilation errors for not doing so in the past. Fixed in both places.

All other comments applied in latest patch.

[EWS]

Committed 269544@main (723eb83c4fdb): <https://commits.webkit.org/269544@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 468264 [details].