From e0df7087bba6c2674e51227865ad10c3a3cf1898 Mon Sep 17 00:00:00 2001 From: Malcolm Smith <20709258+msmithNI@users.noreply.github.com> Date: Thu, 8 Aug 2024 18:20:02 -0500 Subject: [PATCH 1/3] Initial changes for dynamic table cell view tabbableChildren --- .../table-column/anchor/cell-view/index.ts | 8 +- .../table-column/anchor/cell-view/template.ts | 5 +- .../src/table-column/base/cell-view/index.ts | 11 ++- .../menu-button/cell-view/index.ts | 12 +-- .../menu-button/cell-view/templates.ts | 3 +- .../models/keyboard-navigation-manager.ts | 93 ++++++++++++++----- .../tests/table-keyboard-navigation.spec.ts | 43 +++++++-- .../src/table/tests/table.spec.ts | 33 ------- .../src/utilities/directive/dynamic-ref.ts | 62 +++++++++++++ 9 files changed, 191 insertions(+), 79 deletions(-) create mode 100644 packages/nimble-components/src/utilities/directive/dynamic-ref.ts 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 bab8d3b8c2..250ff58cf8 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 @@ -31,6 +31,7 @@ TableColumnAnchorColumnConfig public isPlaceholder = false; /** @internal */ + @observable public anchor?: Anchor; @volatile @@ -60,11 +61,8 @@ TableColumnAnchorColumnConfig return typeof this.cellRecord?.href === 'string'; } - public override get tabbableChildren(): HTMLElement[] { - if (this.showAnchor) { - return [this.anchor!]; - } - return []; + private anchorChanged(): void { + this.tabbableChildren = this.anchor ? [this.anchor] : []; } } 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 4e825bbfa1..ae55ddf93e 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 @@ -1,8 +1,9 @@ /* eslint-disable @typescript-eslint/indent */ -import { html, ref, when } from '@microsoft/fast-element'; +import { html, when } from '@microsoft/fast-element'; import type { TableColumnAnchorCellView } from '.'; import { anchorTag } from '../../../anchor'; import { overflow } from '../../../utilities/directive/overflow'; +import { dynamicRef } from '../../../utilities/directive/dynamic-ref'; // prettier-ignore export const template = html` @@ -17,7 +18,7 @@ export const template = html` > ${when(x => x.showAnchor, html` <${anchorTag} - ${ref('anchor')} + ${dynamicRef('anchor')} ${overflow('hasOverflow')} ${'' /* tabindex managed dynamically by KeyboardNavigationManager */} tabindex="-1" 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 8af808869f..899a58a667 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 @@ -30,12 +30,11 @@ export abstract class TableCellView< public recordId?: string; /** - * Gets the child elements in this cell view that should be able to be reached via Tab/ Shift-Tab, + * 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 []; - } + @observable + public tabbableChildren: Element[] = []; private delegatedEvents: readonly string[] = []; @@ -72,4 +71,8 @@ export abstract class TableCellView< } private delegatedEventHandler: (event: Event) => void = () => {}; + + private tabbableChildrenChanged(): void { + this.$emit('cell-tabbable-children-change', this); + } } diff --git a/packages/nimble-components/src/table-column/menu-button/cell-view/index.ts b/packages/nimble-components/src/table-column/menu-button/cell-view/index.ts index c9c0c3967d..1800f35e1e 100644 --- a/packages/nimble-components/src/table-column/menu-button/cell-view/index.ts +++ b/packages/nimble-components/src/table-column/menu-button/cell-view/index.ts @@ -26,6 +26,7 @@ TableColumnMenuButtonCellRecord, TableColumnMenuButtonColumnConfig > { /** @internal */ + @observable public menuButton?: MenuButton; /** @internal */ @@ -41,13 +42,6 @@ TableColumnMenuButtonColumnConfig return !!this.cellRecord?.value; } - public override get tabbableChildren(): HTMLElement[] { - if (this.showMenuButton) { - return [this.menuButton!]; - } - return []; - } - /** @internal */ public onMenuButtonBeforeToggle( event: CustomEvent @@ -82,6 +76,10 @@ TableColumnMenuButtonColumnConfig // from affecting row selection. e.stopPropagation(); } + + private menuButtonChanged(): void { + this.tabbableChildren = this.menuButton ? [this.menuButton] : []; + } } const menuButtonCellView = TableColumnMenuButtonCellView.compose({ diff --git a/packages/nimble-components/src/table-column/menu-button/cell-view/templates.ts b/packages/nimble-components/src/table-column/menu-button/cell-view/templates.ts index 9927cdc225..9deb16393a 100644 --- a/packages/nimble-components/src/table-column/menu-button/cell-view/templates.ts +++ b/packages/nimble-components/src/table-column/menu-button/cell-view/templates.ts @@ -7,12 +7,13 @@ import { } from '../../../menu-button/types'; import { iconArrowExpanderDownTag } from '../../../icons/arrow-expander-down'; import { cellViewMenuSlotName } from '../types'; +import { dynamicRef } from '../../../utilities/directive/dynamic-ref'; // prettier-ignore export const template = html` ${when(x => x.showMenuButton, html` <${menuButtonTag} - ${ref('menuButton')} + ${dynamicRef('menuButton')} appearance="${ButtonAppearance.ghost}" @beforetoggle="${(x, c) => x.onMenuButtonBeforeToggle(c.event as CustomEvent)}" @mouseover="${x => x.onMenuButtonMouseOver()}" diff --git a/packages/nimble-components/src/table/models/keyboard-navigation-manager.ts b/packages/nimble-components/src/table/models/keyboard-navigation-manager.ts index d2e7f7bccf..9304579460 100644 --- a/packages/nimble-components/src/table/models/keyboard-navigation-manager.ts +++ b/packages/nimble-components/src/table/models/keyboard-navigation-manager.ts @@ -40,6 +40,12 @@ interface TableFocusState { cellContentIndex?: number; } +interface TableCellContentFocusState { + index: number; + columnId?: string; + recordId?: string; +} + /** * Manages the keyboard navigation and focus within the table. * @internal @@ -49,13 +55,16 @@ implements Subscriber { private focusType: TableFocusType = TableFocusType.none; private headerActionIndex = -1; private rowIndex = -1; - private cellContentIndex = -1; private columnIndex = -1; private focusWithinTable = false; private isCurrentlyFocusingElement = false; private readonly tableNotifier: Notifier; private readonly virtualizerNotifier: Notifier; private visibleRowNotifiers: Notifier[] = []; + private readonly cellContentState: TableCellContentFocusState = { + index: -1 + }; + private get inNavigationMode(): boolean { return ( this.focusType !== TableFocusType.cellActionMenu @@ -117,6 +126,10 @@ implements Subscriber { 'cell-blur', this.onCellBlur as EventListener ); + this.table.viewport.addEventListener( + 'cell-tabbable-children-change', + this.onCellTabbableChildrenChange as EventListener + ); } public disconnect(): void { @@ -157,6 +170,10 @@ implements Subscriber { 'cell-blur', this.onCellBlur as EventListener ); + this.table.viewport.removeEventListener( + 'cell-tabbable-children-change', + this.onCellTabbableChildrenChange as EventListener + ); } public handleChange(source: unknown, args: unknown): void { @@ -323,6 +340,28 @@ implements Subscriber { this.setElementFocusable(cell, false); }; + private readonly onCellTabbableChildrenChange = ( + event: CustomEvent + ): void => { + event.stopPropagation(); + const cellView = event.detail; + if ( + this.focusType === TableFocusType.cellContent + && cellView.recordId === this.cellContentState.recordId + && cellView?.column?.columnId === this.cellContentState.columnId + ) { + if ( + this.cellContentState.index >= cellView.tabbableChildren.length + ) { + this.setCellFocusState( + this.columnIndex, + this.rowIndex, + this.focusWithinTable + ); + } + } + }; + private readonly onCaptureKeyDown = (event: KeyboardEvent): void => { let handled = false; if (event.key === keyTab) { @@ -622,7 +661,14 @@ implements Subscriber { this.rowIndex = nextFocusState.rowIndex ?? this.rowIndex; this.columnIndex = nextFocusState.columnIndex ?? this.columnIndex; this.headerActionIndex = nextFocusState.headerActionIndex ?? this.headerActionIndex; - this.cellContentIndex = nextFocusState.cellContentIndex ?? this.cellContentIndex; + if (nextFocusState.cellContentIndex !== undefined) { + this.cellContentState.index = nextFocusState.cellContentIndex; + const row = this.getCurrentRow() as TableRow; + const elements = row?.getFocusableElements(); + this.cellContentState.columnId = elements?.cells[this.columnIndex]?.cell.columnId; + this.cellContentState.recordId = row?.recordId; + } + if (this.hasRowOrCellFocusType()) { this.focusCurrentRow(false); } else { @@ -666,7 +712,7 @@ implements Subscriber { if ( this.focusType === TableFocusType.cellContent && this.columnIndex === cellIndex - && this.cellContentIndex === i + && this.cellContentState.index === i ) { startIndex = focusStates.length - 1; } @@ -853,6 +899,7 @@ implements Subscriber { ); if (contentIndex > -1) { this.setCellContentFocusState( + cell, contentIndex, row.resolvedRowIndex!, columnIndex, @@ -1009,8 +1056,10 @@ implements Subscriber { break; } case TableFocusType.cellContent: { - focusableElement = rowElements.cells[this.columnIndex]?.cell.cellView - .tabbableChildren[this.cellContentIndex]; + focusableElement = rowElements.cells[this.columnIndex]?.cell + .cellView.tabbableChildren[ + this.cellContentState.index + ] as HTMLElement; break; } default: @@ -1273,21 +1322,20 @@ implements Subscriber { } const newColumnIndex = columnIndex ?? this.columnIndex; const newRowIndex = rowIndex ?? this.rowIndex; - - if ( - newColumnIndex >= 0 - && newColumnIndex < rowElements.cells.length - && cellContentIndex >= 0 - && cellContentIndex - < rowElements.cells[newColumnIndex]!.cell.cellView - .tabbableChildren.length - ) { - this.setCellContentFocusState( - cellContentIndex, - newRowIndex, - newColumnIndex, - true - ); + if (newColumnIndex >= 0 && newColumnIndex < rowElements.cells.length) { + const cell = rowElements.cells[newColumnIndex]!.cell; + if ( + cellContentIndex >= 0 + && cellContentIndex < cell.cellView.tabbableChildren.length + ) { + this.setCellContentFocusState( + cell, + cellContentIndex, + newRowIndex, + newColumnIndex, + true + ); + } return true; } @@ -1324,13 +1372,16 @@ implements Subscriber { } private setCellContentFocusState( + cell: TableCell, cellContentIndex: number, rowIndex: number, columnIndex: number, focusElement: boolean ): void { + this.cellContentState.recordId = cell.recordId; + this.cellContentState.columnId = cell.columnId; this.focusType = TableFocusType.cellContent; - this.cellContentIndex = cellContentIndex; + this.cellContentState.index = cellContentIndex; this.setRowCellFocusState(columnIndex, rowIndex, focusElement); } diff --git a/packages/nimble-components/src/table/tests/table-keyboard-navigation.spec.ts b/packages/nimble-components/src/table/tests/table-keyboard-navigation.spec.ts index 522dd56fc8..b5433e54f0 100644 --- a/packages/nimble-components/src/table/tests/table-keyboard-navigation.spec.ts +++ b/packages/nimble-components/src/table/tests/table-keyboard-navigation.spec.ts @@ -1,5 +1,13 @@ /* eslint-disable no-await-in-loop */ -import { customElement, html, observable, ref } from '@microsoft/fast-element'; +import { + children, + customElement, + elements, + html, + observable, + ref, + when +} from '@microsoft/fast-element'; import { FoundationElement } from '@microsoft/fast-foundation'; import { keyArrowDown, @@ -49,6 +57,7 @@ import type { ColumnInternalsOptions } from '../../table-column/base/models/colu import { ColumnValidator } from '../../table-column/base/models/column-validator'; import { mixinSortableColumnAPI } from '../../table-column/mixins/sortable-column'; import { MenuButtonPageObject } from '../../menu-button/testing/menu-button.pageobject'; +import { dynamicRef } from '../../utilities/directive/dynamic-ref'; interface SimpleTableRecord extends TableRecord { id: string; @@ -1238,15 +1247,18 @@ describe('Table keyboard navigation', () => { // prettier-ignore @customElement({ name: interactiveCellViewName, - template: html`Test` + template: html`${when(x => x.isTabbable, html`Test`)}` }) // eslint-disable-next-line @typescript-eslint/no-unused-vars class TestInteractiveCellView extends TableCellView { @observable - public spanElement!: HTMLSpanElement; + public isTabbable = true; + + @observable + public spanElement?: HTMLSpanElement; - public override get tabbableChildren(): HTMLElement[] { - return [this.spanElement]; + private spanElementChanged(): void { + this.tabbableChildren = this.spanElement ? [this.spanElement] : []; } } // prettier-ignore @@ -1286,7 +1298,7 @@ describe('Table keyboard navigation', () => { rowIndex, columnIndex ) as TestInteractiveCellView - ).spanElement; + ).spanElement!; } beforeEach(async () => { @@ -1425,6 +1437,25 @@ describe('Table keyboard navigation', () => { expect(blurSpy).toHaveBeenCalledTimes(1); expect(currentFocusedElement()).not.toBe(cellContent); }); + + it('and then the cell updates to no longer have tabbableChildren, the cell is focused instead', async () => { + const cellView = pageObject.getRenderedCellView( + 0, + 1 + ) as TestInteractiveCellView; + cellView.isTabbable = false; + await waitForUpdatesAsync(); + + // Note: At this point, the table lost focus already, because the focused element in the cell has been removed from the DOM, and + // KeyboardNavigationManager will only set focus to new elements when the table is already focused (so we don't steal focus from + // elsewhere on the page if the table isn't being interacted with). + element.focus(); + await waitForUpdatesAsync(); + + expect(currentFocusedElement()).toBe( + pageObject.getCell(0, 1) + ); + }); }); }); }); diff --git a/packages/nimble-components/src/table/tests/table.spec.ts b/packages/nimble-components/src/table/tests/table.spec.ts index 6d7562d418..76efba6bc1 100644 --- a/packages/nimble-components/src/table/tests/table.spec.ts +++ b/packages/nimble-components/src/table/tests/table.spec.ts @@ -650,39 +650,6 @@ describe('Table', () => { }); describe('uses virtualization', () => { - const focusableCellViewName = uniqueElementName(); - const focusableColumnName = uniqueElementName(); - @customElement({ - name: focusableCellViewName, - template: html`${x => x.text}` - }) - // eslint-disable-next-line @typescript-eslint/no-unused-vars - class TestFocusableCellView extends TableColumnTextCellView { - public override get tabbableChildren(): HTMLElement[] { - return [this.shadowRoot!.firstElementChild as HTMLElement]; - } - } - @customElement({ - name: focusableColumnName - }) - // eslint-disable-next-line @typescript-eslint/no-unused-vars - class TestFocusableTableColumn extends TableColumn { - @attr({ attribute: 'field-name' }) - public fieldName?: string; - - protected override getColumnInternalsOptions(): ColumnInternalsOptions { - return { - cellViewTag: focusableCellViewName, - cellRecordFieldNames: ['value'], - groupHeaderViewTag: tableColumnEmptyGroupHeaderViewTag, - delegatedEvents: [], - validator: new ColumnValidator<[]>([]) - }; - } - } - it('to render fewer rows (based on viewport size)', async () => { await connect(); diff --git a/packages/nimble-components/src/utilities/directive/dynamic-ref.ts b/packages/nimble-components/src/utilities/directive/dynamic-ref.ts new file mode 100644 index 0000000000..cbb279ebff --- /dev/null +++ b/packages/nimble-components/src/utilities/directive/dynamic-ref.ts @@ -0,0 +1,62 @@ +import { + AttachedBehaviorHTMLDirective, + type Behavior, + type CaptureType +} from '@microsoft/fast-element'; + +/** + * A directive that updates a property with a reference to the element. + * Similar to RefBehavior, but sets the property back to undefined when unbound + * (when the source element is removed from the DOM). + * @public + */ +export class DynamicRefBehavior implements Behavior { + private readonly target: unknown; + private propertyName: string; + + /** + * Creates an instance of DynamicRefBehavior. + * @param target - The element to reference. + * @param propertyName - The name of the property to assign the reference to. + */ + public constructor(target: unknown, propertyName: string) { + this.target = target; + this.propertyName = propertyName; + } + + /** + * Bind this behavior to the source. + * @param source - The source to bind to. + */ + + public bind(source: unknown): void { + // @ts-expect-error set property on source + source[this.propertyName] = this.target; + } + + /** + * Unbinds this behavior from the source. + * @param source - The source to unbind from. + */ + public unbind(source: unknown): void { + // @ts-expect-error set property on source + source[this.propertyName] = undefined; + } +} + +/** + * A directive that updates a property with a reference to the source element. + * Similar to RefBehavior, but sets the property back to undefined when unbound + * (when the source element is removed from the DOM). + * @param propertyName - The name of the property to assign the reference to. + * @public + */ +export function dynamicRef( + propertyName: keyof T & string +): CaptureType { + return new AttachedBehaviorHTMLDirective( + 'nimble-dynamic-ref', + DynamicRefBehavior, + propertyName + ); +} From e189c08402f3173e8c5bc9fbf8ad92470bef2944 Mon Sep 17 00:00:00 2001 From: Malcolm Smith <20709258+msmithNI@users.noreply.github.com> Date: Thu, 8 Aug 2024 18:22:42 -0500 Subject: [PATCH 2/3] Change files --- ...le-components-8499a62b-64ea-41e0-877c-6bacca7ae0c8.json | 7 +++++++ 1 file changed, 7 insertions(+) create mode 100644 change/@ni-nimble-components-8499a62b-64ea-41e0-877c-6bacca7ae0c8.json diff --git a/change/@ni-nimble-components-8499a62b-64ea-41e0-877c-6bacca7ae0c8.json b/change/@ni-nimble-components-8499a62b-64ea-41e0-877c-6bacca7ae0c8.json new file mode 100644 index 0000000000..ca3923cfca --- /dev/null +++ b/change/@ni-nimble-components-8499a62b-64ea-41e0-877c-6bacca7ae0c8.json @@ -0,0 +1,7 @@ +{ + "type": "patch", + "comment": "Update TableCellView tabbableChildren to be an observable property", + "packageName": "@ni/nimble-components", + "email": "20709258+msmithNI@users.noreply.github.com", + "dependentChangeType": "patch" +} From 6681a484a6f86233ca659c5bcc309441c02c091b Mon Sep 17 00:00:00 2001 From: Malcolm Smith <20709258+msmithNI@users.noreply.github.com> Date: Thu, 8 Aug 2024 19:32:07 -0500 Subject: [PATCH 3/3] Fix tests and lint issues --- .../models/keyboard-navigation-manager.ts | 2 +- .../tests/table-keyboard-navigation.spec.ts | 23 ++++++++----------- .../src/table/tests/table.spec.ts | 13 ++--------- 3 files changed, 12 insertions(+), 26 deletions(-) diff --git a/packages/nimble-components/src/table/models/keyboard-navigation-manager.ts b/packages/nimble-components/src/table/models/keyboard-navigation-manager.ts index 9304579460..47b8b562f5 100644 --- a/packages/nimble-components/src/table/models/keyboard-navigation-manager.ts +++ b/packages/nimble-components/src/table/models/keyboard-navigation-manager.ts @@ -1335,8 +1335,8 @@ implements Subscriber { newColumnIndex, true ); + return true; } - return true; } return false; diff --git a/packages/nimble-components/src/table/tests/table-keyboard-navigation.spec.ts b/packages/nimble-components/src/table/tests/table-keyboard-navigation.spec.ts index def99486a3..2fb593552d 100644 --- a/packages/nimble-components/src/table/tests/table-keyboard-navigation.spec.ts +++ b/packages/nimble-components/src/table/tests/table-keyboard-navigation.spec.ts @@ -1,13 +1,5 @@ /* eslint-disable no-await-in-loop */ -import { - children, - customElement, - elements, - html, - observable, - ref, - when -} from '@microsoft/fast-element'; +import { customElement, html, observable, when } from '@microsoft/fast-element'; import { FoundationElement } from '@microsoft/fast-foundation'; import { keyArrowDown, @@ -1443,11 +1435,14 @@ describe('Table keyboard navigation', () => { cellView.isTabbable = false; await waitForUpdatesAsync(); - // Note: At this point, the table lost focus already, because the focused element in the cell has been removed from the DOM, and - // KeyboardNavigationManager will only set focus to new elements when the table is already focused (so we don't steal focus from - // elsewhere on the page if the table isn't being interacted with). - element.focus(); - await waitForUpdatesAsync(); + // Note: At this point, the focused element in the cell has been already removed from the DOM. + // KeyboardNavigationManager will only set focus to new elements when the table is already focused (so we don't + // steal focus from elsewhere on the page if the table isn't being interacted with). + // In Chrome, the table loses focus entirely, but not in Firefox/WebKit. + if (element.shadowRoot!.activeElement === null) { + element.focus(); // Refocus table if it's been lost + await waitForUpdatesAsync(); + } expect(currentFocusedElement()).toBe( pageObject.getCell(0, 1) diff --git a/packages/nimble-components/src/table/tests/table.spec.ts b/packages/nimble-components/src/table/tests/table.spec.ts index 9b8e708193..ebc8fa40fd 100644 --- a/packages/nimble-components/src/table/tests/table.spec.ts +++ b/packages/nimble-components/src/table/tests/table.spec.ts @@ -1,17 +1,11 @@ -import { attr, customElement, html } from '@microsoft/fast-element'; +import { html } from '@microsoft/fast-element'; import { parameterizeSpec } from '@ni/jasmine-parameterized'; import { Table, tableTag } from '..'; -import { TableColumn } from '../../table-column/base'; import { TableColumnText, tableColumnTextTag } from '../../table-column/text'; -import { TableColumnTextCellView } from '../../table-column/text/cell-view'; import { waitForUpdatesAsync } from '../../testing/async-helpers'; import { controlHeight } from '../../theme-provider/design-tokens'; import { waitForEvent } from '../../utilities/testing/component'; -import { - type Fixture, - fixture, - uniqueElementName -} from '../../utilities/tests/fixture'; +import { type Fixture, fixture } from '../../utilities/tests/fixture'; import { TableColumnAlignment, TableColumnSortDirection, @@ -21,12 +15,9 @@ import { } from '../types'; import { TablePageObject } from '../testing/table.pageobject'; import { - tableColumnEmptyGroupHeaderViewTag, TableColumnValidationTest, tableColumnValidationTestTag } from '../../table-column/base/tests/table-column.fixtures'; -import type { ColumnInternalsOptions } from '../../table-column/base/models/column-internals'; -import { ColumnValidator } from '../../table-column/base/models/column-validator'; interface SimpleTableRecord extends TableRecord { stringData: string;