Skip to content
Draft
Show file tree
Hide file tree
Changes from all commits
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
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
{
"type": "minor",
"comment": "Adds support for selection-mode on the nimble-chip component to allow for toggleable chips.",
"packageName": "@ni/nimble-components",
"email": "1458528+fredvisser@users.noreply.github.com",
"dependentChangeType": "patch"
}
1 change: 1 addition & 0 deletions packages/nimble-components/src/all-components.ts
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@ import './button';
import './card';
import './card-button';
import './checkbox';
import './chip';
import './combobox';
import './dialog';
import './drawer';
Expand Down
199 changes: 193 additions & 6 deletions packages/nimble-components/src/chip/index.ts
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
import { attr, nullableNumberConverter, observable } from '@ni/fast-element';
import { keyEnter, keyEscape, keySpace } from '@ni/fast-web-utilities';
import {
applyMixins,
DesignSystem,
Expand All @@ -10,7 +11,7 @@ import {
} from '@ni/fast-foundation';
import { styles } from './styles';
import { template } from './template';
import { ChipAppearance } from './types';
import { ChipAppearance, ChipSelectionMode } from './types';
import { slotTextContent } from '../utilities/models/slot-text-content';
import { itemRemoveLabel } from '../label-provider/core/label-tokens';

Expand All @@ -19,6 +20,10 @@ declare global {
'nimble-chip': Chip;
}
}
export {
ChipSelectionMode,
type ChipSelectionMode as ChipSelectionModeType
} from './types';

export type ChipOptions = FoundationElementDefinition &
StartOptions &
Expand All @@ -34,12 +39,29 @@ export class Chip extends FoundationElement {
@attr({ mode: 'boolean' })
public disabled = false;

@attr({ attribute: 'selection-mode' })
public selectionMode: ChipSelectionMode;

@attr({ mode: 'boolean' })
public selected = false;

@attr()
public appearance: ChipAppearance = ChipAppearance.outline;

@attr({ attribute: 'tabindex', converter: nullableNumberConverter })
public override tabIndex!: number;

/**
* Indicates whether the remove button is currently in a mousedown state.
* Used to prevent the chip's active styling from showing when the remove button is being clicked.
*
* @internal
* @remarks
* This attribute is automatically managed by handleRemoveMousedown and should not be set directly.
*/
@attr({ attribute: 'remove-button-active', mode: 'boolean' })
public removeButtonActive = false;

/** @internal */
public readonly content?: HTMLElement[];

Expand All @@ -60,22 +82,187 @@ export class Chip extends FoundationElement {
/** @internal */
public contentSlot!: HTMLSlotElement;

private managingTabIndex = false;
private suppressTabIndexChanged = false;
private mouseUpHandler: EventListener | null = null;

public override connectedCallback(): void {
super.connectedCallback();
this.updateManagedTabIndex();
}

public override disconnectedCallback(): void {
super.disconnectedCallback();
if (this.mouseUpHandler) {
document.removeEventListener('mouseup', this.mouseUpHandler);
this.mouseUpHandler = null;
}
}

/** @internal */
public clickHandler(e: MouseEvent): boolean {
if (this.disabled) {
return false;
}

if (this.selectionMode === ChipSelectionMode.single) {
e.stopPropagation();
this.selected = !this.selected;
this.$emit('selected-change');
return false;
}
return true;
}

/** @internal */
public keyupHandler(e: KeyboardEvent): boolean {
if (this.disabled) {
return false;
}
switch (e.key) {
case keySpace:
if (this.selectionMode === ChipSelectionMode.single) {
e.stopPropagation();
this.selected = !this.selected;
this.$emit('selected-change');
}
return true;
default:
return true;
}
}

/** @internal */
public handleRemoveClick(): void {
public keydownHandler(e: KeyboardEvent): boolean {
if (this.disabled) {
return false;
}
switch (e.key) {
case keySpace:
if (this.selectionMode === ChipSelectionMode.single) {
return false;
}
return true;
case keyEnter:
if (this.selectionMode === ChipSelectionMode.single) {
e.stopPropagation();
this.selected = !this.selected;
this.$emit('selected-change');
return false;
}
return true;
case keyEscape:
if (this.removable) {
e.stopPropagation();
this.$emit('remove');
return false;
}
return true;
default:
return true;
}
}

/** @internal */
public handleRemoveClick(event: MouseEvent): void {
event.stopPropagation();
if (this.removable) {
this.$emit('remove');
}
}

/**
* Handles mousedown events on the remove button.
* Sets removeButtonActive to true and registers a document-level mouseup handler to clear it.
*
* @internal
* @remarks
* The mouseup listener is added to document to ensure it fires even if the mouse moves
* outside the chip before being released. The listener is cleaned up in disconnectedCallback
* to prevent memory leaks.
*/
public handleRemoveMousedown(event: MouseEvent): void {
event.stopPropagation();
this.removeButtonActive = true;

// Clean up any existing listener first
if (this.mouseUpHandler) {
document.removeEventListener('mouseup', this.mouseUpHandler);
}

this.mouseUpHandler = (): void => {
this.removeButtonActive = false;
if (this.mouseUpHandler) {
document.removeEventListener('mouseup', this.mouseUpHandler);
this.mouseUpHandler = null;
}
};
document.addEventListener('mouseup', this.mouseUpHandler);
}

/** @internal */
public handleRemoveKeyup(event: KeyboardEvent): void {
event.stopPropagation();
}

protected selectionModeChanged(
_oldValue: ChipSelectionMode | undefined,
_newValue: ChipSelectionMode | undefined
): void {
this.updateManagedTabIndex();
}

protected disabledChanged(_oldValue: boolean, _newValue: boolean): void {
this.updateManagedTabIndex();
}

protected tabIndexChanged(): void {
if (this.suppressTabIndexChanged) {
this.suppressTabIndexChanged = false;
return;
}

this.managingTabIndex = false;
}

private updateManagedTabIndex(): void {
if (!this.$fastController?.isConnected) {
return;
}

const shouldManage = this.selectionMode === ChipSelectionMode.single && !this.disabled;

if (shouldManage) {
if (!this.hasAttribute('tabindex')) {
this.setManagedTabIndex(0);
}
} else {
this.removeManagedTabIndex();
}
}

private setManagedTabIndex(value: number): void {
this.managingTabIndex = true;
this.suppressTabIndexChanged = true;
this.tabIndex = value;
}

private removeManagedTabIndex(): void {
if (!this.managingTabIndex) {
return;
}

this.managingTabIndex = false;
this.suppressTabIndexChanged = true;
this.removeAttribute('tabindex');
}
}
applyMixins(Chip, StartEnd);

const nimbleChip = Chip.compose<ChipOptions>({
baseName: 'chip',
template,
styles,
shadowOptions: {
delegatesFocus: true
}
styles
});

DesignSystem.getOrCreate().withPrefix('nimble').register(nimbleChip());
Expand Down
17 changes: 12 additions & 5 deletions packages/nimble-components/src/chip/specs/README.md
Original file line number Diff line number Diff line change
Expand Up @@ -64,10 +64,13 @@ _The key elements of the component's public API surface:_
- `removable` - set to show the remove button
- `appearance` - supports `outline` and `block` appearances
- `disabled` - styles the chip in the typical Nimble manner, including any user-slotted content (text and icon), and additionally hides the remove button.
- `selection-mode` - controls whether the chip can be selected. Values: `'none'` (default) or `'single'`. When `'single'`, the chip acts as a toggle button with `role="button"` and `aria-pressed`.
Copy link
Contributor

Choose a reason for hiding this comment

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

'single' implies there might someday be a 'multiple'. I'm not sure about how that would work though. Right now we only have individual chips that are not aware of other chips. We don't have a container component like the table is for rows or the radio group is for radio buttons. Without that, I'm not sure what would enforce how many chips can be selected and thus I'm not sure it makes sense for clients to specify it via the API on the chip. I don't want to imply that Nimble will coordinate selection state across chips somehow.

Some options:

  1. take the time to design that parent component and move the selection-mode API to the parent component. But then we'd need to figure out other aspects of its behavior like sizing / layout / spacing.
  2. change this to a boolean attribute like selectable for now. If it later becomes important for apps to enforce single vs multiple selection mode, we'd have some options:
    • provide instructions for apps to implement single vs multi select patterns themselves. (It might require adding some new cancelable "beforeselection" event, I haven't thought it through)
    • build a Nimble container component with an API to specify selection behavior and make a breaking change to remove the attribute from the chip if it's now redundant.

- `selected` - indicates the current selection state when `selection-mode="single"`. When `true`, the chip displays with selected background styling.
- _Methods_
- _None_
- _Events_
- `remove` - fired when the chip remove button is pressed.
- `remove` - fired when the chip remove button is pressed, or when Escape key is pressed on a removable chip.
- `selected-change` - fired when the user toggles the chip's selected state via click or keyboard (Space/Enter). Only emitted when `selection-mode="single"`.
- _Slots_
- `start` - icon placed to the left of the chip text
- (default) - for the primary label text
Expand Down Expand Up @@ -145,12 +148,18 @@ We will provide styling for the `disabled` attribute state.
_Consider the accessibility of the component, including:_

- _Keyboard Navigation and Focus_
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you update the API section above to include the new APIs too? The other md file seems more like copilot instructions for this specific feature but it's still helpful to keep the entire API summarized in this higher level spec.

- when the chip component is removable, the remove button will be focusable, otherwise it will not receive focus (following the `nimble-banner` pattern).
- When the chip is selectable (`selection-mode="single"`) and removable:
- The chip itself is focusable and receives keyboard events
- Space/Enter toggles the selected state
- Escape removes the chip (emits `remove` event)
Copy link
Contributor

Choose a reason for hiding this comment

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

Did you consider whether Escape should also emit the event when the chip is not selectable? I'd advocate for it to work in both cases unless there's a reason not to.

- The remove button is **not** focusable (`tabindex="-1"`) to avoid nested interactive controls (violates [WCAG 4.1.2](https://dequeuniversity.com/rules/axe/4.11/nested-interactive))
Copy link
Contributor

Choose a reason for hiding this comment

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

This line is unclear to me. Is it saying that this proposal violates the rule and we're ok with it or that this choice is made to avoid violating that rule? If it's the former, what's the justification for violating the rule?

- When the chip is removable but not selectable (`selection-mode="none"`):
- The remove button is focusable and can be activated with Space or Enter
- _Form Input_
- N/A
- _Use with Assistive Technology_
- When selectable, the chip has `role="button"` and `aria-pressed` to indicate its toggle state
- a `chip`'s accessible name comes from the element's contents by default
- no ARIA `role` seems necessary to define for the chip, as it isn't interactive itself (only the remove button is which has a `role`). The only valid role seemed to be [`status`](https://developer.mozilla.org/en-US/docs/Web/Accessibility/ARIA/Reference/Roles/status_role), but that also didn't seem helpful from an accessibility perspective, particularly since it mainly relates to providing helpful information when the content changes (which we don't expect).
- the remove button will have its content set to provide a label provider token for "Remove".
- title will not be set, which aligns with decisions for the filterable select clear button and the banner
- ideally this would include the contents of the chip itself (so a screen reader would announce "Remove <Chip>") but differing word order between
Copy link
Contributor

Choose a reason for hiding this comment

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

There's a Future Work section below that mentions making the chip selectable which could be removed now.

Expand All @@ -160,8 +169,6 @@ _Consider the accessibility of the component, including:_

### Future work

- Make chip selectable (there are already UX designs)
- Currently there are no use-cases for chips requiring them to be selectable, but there are many use-cases in the wild where this is needed.
- Provide error state for the chip (there are already UX designs)
- Again, there are no current use-cases requiring a chip to present with error information, but it is not unreasonable to expect we may have such a use-case in the future.
- Create a chip container component that manages chip layout, and removal
Expand Down
Loading