fix(datatable): improve sortable header accessibility

src/DataTable/TableHeaderCell.tsx

@@ -31,7 +31,7 @@ interface TableHeaderCellProps {
   render: (type: 'Header') => React.ReactNode;
   /** Indicates whether the column is sorted in descending order */
   isSortedDesc?: boolean;
-  /** Gets props related to sorting that will be passed to th */
+  /** Gets props related to sorting that will be passed to the sort button */
   getSortByToggleProps?: (...args: any[]) => Record<string, any>;
   /** Indicates whether a column is sortable */
   canSort?: boolean;
@@ -48,14 +48,45 @@ function TableHeaderCell({
   isSortedDesc = false,
   headerClassName,
 }: TableHeaderCellProps) {
+  const { key, ...headerProps } = getHeaderProps();
   const toggleProps = canSort && getSortByToggleProps ? getSortByToggleProps() : {};
+  // Per WAI-ARIA APG sortable table guidance, only the actively sorted header should expose aria-sort.
+  let ariaSort: React.AriaAttributes['aria-sort'];
+  if (isSorted) {
+    ariaSort = isSortedDesc ? 'descending' : 'ascending';
+  }
+  const headerContentClassName = classNames('pgn__data-table-header-content', headerClassName);
 
   return (
-    <th {...getHeaderProps(toggleProps)}>
-      <span className={classNames('d-flex align-items-center', headerClassName)}>
-        <span>{render('Header')}</span>
-        {canSort && <SortIndicator isSorted={isSorted} isSortedDesc={isSortedDesc || false} />}
-      </span>
+    <th
+      key={key}
+      {...headerProps}
+      scope="col"
+      aria-sort={ariaSort}
+      className={classNames(
+        headerProps.className,
+        { 'pgn__data-table-sortable-header': canSort },
+      )}
+    >
+      {canSort ? (
+        <button
+          {...toggleProps}
+          type="button"
+          className={classNames(
+            'pgn__data-table-sort-button',
+            toggleProps.className,
+          )}
+        >
+          <span className={headerContentClassName}>
+            <span>{render('Header')}</span>
+            <SortIndicator isSorted={isSorted} isSortedDesc={isSortedDesc || false} />
+          </span>
+        </button>
+      ) : (
+        <span className={headerContentClassName}>
+          <span>{render('Header')}</span>
+        </span>
+      )}
     </th>
   );
 }

src/DataTable/TableHeaderRow.jsx

@@ -5,24 +5,32 @@ import TableHeaderCell from './TableHeaderCell';
 function TableHeaderRow({ headerGroups }) {
   return (
     <thead>
-      {headerGroups.map(headerGroup => (
-        <tr {...headerGroup.getHeaderGroupProps()}>
-          {headerGroup.headers.map(column => (
-            <TableHeaderCell {...column} {...column.getHeaderProps()} />
-          ))}
-        </tr>
-      ))}
+      {headerGroups.map((headerGroup) => {
+        const { key: headerGroupKey, ...headerGroupProps } = headerGroup.getHeaderGroupProps();
+
+        return (
+          <tr key={headerGroupKey} {...headerGroupProps}>
+            {headerGroup.headers.map((column) => {
+              const { key: headerKey } = column.getHeaderProps();
+
+              return (
+                <TableHeaderCell key={headerKey} {...column} />
+              );
+            })}
+          </tr>
+        );
+      })}
     </thead>
   );
 }
 
 TableHeaderRow.propTypes = {
   headerGroups: PropTypes.arrayOf(PropTypes.shape({
     headers: PropTypes.arrayOf(PropTypes.shape({
-      /** Props for the TableHeaderCell component. Must include a key */
+      /** Props for the TableHeaderCell component. Must include a React key */
       getHeaderProps: PropTypes.func.isRequired,
     })).isRequired,
-    /** Returns props for the header tr element */
+    /** Returns props for the header tr element. Must include a React key */
     getHeaderGroupProps: PropTypes.func.isRequired,
   })).isRequired,
 };

src/DataTable/index.scss

@@ -146,6 +146,36 @@
     background-color: var(--pgn-color-light-300);
     padding: var(--pgn-spacing-data-table-padding-cell);
     text-align: start;
+
+    // The button owns sortable header padding so the full visible header remains the sort hit target.
+    &.pgn__data-table-sortable-header {
+      padding: 0;
+      position: relative;
+    }
+  }
+
+  .pgn__data-table-header-content {
+    display: flex;
+    align-items: center;
+  }
+
+  .pgn__data-table-sort-button {
+    background: transparent;
+    border: 0;
+    color: inherit;
+    cursor: pointer;
+    font: inherit;
+    padding: var(--pgn-spacing-data-table-padding-cell);
+    text-align: inherit;
+    width: 100%;
+    box-sizing: border-box;
+
+    // Extend pointer hit testing to the full sortable th when row height is set by another header cell.
+    &::before {
+      content: "";
+      position: absolute;
+      inset: 0;
+    }
   }
 
   td {

src/DataTable/tests/DataTable.test.jsx

@@ -1,5 +1,7 @@
 import React, { useContext } from 'react';
-import { render, screen, waitFor } from '@testing-library/react';
+import {
+  render, screen, waitFor, within,
+} from '@testing-library/react';
 import userEvent from '@testing-library/user-event';
 import * as reactTable from 'react-table';
 import { IntlProvider } from 'react-intl';
@@ -145,6 +147,36 @@ describe('<DataTable />', () => {
     expect(screen.getAllByRole('row')).toHaveLength(props.data.length + 1); // (need + 1 to include header row)
   });
 
+  it('sorts rows when activating a sortable header button', async () => {
+    const getNameColumnValues = () => screen.getAllByRole('row')
+      .slice(1)
+      .map(row => within(row).getAllByRole('cell')[0].textContent);
+
+    render(<DataTableWrapper {...props} isSortable />);
+
+    expect(getNameColumnValues()).toEqual([
+      'Lil Bub',
+      'Grumpy Cat',
+      'Smoothie',
+      'Maru',
+      'Keyboard Cat',
+      'Long Cat',
+      'Zeno',
+    ]);
+
+    await userEvent.click(screen.getByRole('button', { name: 'Name' }));
+
+    await waitFor(() => expect(getNameColumnValues()).toEqual([
+      'Grumpy Cat',
+      'Keyboard Cat',
+      'Lil Bub',
+      'Long Cat',
+      'Maru',
+      'Smoothie',
+      'Zeno',
+    ]));
+  });
+
   it('displays a table footer', () => {
     render(<DataTableWrapper {...props} />);
     expect(screen.getByTestId('table-footer')).toBeInTheDocument();

src/DataTable/tests/TableHeaderCell.test.jsx

@@ -1,9 +1,10 @@
 import React from 'react';
 import { render, screen } from '@testing-library/react';
+import userEvent from '@testing-library/user-event';
 
 import TableHeaderCell from '../TableHeaderCell';
 
-const sortByToggleProps = { foo: 'bar' };
+const sortByToggleProps = { 'data-sort-prop': 'bar' };
 const props = {
   getHeaderProps: () => ({ className: 'red' }),
   render: () => 'Title',
@@ -20,21 +21,41 @@ function FakeTable({ ...rest }) {
 
 describe('<TableHeaderCell />', () => {
   describe('unsorted', () => {
-    render(<FakeTable {...props} />);
-    const cell = screen.getByRole('columnheader');
-    const innerCell = cell.firstChild;
-
     it('renders a table header cell', () => {
+      render(<FakeTable {...props} />);
+      const cell = screen.getByRole('columnheader');
       expect(cell).toBeInTheDocument();
     });
 
     it('adds props to the cell', () => {
+      render(<FakeTable {...props} />);
+      const cell = screen.getByRole('columnheader');
       expect(cell.className).toBe('red');
     });
 
+    it('adds column scope to the cell', () => {
+      render(<FakeTable {...props} />);
+      const cell = screen.getByRole('columnheader');
+      expect(cell).toHaveAttribute('scope', 'col');
+    });
+
     it('adds the headerClassName to inner span', () => {
+      render(<FakeTable {...props} />);
+      const cell = screen.getByRole('columnheader');
+      const innerCell = cell.firstChild;
       expect(innerCell.className).toContain(props.headerClassName);
     });
+
+    it('does not render a button for non-sortable headers', () => {
+      render(<FakeTable {...props} />);
+      expect(screen.queryByRole('button')).not.toBeInTheDocument();
+    });
+
+    it('does not add sortable header styling to non-sortable headers', () => {
+      render(<FakeTable {...props} />);
+      const cell = screen.getByRole('columnheader');
+      expect(cell).not.toHaveClass('pgn__data-table-sortable-header');
+    });
   });
 
   describe('with sorting', () => {
@@ -56,10 +77,70 @@ describe('<TableHeaderCell />', () => {
       expect(sortIndicator).toBeInTheDocument();
     });
 
-    it('adds the toggle props to the header props if toggle props are available', () => {
+    it('renders the sort toggle as a button', () => {
+      render(<FakeTable {...props} canSort />);
+      expect(screen.getByRole('button', { name: 'Title' })).toBeInTheDocument();
+    });
+
+    it('adds sortable header styling to preserve the sort button hit target', () => {
+      render(<FakeTable {...props} canSort />);
+      const cell = screen.getByRole('columnheader');
+      const button = screen.getByRole('button', { name: 'Title' });
+      expect(cell).toHaveClass('pgn__data-table-sortable-header');
+      expect(button).toHaveClass('pgn__data-table-sort-button');
+    });
+
+    it('adds headerClassName to sortable header content', () => {
+      render(<FakeTable {...props} canSort />);
+      const button = screen.getByRole('button', { name: 'Title' });
+      expect(button).not.toHaveClass(props.headerClassName);
+      expect(button.firstChild).toHaveClass(props.headerClassName);
+    });
+
+    it('adds ascending sort state to the header cell when sorted ascending', () => {
+      render(<FakeTable {...props} canSort isSorted />);
+      const cell = screen.getByRole('columnheader');
+      expect(cell).toHaveAttribute('aria-sort', 'ascending');
+    });
+
+    it('adds descending sort state to the header cell when sorted descending', () => {
+      render(<FakeTable {...props} canSort isSorted isSortedDesc />);
+      const cell = screen.getByRole('columnheader');
+      expect(cell).toHaveAttribute('aria-sort', 'descending');
+    });
+
+    it('does not add inactive sort state to unsorted header cells', () => {
+      render(<FakeTable {...props} canSort />);
+      const cell = screen.getByRole('columnheader');
+      expect(cell).not.toHaveAttribute('aria-sort');
+    });
+
+    it('adds the toggle props to the sort button if toggle props are available', () => {
+      render(<FakeTable {...props} canSort />);
+      const button = screen.getByRole('button', { name: 'Title' });
+      expect(button).toHaveAttribute('data-sort-prop', sortByToggleProps['data-sort-prop']);
+    });
+
+    it('does not pass toggle props to the header props', () => {
       const headerPropsSpy = jest.fn().mockReturnValueOnce({});
       render(<FakeTable {...props} canSort getHeaderProps={headerPropsSpy} />);
-      expect(headerPropsSpy).toHaveBeenCalledWith(sortByToggleProps);
+      expect(headerPropsSpy).toHaveBeenCalledWith();
+    });
+
+    it('makes sortable headers keyboard reachable and operable', async () => {
+      const user = userEvent.setup();
+      const handleSort = jest.fn();
+      render(<FakeTable {...props} canSort getSortByToggleProps={() => ({ onClick: handleSort })} />);
+      const button = screen.getByRole('button', { name: 'Title' });
+
+      await user.tab();
+      expect(button).toHaveFocus();
+
+      await user.keyboard('{Enter}');
+      expect(handleSort).toHaveBeenCalledTimes(1);
+
+      await user.keyboard(' ');
+      expect(handleSort).toHaveBeenCalledTimes(2);
     });
   });
 });

src/DataTable/tests/TableHeaderRow.test.jsx

@@ -31,24 +31,51 @@ const props = {
   }],
 };
 
-describe('<TableHeaderRow />', () => {
+function renderTableHeaderRow() {
   render(<table><TableHeaderRow {...props} /></table>);
-  const head = screen.getByRole('rowgroup');
-  const row = screen.getByRole('row');
-  const cells = screen.getAllByRole('columnheader');
+}
 
+describe('<TableHeaderRow />', () => {
   it('renders a table head and row', () => {
+    renderTableHeaderRow();
+
+    const head = screen.getByRole('rowgroup');
+    const row = screen.getByRole('row');
+
     expect(head).toBeInTheDocument();
     expect(row).toBeInTheDocument();
   });
 
   it('adds props to the row', () => {
+    renderTableHeaderRow();
+
+    const row = screen.getByRole('row');
+
     expect(row.className).toEqual('red');
   });
 
   it('renders cells', () => {
+    renderTableHeaderRow();
+
+    const cells = screen.getAllByRole('columnheader');
+
     expect(cells.length).toEqual(2);
     expect(cells[0]).toHaveTextContent(header1Name);
     expect(cells[1]).toHaveTextContent(header2Name);
   });
+
+  it('does not spread React keys from react-table prop getters', () => {
+    const consoleError = jest.spyOn(console, 'error').mockImplementation(() => {});
+
+    try {
+      renderTableHeaderRow();
+
+      const keySpreadWarning = consoleError.mock.calls.some(([message]) => (
+        String(message).includes('A props object containing a "key" prop')
+      ));
+      expect(keySpreadWarning).toEqual(false);
+    } finally {
+      consoleError.mockRestore();
+    }
+  });
 });