[Webkit-unassigned] [Bug 261680] AX: columnHeaders() and rowHeaders() are performed on the main thread in ITM

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Tue Sep 19 18:51:50 PDT 2023


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

--- Comment #12 from Andres Gonzalez <andresg_22 at apple.com> ---
(In reply to Joshua Hoffman from comment #11)
> Created attachment 467760 [details]
> Patch

diff --git a/Source/WebCore/accessibility/AccessibilityObjectInterface.h b/Source/WebCore/accessibility/AccessibilityObjectInterface.h
index e727148f9bbb..0fde9758e103 100644
--- a/Source/WebCore/accessibility/AccessibilityObjectInterface.h
+++ b/Source/WebCore/accessibility/AccessibilityObjectInterface.h
@@ -892,6 +892,12 @@ public:
     // Table cell support.
     virtual bool isTableCell() const = 0;
     virtual bool isExposedTableCell() const = 0;
+    virtual bool isColumnHeaderCell() const { return false; }
+    virtual bool isRowHeaderCell() const { return false; }

AG: Can we call these just isColumnHeader and isRowHeader?

+    bool isTableCellInSameRowGroup(AXCoreObject*);
+    bool isTableCellInSameColGroup(AXCoreObject*);

AG: and these isInSameRowGroup and isInSameColumnGroup?

+    virtual AXID rowGroupAncestorID() const { return AXID(); }
+    virtual String scope() const { return String(); }

AG: in both of these:

return { };

     // Returns the start location and row span of the cell.
     virtual std::pair<unsigned, unsigned> rowIndexRange() const = 0;
     // Returns the start location and column span of the cell.
@@ -907,6 +913,7 @@ public:
     // Table row support.
     virtual bool isTableRow() const = 0;
     virtual unsigned rowIndex() const = 0;
+    virtual AXCoreObject* headerObject() { return nullptr; }

AG: maybe headerCell() would be more appropriate?

     // ARIA tree/grid row support.
     virtual bool isARIATreeGridRow() const = 0;
@@ -1762,6 +1769,25 @@ inline unsigned AXCoreObject::tableLevel() const
     return level;
 }

+inline bool AXCoreObject::isTableCellInSameRowGroup(AXCoreObject* otherTableCell)
+{
+    if (!otherTableCell)
+        return false;
+
+    AXID ancestorID = rowGroupAncestorID();
+    return ancestorID ? ancestorID == otherTableCell->rowGroupAncestorID() : false;
+}

AG: the return line can be written as:

    return ancestorID .isValid() && ancestorID == otherTableCell->rowGroupAncestorID();

+
+inline bool AXCoreObject::isTableCellInSameColGroup(AXCoreObject* tableCell)
+{
+    auto columnRange = columnIndexRange();
+    auto otherColumnRange = tableCell->columnIndexRange();
+
+    if (columnRange.first <= (otherColumnRange.first + otherColumnRange.second))
+        return true;
+    return false;
+}

AG: instead of this if statement you can write:

return columnRange.first <= otherColumnRange.first + otherColumnRange.second;

+
 inline String AXCoreObject::ariaLandmarkRoleDescription() const
 {
     switch (roleValue()) {
diff --git a/Source/WebCore/accessibility/AccessibilityTableCell.cpp b/Source/WebCore/accessibility/AccessibilityTableCell.cpp
index 894462dd065c..d0b21617f6e3 100644
--- a/Source/WebCore/accessibility/AccessibilityTableCell.cpp
+++ b/Source/WebCore/accessibility/AccessibilityTableCell.cpp
@@ -226,34 +226,16 @@ bool AccessibilityTableCell::isRowHeaderCell() const
     }
     return false;
 }
-
-bool AccessibilityTableCell::isTableCellInSameRowGroup(AXCoreObject* otherTableCell)
-{
-    Node* parentNode = node();
-    for ( ; parentNode; parentNode = parentNode->parentNode()) {
-        if (parentNode->hasTagName(theadTag) || parentNode->hasTagName(tbodyTag) || parentNode->hasTagName(tfootTag))
-            break;
-    }

-    Node* otherParentNode = otherTableCell->node();
-    for ( ; otherParentNode; otherParentNode = otherParentNode->parentNode()) {
-        if (otherParentNode->hasTagName(theadTag) || otherParentNode->hasTagName(tbodyTag) || otherParentNode->hasTagName(tfootTag))
-            break;
+AXID AccessibilityTableCell::rowGroupAncestorID() const
+{
+    for (auto* parent = parentObject(); parent; parent = parent->parentObject()) {
+        if (parent->hasTagName(theadTag) || parent->hasTagName(tbodyTag) || parent->hasTagName(tfootTag))
+            return parent->objectID();
     }

AG: can we write this using findAncestor?

-
-    return otherParentNode == parentNode;
+    return AXID();
 }

AG: return { };

-bool AccessibilityTableCell::isTableCellInSameColGroup(AXCoreObject* tableCell)
-{
-    auto colRange = columnIndexRange();
-    auto otherColRange = tableCell->columnIndexRange();
-
-    if (colRange.first <= (otherColRange.first + otherColRange.second))
-        return true;
-    return false;
-}
-
 String AccessibilityTableCell::expandedTextValue() const
 {
     return getAttribute(abbrAttr);
@@ -285,8 +267,7 @@ AXCoreObject::AccessibilityChildrenVector AccessibilityTableCell::columnHeaders(
             continue;

         ASSERT(is<AccessibilityObject>(tableCell));
-        const AtomString& scope = downcast<AccessibilityObject>(tableCell)->getAttribute(scopeAttr);
-        if (scope == "colgroup"_s && isTableCellInSameColGroup(tableCell))
+        if (tableCell->scope() == "colgroup"_s && isTableCellInSameColGroup(tableCell))
             headers.append(tableCell);
         else if (downcast<AccessibilityObject>(tableCell)->isColumnHeaderCell())
             headers.append(tableCell);
@@ -309,9 +290,8 @@ AXCoreObject::AccessibilityChildrenVector AccessibilityTableCell::rowHeaders()
         auto* tableCell = parent->cellForColumnAndRow(column, rowRange.first);
         if (!tableCell || tableCell == this || headers.contains(tableCell))
             continue;
-
-        const AtomString& scope = downcast<AccessibilityObject>(tableCell)->getAttribute(scopeAttr);
-        if (scope == "rowgroup"_s && isTableCellInSameRowGroup(tableCell))
+
+        if (tableCell->scope() == "rowgroup"_s && isTableCellInSameRowGroup(tableCell))
             headers.append(tableCell);
         else if (downcast<AccessibilityObject>(tableCell)->isRowHeaderCell())
             headers.append(tableCell);
diff --git a/Source/WebCore/accessibility/isolatedtree/AXIsolatedObject.cpp b/Source/WebCore/accessibility/isolatedtree/AXIsolatedObject.cpp
index 79a35120c760..602b3426be2d 100644
--- a/Source/WebCore/accessibility/isolatedtree/AXIsolatedObject.cpp
+++ b/Source/WebCore/accessibility/isolatedtree/AXIsolatedObject.cpp
@@ -209,6 +209,10 @@ void AXIsolatedObject::initializeProperties(const Ref<AccessibilityObject>& axOb
         setProperty(AXPropertyName::RowIndexRange, object.rowIndexRange());
         setProperty(AXPropertyName::AXColumnIndex, object.axColumnIndex());
         setProperty(AXPropertyName::AXRowIndex, object.axRowIndex());
+        setProperty(AXPropertyName::IsColumnHeaderCell, object.isColumnHeaderCell());
+        setProperty(AXPropertyName::IsRowHeaderCell, object.isRowHeaderCell());
+        setProperty(AXPropertyName::Scope, object.scope());

AG: this is a String right? If so, you need to cache .isolatedcopy().

+        setProperty(AXPropertyName::RowGroupAncestorID, object.rowGroupAncestorID());
     }

     if (object.isTableColumn()) {
@@ -226,6 +230,9 @@ void AXIsolatedObject::initializeProperties(const Ref<AccessibilityObject>& axOb
         setObjectProperty(AXPropertyName::DisclosedByRow, object.disclosedByRow());
     }

+    if (object.isARIATreeGridRow() || object.isTableRow())
+        setObjectProperty(AXPropertyName::HeaderObject, object.headerObject());
+
     if (object.isTreeItem()) {
         setProperty(AXPropertyName::IsTreeItem, true);
         setObjectVectorProperty(AXPropertyName::ARIATreeItemContent, object.ariaTreeItemContent());
@@ -940,18 +947,12 @@ T AXIsolatedObject::getOrRetrievePropertyValue(AXPropertyName propertyName)
             });
             break;
         }
-        case AXPropertyName::ColumnHeaders:
-            value = axIDs(axObject->columnHeaders());
-            break;
         case AXPropertyName::InnerHTML:
             value = axObject->innerHTML().isolatedCopy();
             break;
         case AXPropertyName::OuterHTML:
             value = axObject->outerHTML().isolatedCopy();
             break;
-        case AXPropertyName::RowHeaders:
-            value = axIDs(axObject->rowHeaders());
-            break;
         default:
             break;
         }
@@ -1834,7 +1835,40 @@ std::optional<String> AXIsolatedObject::attributeValue(const String& attributeNa

 AXCoreObject::AccessibilityChildrenVector AXIsolatedObject::columnHeaders()
 {
-    return tree()->objectsForIDs(const_cast<AXIsolatedObject*>(this)->getOrRetrievePropertyValue<Vector<AXID>>(AXPropertyName::ColumnHeaders));
+    AccessibilityChildrenVector headers;
+    if (isTable()) {
+        auto columnsCopy = columns();
+        for (const auto& column : columnsCopy) {
+            if (auto* header = column->columnHeader())
+                headers.append(header);
+        }
+    } else if (isExposedTableCell()) {
+        auto* parent = exposedTableAncestor();
+        if (!parent)
+            return { };
+
+        // Choose columnHeaders as the place where the "headers" attribute is reported.
+        headers = relatedObjects(AXRelationType::Headers);
+        // If the headers attribute returned valid values, then do not further search for column headers.
+        if (!headers.isEmpty())
+            return headers;
+
+        auto rowRange = rowIndexRange();
+        auto colRange = columnIndexRange();
+
+        for (unsigned row = 0; row < rowRange.first; row++) {
+            auto* tableCell = parent->cellForColumnAndRow(colRange.first, row);
+            if (!tableCell || tableCell == this || headers.contains(tableCell))
+                continue;
+
+            if (tableCell->scope() == "colgroup"_s && isTableCellInSameColGroup(tableCell))
+                headers.append(tableCell);
+            else if (tableCell->isColumnHeaderCell())
+                headers.append(tableCell);
+        }
+    }
+
+    return headers;
 }

 String AXIsolatedObject::innerHTML() const
@@ -1849,7 +1883,33 @@ String AXIsolatedObject::outerHTML() const

 AXCoreObject::AccessibilityChildrenVector AXIsolatedObject::rowHeaders()
 {
-    return tree()->objectsForIDs(const_cast<AXIsolatedObject*>(this)->getOrRetrievePropertyValue<Vector<AXID>>(AXPropertyName::RowHeaders));
+    AccessibilityChildrenVector headers;
+    if (isTable()) {
+        auto rowsCopy = rows();
+        for (const auto& row : rowsCopy) {
+            if (auto* header = row->headerObject())
+                headers.append(header);
+        }
+    } else if (isExposedTableCell()) {
+        auto* parent = exposedTableAncestor();
+        if (!parent)
+            return headers;

AG: In the column method you return { } here, which I prefer as well.

+
+        auto rowRange = rowIndexRange();
+        auto colRange = columnIndexRange();
+        for (unsigned column = 0; column < colRange.first; column++) {
+            auto* tableCell = parent->cellForColumnAndRow(column, rowRange.first);
+            if (!tableCell || tableCell == this || headers.contains(tableCell))
+                continue;
+
+            if (tableCell->scope() == "rowgroup"_s && isTableCellInSameRowGroup(tableCell))
+                headers.append(tableCell);
+            else if (tableCell->isRowHeaderCell())
+                headers.append(tableCell);
+        }
+    }
+
+    return headers;
 }

 #if !PLATFORM(MAC)
diff --git a/Source/WebCore/accessibility/isolatedtree/AXIsolatedObject.h b/Source/WebCore/accessibility/isolatedtree/AXIsolatedObject.h
index 8d91d1e5f2dc..81d16ec18c67 100644
--- a/Source/WebCore/accessibility/isolatedtree/AXIsolatedObject.h
+++ b/Source/WebCore/accessibility/isolatedtree/AXIsolatedObject.h
@@ -162,6 +162,10 @@ private:
     std::pair<unsigned, unsigned> columnIndexRange() const override { return pairAttributeValue<unsigned>(AXPropertyName::ColumnIndexRange); }
     int axColumnIndex() const override { return intAttributeValue(AXPropertyName::AXColumnIndex); }
     int axRowIndex() const override { return intAttributeValue(AXPropertyName::AXRowIndex); }
+    bool isColumnHeaderCell() const final { return boolAttributeValue(AXPropertyName::IsColumnHeaderCell); }
+    bool isRowHeaderCell() const final { return boolAttributeValue(AXPropertyName::IsRowHeaderCell); }
+    String scope() const final { return stringAttributeValue(AXPropertyName::Scope); }
+    AXID rowGroupAncestorID() const final { return propertyValue<AXID>(AXPropertyName::RowGroupAncestorID); }

     // Table column support.
     bool isTableColumn() const override { return boolAttributeValue(AXPropertyName::IsTableColumn); }
@@ -171,6 +175,7 @@ private:
     // Table row support.
     bool isTableRow() const override { return boolAttributeValue(AXPropertyName::IsTableRow); }
     unsigned rowIndex() const override { return unsignedAttributeValue(AXPropertyName::RowIndex); }
+    AXCoreObject* headerObject() final { return objectAttributeValue(AXPropertyName::HeaderObject); };

     // ARIA tree/grid row support.
     bool isARIATreeGridRow() const override { return boolAttributeValue(AXPropertyName::IsARIATreeGridRow); }
diff --git a/Source/WebCore/accessibility/isolatedtree/AXIsolatedTree.cpp b/Source/WebCore/accessibility/isolatedtree/AXIsolatedTree.cpp
index 166832e864c9..bb59bd515a1e 100644
--- a/Source/WebCore/accessibility/isolatedtree/AXIsolatedTree.cpp
+++ b/Source/WebCore/accessibility/isolatedtree/AXIsolatedTree.cpp
@@ -531,6 +531,9 @@ void AXIsolatedTree::updateNodeProperties(AXCoreObject& axObject, const Vector<A
             propertyMap.set(AXPropertyName::IsChecked, axObject.isChecked());
             propertyMap.set(AXPropertyName::ButtonState, axObject.checkboxOrRadioValue());
             break;
+        case AXPropertyName::IsColumnHeaderCell:
+            propertyMap.set(AXPropertyName::IsColumnHeaderCell, axObject.isColumnHeaderCell());
+            break;
         case AXPropertyName::IsEnabled:
             propertyMap.set(AXPropertyName::IsEnabled, axObject.isEnabled());
             break;
@@ -543,6 +546,9 @@ void AXIsolatedTree::updateNodeProperties(AXCoreObject& axObject, const Vector<A
         case AXPropertyName::IsSelected:
             propertyMap.set(AXPropertyName::IsSelected, axObject.isSelected());
             break;
+        case AXPropertyName::IsRowHeaderCell:
+            propertyMap.set(AXPropertyName::IsRowHeaderCell, axObject.isRowHeaderCell());
+            break;
         case AXPropertyName::MaxValueForRange:
             propertyMap.set(AXPropertyName::MaxValueForRange, axObject.maxValueForRange());
             break;
@@ -564,6 +570,9 @@ void AXIsolatedTree::updateNodeProperties(AXCoreObject& axObject, const Vector<A
         case AXPropertyName::AXRowIndex:
             propertyMap.set(AXPropertyName::AXRowIndex, axObject.axRowIndex());
             break;
+        case AXPropertyName::Scope:
+            propertyMap.set(AXPropertyName::Scope, axObject.scope());

AG: axObject.scope().isolatedCopy();

+            break;
         case AXPropertyName::SetSize:
             propertyMap.set(AXPropertyName::SetSize, axObject.setSize());
             break;
diff --git a/Source/WebCore/accessibility/isolatedtree/AXIsolatedTree.h b/Source/WebCore/accessibility/isolatedtree/AXIsolatedTree.h
index f83675983b94..67170b88c7d7 100644
--- a/Source/WebCore/accessibility/isolatedtree/AXIsolatedTree.h
+++ b/Source/WebCore/accessibility/isolatedtree/AXIsolatedTree.h
@@ -105,6 +105,7 @@ enum class AXPropertyName : uint16_t {
     HasPlainText,
     HasUnderline,
     HeaderContainer,
+    HeaderObject,
     HeadingLevel,
     HelpText,
     HierarchicalLevel,
@@ -119,6 +120,7 @@ enum class AXPropertyName : uint16_t {
     IsAttachment,
     IsBusy,
     IsChecked,
+    IsColumnHeaderCell,

AG: IsColumnHeader

     IsControl,
     IsEnabled,
     IsExpanded,
@@ -148,6 +150,7 @@ enum class AXPropertyName : uint16_t {
     IsMultiSelectable,
     IsPressed,
     IsRequired,
+    IsRowHeaderCell,

AG: IsRowHeader

     IsSecureField,
     IsSelected,
     IsSelectedOptionActive,
@@ -196,10 +199,12 @@ enum class AXPropertyName : uint16_t {
     RolePlatformString,
     RoleDescription,
     Rows,
+    RowGroupAncestorID,
     RowHeaders,
     RowIndex,
     RowIndexRange,
     ScreenRelativePosition,
+    Scope,
     SelectedChildren,
     SessionID,
     SetSize,

-- 
You are receiving this mail because:
You are the assignee for the bug.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.webkit.org/pipermail/webkit-unassigned/attachments/20230920/91dd0d86/attachment-0001.htm>


More information about the webkit-unassigned mailing list