Skip to content
Merged
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
66 changes: 40 additions & 26 deletions components/__tests__/__snapshots__/story-snapshots.test.jsx.snap

Large diffs are not rendered by default.

9 changes: 8 additions & 1 deletion components/combobox/__docs__/Combobox.stories.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -54,12 +54,15 @@ const accountsWithIcons = accounts.map((account) => ({
export const Base: Story = {
render: () => {
const [inputValue, setInputValue] = useState('');
const [selection, setSelection] = useState<typeof accountsWithIcons>([]);
const [selection, setSelection] = useState<typeof accountsWithIcons>(
accountsWithIcons.slice(0, 2)
);

return (
<Combobox
id="base-combobox"
labels={{ label: 'Search', placeholder: 'Search Salesforce' }}
multiple
options={accountsWithIcons.filter(
(account) =>
!inputValue ||
Expand Down Expand Up @@ -98,6 +101,7 @@ export const BaseWithMenuSubheader: Story = {
<Combobox
id="base-with-subheader"
labels={{ label: 'Search', placeholder: 'Search Salesforce' }}
multiple
options={optionsWithSeparator}
selection={selection}
value={inputValue}
Expand All @@ -107,6 +111,9 @@ export const BaseWithMenuSubheader: Story = {
setSelection(newSelection);
setInputValue('');
},
onRequestRemoveSelectedOption: (_event, { selection: newSelection }) => {
setSelection(newSelection);
},
}}
/>
);
Expand Down
126 changes: 126 additions & 0 deletions components/combobox/__tests__/combobox.test.jsx
Original file line number Diff line number Diff line change
Expand Up @@ -183,6 +183,132 @@ describe('SLDSCombobox', () => {
expect(nodes.combobox).toHaveAttribute('aria-expanded', 'true');
});

// Regression (issue #3211): selected pills must expose consistent
// multi-selection semantics — each pill is a selected option, the owning
// listbox is aria-multiselectable, and a roving tabindex keeps exactly one
// pill tabbable.
it('base multiple: owning listbox is aria-multiselectable, pills are aria-selected options with roving tabindex', () => {
const { container } = render(
<DemoComponent
multiple
initialSelection={[
accountsWithIcon[0],
accountsWithIcon[1],
accountsWithIcon[2],
]}
/>
);
const { selectedListbox } = getNodes(container);

expect(selectedListbox).toHaveAttribute('role', 'listbox');
expect(selectedListbox).toHaveAttribute('aria-multiselectable', 'true');

const pills = selectedListbox.querySelectorAll('[role="option"]');
expect(pills).toHaveLength(3);
pills.forEach((pill) => {
expect(pill).toHaveAttribute('aria-selected', 'true');
});

// Roving tabindex: only the active pill (index 0) is in the tab order.
const tabbable = [...pills].filter(
(pill) => pill.getAttribute('tabindex') === '0'
);
expect(tabbable).toHaveLength(1);
expect(pills[0]).toHaveAttribute('tabindex', '0');
expect(pills[1]).toHaveAttribute('tabindex', '-1');
expect(pills[2]).toHaveAttribute('tabindex', '-1');
});

it('inline-listbox multiple: container is the aria-multiselectable listbox and inner list is a group', () => {
const { container } = render(
<DemoComponent
variant="inline-listbox"
multiple
initialSelection={[
accountsWithIcon[0],
accountsWithIcon[1],
accountsWithIcon[2],
]}
/>
);
const listboxContainer = container.querySelector(
`#${defaultProps.id}-selected-listbox`
);

expect(listboxContainer).toHaveAttribute('role', 'listbox');
expect(listboxContainer).toHaveAttribute('aria-multiselectable', 'true');

const group = listboxContainer.querySelector('.slds-listbox');
expect(group).toHaveAttribute('role', 'group');
expect(group).not.toHaveAttribute('aria-multiselectable');

const pills = listboxContainer.querySelectorAll('[role="option"]');
expect(pills).toHaveLength(3);
pills.forEach((pill) => {
expect(pill).toHaveAttribute('aria-selected', 'true');
});
});

it('single selection does not mark the listbox aria-multiselectable', () => {
const { container } = render(
<DemoComponent initialSelection={[accountsWithIcon[0]]} />
);
const { selectedListbox } = getNodes(container);

expect(selectedListbox).toHaveAttribute('role', 'listbox');
expect(selectedListbox).not.toHaveAttribute('aria-multiselectable');
});

// Regression (issue #3211): when a controlled parent shrinks `selection`
// while the active pill is the last one, the roving-tabindex index must
// be clamped so a pill stays tabbable. Otherwise the active index points
// past the end of `selection`, no pill gets tabindex=0, and the selected-
// pill listbox becomes unreachable by keyboard.
it('clamps the roving tabindex so a pill stays reachable after the controlled selection shrinks', () => {
const props = { ...defaultProps, multiple: true, options: [], events: {} };
const { container, rerender } = render(
<IconSettings iconPath="/assets/icons">
<Combobox
{...props}
value=""
selection={[
accountsWithIcon[0],
accountsWithIcon[1],
accountsWithIcon[2],
]}
/>
</IconSettings>
);

// Move the roving tabindex onto the LAST pill (as arrow/focus would).
let pills = getNodes(container).selectedListbox.querySelectorAll(
'[role="option"]'
);
expect(pills).toHaveLength(3);
fireEvent.focus(pills[2]);

// Controlled parent removes the last (active) pill out from under us.
rerender(
<IconSettings iconPath="/assets/icons">
<Combobox
{...props}
value=""
selection={[accountsWithIcon[0], accountsWithIcon[1]]}
/>
</IconSettings>
);

pills = getNodes(container).selectedListbox.querySelectorAll(
'[role="option"]'
);
expect(pills).toHaveLength(2);
const tabbable = [...pills].filter(
(pill) => pill.getAttribute('tabindex') === '0'
);
expect(tabbable).toHaveLength(1);
expect(pills[1]).toHaveAttribute('tabindex', '0');
});

it('menu filters to second item, menu listbox menu item 2 aria-selected is true, input activedescendent has item 2 id, after pressing down arrow, enter selects item 2', () => {
const { container } = render(
<DemoComponent
Expand Down
31 changes: 26 additions & 5 deletions components/combobox/combobox.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -765,13 +765,31 @@ const Combobox: React.FC<ComboboxProps> = (props) => {
}
}, [options, getOptions, activeOption]);

// Handle selection change - update active selected option
// Handle selection change - keep the active selected option and its roving
// tabindex in sync with `selection`. Besides seeding the initial active
// pill, this clamps the active index when `selection` shrinks. Without the
// clamp, a controlled parent removing a pill through any path other than
// the built-in remove handler could leave the active index pointing past
// the end of `selection`, so no pill receives tabindex=0 and the selected-
// pill listbox becomes unreachable by keyboard (WCAG 2.1.1 / 2.4.3).
useEffect(() => {
if (selection.length > 0 && !activeSelectedOption) {
setActiveSelectedOption(selection[0]);
setActiveSelectedOptionIndex(0);
if (selection.length === 0) {
if (activeSelectedOption !== undefined) {
setActiveSelectedOption(undefined);
setActiveSelectedOptionIndex(0);
}
return;
}

if (!activeSelectedOption || !selection[activeSelectedOptionIndex]) {
const clampedIndex =
activeSelectedOptionIndex >= selection.length
? selection.length - 1
: activeSelectedOptionIndex;
setActiveSelectedOption(selection[clampedIndex]);
setActiveSelectedOptionIndex(clampedIndex);
}
}, [selection, activeSelectedOption]);
}, [selection, activeSelectedOption, activeSelectedOptionIndex]);

// Cleanup on unmount
useEffect(() => {
Expand Down Expand Up @@ -991,6 +1009,7 @@ const Combobox: React.FC<ComboboxProps> = (props) => {
selectedListboxRef={setSelectedListboxRefCallback}
selection={selection as SelectedListBoxOption[]}
listboxHasFocus={listboxHasFocus}
multiselectable={multiple}
/>
{errorText && (
<div className="slds-has-error">
Expand Down Expand Up @@ -1034,6 +1053,7 @@ const Combobox: React.FC<ComboboxProps> = (props) => {
selectedListboxRef={setSelectedListboxRefCallback}
selection={selection as SelectedListBoxOption[]}
listboxHasFocus={listboxHasFocus}
multiselectable={multiple}
/>
) : null}
<div
Expand Down Expand Up @@ -1381,6 +1401,7 @@ const Combobox: React.FC<ComboboxProps> = (props) => {
listboxHasFocus={listboxHasFocus}
variant={variant}
renderAtSelectionLength={2}
multiselectable={multiple}
/>
{errorText && (
<div className="slds-has-error">
Expand Down
33 changes: 33 additions & 0 deletions components/pill-container/__tests__/pill-container.test.jsx
Original file line number Diff line number Diff line change
Expand Up @@ -119,6 +119,39 @@ describe('SLDSPillContainer', () => {
});
});

// Regression (issue #3211): the pill container is a listbox of selected
// options, so the owning listbox must be aria-multiselectable and each
// pill must be an aria-selected option with a roving tabindex.
it('exposes multi-selection semantics with a roving tabindex', () => {
const { container } = render(
<IconSettings iconPath="/assets/icons">
<PillContainer options={options} />
</IconSettings>
);

const listbox = container.querySelector('ul.slds-listbox');
expect(listbox).toHaveAttribute('role', 'listbox');
expect(listbox).toHaveAttribute('aria-multiselectable', 'true');

const pills = container.querySelectorAll('[role="option"]');
expect(pills).toHaveLength(7);
pills.forEach((pill) => {
expect(pill).toHaveAttribute('aria-selected', 'true');
});

// Roving tabindex: only the active pill (index 0) is in the tab order.
const tabbable = [...pills].filter(
(pill) => pill.getAttribute('tabindex') === '0'
);
expect(tabbable).toHaveLength(1);
expect(pills[0]).toHaveAttribute('tabindex', '0');
pills.forEach((pill, index) => {
if (index > 0) {
expect(pill).toHaveAttribute('tabindex', '-1');
}
});
});

it('Handles keyboard navigation properly', () => {
const requestRemoveData = [];

Expand Down
1 change: 1 addition & 0 deletions components/pill-container/index.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -272,6 +272,7 @@ const PillContainer = ({
isPillContainer
labels={labels}
listboxHasFocus={listboxHasFocus}
multiselectable
renderAtSelectionLength={0}
selection={options}
style={style}
Expand Down
13 changes: 13 additions & 0 deletions components/pill-container/private/selected-listbox.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -83,6 +83,12 @@ export interface SelectedListBoxProps {
isPillContainer?: boolean;
labels?: SelectedListBoxLabels;
listboxHasFocus?: boolean;
/**
* When `true`, the owning listbox element is marked `aria-multiselectable`.
* Required so that a listbox containing more than one `aria-selected` pill
* exposes consistent selection semantics to assistive technology.
*/
multiselectable?: boolean;
renderAtSelectionLength?: number;
selectedListboxRef?: (ref: HTMLUListElement | null) => void;
selection?: SelectedListBoxOption[];
Expand Down Expand Up @@ -168,6 +174,7 @@ const SelectedListBox = ({
isBare,
events,
labels,
multiselectable,
}: SelectedListBoxProps) =>
selection.length >= renderAtSelectionLength ? (
<div // eslint-disable-line jsx-a11y/role-supports-aria-props
Expand All @@ -190,6 +197,9 @@ const SelectedListBox = ({
style={style}
// Remove role and aria-orientation after slds-has-inline-listbox is deprecated in Combobox
role={containerRole}
aria-multiselectable={
multiselectable && containerRole === 'listbox' ? true : undefined
}
aria-orientation={
containerAriaOrientation as React.AriaAttributes['aria-orientation']
}
Expand All @@ -202,6 +212,9 @@ const SelectedListBox = ({
})}
aria-label={assistiveText?.selectedListboxLabel}
role={listboxRole}
aria-multiselectable={
multiselectable && listboxRole === 'listbox' ? true : undefined
}
aria-orientation={
listboxAriaOrientation as React.AriaAttributes['aria-orientation']
}
Expand Down
28 changes: 28 additions & 0 deletions components/pill/__tests__/pill.test.jsx
Original file line number Diff line number Diff line change
Expand Up @@ -292,6 +292,34 @@ describe('SLDSPill', () => {
const anchor = container.querySelector('.slds-pill__action');
expect(anchor).not.toBeInTheDocument();
});

// Regression: the pill must expose tabIndex and aria-selected on its root
// element so selected-listbox pills are focusable and support roving
// tabindex / arrow-key navigation (issue #3211).
it('applies tabIndex and aria-selected to the root element', () => {
const { container } = render(
<SLDSPill
labels={{ label: LABEL }}
variant="option"
tabIndex="0"
aria-selected="true"
onRemove={vi.fn()}
/>
);

const pill = container.querySelector('.slds-pill');
expect(pill).toHaveAttribute('tabindex', '0');
expect(pill).toHaveAttribute('aria-selected', 'true');
});

it('omits tabIndex when not provided', () => {
const { container } = render(
<SLDSPill labels={{ label: LABEL }} variant="option" onRemove={vi.fn()} />
);

const pill = container.querySelector('.slds-pill');
expect(pill).not.toHaveAttribute('tabindex');
});
});

describe('Linked Custom', () => {
Expand Down
14 changes: 14 additions & 0 deletions components/pill/index.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -65,6 +65,10 @@ export interface PillProps {
onKeyDown?: (event: KeyboardEvent<HTMLSpanElement>) => void;
/** Remove callback */
onRemove?: () => void;
/** Tab order index. Applied to the root element so the pill can receive focus (e.g. roving tabindex for pill navigation). */
tabIndex?: number | string;
/** Marks the pill as selected. Used when the pill acts as a listbox `option`. */
'aria-selected'?: boolean | 'true' | 'false';
/** Pill variant */
variant?: PillVariant;
}
Expand Down Expand Up @@ -100,6 +104,8 @@ const Pill = forwardRef<PillRef, PillProps>(({
onFocus,
onKeyDown,
onRemove,
tabIndex,
'aria-selected': ariaSelected,
variant = 'link',
}, ref) => {
const rootRef = useRef<HTMLSpanElement | null>(null);
Expand Down Expand Up @@ -219,6 +225,14 @@ const Pill = forwardRef<PillRef, PillProps>(({
<span
ref={rootRef}
role={role}
aria-selected={ariaSelected}
tabIndex={
tabIndex === undefined
? undefined
: typeof tabIndex === 'string'
? Number(tabIndex)
: tabIndex
}
className={classNames(
'slds-pill',
{
Expand Down
Loading