From 5be33e36abffe890b932d57f8412f46f4df6714d Mon Sep 17 00:00:00 2001 From: Malcolm Smith <20709258+msmithNI@users.noreply.github.com> Date: Tue, 25 Jun 2024 13:23:35 -0500 Subject: [PATCH] Add keyboard navigation to table (#2172) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit # Pull Request ## ๐Ÿคจ Rationale Resolves #1137 ## ๐Ÿ‘ฉโ€๐Ÿ’ป Implementation See #2173 for the most up-to-date expected behavior of keyboard navigation that this PR implements. ([Direct link to updated HLD](https://github.com/ni/nimble/blob/96e1725fc7cc8b3fdf5f5197b090e8f95f86cd5f/packages/nimble-components/src/table/specs/table-keyboard-navigation-hld.md)) The `KeyboardNavigationManager` class does the bulk of the work: - Tracks what is currently focused / should be focused (via properties `focusType`, and `row/column/cellContent/headerActionIndex`) - Note: If the user clicks in the table away from the current keyboard focus, the focus state would end up out-of-date. There's code in `handleFocus()` to handle this (see additional comments there) - Listens for table keypresses to handle arrow key navigation, Tab/Shift-Tab, PgUp/PgDn/etc key presses - If the user unfocuses and refocuses the table, re-focuses the appropriate elements in the table (`handleFocus()` again) - Tab/Shift-Tab behavior should match the HLD / expectations for ARIA treegrid. (Basically, Tab/Shift-Tab go through the tabbable elements of the header row / current row, not including cells/column headers, until the end is reached, then the table blurs and elements before/after it will be focused with additional Tab presses.) The overall approach of only setting `tabindex=0` on the single element in the table we want focused is called [roving tabindex](https://www.freecodecamp.org/news/html-roving-tabindex-attribute-explained-with-examples/). To ensure that we control what elements in the table should get focused, this PR updates `tabindex` on many elements in the table: - The table itself is `tabindex=0` to ensure that it can be focused via `Tab` - Elements that are focusable by default such as buttons/anchors, are explicitly set to `tabindex=-1` to start with, until the keyboard navigation code wants to focus them (then they're set to `tabindex=0` just before being focused). ## ๐Ÿงช Testing - Manual testing (mostly Chrome, some Firefox). - Earlier builds had some Safari testing, but we should explicitly re-test there before submission. - Added autotests Keyboard navigation can be used on any/all of the [existing table Storybook pages](https://60e89457a987cf003efc0a5b-gjoayfsquj.chromatic.com/?path=/story/components-table--table), once the table is focused. ## โœ… Checklist - [x] I have updated the project documentation to reflect my changes or determined no changes are needed. - Added keyboard nav docs to [table Storybook docs page](https://60e89457a987cf003efc0a5b-gjoayfsquj.chromatic.com/?path=/docs/components-table--docs) (in Accessibility section) --- ...-df1cc26c-9976-477a-8fa0-f9b72492eaa0.json | 7 + .../table-column/anchor/cell-view/index.ts | 13 + .../table-column/anchor/cell-view/template.ts | 6 +- .../anchor/tests/table-column-anchor.spec.ts | 78 + .../src/table-column/base/cell-view/index.ts | 8 + .../src/table/components/cell/index.ts | 27 + .../src/table/components/cell/styles.ts | 19 + .../src/table/components/cell/template.ts | 12 +- .../cell/tests/table-cell.pageobject.ts | 2 +- .../components/cell/tests/table-cell.spec.ts | 64 + .../src/table/components/group-row/index.ts | 27 +- .../src/table/components/group-row/styles.ts | 7 + .../table/components/group-row/template.ts | 2 +- .../group-row/tests/table-group-row.spec.ts | 28 + .../src/table/components/header/styles.ts | 8 + .../src/table/components/row/index.ts | 57 +- .../src/table/components/row/styles.ts | 40 + .../src/table/components/row/template.ts | 6 +- .../components/row/tests/table-row.spec.ts | 32 + packages/nimble-components/src/table/index.ts | 86 +- .../models/keyboard-navigation-manager.ts | 1373 ++++++++++++++++ .../src/table/models/table-update-tracker.ts | 25 +- .../src/table/models/virtualizer.ts | 51 +- .../nimble-components/src/table/styles.ts | 22 + .../nimble-components/src/table/template.ts | 24 +- .../src/table/testing/table.pageobject.ts | 38 +- .../src/table/tests/table-action-menu.spec.ts | 40 + .../tests/table-keyboard-navigation.spec.ts | 1426 +++++++++++++++++ .../src/table/tests/table.spec.ts | 4 + .../src/table/tests/types.spec.ts | 6 + packages/nimble-components/src/table/types.ts | 42 + .../src/utilities/tests/component.ts | 16 + .../src/nimble/table/table-matrix.stories.ts | 33 +- packages/storybook/src/nimble/table/table.mdx | 67 +- packages/storybook/src/utilities/matrix.ts | 42 +- 35 files changed, 3641 insertions(+), 97 deletions(-) create mode 100644 change/@ni-nimble-components-df1cc26c-9976-477a-8fa0-f9b72492eaa0.json create mode 100644 packages/nimble-components/src/table/models/keyboard-navigation-manager.ts create mode 100644 packages/nimble-components/src/table/tests/table-keyboard-navigation.spec.ts diff --git a/change/@ni-nimble-components-df1cc26c-9976-477a-8fa0-f9b72492eaa0.json b/change/@ni-nimble-components-df1cc26c-9976-477a-8fa0-f9b72492eaa0.json new file mode 100644 index 0000000000..362e0db3b6 --- /dev/null +++ b/change/@ni-nimble-components-df1cc26c-9976-477a-8fa0-f9b72492eaa0.json @@ -0,0 +1,7 @@ +{ + "type": "minor", + "comment": "Add keyboard navigation functionality to the table component", + "packageName": "@ni/nimble-components", + "email": "20709258+msmithNI@users.noreply.github.com", + "dependentChangeType": "patch" +} diff --git a/packages/nimble-components/src/table-column/anchor/cell-view/index.ts b/packages/nimble-components/src/table-column/anchor/cell-view/index.ts index 9f97ac797a..b124fff803 100644 --- a/packages/nimble-components/src/table-column/anchor/cell-view/index.ts +++ b/packages/nimble-components/src/table-column/anchor/cell-view/index.ts @@ -54,9 +54,22 @@ TableColumnAnchorColumnConfig return ''; } + /** @internal */ + @volatile + public get showAnchor(): boolean { + return typeof this.cellRecord?.href === 'string'; + } + public override focusedRecycleCallback(): void { this.anchor?.blur(); } + + public override get tabbableChildren(): HTMLElement[] { + if (this.showAnchor) { + return [this.anchor!]; + } + return []; + } } const anchorCellView = TableColumnAnchorCellView.compose({ diff --git a/packages/nimble-components/src/table-column/anchor/cell-view/template.ts b/packages/nimble-components/src/table-column/anchor/cell-view/template.ts index d369615412..5d09e866d8 100644 --- a/packages/nimble-components/src/table-column/anchor/cell-view/template.ts +++ b/packages/nimble-components/src/table-column/anchor/cell-view/template.ts @@ -15,10 +15,12 @@ export const template = html` }}" class="${x => (x.isPlaceholder ? 'placeholder' : '')}" > - ${when(x => typeof x.cellRecord?.href === 'string', html` + ${when(x => x.showAnchor, html` <${anchorTag} ${ref('anchor')} ${overflow('hasOverflow')} + ${'' /* tabindex managed dynamically by KeyboardNavigationManager */} + tabindex="-1" href="${x => x.cellRecord?.href}" hreflang="${x => x.columnConfig?.hreflang}" ping="${x => x.columnConfig?.ping}" @@ -33,7 +35,7 @@ export const template = html` > ${x => x.text} `)} - ${when(x => typeof x.cellRecord?.href !== 'string', html` + ${when(x => !x.showAnchor, html` (x.hasOverflow ? x.text : null)} diff --git a/packages/nimble-components/src/table-column/anchor/tests/table-column-anchor.spec.ts b/packages/nimble-components/src/table-column/anchor/tests/table-column-anchor.spec.ts index 27b6c2dc5a..205fdfb5a8 100644 --- a/packages/nimble-components/src/table-column/anchor/tests/table-column-anchor.spec.ts +++ b/packages/nimble-components/src/table-column/anchor/tests/table-column-anchor.spec.ts @@ -1,5 +1,6 @@ import { html, ref } from '@microsoft/fast-element'; import { parameterizeSpec } from '@ni/jasmine-parameterized'; +import { keyArrowDown, keyEscape, keyTab } from '@microsoft/fast-web-utilities'; import { tableTag, type Table } from '../../../table'; import { TableColumnAnchor, tableColumnAnchorTag } from '..'; import { waitForUpdatesAsync } from '../../../testing/async-helpers'; @@ -8,6 +9,7 @@ import { TableColumnSortDirection, TableRecord } from '../../../table/types'; import { TablePageObject } from '../../../table/testing/table.pageobject'; import { wackyStrings } from '../../../utilities/tests/wacky-strings'; import type { Anchor } from '../../../anchor'; +import { sendKeyDownEvent } from '../../../utilities/tests/component'; interface SimpleTableRecord extends TableRecord { label?: string | null; @@ -164,6 +166,16 @@ describe('TableColumnAnchor', () => { expect(pageObject.getCellTitle(0, 0)).toBe(''); }); + it('cell view tabbableChildren is an empty array', async () => { + const cellContents = 'value'; + await table.setData([{ label: cellContents }]); + await connect(); + await waitForUpdatesAsync(); + + const cellView = pageObject.getRenderedCellView(0, 0); + expect(cellView.tabbableChildren).toEqual([]); + }); + describe('various string values render as expected', () => { parameterizeSpec(wackyStrings, (spec, name) => { spec(`data "${name}" renders correctly`, async () => { @@ -247,6 +259,16 @@ describe('TableColumnAnchor', () => { ).toBeFalse(); }); + it('cell view tabbableChildren returns the anchor', async () => { + await table.setData([{ link: 'foo' }]); + await connect(); + await waitForUpdatesAsync(); + + const cellView = pageObject.getRenderedCellView(0, 0); + const anchor = pageObject.getRenderedCellAnchor(0, 0); + expect(cellView.tabbableChildren).toEqual([anchor]); + }); + const linkOptionData = [ { name: 'hreflang', accessor: (x: Anchor) => x.hreflang }, { name: 'ping', accessor: (x: Anchor) => x.ping }, @@ -616,5 +638,61 @@ describe('TableColumnAnchor', () => { placeholder ); }); + + it('for cells with placeholder, cellView tabbableChildren is an empty array', async () => { + await initializeColumnAndTable([{}], 'placeholder'); + + const cellView = pageObject.getRenderedCellView(0, 0); + expect(cellView.tabbableChildren).toEqual([]); + }); + }); + + describe('keyboard navigation', () => { + beforeEach(async () => { + const tableData = [ + { + id: '1', + label: 'Link 1a', + link: 'http://www.ni.com/a1' + } + ]; + await table.setData(tableData); + column.groupIndex = null; + await connect(); + await waitForUpdatesAsync(); + table.focus(); + await waitForUpdatesAsync(); + }); + + afterEach(async () => { + await disconnect(); + }); + + function isAnchorFocused(anchor: Anchor): boolean { + return anchor.shadowRoot?.activeElement !== null; + } + + describe('with cell[0, 0] focused,', () => { + beforeEach(async () => { + await sendKeyDownEvent(table, keyArrowDown); + }); + + it('anchors in cells are reachable via Tab', async () => { + await sendKeyDownEvent(table, keyTab); + + expect( + isAnchorFocused(pageObject.getRenderedCellAnchor(0, 0)) + ).toBe(true); + }); + + it('when an anchor is focused, pressing Esc will blur the anchor', async () => { + await sendKeyDownEvent(table, keyTab); + await sendKeyDownEvent(table, keyEscape); + + expect( + isAnchorFocused(pageObject.getRenderedCellAnchor(0, 0)) + ).toBe(false); + }); + }); }); }); diff --git a/packages/nimble-components/src/table-column/base/cell-view/index.ts b/packages/nimble-components/src/table-column/base/cell-view/index.ts index fcae105db2..37e67d6e05 100644 --- a/packages/nimble-components/src/table-column/base/cell-view/index.ts +++ b/packages/nimble-components/src/table-column/base/cell-view/index.ts @@ -29,6 +29,14 @@ export abstract class TableCellView< @observable public recordId?: string; + /** + * Gets the child elements in this cell view that should be able to be reached via Tab/ Shift-Tab, + * if any. + */ + public get tabbableChildren(): HTMLElement[] { + return []; + } + private delegatedEvents: readonly string[] = []; /** diff --git a/packages/nimble-components/src/table/components/cell/index.ts b/packages/nimble-components/src/table/components/cell/index.ts index 20c215cc81..c2bc7aa0b4 100644 --- a/packages/nimble-components/src/table/components/cell/index.ts +++ b/packages/nimble-components/src/table/components/cell/index.ts @@ -9,6 +9,7 @@ import type { } from '../../../table-column/base/types'; import { styles } from './styles'; import { template } from './template'; +import type { TableCellView } from '../../../table-column/base/cell-view'; declare global { interface HTMLElementTagNameMap { @@ -47,11 +48,21 @@ export class TableCell< @observable public cellViewTemplate?: ViewTemplate; + /** @internal */ + @observable + public cellViewContainer!: HTMLElement; + @observable public nestingLevel = 0; public readonly actionMenuButton?: MenuButton; + /** @internal */ + public get cellView(): TableCellView { + return this.cellViewContainer + .firstElementChild as TableCellView; + } + public onActionMenuBeforeToggle( event: CustomEvent ): void { @@ -64,6 +75,22 @@ export class TableCell< this.menuOpen = event.detail.newState; this.$emit('cell-action-menu-toggle', event.detail); } + + public onActionMenuBlur(): void { + this.$emit('cell-action-menu-blur', this); + } + + public onCellViewFocusIn(): void { + this.$emit('cell-view-focus-in', this); + } + + public onCellFocusIn(): void { + this.$emit('cell-focus-in', this); + } + + public onCellBlur(): void { + this.$emit('cell-blur', this); + } } const nimbleTableCell = TableCell.compose({ diff --git a/packages/nimble-components/src/table/components/cell/styles.ts b/packages/nimble-components/src/table/components/cell/styles.ts index 55c05ed6a4..7fcb88c73d 100644 --- a/packages/nimble-components/src/table/components/cell/styles.ts +++ b/packages/nimble-components/src/table/components/cell/styles.ts @@ -1,10 +1,13 @@ import { css } from '@microsoft/fast-element'; import { display } from '../../../utilities/style/display'; import { + borderHoverColor, + borderWidth, controlHeight, controlSlimHeight, mediumPadding } from '../../../theme-provider/design-tokens'; +import { focusVisible } from '../../../utilities/style/focus'; export const styles = css` ${display('flex')} @@ -22,6 +25,15 @@ export const styles = css` --ni-private-table-cell-action-menu-display: block; } + :host(${focusVisible}) { + outline: calc(2 * ${borderWidth}) solid ${borderHoverColor}; + outline-offset: -2px; + } + + .cell-view-container { + display: contents; + } + .cell-view { overflow: hidden; } @@ -34,4 +46,11 @@ export const styles = css` height: ${controlSlimHeight}; align-self: center; } + + ${ + /* This CSS class is applied dynamically by KeyboardNavigationManager */ '' + } + .action-menu.cell-action-menu-focused { + display: block; + } `; diff --git a/packages/nimble-components/src/table/components/cell/template.ts b/packages/nimble-components/src/table/components/cell/template.ts index b3a30a4cdd..85b65ab13a 100644 --- a/packages/nimble-components/src/table/components/cell/template.ts +++ b/packages/nimble-components/src/table/components/cell/template.ts @@ -10,15 +10,23 @@ import { tableCellActionMenuLabel } from '../../../label-provider/table/label-to // prettier-ignore export const template = html` -