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

Table keyboard interaction (draft / partial implementation) #2070

Closed
wants to merge 34 commits into from
Closed
Show file tree
Hide file tree
Changes from 8 commits
Commits
Show all changes
34 commits
Select commit Hold shift + click to select a range
e823d57
Prototype as of 4/3
msmithNI Apr 4, 2024
b974ba5
Merge from main
msmithNI Apr 30, 2024
4dad3d3
Updated keyboard nav code
msmithNI May 2, 2024
07cc96b
Lint/prettier
msmithNI May 2, 2024
f57ac31
Add TODO
msmithNI May 2, 2024
6f2126b
Merge from main
msmithNI May 2, 2024
893db0c
Merge branch 'main' into users/masmith/table-keyboard-interaction-temp
msmithNI May 2, 2024
e185f0d
Fix test, less console.log noise when body focused
msmithNI May 3, 2024
65ab3dd
Remove no-op call
msmithNI May 6, 2024
078fccb
Bugfix for Safari Mac issue, temporary fix for scrolling down via Dow…
msmithNI May 10, 2024
afaa3cb
Remove tabIndexOverride
msmithNI May 10, 2024
247b51a
Undo no-op change
msmithNI May 10, 2024
0ba987e
Feature complete keyboard nav
msmithNI May 17, 2024
0a92870
Cleanup / fix F2 for single-element cells
msmithNI May 17, 2024
7a7ce21
prettier
msmithNI May 17, 2024
e29b789
Fix action menu Shift-Tab issue
msmithNI May 17, 2024
5110178
Prettier
msmithNI May 17, 2024
4248deb
Simplify onTabPressed
msmithNI May 20, 2024
531eac3
Fix anchor view tabbableChildren implementation, get rid of extra vie…
msmithNI May 20, 2024
249c1bf
Cleanup (methods to set focus type, Tab for headers uses similar logi…
msmithNI May 21, 2024
c6244a1
Merge branch 'main' into users/masmith/table-keyboard-interaction-temp
msmithNI May 22, 2024
8f3c5ce
Fix build
msmithNI May 22, 2024
cacf215
Uptake tabindex changes
msmithNI May 22, 2024
adfb7a7
Fix column header tabIndex behavior
msmithNI May 22, 2024
b23ea0d
Fix issues scrolling with a focused cell (+ switch back to observers)
msmithNI May 22, 2024
0a26787
Fix build
msmithNI May 22, 2024
0d5ff72
Refactoring/cleanup
msmithNI May 29, 2024
6dbad6d
Cleanup
msmithNI May 30, 2024
44f0936
Remove (flatten) TableFocusState into kbd nav code
msmithNI May 30, 2024
f6e6a34
Fix some issues with anchor columns / action menus
msmithNI May 30, 2024
e472265
Add anchor to lastName in Storybook (for testing)
msmithNI May 30, 2024
cccd1d2
Fix another 'single interactive element' corner case with Tab
msmithNI May 30, 2024
9a32fb3
Remove 'single interactive element' special case
msmithNI May 30, 2024
c80b0f0
Update Enter (when cell focused) to focus content, but not activate it
msmithNI May 31, 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
9 changes: 7 additions & 2 deletions packages/nimble-components/src/anchor/index.ts
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
import { attr } from '@microsoft/fast-element';
import { attr, observable } from '@microsoft/fast-element';
import {
msmithNI marked this conversation as resolved.
Show resolved Hide resolved
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not able to get row selection to work. From the HLD:

If a row is focused, pressing Space will select/unselect that row

When I focus an entire row, space either does nothing or scrolls the page. I tried this with both single select and multi select mode on the hierarchical table (again in macOS Firefox).

In multiselect mode I am able to focus the selection checkbox and press Space to toggle it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Just not yet implemented (updated the PR description), but should be straightforward.

DesignSystem,
Anchor as FoundationAnchor,
Expand All @@ -8,6 +8,7 @@ import { AnchorBase } from '../anchor-base';
import { styles } from './styles';
import { template } from './template';
import type { AnchorAppearance } from './types';
import type { TabIndexOverride } from '../patterns/tab-index-override/types';

declare global {
interface HTMLElementTagNameMap {
Expand All @@ -18,7 +19,7 @@ declare global {
/**
* A nimble-styled anchor
*/
export class Anchor extends AnchorBase {
export class Anchor extends AnchorBase implements TabIndexOverride {
/**
* @public
* @remarks
Expand Down Expand Up @@ -47,6 +48,10 @@ export class Anchor extends AnchorBase {
*/
@attr({ attribute: 'contenteditable' })
public override contentEditable!: string;

/** @internal */
@observable
public tabIndexOverride = 0;
}
msmithNI marked this conversation as resolved.
Show resolved Hide resolved

// FoundationAnchor already applies the StartEnd mixin, so we don't need to do it here.
Expand Down
1 change: 1 addition & 0 deletions packages/nimble-components/src/anchor/template.ts
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@ AnchorOptions
><a
class="control"
part="control"
tabindex="${x => x.tabIndexOverride}"
download="${x => x.download}"
href=${x => x.href}
hreflang="${x => x.hreflang}"
Expand Down
12 changes: 9 additions & 3 deletions packages/nimble-components/src/checkbox/index.ts
Original file line number Diff line number Diff line change
@@ -1,11 +1,13 @@
import {
DesignSystem,
Checkbox as FoundationCheckbox,
CheckboxOptions,
checkboxTemplate as template
CheckboxOptions
} from '@microsoft/fast-foundation';
import { observable } from '@microsoft/fast-element';
import { check16X16, minus16X16 } from '@ni/nimble-tokens/dist/icons/js';
import { styles } from './styles';
import { checkboxTemplate as template } from './template';
import type { TabIndexOverride } from '../patterns/tab-index-override/types';

declare global {
interface HTMLElementTagNameMap {
Expand All @@ -16,7 +18,11 @@ declare global {
/**
* A nimble-styled checkbox control.
*/
export class Checkbox extends FoundationCheckbox {}
export class Checkbox extends FoundationCheckbox implements TabIndexOverride {
/** @internal */
@observable
public tabIndexOverride = 0;
}

const nimbleCheckbox = Checkbox.compose<CheckboxOptions>({
baseName: 'checkbox',
Expand Down
45 changes: 45 additions & 0 deletions packages/nimble-components/src/checkbox/template.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,45 @@
/* eslint-disable @typescript-eslint/prefer-optional-chain */
import { ViewTemplate, html, slotted } from '@microsoft/fast-element';
import type {
CheckboxOptions,
OverrideFoundationElementDefinition
} from '@microsoft/fast-foundation';
import type { Checkbox } from '.';

/**
* The template for the {@link @microsoft/fast-foundation#(Checkbox:class)} component.
* @public
*/
export const checkboxTemplate = (
_: unknown,
definition: OverrideFoundationElementDefinition<CheckboxOptions>
): ViewTemplate<Checkbox> => html<Checkbox>`
<template
role="checkbox"
aria-checked="${x => x.checked}"
aria-required="${x => x.required}"
aria-disabled="${x => x.disabled}"
aria-readonly="${x => x.readOnly}"
tabindex="${x => (x.disabled ? null : x.tabIndexOverride)}"
msmithNI marked this conversation as resolved.
Show resolved Hide resolved
@keypress="${(x, c) => x.keypressHandler(c.event as KeyboardEvent)}"
@click="${(x, c) => x.clickHandler(c.event as MouseEvent)}"
class="${x => (x.readOnly ? 'readonly' : '')} ${x => (x.checked ? 'checked' : '')} ${x => (x.indeterminate ? 'indeterminate' : '')}"
>
<div part="control" class="control">
<slot name="checked-indicator">
${definition.checkedIndicator || ''}
</slot>
<slot name="indeterminate-indicator">
${definition.indeterminateIndicator || ''}
</slot>
</div>
<label
part="label"
class="${x => (x.defaultSlottedNodes && x.defaultSlottedNodes.length
? 'label'
: 'label label__hidden')}"
>
<slot ${slotted('defaultSlottedNodes')}></slot>
</label>
</template>
`;
9 changes: 8 additions & 1 deletion packages/nimble-components/src/menu-button/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@ import { styles } from './styles';
import { template } from './template';
import type { ButtonPattern } from '../patterns/button/types';
import type { AnchoredRegion } from '../anchored-region';
import type { TabIndexOverride } from '../patterns/tab-index-override/types';

declare global {
interface HTMLElementTagNameMap {
Expand All @@ -27,7 +28,9 @@ declare global {
/**
* A nimble-styled menu button control.
*/
export class MenuButton extends FoundationElement implements ButtonPattern {
export class MenuButton
extends FoundationElement
implements ButtonPattern, TabIndexOverride {
@attr
public appearance: ButtonAppearance = ButtonAppearance.outline;

Expand Down Expand Up @@ -64,6 +67,10 @@ export class MenuButton extends FoundationElement implements ButtonPattern {
@observable
public readonly slottedMenus?: HTMLElement[];

/** @internal */
@observable
public tabIndexOverride = 0;

/**
* Used to maintain the internal state of whether the last menu item should be focused instead
* of the first menu item the next time the menu is opened.
Expand Down
1 change: 1 addition & 0 deletions packages/nimble-components/src/menu-button/template.ts
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@ export const template = html<MenuButton>`
?content-hidden="${x => x.contentHidden}"
?checked="${x => x.open}"
?disabled="${x => x.disabled}"
:tabIndexOverride="${x => x.tabIndexOverride}"
aria-haspopup="true"
aria-expanded="${x => x.open}"
@change="${(x, c) => x.toggleButtonCheckedChangeHandler(c.event)}"
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
/**
* Implemented by controls that typically set tabindex="0" in their templates,
* but allow overriding that behavior in some Nimble-internal cases (i.e. the table)
*/
export interface TabIndexOverride {
tabIndexOverride: number;
}
Original file line number Diff line number Diff line change
Expand Up @@ -57,6 +57,10 @@ TableColumnAnchorColumnConfig
public override focusedRecycleCallback(): void {
this.anchor?.blur();
}

public override get tabbableChildren(): HTMLElement[] {
return this.anchor !== undefined ? [this.anchor] : [];
}
}

const anchorCellView = TableColumnAnchorCellView.compose({
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@ export const template = html<TableColumnAnchorCellView>`
<${anchorTag}
${ref('anchor')}
${overflow('hasOverflow')}
:tabIndexOverride="${_ => -1}"
href="${x => x.cellRecord?.href}"
hreflang="${x => x.columnConfig?.hreflang}"
ping="${x => x.columnConfig?.ping}"
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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[] {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

If a table column type will contain focusable/tabbable elements, our guidance would be:

  • That column's cell view overrides this property, and returns the focusable element(s) in the array
  • The focusable elements should be tabindex=-1 by default

KeyboardNavigationManager uses this when handling Tab/Shift-Tab for a given row, and to reflect the correct focus state if one of these elements is focused (TableFocusType.cellContent).

Currently KeyboardNavigationManager updates tabindex of these elements directly (except for the tabIndexOverride cases described previously). For arbitrary cell content, this might not be sufficient, since the element that should be tabindex=0 could be an element descendant in a shadow root or something similar. So we may need to add an additional API/method to handle focusing/unfocusing these children.

return [];
}

private delegatedEvents: readonly string[] = [];

/**
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,8 @@ export const ExampleSortType = {
secondColumnDescendingFirstColumnAscending:
'SecondColumnDescendingFirstColumnAscending',
firstColumnAscendingSecondColumnDisabled:
'FirstColumnAscendingSecondColumnDisabled'
'FirstColumnAscendingSecondColumnDisabled',
allColumnsDisabled: 'AllColumnsDisabled'
} as const;
export type ExampleSortType =
(typeof ExampleSortType)[keyof typeof ExampleSortType];
Expand Down
15 changes: 15 additions & 0 deletions packages/nimble-components/src/table/components/cell/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down Expand Up @@ -52,6 +53,11 @@ export class TableCell<

public readonly actionMenuButton?: MenuButton;

/** @internal */
public get cellView(): TableCellView<TCellRecord> {
return this.shadowRoot?.firstElementChild as TableCellView<TCellRecord>;
}

public onActionMenuBeforeToggle(
event: CustomEvent<MenuButtonToggleEventDetail>
): void {
Expand All @@ -64,6 +70,15 @@ export class TableCell<
this.menuOpen = event.detail.newState;
this.$emit('cell-action-menu-toggle', event.detail);
}

/** @internal */
public onActionMenuButtonBlur(): void {
// The table keyboard navigation code adds a 'focused' class on action menu buttons before focusing them
// via keyboard, to ensure they're visible to focus. This code ensures we remove that CSS class when the menu
// button is no longer focused (which may not be for a keyboard nav reason, i.e. a mouse click elsewhere on
// the table)
this.actionMenuButton!.classList.remove('focused');
Copy link
Contributor

Choose a reason for hiding this comment

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

This is an implicit coupling between the table cell and the keyboard navigation manager which seems likely to get out of date if we change the keyboard manager implementation (e.g. rename this class). Is there anything we can do to make it more explicit and keep the knowledge of the focused class within one file?

For example, could the cell fire an event when the action menu is blurred and the keyboard navigation manager listen for that event?

}
}

const nimbleTableCell = TableCell.compose({
Expand Down
11 changes: 11 additions & 0 deletions packages/nimble-components/src/table/components/cell/styles.ts
Original file line number Diff line number Diff line change
@@ -1,10 +1,12 @@
import { css } from '@microsoft/fast-element';
import { display } from '@microsoft/fast-foundation';
import {
borderHoverColor,
controlHeight,
controlSlimHeight,
mediumPadding
} from '../../../theme-provider/design-tokens';
import { focusVisible } from '../../../utilities/style/focus';

export const styles = css`
${display('flex')}
Expand All @@ -22,6 +24,11 @@ export const styles = css`
--ni-private-table-cell-action-menu-display: block;
}

:host(${focusVisible}) {
outline: 2px solid ${borderHoverColor};
outline-offset: -2px;
}

.cell-view {
overflow: hidden;
display: flex;
Expand All @@ -36,4 +43,8 @@ export const styles = css`
height: ${controlSlimHeight};
align-self: center;
}

.action-menu.focused {
display: block;
atmgrifter00 marked this conversation as resolved.
Show resolved Hide resolved
}
`;
Original file line number Diff line number Diff line change
Expand Up @@ -16,9 +16,11 @@ export const template = html<TableCell>`
<${menuButtonTag} ${ref('actionMenuButton')}
content-hidden
appearance="${ButtonAppearance.ghost}"
:tabIndexOverride="${_ => -1}"
@beforetoggle="${(x, c) => x.onActionMenuBeforeToggle(c.event as CustomEvent<MenuButtonToggleEventDetail>)}"
@toggle="${(x, c) => x.onActionMenuToggle(c.event as CustomEvent<MenuButtonToggleEventDetail>)}"
@click="${(_, c) => c.event.stopPropagation()}"
@blur="${x => x.onActionMenuButtonBlur()}"
class="action-menu"
title="${x => (x.actionMenuLabel ? x.actionMenuLabel : tableCellActionMenuLabel.getValueFor(x))}"
>
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@ import type { TableColumn } from '../../../table-column/base';
import { styles } from './styles';
import { template } from './template';
import {
TableRowFocusableElements,
TableRowSelectionState,
TableRowSelectionToggleEventDetail
} from '../../types';
Expand All @@ -29,6 +30,9 @@ export class TableGroupRow extends FoundationElement {
@observable
public nestingLevel = 0;

@observable
public dataIndex?: number;

@observable
public immediateChildCount?: number;

Expand Down Expand Up @@ -100,6 +104,11 @@ export class TableGroupRow extends FoundationElement {
this.$emit('group-selection-toggle', detail);
}

/** @internal */
public getFocusableElements(): TableRowFocusableElements {
return { selectionCheckbox: this.selectionCheckbox, cells: [] };
}

private selectionStateChanged(): void {
this.setSelectionCheckboxState();
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@ import { display } from '@microsoft/fast-foundation';
import { White } from '@ni/nimble-tokens/dist/styledictionary/js/tokens';
import {
applicationBackgroundColor,
borderHoverColor,
borderWidth,
controlHeight,
fillHoverColor,
Expand All @@ -14,6 +15,7 @@ import { hexToRgbaCssColor } from '../../../utilities/style/colors';
import { themeBehavior } from '../../../utilities/style/theme';
import { userSelectNone } from '../../../utilities/style/user-select';
import { styles as expandCollapseStyles } from '../../../patterns/expand-collapse/styles';
import { focusVisible } from '../../../utilities/style/focus';

export const styles = css`
${display('grid')}
Expand Down Expand Up @@ -55,6 +57,11 @@ export const styles = css`
background-color: ${fillHoverColor};
}

:host(${focusVisible}) {
outline: 2px solid ${borderHoverColor};
outline-offset: -2px;
}

.expand-collapse-button {
margin-left: calc(
${mediumPadding} + ${standardPadding} * 2 *
Expand Down
Original file line number Diff line number Diff line change
@@ -1,12 +1,14 @@
import { css } from '@microsoft/fast-element';
import { display } from '@microsoft/fast-foundation';
import {
borderHoverColor,
controlHeight,
iconColor,
mediumPadding,
tableHeaderFont,
tableHeaderFontColor
} from '../../../theme-provider/design-tokens';
import { focusVisible } from '../../../utilities/style/focus';

export const styles = css`
${display('flex')}
Expand All @@ -23,6 +25,11 @@ export const styles = css`
cursor: default;
}

:host(${focusVisible}) {
outline: 2px solid ${borderHoverColor};
outline-offset: -2px;
}

.sort-indicator,
.grouped-indicator {
flex: 0 0 auto;
Expand Down
Loading
Loading