Repository navigation
Keep columnheader role on sortable cell table headers - #20
Conversation
DefaultHeaderOrFooterBuilder put role="button" and tabindex=-1 on
sortable <th> elements, replacing their columnheader role, so screen
readers got no column header and aria-sort had nowhere to go.
The th now keeps its role and carries aria-sort ("ascending" or
"descending") on the sorted column. The role="button" and tabindex=-1
move to a div wrapping the header content. It's a div rather than a
<button> because the table sorts on Enter keydown, and the click a
<button> synthesizes from Enter would sort again.
The sort icon's alt text ("Ascending sort"/"Descending sort") is now
empty, since aria-sort reports the direction and the alt text was
being read as part of the header's name.
Addresses rstudio/rstudio#19170.
Moving the sortable header content into a button div moved the __gwt_header marker off the th. Clicks on the th's padding, or on a sorted header's button itself rather than its text, then found no header: Header.onBrowserEvent and onPreviewColumnSortEvent didn't run, so a header couldn't veto those sorts, and headers that sort through their own cell (like a ClickableTextCell) didn't respond. Add enableHeaderHandlers(), the header counterpart of enableColumnHandlers(), and mark sortable th elements with it, as unsorted ones were before. Addresses rstudio/rstudio#19170.
Marking a sorted header's th sent clicks on its sort icon to the header's cell with the th as the parent, so an interactive header cell such as an ActionCell treated an icon click as a click on itself. Before the sort button existed, sorted headers never passed icon clicks to the cell, while unsorted headers rendered straight into the th, so events on its padding reached the header. Mark the th only when the column is unsorted, which keeps both behaviors. Addresses rstudio/rstudio#19170.
gtritchie
left a comment
gtritchie
left a comment
There was a problem hiding this comment.
Code review findings: one regression in RStudio (custom header builders), plus smaller items.
| * @return the rendered HTML | ||
| */ | ||
| private SafeHtml getSortIcon(boolean isAscending) { | ||
| // The icon is decorative: the header builder reports the sort direction with |
There was a problem hiding this comment.
Custom header builders lose the sort direction. The alt text is removed here in shared code, but only DefaultHeaderOrFooterBuilder adds aria-sort. RStudio's EnvironmentObjectGrid.GridHeaderBuilder (Environment pane, grid view) extends AbstractHeaderOrFooterBuilder, calls renderSortableHeader(cell, ..., i == host_.getSortColumn(), host_.getAscendingSort()), and never sets aria-sort. Before this change, screen readers read "Ascending sort"/"Descending sort" from the icon there. After it, the icon has alt="" and the th has no aria-sort, so the sort direction is no longer exposed.
One option: a protected helper on AbstractHeaderOrFooterBuilder that sets aria-sort on the th, called by DefaultHeaderOrFooterBuilder and by RStudio's GridHeaderBuilder. Alternatively, update GridHeaderBuilder in RStudio alongside the SDK bump. The comment here ("the header builder reports the sort direction") should also say that a custom builder has to do this itself.
There was a problem hiding this comment.
Not changing this in the fork. Agreed that a custom builder has to set aria-sort itself. The RStudio branch for rstudio/rstudio#19170 (not pushed yet) already does that in EnvironmentObjectGrid.GridHeaderBuilder: it sets aria-sort on the sorted column's th, and also wraps each header in the same role="button" div. In a dev server against this SDK, the Environment grid's th gets aria-sort ascending/descending as you sort. That RStudio branch can't ship without this SDK, since it also calls enableHeaderHandlers(), so the alt text can't go away there without aria-sort arriving. No other RStudio header builder renders sortable headers (rstudio-pro has none either).
| // rather than a <button> because the table sorts on Enter keydown, and the click | ||
| // a <button> synthesizes from Enter would sort a second time. | ||
| if (isSorted) { | ||
| th.attribute("aria-sort", isSortAscending ? "ascending" : "descending"); |
There was a problem hiding this comment.
aria-sort can be wrong on a header that spans several columns. This predates the PR, but aria-sort now inherits it. In buildHeaderOrFooterImpl, each column under a shared header overwrites isSorted = (column == sortedColumn), so for colspan > 1 it reflects only the last column. If the header covers sortable columns A and B and the table is sorted by A, the th gets no aria-sort (and no sorted style or icon). It's an unusual setup, so this is low priority.
There was a problem hiding this comment.
Not changing this. No RStudio table shares one header across several columns: I checked every addColumn call for a header object passed more than once, and the Environment grid's custom builder renders one th per column. As you note, the sorted style and icon already had the same limitation before this PR.
| // reached the header. Keep that. Sorted ones never passed sort icon events to the | ||
| // header, and marking their th would make an interactive header cell (such as an | ||
| // ActionCell) treat an icon click as its own. | ||
| enableHeaderHandlers(th, header); |
There was a problem hiding this comment.
The header cell gets different parent elements depending on where the click lands (minor). With both the unsorted th and the inner button div marked as the header, Header.onBrowserEvent gets the th for padding clicks and the div for content clicks. Under the th, parent.getFirstChildElement() is now the button wrapper, not the cell's content. ActionCell and ButtonCellBase check the event target, so they're fine. A cell that rewrites parent on click, such as EditTextCell, would run setValue(context, th, ...) on a padding click, replacing the th's innerHTML and dropping the role=button div until the headers redraw. This is a rare configuration; mentioning it in case it's worth a note in the comment.
There was a problem hiding this comment.
Not changing this. RStudio's sortable headers use TextCell (string and TextHeader headers), SafeHtml headers, and ResizableHeader's cell, which consumes no events. So Header.onBrowserEvent never runs for them, and RStudio has no EditTextCell or other parent-rewriting cell in a header. Unsorted headers also got the th as parent before this PR, for every click. The difference now is that getFirstChildElement() is the wrapper rather than the content.
| Element iconWrapper = getHeaderElement(table, 0).getFirstChildElement().getFirstChildElement(); | ||
| iconWrapper.getFirstChildElement().dispatchEvent(click); | ||
| iconWrapper.getLastChild().<Element>cast().dispatchEvent(click); | ||
| assertEquals(Arrays.asList("th", "div"), cellClicks); |
There was a problem hiding this comment.
Test gap: after push(column), the test checks cellClicks and previews but not sorts. The PR description says an icon click on a sorted header still sorts (without a preview) and the text click is vetoed by the preview. Asserting sorts here (e.g. assertEquals(Arrays.asList(false), sorts), assuming the icon click toggles to descending) would cover that.
There was a problem hiding this comment.
Not adding this: it doesn't change behavior in RStudio. For the record, the icon click on the sorted header does sort without a preview (the first version of this test asserted no sort there and failed for that reason), and the text click is vetoed. Happy to add the assertion if you'd like it pinned.
| AbstractImagePrototype proto = | ||
| AbstractImagePrototype.create(table.getResources().sortAscending()); | ||
| sortAscIconHtml = addAltText(SafeHtmlUtils.fromTrustedString(proto.getHTML()), "Ascending sort"); | ||
| sortAscIconHtml = addAltText(SafeHtmlUtils.fromTrustedString(proto.getHTML()), ""); |
There was a problem hiding this comment.
Nit: both callers now pass "", so addAltText's altText parameter and the htmlEscape call no longer do anything. A helper that just inserts alt="" would be clearer.
There was a problem hiding this comment.
Agreed that the parameter and the escaping are now dead, but leaving it as is: no user-visible effect, and it keeps the diff to the alt values. Happy to simplify it if you prefer.
* Give the Files pane file-type column header readable text The icon column's header was an empty <span aria-label="File Type">, where aria-label is not permitted, so the header had no name. It now holds visually hidden, localized "File type" text. Addresses #19170. * Report sort state on Environment grid column headers The grid's custom header builder now sets aria-sort on the sorted column's <th> and wraps each header in a role="button" div, matching GWT's DefaultHeaderOrFooterBuilder. The wrapper's text gets its own ellipsis, since text-overflow on the th doesn't reach it. Addresses #19170. * Retarget changelist sort icon padding past the header button GWT now wraps sortable header content in a role="button" div, so ".cellTableSortableHeader > div" would pad every sortable header's button instead of only the sorted header's icon container. Addresses #19170. * Add NEWS entry for sortable column header semantics Addresses #19170. * Mark the sortable column header NEWS entry as accessibility Addresses #19170. * Route clicks anywhere in an Environment grid header to its cell The grid sorts through each header's ClickableTextCell. For the sorted column, GWT marks only the text div inside the sort icon wrapper as the header, so a click on the new sort button itself (such as assistive technology activating it) found no header and didn't sort. Marking the th with enableHeaderHandlers() routes those clicks too. Addresses #19170. * Add an e2e test for Files pane column header semantics Checks that the Files pane's headers (including the icon column's "File type") are column headers, and that clicking the Name sort button puts aria-sort on that header alone and reverses it on each click. Fails against GWT 2.13.1-montauk-daisy-1, where the sortable th has role="button" and so isn't a column header. Addresses #19170. * Update GWT to 2.13.1-montauk-daisy-2 The new fork build keeps the columnheader role on sortable cell table headers, adds aria-sort to the sorted column, and moves the sort control into a role="button" div inside the th (rstudio/gwt#20). The Environment grid change on this branch depends on its new enableHeaderHandlers(). Addresses #19170. * Share sort state between the Environment header class and aria-sort Addresses #19170 * Build the file-type header span with SafeHtmlUtil.createOpenTag Addresses #19170 * Ellipsize sorted Environment grid headers Addresses #19170 * Redraw sortable headers after sorting an empty table GWT tables redraw their headers only when rows are pushed, and sorting an empty list in place pushes none, so the sort icon and aria-sort kept showing the previous sort. Addresses #19170 * Simplify the Files pane column header e2e test Addresses #19170 * Redraw Environment grid headers after sorting Sorting an empty environment pushes no rows, so the grid's sort icon and aria-sort kept showing the previous sort. The new e2e test also covers the grid's hand-built header markup. Addresses #19170 * Update the Files sort test's comment on header markup Addresses #19170 * Restore the Environment sort after the grid header test Addresses #19170
DefaultHeaderOrFooterBuilderputrole="button"andtabindex="-1"on each sortable<th>. That replaced the cell's columnheader role, so screen readers got no column header for any sortable column, and the sort direction couldn't be exposed witharia-sort, which belongs on the column header. Axe'sempty-table-headerrule also skipped these cells (it selectsth:not([role])), so empty sortable headers went unreported. Addresses rstudio/rstudio#19170.Changes
<th>keeps its columnheader role. The sorted column's<th>getsaria-sort="ascending"or"descending"; other columns get noaria-sort.role="button"andtabindex="-1"move to a<div>inside the<th>that wraps the header content (and the sort icon, when sorted). It's a<div>rather than a<button>because the table sorts on Enter keydown, and the click a<button>synthesizes from Enter would sort a second time.aria-sortreports the direction, and the alt text was being read as part of the header's name (e.g. "Descending sort Name").protected final AbstractHeaderOrFooterBuilder.enableHeaderHandlers(builder, header), the header counterpart ofenableColumnHandlers(): it puts the header marker attribute on an element, so events on it reach the header.renderHeader()now uses it.<th>is marked with it. Unsorted headers used to render straight into the<th>, so this keeps events on the<th>'s padding reaching the header (includingonPreviewColumnSortEvent). A sorted header's<th>is not marked. Sorted headers never passed sort icon clicks to the header, and marking the<th>would hand the whole icon wrapper to the header's cell, so an interactive cell such asActionCellwould treat an icon click as its own.Not changed: the sort control still isn't in the tab order and Space doesn't sort, as before. Clicks on a sorted header's icon or padding still sort without calling
onPreviewColumnSortEvent, as in upstream GWT.Testing
Three new tests in
AbstractCellTableTestBase, so they run for bothCellTableandDataGrid:testSortableHeaderAria: no role on the<th>, the inner button's role and tabindex,aria-sortfollowing the sort direction on the sorted column only, and an empty alt on the sort icon.testSortButtonActivation: a click or Enter on the inner button sorts once.testSortableHeaderEventsReachHeader: a click on an unsorted header's<th>reaches the header's cell and preview, and the preview can veto the sort; on a sorted header, a click on the text reaches the cell but a click on the sort icon doesn't.Each failed before the change it covers. Results:
CellTableTestandDataGridTestpass in HtmlUnit (86 tests).checkstyleandapicheckpass, andant clean dist-devbuilds.aria-sortfollows clicks on the Name sort button. It fails against2.13.1-montauk-daisy-1.empty-table-header.