[gtritchie]

DataGrid keeps its header fixed while the body scrolls by rendering three separate <table> elements (header, body, footer) inside a HeaderPanel. Nothing ties them together, so assistive technology sees a table of column headers with no data and a separate data table with no headers, and moving through body cells announces no column header. CellTable renders a single <table> and is unaffected. Addresses rstudio/rstudio#19173.

Changes

• The DataGrid element gets role="table". Its three inner <table> elements get role="presentation", and their thead/tbody/tfoot get role="rowgroup". Everything is a DOM descendant of the role="table" element (the divs in between are generic), so the browser builds one table and computes column headers by cell position. The DOM, CSS and layout are unchanged.
• A presentational table also makes its rows and cells presentational, so they need explicit roles. New protected void AbstractCellTable.onSectionRendered(TableSectionElement section), a no-op by default, is called with newly rendered rows before they enter the document: by Impl.replaceAllRows (full body redraws, and header/footer rendering) with the table's own detached section, and by Impl.replaceChildren (partial body redraws) with the temporary section that holds only the new rows. DataGrid overrides it to give each <tr> role="row", each <th> columnheader (in the head or foot) or rowheader (in the body), and each <td> cell. Doing this after rendering covers every table builder, including custom ones, without changing them.
• Rows and cells that already have a role keep it; an empty role counts as none. A row whose role is something other than row, such as a layout-only row a builder marks presentation, is skipped along with its cells.
• The loading-indicator and empty-table containers are FlexTables that replace the body table while loading or empty. Their table gets role="presentation", and while they hold a widget their tbody, row and cell get rowgroup, row and cell, so the message reads as a row of the grid (Chrome: row > cell "Environment is empty") rather than a nested table or text outside any row. A null widget, which disables the message, gets no row roles, so the empty cell doesn't read as a blank row.

Not changed: when a grid has a visible footer, HeaderPanel puts the footer container before the content container in the DOM, so the footer rowgroup comes before the body rows in the accessibility tree. A <th> in the footer is a columnheader, as Chrome maps a <th> in a native <tfoot>. No RStudio grid has a footer.

RStudio needs a companion change shipped with the SDK update: rows that only exist for layout (the virtualized grid's padding rows, the Environment list's width-setting row) must carry role="presentation", or they read as blank rows. That change and the GWT_VERSION bump go in the same rstudio PR.

Testing

New tests in DataGridTest:

• testTableRoles: the container, table, rowgroup, row, columnheader and cell roles after a full render.
• testTableRolesAfterPartialRedraw: a row replaced by setRowData(start, values) gets row and cell roles.
• testPartialRedrawOnlyStampsNewRows: a partial redraw leaves rows outside the replaced range untouched.
• testTableRolesKeepExplicitRoles: a builder row marked presentation keeps that role, and its cells get none.
• testTableRolesTreatEmptyRoleAsAbsent: a row and cell with role="" get row and cell.
• testTableRolesKeepAriaSort: a sorted header's <th> has both columnheader and aria-sort.
• testTableRolesBodyThIsRowHeader: a <th> rendered in the body is a rowheader.
• testTableRolesFooter: a footer row is a row and its <th> a columnheader.
• testLoadingAndEmptyMessagesArePresentational: in the loading and empty states, the displayed widget is in a cell of a row of a rowgroup in a presentational table inside the grid.
• testNullLoadingIndicatorAddsNoRow: with the loading indicator disabled, its empty cell and row have no role.

testSortableHeaderAria in AbstractCellTableTestBase asserted that a header <th> has no role. It now requires no role attribute for CellTable and columnheader for DataGrid.

Each test except testTableRolesFooter failed before the change it covers, or, for testTableRolesBodyThIsRowHeader, fails when the code it covers is broken (rowheader swapped for columnheader). testTableRolesFooter covers footer rendering, which the role code already handled.

Results:

• CellTableTest and DataGridTest pass in HtmlUnit (96 tests).
• checkstyle and apicheck pass, and ant clean dist-dev builds.
• With RStudio built against this SDK, e2e tests confirm that the Files pane (300 files, so the virtualized grid adds padding rows) and the Environment grid view each expose one table with column headers and body cells, and that an empty Environment grid shows its message as the table's second row. Against 2.13.1-montauk-daisy-2, the Files and empty Environment tests fail with two tables.
• Chrome's accessibility tree (CDP) for the Files pane shows one table: a header row of five column headers, then the file rows, with no nested table.
• VoiceOver in Chrome and Safari: in the Files pane and Environment grid view, moving through body cells announces each cell's column header. In an empty Environment grid view, Safari read "Environment is empty" as a row of the table; that check ran before the message container got its row roles.

[gtritchie]

Code review findings

Actionable items from an automated review.

Important

1. user/src/com/google/gwt/user/cellview/client/DataGrid.java:694 — the loading/empty message isn't in a row. With the FlexTable presentational, Chrome's accessibility tree for an empty grid shows table > rowgroup > row > columnheader… followed by a bare generic "Environment is empty". VoiceOver reads it anyway, but Windows screen readers that use Chrome's tree have no table row to move to. Fix: give the container's tr/td role="row"/role="cell", which Chrome then exposes as row > cell "Environment is empty".

2. user/src/com/google/gwt/user/cellview/client/DataGrid.java:521 — rows with no role become role="row", so layout rows have to opt out. On rstudio main, VirtualizedDataGrid.drawPaddingRow sets no role, so the Files pane would expose blank rows above and below the visible window. The companion RStudio change (85badb60ec on bugfix/datagrid-table-roles) marks them presentation. Fix: ship the SDK update together with that commit.

3. user/src/com/google/gwt/user/cellview/client/AbstractCellTable.java:600 — replaceChildren passes the whole tbody to onSectionRendered, so every partial redraw re-checks every row and cell. That's about 1,800 attribute reads for a one-row update in a 300×5 grid. The presenter takes this path for any change under 30% of rows, up to twice per flush. Fix: apply roles only to the inserted rows (e.g. pass the inserted range, or process newSection's rows before moving them).

Suggestions

4. user/test/com/google/gwt/user/cellview/client/AbstractCellTableTestBase.java:1227 (and :1236) — for CellTable, assertEquals("", getAttribute("role")) also passes when role="" is present, because DOMImpl.getAttribute returns '' for a missing attribute. Fix: keep assertFalse(hasAttribute("role")) when getHeaderCellRole() is empty.

5. user/test/com/google/gwt/user/cellview/client/DataGridTest.java:103 — no test renders a footer, so footer row/th roles and the onSectionRendered call from createHeaders(true) are untested. Fix: add a column with a footer header and assert the tfoot row and cell roles.

6. user/src/com/google/gwt/user/cellview/client/DataGrid.java:523 — a row with role="" passes hasAttribute, fails the "row" check, and is skipped, so its cells drop out of the accessibility tree. No current builder does this. Fix: read the role once and treat "" as absent, which also removes the duplicate attribute lookup.

7. user/src/com/google/gwt/user/cellview/client/AbstractCellTable.java:531 (and :600) — the hook call sits between the "Reattach the section…" comment and reattachSectionElement, so the comment now describes the wrong line. Fix: move the call above the comment.

8. user/src/com/google/gwt/user/cellview/client/DataGrid.java:517 — isBody comes from a tag-name string comparison, and the cell role is chosen with a nested ternary inside the loop. Fix: pass the th role into applyTableRoles, computed once in onSectionRendered from section == tableData.section.

[gtritchie]

Addressed in 1a73a40..ea4d423:

1. Message not in a row — fixed (aaaf133). While a container holds a widget, its tbody, row and cell get rowgroup/row/cell; Chrome shows row > cell "Environment is empty". roborev then caught that a null widget (which disables the message) would leave a blank row, so roles are only set while there is a widget (ea4d423, testNullLoadingIndicatorAddsNoRow).
2. Padding rows on rstudio main — no fork change. The SDK bump will land on the same rstudio branch as 85badb60ec; the description now says so.
3. Partial redraw walks the whole body — fixed (1a73a40). replaceChildren now passes the temporary section holding only the new rows, before they are moved in (testPartialRedrawOnlyStampsNewRows).
4. role="" passes the CellTable header assertion — fixed (125dca0): no role attribute is required when none is expected.
5. No footer test — added testTableRolesFooter (125dca0). A footer <th> is a columnheader, as Chrome maps a <th> in a native <tfoot>.
6. role="" row skipped — fixed (05c0d87): each role is read once and an empty role counts as none, for rows and cells (testTableRolesTreatEmptyRoleAsAbsent).
7. Comment split from reattachSectionElement — fixed (1a73a40); the call now sits above the comment, and in replaceChildren it moved next to convertToSectionElement.
8. Body/head check — the th role is now chosen once per section (05c0d87). It stays a tag-name check instead of section == tableData.section, because after (3) a partial redraw passes a temporary tbody, which isn't tableData.section.

[gtritchie]

Re-reviewed through ea4d423. All 8 findings from the earlier comment are addressed: 1 by the companion-change note in the description, the rest in code and tests. CellTableTest and DataGridTest pass locally in HtmlUnit (96 tests). No new findings worth addressing.