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.

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

Support dynamic tabbableChildren for TableCellView #2340

Closed
wants to merge 4 commits into from
Closed
Changes from 1 commit
Commits
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
Next Next commit
Initial changes for dynamic table cell view tabbableChildren
msmithNI committed Aug 8, 2024

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature.
commit e0df7087bba6c2674e51227865ad10c3a3cf1898
Original file line number Diff line number Diff line change
@@ -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] : [];
}
}

Original file line number Diff line number Diff line change
@@ -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<TableColumnAnchorCellView>`
@@ -17,7 +18,7 @@ export const template = html<TableColumnAnchorCellView>`
>
${when(x => x.showAnchor, html<TableColumnAnchorCellView>`
<${anchorTag}
${ref('anchor')}
${dynamicRef('anchor')}
${overflow('hasOverflow')}
${'' /* tabindex managed dynamically by KeyboardNavigationManager */}
tabindex="-1"
Original file line number Diff line number Diff line change
@@ -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);
}
}
Original file line number Diff line number Diff line change
@@ -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<MenuButtonToggleEventDetail>
@@ -82,6 +76,10 @@ TableColumnMenuButtonColumnConfig
// from affecting row selection.
e.stopPropagation();
}

private menuButtonChanged(): void {
this.tabbableChildren = this.menuButton ? [this.menuButton] : [];
}
}

const menuButtonCellView = TableColumnMenuButtonCellView.compose({
Original file line number Diff line number Diff line change
@@ -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<TableColumnMenuButtonCellView>`
${when(x => x.showMenuButton, html<TableColumnMenuButtonCellView>`
<${menuButtonTag}
${ref('menuButton')}
${dynamicRef('menuButton')}
appearance="${ButtonAppearance.ghost}"
@beforetoggle="${(x, c) => x.onMenuButtonBeforeToggle(c.event as CustomEvent<MenuButtonToggleEventDetail>)}"
@mouseover="${x => x.onMenuButtonMouseOver()}"
Original file line number Diff line number Diff line change
@@ -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<TableCellView>
): 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
) {
Comment on lines +348 to +355
Copy link
Contributor

Choose a reason for hiding this comment

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

I think you could collapse these two ifs into one.

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);
}

Original file line number Diff line number Diff line change
@@ -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<TestInteractiveCellView>`<span tabindex="-1" ${ref('spanElement')}>Test</span>`
template: html<TestInteractiveCellView>`${when(x => x.isTabbable, html<TestInteractiveCellView>`<span tabindex="-1" ${dynamicRef('spanElement')}>Test</span>`)}`
})
// 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 () => {
Copy link
Contributor

Choose a reason for hiding this comment

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

I presume if we went with this approach (which I think I'm fine with), that there are some other tests we still need to add, yes? Namely, that after the tabbableChildren change (but not completely removed, as this test covers that) that tabbing (and shift-tabbing) behave as expected.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Agreed, those would be good tests to add too.

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)
);
});
});
});
});
33 changes: 0 additions & 33 deletions packages/nimble-components/src/table/tests/table.spec.ts
Original file line number Diff line number Diff line change
@@ -650,39 +650,6 @@ describe('Table', () => {
});

describe('uses virtualization', () => {
const focusableCellViewName = uniqueElementName();
const focusableColumnName = uniqueElementName();
@customElement({
name: focusableCellViewName,
template: html<TestFocusableCellView>`<span tabindex="0"
>${x => x.text}</span
>`
})
// 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();

Original file line number Diff line number Diff line change
@@ -0,0 +1,62 @@
import {
Copy link
Contributor

Choose a reason for hiding this comment

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

Does FAST have unit tests for their RefBehavior that we could leverage for patterns to write unit tests for this class?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not that I've found, but writing our own should be pretty quick anyway.

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<T = unknown>(
propertyName: keyof T & string
): CaptureType<T> {
return new AttachedBehaviorHTMLDirective(
'nimble-dynamic-ref',
DynamicRefBehavior,
propertyName
);
}