From 43f775ed87237bdbab43f8bcd7afb75f1a9667f8 Mon Sep 17 00:00:00 2001
From: Kieran Nichols <kieran.nichols@tylertech.com>
Date: Fri, 27 Oct 2023 15:01:26 -0400
Subject: [PATCH] fix(table): sortable column headers will now use `<button>`
 element for a11y purposes (#416)

---
 src/lib/table/_mixins.scss        | 22 ++++++++++-
 src/lib/table/table-foundation.ts | 19 +++++++++-
 src/lib/table/table-utils.ts      | 16 +++++++-
 src/lib/table/types.ts            |  1 +
 src/test/spec/table/table.spec.ts | 61 ++++++++++++++++++++++++++++++-
 5 files changed, 112 insertions(+), 7 deletions(-)

diff --git a/src/lib/table/_mixins.scss b/src/lib/table/_mixins.scss
index 342aa0bd8..912c5a9b4 100644
--- a/src/lib/table/_mixins.scss
+++ b/src/lib/table/_mixins.scss
@@ -108,10 +108,14 @@
             display: none;
           }
         }
-  
+
         &--sortable {
           @include head-cell-sortable(forge-table-head__cell-sort-icon, forge-table-head__cell__sort-order);
-  
+
+          button {
+            @include head-cell-sortable-button;
+          }
+
           &.forge-table-head__cell--sorted-ascending {
             .forge-table-head__cell-sort-icon {
               @include head-cell-sort-icon-ascending;
@@ -433,6 +437,20 @@
   }
 }
 
+@mixin head-cell-sortable-button {
+  cursor: pointer;
+  border: none;
+  background: transparent;
+  text-align: inherit;
+  margin: inherit;
+  padding-block: 4px;
+  padding-inline: 0;
+  font: inherit;
+  color: inherit;
+  width: 100%;
+  outline-offset: 4px;
+}
+
 /// The base table body row styles for row interactions.
 @mixin body-row() {
   height: variables.$tbody-row-height;
diff --git a/src/lib/table/table-foundation.ts b/src/lib/table/table-foundation.ts
index 503fba5f1..f0ad422aa 100644
--- a/src/lib/table/table-foundation.ts
+++ b/src/lib/table/table-foundation.ts
@@ -105,6 +105,7 @@ export class TableFoundation implements ITableFoundation {
   private _rowDoubleClickListener: (evt: Event) => void;
   private _selectRowListener: (evt: Event) => void;
   private _selectAllListener: (evt: Event) => void;
+  private _sortableHeadCellKeydownListener: EventListener;
   private _headRowMouseDownListener: (evt: MouseEvent) => void;
   private _headRowContextMenuListener: (evt: MouseEvent) => void;
   private _documentMouseMoveListener: (evt: MouseEvent) => void;
@@ -132,6 +133,7 @@ export class TableFoundation implements ITableFoundation {
     this._rowDoubleClickListener = evt => this._onRowDoubleClick(evt);
     this._selectRowListener = evt => this._onRowSelected(evt);
     this._selectAllListener = evt => this._onSelectAll(evt);
+    this._sortableHeadCellKeydownListener = (evt: KeyboardEvent) => this._onSortableHeadCellKeydown(evt);
     this._headRowMouseDownListener = evt => this._onHeadRowMouseDown(evt);
     this._headRowContextMenuListener = evt => this._onHeadRowContextMenu(evt);
     this._documentMouseMoveListener = evt => this._onMouseMove(evt);
@@ -224,6 +226,7 @@ export class TableFoundation implements ITableFoundation {
       doubleClickListener: this._allowRowClick ? this._rowDoubleClickListener : null,
       selectListener: this._select ? this._selectRowListener : null,
       selectAllListener: this._multiselect ? this._selectAllListener : null,
+      sortableHeadCellKeydownListener: this._sortableHeadCellKeydownListener,
       headRowMouseDownListener: this._headRowMouseDownListener,
       headRowContextMenuListener: this._headRowContextMenuListener,
       filterListener: this._filter ? this._filterListener : null,
@@ -905,7 +908,11 @@ export class TableFoundation implements ITableFoundation {
   }
 
   private _onHeadRowContextMenu(evt: MouseEvent): void {
-    evt.preventDefault();
+    // We only handle this event on MacOS due to the `ctrl+click` combination triggering the contextmenu event...
+    // So we only detect that scenario here to still allow for the default context menu on Mac when right-clicking
+    if (evt.ctrlKey) {
+      evt.preventDefault();
+    }
   }
 
   private _onHeadRowMouseDown(evt: MouseEvent): void {
@@ -979,6 +986,16 @@ export class TableFoundation implements ITableFoundation {
     this._headCellMouseDownIndex = undefined;
   }
 
+  private _onSortableHeadCellKeydown(evt: KeyboardEvent): void {
+    if (evt.key === ' ' || evt.key === 'Enter') {
+      const composedPath = getEventPath(evt);
+      const rowElement = composedPath.find(el => el.tagName === 'TR') as HTMLTableRowElement;
+      const thElement = composedPath.find(el => el.tagName === 'TH') as HTMLTableCellElement;
+      const cellIndex = Array.from(rowElement.cells).findIndex(c => c === thElement);
+      this._onSort(cellIndex);
+    }
+  }
+
   /**
    * Called when a click event is triggered on the table header row.
    * We use this to capture all click events on the row, and determine which
diff --git a/src/lib/table/table-utils.ts b/src/lib/table/table-utils.ts
index ca146e718..bb79f5a9e 100644
--- a/src/lib/table/table-utils.ts
+++ b/src/lib/table/table-utils.ts
@@ -179,7 +179,14 @@ export class TableUtils {
       addClass([TABLE_CONSTANTS.classes.TABLE_CELL, TABLE_CONSTANTS.classes.TABLE_HEAD_CELL], th);
 
       // We wrap the header text in a div for ease of alignment
-      const cellContainer = document.createElement('div');
+      let cellContainer: HTMLElement;
+      if (columnConfig.sortable) {
+        cellContainer = document.createElement('button');
+        (cellContainer as HTMLButtonElement).type = 'button';
+        cellContainer.addEventListener('keydown', tableConfiguration.sortableHeadCellKeydownListener);
+      } else {
+        cellContainer = document.createElement('div');
+      }
       cellContainer.classList.add(TABLE_CONSTANTS.classes.TABLE_HEAD_CELL_CONTAINER);
 
       // Add tooltip for multisort
@@ -286,6 +293,7 @@ export class TableUtils {
    */
   public static setSortDirection(tableElement: HTMLTableElement, columnIndex: number, sortDirection: SortDirection): void {
     const cell = TableUtils._getHeaderCellByIndex(tableElement, columnIndex);
+    tableElement.querySelectorAll('th[aria-sort]').forEach(th => th.removeAttribute('aria-sort'));
     TableUtils._setColumnSortDirection(cell, sortDirection);
   }
 
@@ -294,7 +302,7 @@ export class TableUtils {
    * @param thElement
    * @param sortDirection
    */
-  private static _setColumnSortDirection(thElement: HTMLTableHeaderCellElement, sortDirection: SortDirection | undefined): void {
+  private static _setColumnSortDirection(thElement: HTMLTableCellElement, sortDirection: SortDirection | undefined): void {
     if (thElement.classList.contains(TABLE_CONSTANTS.classes.TABLE_HEAD_CELL_SORTED_ASCENDING)) {
       thElement.classList.remove(TABLE_CONSTANTS.classes.TABLE_HEAD_CELL_SORTED_ASCENDING);
     }
@@ -304,8 +312,10 @@ export class TableUtils {
     }
 
     if (!sortDirection || sortDirection === SortDirection.Descending) {
+      thElement.setAttribute('aria-sort', 'descending');
       thElement.classList.add(TABLE_CONSTANTS.classes.TABLE_HEAD_CELL_SORTED_DESCENDING);
     } else {
+      thElement.setAttribute('aria-sort', 'ascending');
       thElement.classList.add(TABLE_CONSTANTS.classes.TABLE_HEAD_CELL_SORTED_ASCENDING);
     }
   }
@@ -1052,6 +1062,8 @@ export class TableUtils {
   public static removeColumnSort(tableElement: HTMLTableElement, columnIndex: number): void {
     const cell = TableUtils._getHeaderCellByIndex(tableElement, columnIndex);
 
+    cell.removeAttribute('aria-sort');
+
     // Remove any existing sort direction classes from the existing th element
     if (cell.classList.contains(TABLE_CONSTANTS.classes.TABLE_HEAD_CELL_SORTED_ASCENDING)) {
       cell.classList.remove(TABLE_CONSTANTS.classes.TABLE_HEAD_CELL_SORTED_ASCENDING);
diff --git a/src/lib/table/types.ts b/src/lib/table/types.ts
index b4570c3e0..af4d74c3d 100644
--- a/src/lib/table/types.ts
+++ b/src/lib/table/types.ts
@@ -36,6 +36,7 @@ export interface ITableConfiguration {
   doubleClickListener: ((evt: Event) => void) | null;
   selectListener: ((evt: Event) => void) | null;
   selectAllListener: ((evt: Event) => void) | null;
+  sortableHeadCellKeydownListener: EventListener;
   headRowMouseDownListener: (evt: Event) => void;
   headRowContextMenuListener: (evt: Event) => void;
   filterListener: ((value: any, columnIndex: number) => void) | null;
diff --git a/src/test/spec/table/table.spec.ts b/src/test/spec/table/table.spec.ts
index 69cc3d8c2..16a06ba4c 100644
--- a/src/test/spec/table/table.spec.ts
+++ b/src/test/spec/table/table.spec.ts
@@ -259,7 +259,9 @@ describe('TableComponent', function(this: ITestContext) {
 
       expect(firstCell.classList.contains(TABLE_CONSTANTS.classes.TABLE_HEAD_CELL_SORTABLE)).toBe(true, 'Expected sortable class on first cell');
       expect(secondCell.classList.contains(TABLE_CONSTANTS.classes.TABLE_HEAD_CELL_SORTABLE)).toBe(true, 'Expected sortable class on second cell');
-      expect(firstCell.classList.contains(TABLE_CONSTANTS.classes.TABLE_HEAD_CELL_SORTED_DESCENDING)).toBe(true, 'Expected sort ascending class on first cell');
+      expect(firstCell.classList.contains(TABLE_CONSTANTS.classes.TABLE_HEAD_CELL_SORTED_DESCENDING)).toBe(true, 'Expected sort descending class on first cell');
+      expect(firstCell.getAttribute('aria-sort')).toBe('descending', 'Expected aria-sort to be ascending');
+      expect(secondCell.hasAttribute('aria-sort')).toBeFalse();
       expect(activelySortedCells.length).toBe(1, 'Expected only 1 actively sorted column');
       expect(sortIconElement).toBeDefined();
       expect((<HTMLElement>sortIconElement).classList.contains(TABLE_CONSTANTS.classes.TABLE_HEAD_CELL_SORT_ICON_ACTIVE)).toBe(true);
@@ -917,6 +919,40 @@ describe('TableComponent', function(this: ITestContext) {
       expect(callback).toHaveBeenCalled();
     });
 
+    it('should emit sort event when pressing space key on sortable column', async function(this: ITestContext) {
+      this.context = setupTestContext();
+      const testColumns = deepCopy(columns);
+      testColumns[0].sortable = true;
+
+      this.context.component.columnConfigurations = testColumns;
+
+      const callback = jasmine.createSpy('callback');
+      this.context.component.addEventListener(TABLE_CONSTANTS.events.SORT, callback);
+
+      const headerRow = getTableHeaderRow(this.context.getTableElement());
+      const firstCell = headerRow.cells.item(0) as HTMLTableCellElement;
+      firstCell.querySelector('button')?.dispatchEvent(new KeyboardEvent('keydown', { key: ' ' }));
+
+      expect(callback).toHaveBeenCalled();
+    });
+
+    it('should emit sort event when pressing enter key on sortable column', async function(this: ITestContext) {
+      this.context = setupTestContext();
+      const testColumns = deepCopy(columns);
+      testColumns[0].sortable = true;
+
+      this.context.component.columnConfigurations = testColumns;
+
+      const callback = jasmine.createSpy('callback');
+      this.context.component.addEventListener(TABLE_CONSTANTS.events.SORT, callback);
+
+      const headerRow = getTableHeaderRow(this.context.getTableElement());
+      const firstCell = headerRow.cells.item(0) as HTMLTableCellElement;
+      firstCell.querySelector('button')?.dispatchEvent(new KeyboardEvent('keydown', { key: 'Enter' }));
+
+      expect(callback).toHaveBeenCalled();
+    });
+
     it('should not emit sort event clicking non-sortable column', function(this: ITestContext) {
       this.context = setupTestContext();
       this.context.component.columnConfigurations = columns;
@@ -930,6 +966,20 @@ describe('TableComponent', function(this: ITestContext) {
       expect(callback).not.toHaveBeenCalled();
     });
 
+    it('should not emit sort event pressing enter or space key on non-sortable column', function(this: ITestContext) {
+      this.context = setupTestContext();
+      this.context.component.columnConfigurations = columns;
+
+      const callback = jasmine.createSpy('callback');
+      this.context.component.addEventListener(TABLE_CONSTANTS.events.SORT, callback);
+
+      const headerRow = getTableHeaderRow(this.context.getTableElement());
+      headerRow.cells.item(0)!.dispatchEvent(new KeyboardEvent('keydown', { key: ' ' }));
+      headerRow.cells.item(0)!.dispatchEvent(new KeyboardEvent('keydown', { key: 'Enter' }));
+
+      expect(callback).not.toHaveBeenCalled();
+    });
+
     it('should toggle sort direction when clicking same column', function(this: ITestContext) {
       this.context = setupTestContext();
       const testColumns = deepCopy(columns);
@@ -942,9 +992,11 @@ describe('TableComponent', function(this: ITestContext) {
 
       clickTableCell(firstCell);
       expect(firstCell.classList.contains(TABLE_CONSTANTS.classes.TABLE_HEAD_CELL_SORTED_ASCENDING)).toBe(true);
+      expect(firstCell.getAttribute('aria-sort')).toBe('ascending');
 
       clickTableCell(firstCell);
       expect(firstCell.classList.contains(TABLE_CONSTANTS.classes.TABLE_HEAD_CELL_SORTED_DESCENDING)).toBe(true);
+      expect(firstCell.getAttribute('aria-sort')).toBe('descending');
     });
 
     it('should not sort column when clicking non-sortable column', function(this: ITestContext) {
@@ -968,6 +1020,7 @@ describe('TableComponent', function(this: ITestContext) {
 
       expect(callback).not.toHaveBeenCalled();
       expect(hasSortClass).toBe(false);
+      expect(secondCell.hasAttribute('aria-sort')).toBeFalse();
       expect(sortIconElement).toBeNull();
     });
 
@@ -990,7 +1043,9 @@ describe('TableComponent', function(this: ITestContext) {
       const secondCellSortIcon = secondCell.querySelector(`.${TABLE_CONSTANTS.classes.TABLE_HEAD_CELL_SORT_ICON}`) as HTMLElement;
 
       expect(firstCell.classList.contains(TABLE_CONSTANTS.classes.TABLE_HEAD_CELL_SORTED_ASCENDING)).toBe(false);
+      expect(firstCell.hasAttribute('aria-sort')).toBeFalse();
       expect(secondCell.classList.contains(TABLE_CONSTANTS.classes.TABLE_HEAD_CELL_SORTED_ASCENDING)).toBe(true);
+      expect(secondCell.getAttribute('aria-sort')).toBe('ascending');
       expect(firstCellSortIcon.classList.contains(TABLE_CONSTANTS.classes.TABLE_HEAD_CELL_SORT_ICON_ACTIVE)).toBe(false);
       expect(secondCellSortIcon.classList.contains(TABLE_CONSTANTS.classes.TABLE_HEAD_CELL_SORT_ICON_ACTIVE)).toBe(true);
     });
@@ -1018,9 +1073,11 @@ describe('TableComponent', function(this: ITestContext) {
       const firstCellSortIcon = firstCell.querySelector(`.${TABLE_CONSTANTS.classes.TABLE_HEAD_CELL_SORT_ICON}`) as HTMLElement;
       const secondCellSortIcon = secondCell.querySelector(`.${TABLE_CONSTANTS.classes.TABLE_HEAD_CELL_SORT_ICON}`) as HTMLElement;
 
-      expect(firstCell.classList.contains(TABLE_CONSTANTS.classes.TABLE_HEAD_CELL_SORTED_ASCENDING)).toBe(true, 'First column should have descending sort class');
+      expect(firstCell.classList.contains(TABLE_CONSTANTS.classes.TABLE_HEAD_CELL_SORTED_ASCENDING)).toBe(true, 'First column should have ascending sort class');
+      expect(firstCell.getAttribute('aria-sort')).toBe('ascending');
       expect(secondCell.classList.contains(TABLE_CONSTANTS.classes.TABLE_HEAD_CELL_SORTED_ASCENDING)).toBe(false, 'Second column should not have descending sort class');
       expect(secondCell.classList.contains(TABLE_CONSTANTS.classes.TABLE_HEAD_CELL_SORTED_DESCENDING)).toBe(false, 'Second column should not have ascending sort class');
+      expect(secondCell.hasAttribute('aria-sort')).toBeFalse();
       expect(firstCellSortIcon.classList.contains(TABLE_CONSTANTS.classes.TABLE_HEAD_CELL_SORT_ICON_ACTIVE)).toBe(true, 'First column should have active sort icon');
       expect(secondCellSortIcon.classList.contains(TABLE_CONSTANTS.classes.TABLE_HEAD_CELL_SORT_ICON_ACTIVE)).toBe(false, 'Second column should not have active sort icon');
     });