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

Support dynamic tabbableChildren for TableCellView #2340

Closed
wants to merge 4 commits into from
Closed

Support dynamic tabbableChildren for TableCellView #2340

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
e0df708
Initial changes for dynamic table cell view tabbableChildren
msmithNI Aug 8, 2024
e189c08
Change files
msmithNI Aug 8, 2024
3e0cc93
Merge branch 'main' into users/masmith/tableDynamicTabbableChildren-1
msmithNI Aug 8, 2024
6681a48
Fix tests and lint issues
msmithNI Aug 9, 2024
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
7 changes: 7 additions & 0 deletions change/@ni-nimble-components-8499a62b-64ea-41e0-877c-6bacca7ae0c8.json
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
{
Copy link
Member

Choose a reason for hiding this comment

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

@msmithNI: @jattasNI @rajsite @atmgrifter00 interested in your thoughts on this implementation direction so far (but note the concerns I listed in the PR description).

Copy link
Member

Choose a reason for hiding this comment

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

The general direction looks reasonable to me.

For the cell-tabbable-children-change I'm a bit curious how often that event is firing. Is it firing once per row continuously as you scroll for example? That could potentially be excessive object creation / gc pressure on scroll.

In Chrome, if a focused element inside a cell is removed from the DOM, the table loses focus entirely.

Do you have a screencap of how that manifests? Not exactly clear how to try out the UX for that situation and if it's a new change in behavior or existing.

Copy link
Contributor

Choose a reason for hiding this comment

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

No objections to the overall direction from me. (I left a couple comments while wrapping my head around the PR because I couldn't stop myself; neither one is existential, just normal code review feedback)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

When cell-tabbable-children-change fires:

  • With what's in this PR, there's some unnecessary firing as the table loads with initial data. If we omit firing when !cellView.$fastController.isConnected, we could prevent that though.
  • During scrolling:
    • Anchor column: Yes, depending on what's in the cells. With the Storybook example, yes, since the recycled cells change between having valid links / no link / placeholder text, as you scroll (depending on the data).
    • Menu button: Yes with this current PR. But not if we skip firing when the FAST controller is not connected.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Do you have a screencap of how that manifests?

Not currently, I was just going by the behavior of the new autotest failing in Chrome without the code to refocus the table. If we updated an example to periodically update the table data with an anchor column periodically switching between a valid URL / no URL, that would probably show it.

"type": "patch",
"comment": "Update TableCellView tabbableChildren to be an observable property",
Copy link
Member

@rajsite rajsite Dec 3, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Closing the draft PR as the discussion is stale. Can reopen if continuing progress.

FYI @msmithNI @fredvisser I'm betting this refactor is needed as part of the editable table cells implementation where I suspect new tabbable elements will be created dynamically
#2477

"packageName": "@ni/nimble-components",
"email": "[email protected]",
"dependentChangeType": "patch"
}
8 changes: 3 additions & 5 deletions packages/nimble-components/src/table-column/anchor/cell-view/index.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,7 @@ TableColumnAnchorColumnConfig
public isPlaceholder = false;

/** @internal */
@observable
public anchor?: Anchor;

@volatile
Expand Down Expand Up @@ -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] : [];
}
}

Expand Down
5 changes: 3 additions & 2 deletions packages/nimble-components/src/table-column/anchor/cell-view/template.ts
View file
Open in desktop
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>`
Expand All @@ -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"
Expand Down
11 changes: 7 additions & 4 deletions packages/nimble-components/src/table-column/base/cell-view/index.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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[] = [];

Expand Down Expand Up @@ -72,4 +71,8 @@ export abstract class TableCellView<
}

private delegatedEventHandler: (event: Event) => void = () => {};

private tabbableChildrenChanged(): void {
this.$emit('cell-tabbable-children-change', this);
}
}
12 changes: 5 additions & 7 deletions packages/nimble-components/src/table-column/menu-button/cell-view/index.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,7 @@ TableColumnMenuButtonCellRecord,
TableColumnMenuButtonColumnConfig
> {
/** @internal */
@observable
public menuButton?: MenuButton;

/** @internal */
Expand All @@ -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>
Expand Down Expand Up @@ -82,6 +76,10 @@ TableColumnMenuButtonColumnConfig
// from affecting row selection.
e.stopPropagation();
}

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

const menuButtonCellView = TableColumnMenuButtonCellView.compose({
Expand Down
3 changes: 2 additions & 1 deletion packages/nimble-components/src/table-column/menu-button/cell-view/templates.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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()}"
Expand Down
95 changes: 73 additions & 22 deletions packages/nimble-components/src/table/models/keyboard-navigation-manager.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -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
Expand Down Expand Up @@ -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 {
Expand Down Expand Up @@ -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 {
Expand Down Expand Up @@ -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) {
Expand Down Expand Up @@ -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 {
Expand Down Expand Up @@ -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;
}
Expand Down Expand Up @@ -853,6 +899,7 @@ implements Subscriber {
);
if (contentIndex > -1) {
this.setCellContentFocusState(
cell,
contentIndex,
row.resolvedRowIndex!,
columnIndex,
Expand Down Expand Up @@ -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:
Expand Down Expand Up @@ -1273,22 +1322,21 @@ 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
);
return 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;
}
}

return false;
Expand Down Expand Up @@ -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);
}

Expand Down
38 changes: 32 additions & 6 deletions packages/nimble-components/src/table/tests/table-keyboard-navigation.spec.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
/* eslint-disable no-await-in-loop */
import { customElement, html, observable, ref } from '@microsoft/fast-element';
import { customElement, html, observable, when } from '@microsoft/fast-element';
import { FoundationElement } from '@microsoft/fast-foundation';
import {
keyArrowDown,
Expand Down Expand Up @@ -49,6 +49,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;
Expand Down Expand Up @@ -1235,15 +1236,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;

public override get tabbableChildren(): HTMLElement[] {
return [this.spanElement];
@observable
public spanElement?: HTMLSpanElement;

private spanElementChanged(): void {
this.tabbableChildren = this.spanElement ? [this.spanElement] : [];
}
}
// prettier-ignore
Expand Down Expand Up @@ -1283,7 +1287,7 @@ describe('Table keyboard navigation', () => {
rowIndex,
columnIndex
) as TestInteractiveCellView
).spanElement;
).spanElement!;
}

beforeEach(async () => {
Expand Down Expand Up @@ -1422,6 +1426,28 @@ 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 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)
);
});
});
});
});
Expand Down
Loading