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

Mon Sep 18 17:45:36 PDT 2023

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

Tyler Wilcock <tyler_w at apple.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |tyler_w at apple.com

--- Comment #3 from Tyler Wilcock <tyler_w at apple.com> ---
Comment on attachment 467739
  --> https://bugs.webkit.org/attachment.cgi?id=467739
Patch

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

> Source/WebCore/accessibility/AccessibilityObject.h:155
> +    String scope() const override { return getAttribute(HTMLNames::scopeAttr); }

Can this be final instead of override?

> Source/WebCore/accessibility/AccessibilityObjectInterface.h:1789
> +        if (parent->roleValue() == AccessibilityRole::RowGroup)

This is a behavior change relative to what this function used to do (compare thead, tbody, or tfoot ancestors). Maybe we need a new AXID property and virtual function called row-group container which we cache for table-cells only and compare here?

> Source/WebCore/accessibility/AccessibilityTableRow.h:44
> +    AXCoreObject* headerObject() override;

Can this be final?

> Source/WebCore/accessibility/isolatedtree/AXIsolatedObject.cpp:1849
> +        AccessibilityChildrenVector columnsCopy = columns();

Can this be auto?

> Source/WebCore/accessibility/isolatedtree/AXIsolatedObject.cpp:1875
> +            const String& headerScope = tableCell->scope();
> +
> +            if (headerScope == "colgroup"_s && isTableCellInSameColGroup(tableCell))

Do we need the headerScope temporary, or can we do this:

if (tableCell->scope() == "colgroup"_s ...)

> Source/WebCore/accessibility/isolatedtree/AXIsolatedObject.cpp:1899
> +        AccessibilityChildrenVector rowsCopy = rows();

Can this be auto?

> Source/WebCore/accessibility/isolatedtree/AXIsolatedObject.cpp:1905
> +        AXCoreObject* parent = exposedTableAncestor();

Can this be auto*?

> Source/WebCore/accessibility/isolatedtree/AXIsolatedObject.cpp:1916
> +            const String& headerScope = scope();

Do we need this headerScope temporary?

> Source/WebCore/accessibility/isolatedtree/AXIsolatedObject.h:167
> +    bool isColumnHeaderCell() const override { return boolAttributeValue(AXPropertyName::IsColumnHeaderCell); }
> +    bool isRowHeaderCell() const override { return boolAttributeValue(AXPropertyName::IsRowHeaderCell); }
> +    String scope() const override { return stringAttributeValue(AXPropertyName::Scope); }

Can these be final?

> Source/WebCore/accessibility/isolatedtree/AXIsolatedObject.h:177
> +    AXCoreObject* headerObject() override { return objectAttributeValue(AXPropertyName::HeaderObject); };

Can this be final?

> LayoutTests/accessibility/column-header-scope.html:21
> +	<table id="table1">
> +		<tr>
> +			<th scope="col" id="title-cell">Title</th>
> +			<th>Col 1</th>
> +			<th>Col 2</th>
> +			<th>Col 3</th>
> +		</tr>
> +		<tr>
> +			<td>Data abc</td>
> +			<td>Data def</td>
> +			<td>Data ghi</td>
> +			<td>Data jkl</td>
> +		</tr>
> +	</table>

Seems like a lot of lines in this test have 8 space indents — I think we've been trying to stick with 4. Also, I think we typically don't indent after the <body>. So like this:

<body>
<table>
    ....
</table>
<script>
...
</script>

> LayoutTests/accessibility/column-header-scope.html:33
> +			var thirdRow = "<tr><td id='cell2'>Data abc</td><td>Data def</td><td>Data ghi</td><td>Data jkl</td></tr>";

Is this used?

> LayoutTests/accessibility/column-header-scope.html:36
> +			testOutput += "The table cell at (0, 1) should have exactly 1 column header, currently it has " + colHeaders1.length + " column header(s).\n";
> +			testOutput += "The table cell at (2, 0) should have exactly 0 row headers, currently it has " + rowHeaders1.length + " row header(s).\n";

Format strings might make this a bit nicer. e.g.

testOutput += `The table cell at (0, 1) should have exactly 1 column header, currently it has ${colHeaders1.length} column header(s).\n`;

Same for other situations through this file.

> LayoutTests/accessibility/column-header-scope.html:41
> +

Extra newline here I think.

> LayoutTests/accessibility/table-headers-changing.html:44
> +        var head_row = document.getElementById("row-in-head");

I think our style has been to use for camelCase over snake_case.

-- 
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/20230919/6ba5cb4b/attachment.htm>