Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(table): sortable column headers will now use <button> element for a11y purposes #416

Merged
merged 1 commit into from
Oct 27, 2023
Merged

fix(table): sortable column headers will now use <button> element for a11y purposes #416

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
22 changes: 20 additions & 2 deletions src/lib/table/_mixins.scss
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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;
Expand Down
19 changes: 18 additions & 1 deletion src/lib/table/table-foundation.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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);
Expand Down Expand Up @@ -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,
Expand Down Expand Up @@ -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 {
Expand Down Expand Up @@ -979,6 +986,16 @@ export class TableFoundation implements ITableFoundation {
this._headCellMouseDownIndex = undefined;
}

private _onSortableHeadCellKeydown(evt: KeyboardEvent): void {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should this use click events instead of keydown and mousedown now that it's a button?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I had the same thought, but when I went to adjust that I realized the mousedown handler is currently overloaded to support multiple pieces of functionality and I didn't have the time to refactor everything out.

If we want to hold off on merging this until that is refactored, I'm fine to do so, but I don't know when I can get to it.

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
Expand Down
16 changes: 14 additions & 2 deletions src/lib/table/table-utils.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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);
}

Expand All @@ -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);
}
Expand All @@ -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);
}
}
Expand Down Expand Up @@ -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);
Expand Down
1 change: 1 addition & 0 deletions src/lib/table/types.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down
61 changes: 59 additions & 2 deletions src/test/spec/table/table.spec.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand Down Expand Up @@ -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;
Expand All @@ -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);
Expand All @@ -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) {
Expand All @@ -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();
});

Expand All @@ -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);
});
Expand Down Expand Up @@ -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');
});
Expand Down