Bug 267056 – AX: Tables with one row of all header cells should not be exposed as data tables

• Status: RESOLVED
• Resolution: FIXED
• Component: Accessibility

[Joshua Hoffman]

WebKit currently exposes tables with one row of all header cells as data tables, even though other single row tables are not. These should be considered layout tables.

[Radar WebKit Bug Importer]

<rdar://problem/120431884>

[Joshua Hoffman]

Created attachment 469280 [details]
Patch

[Joshua Hoffman]

rdar://119941473

[Tyler Wilcock]

Comment on attachment 469280 [details]
Patch

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

> COMMIT_MESSAGE:13
> +AXObjectCache::childrenChanged was also updated to recompute a tables exposability when
> +rows change outside of table sections (tbody, thead, tfoot).

Is this required for the test in this patch to pass? Or does this fix some other bug that isn't tested?

> Source/WebCore/accessibility/AccessibilityTable.cpp:339
> +        if (firstRow && currentParent == firstRow && rowIsAllTableHeaderCells &&  cellCountForEachRow.get(currentParent.get()) >= 1 && rowCount >= 2)

Seems like we accidentally added an extra space between && and cellCountForEachRow.

> LayoutTests/accessibility/single-th-layout-table.html:35
> +        await waitFor(() => accessibilityController.accessibleElementById('table1') != null);

Can we also verify the table's role, row count, and column count? Just to make sure it isn't being exposed as a group or something.

[Joshua Hoffman]

Comment on attachment 469280 [details]
Patch

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

>> COMMIT_MESSAGE:13
>> +rows change outside of table sections (tbody, thead, tfoot).
> 
> Is this required for the test in this patch to pass? Or does this fix some other bug that isn't tested?

Yes, as we have special logic to determine if the presence thead/tbody/tfoot indicate data tables, we want this patch to be agnostic of those factors. This test will implicitly test this addition, as well, as this layout test would not work without it.

>> Source/WebCore/accessibility/AccessibilityTable.cpp:339
>> +        if (firstRow && currentParent == firstRow && rowIsAllTableHeaderCells &&  cellCountForEachRow.get(currentParent.get()) >= 1 && rowCount >= 2)
> 
> Seems like we accidentally added an extra space between && and cellCountForEachRow.

Will remove that, thanks!

>> LayoutTests/accessibility/single-th-layout-table.html:35
>> +        await waitFor(() => accessibilityController.accessibleElementById('table1') != null);
> 
> Can we also verify the table's role, row count, and column count? Just to make sure it isn't being exposed as a group or something.

Yep, good call.

[Joshua Hoffman]

Created attachment 469294 [details]
Patch

[EWS]

Committed 272716@main (51f8361485f6): <https://commits.webkit.org/272716@main>

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