Skip to content

Commit 725bceb

Browse files
LFDanLuLFDanLu
authored
Fix keyboard navigation for collection and list items with falsy keys (#1114)
* fixing keyboard delegates and key lookup so it works with empty string * modifying tests so "" item key navigation is tested * Adding stories and menu test for item falsy key * fixing lint and test * moving falsy trigger story to end
1 parent 838cf99 commit 725bceb

File tree

12 files changed

+177
-25
lines changed

12 files changed

+177
-25
lines changed

‎packages/@react-aria/actiongroup/src/ActionGroupKeyboardDelegate.ts

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -67,7 +67,7 @@ export class ActionGroupKeyboardDelegate<T> implements KeyboardDelegate {
6767
getNextKey(key) {
6868
do {
6969
key = this.collection.getKeyAfter(key);
70-
if (!key) {
70+
if (key == null) {
7171
key = this.collection.getFirstKey();
7272
}
7373
} while (this.disabledKeys.has(key));
@@ -77,7 +77,7 @@ export class ActionGroupKeyboardDelegate<T> implements KeyboardDelegate {
7777
getPreviousKey(key) {
7878
do {
7979
key = this.collection.getKeyBefore(key);
80-
if (!key) {
80+
if (key == null) {
8181
key = this.collection.getLastKey();
8282
}
8383
} while (this.disabledKeys.has(key));

‎packages/@react-aria/selection/src/ListKeyboardDelegate.ts

Lines changed: 5 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -28,7 +28,7 @@ export class ListKeyboardDelegate<T> implements KeyboardDelegate {
2828

2929
getKeyBelow(key: Key) {
3030
key = this.collection.getKeyAfter(key);
31-
while (key) {
31+
while (key != null) {
3232
let item = this.collection.getItem(key);
3333
if (item.type === 'item' && !this.disabledKeys.has(key)) {
3434
return key;
@@ -40,7 +40,7 @@ export class ListKeyboardDelegate<T> implements KeyboardDelegate {
4040

4141
getKeyAbove(key: Key) {
4242
key = this.collection.getKeyBefore(key);
43-
while (key) {
43+
while (key != null) {
4444
let item = this.collection.getItem(key);
4545
if (item.type === 'item' && !this.disabledKeys.has(key)) {
4646
return key;
@@ -52,7 +52,7 @@ export class ListKeyboardDelegate<T> implements KeyboardDelegate {
5252

5353
getFirstKey() {
5454
let key = this.collection.getFirstKey();
55-
while (key) {
55+
while (key != null) {
5656
let item = this.collection.getItem(key);
5757
if (item.type === 'item' && !this.disabledKeys.has(key)) {
5858
return key;
@@ -64,7 +64,7 @@ export class ListKeyboardDelegate<T> implements KeyboardDelegate {
6464

6565
getLastKey() {
6666
let key = this.collection.getLastKey();
67-
while (key) {
67+
while (key != null) {
6868
let item = this.collection.getItem(key);
6969
if (item.type === 'item' && !this.disabledKeys.has(key)) {
7070
return key;
@@ -119,7 +119,7 @@ export class ListKeyboardDelegate<T> implements KeyboardDelegate {
119119

120120
let collection = this.collection;
121121
let key = fromKey || this.getFirstKey();
122-
while (key) {
122+
while (key != null) {
123123
let item = collection.getItem(key);
124124
let substring = item.textValue.slice(0, search.length);
125125
if (item.textValue && this.collator.compare(substring, search) === 0) {

‎packages/@react-aria/selection/src/useSelectableCollection.ts

Lines changed: 6 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -112,7 +112,7 @@ export function useSelectableCollection(options: SelectableCollectionOptions): S
112112
? delegate.getKeyBelow(manager.focusedKey)
113113
: delegate.getFirstKey();
114114

115-
if (nextKey) {
115+
if (nextKey != null) {
116116
manager.setFocusedKey(nextKey);
117117
if (manager.selectionMode === 'single' && selectOnFocus) {
118118
manager.replaceSelection(nextKey);
@@ -138,7 +138,7 @@ export function useSelectableCollection(options: SelectableCollectionOptions): S
138138
? delegate.getKeyAbove(manager.focusedKey)
139139
: delegate.getLastKey();
140140

141-
if (nextKey) {
141+
if (nextKey != null) {
142142
manager.setFocusedKey(nextKey);
143143
if (manager.selectionMode === 'single' && selectOnFocus) {
144144
manager.replaceSelection(nextKey);
@@ -161,7 +161,7 @@ export function useSelectableCollection(options: SelectableCollectionOptions): S
161161
if (delegate.getKeyLeftOf) {
162162
e.preventDefault();
163163
let nextKey = delegate.getKeyLeftOf(manager.focusedKey);
164-
if (nextKey) {
164+
if (nextKey != null) {
165165
manager.setFocusedKey(nextKey);
166166
if (manager.selectionMode === 'single' && selectOnFocus) {
167167
manager.replaceSelection(nextKey);
@@ -177,7 +177,7 @@ export function useSelectableCollection(options: SelectableCollectionOptions): S
177177
if (delegate.getKeyRightOf) {
178178
e.preventDefault();
179179
let nextKey = delegate.getKeyRightOf(manager.focusedKey);
180-
if (nextKey) {
180+
if (nextKey != null) {
181181
manager.setFocusedKey(nextKey);
182182
if (manager.selectionMode === 'single' && selectOnFocus) {
183183
manager.replaceSelection(nextKey);
@@ -219,7 +219,7 @@ export function useSelectableCollection(options: SelectableCollectionOptions): S
219219
if (delegate.getKeyPageBelow) {
220220
e.preventDefault();
221221
let nextKey = delegate.getKeyPageBelow(manager.focusedKey);
222-
if (nextKey) {
222+
if (nextKey != null) {
223223
manager.setFocusedKey(nextKey);
224224
if (e.shiftKey && manager.selectionMode === 'multiple') {
225225
manager.extendSelection(nextKey);
@@ -231,7 +231,7 @@ export function useSelectableCollection(options: SelectableCollectionOptions): S
231231
if (delegate.getKeyPageAbove) {
232232
e.preventDefault();
233233
let nextKey = delegate.getKeyPageAbove(manager.focusedKey);
234-
if (nextKey) {
234+
if (nextKey != null) {
235235
manager.setFocusedKey(nextKey);
236236
if (e.shiftKey && manager.selectionMode === 'multiple') {
237237
manager.extendSelection(nextKey);

‎packages/@react-aria/tabs/src/TabsKeyboardDelegate.ts

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -81,7 +81,7 @@ export class TabsKeyboardDelegate<T> implements KeyboardDelegate {
8181
getNextKey(key) {
8282
do {
8383
key = this.collection.getKeyAfter(key);
84-
if (!key) {
84+
if (key == null) {
8585
key = this.collection.getFirstKey();
8686
}
8787
} while (this.disabledKeys.has(key));
@@ -91,7 +91,7 @@ export class TabsKeyboardDelegate<T> implements KeyboardDelegate {
9191
getPreviousKey(key) {
9292
do {
9393
key = this.collection.getKeyBefore(key);
94-
if (!key) {
94+
if (key == null) {
9595
key = this.collection.getLastKey();
9696
}
9797
} while (this.disabledKeys.has(key));

‎packages/@react-spectrum/actiongroup/stories/ActionGroup.stories.tsx

Lines changed: 10 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -92,6 +92,16 @@ storiesOf('ActionGroup', module)
9292
</Flex>
9393
)
9494
)
95+
.add(
96+
'with falsy item key',
97+
() => (
98+
<ActionGroup onAction={action('onAction')}>
99+
<Item key="add">Add</Item>
100+
<Item key="">Delete</Item>
101+
<Item key="edit">Edit</Item>
102+
</ActionGroup>
103+
)
104+
)
95105
.add(
96106
'isDisabled',
97107
() => render({isDisabled: true, defaultSelectedKeys: ['1']}, docItems)

‎packages/@react-spectrum/actiongroup/test/ActionGroup.test.js

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -217,9 +217,9 @@ describe('ActionGroup', function () {
217217
let tree = render(
218218
<Provider theme={theme} locale={props.locale}>
219219
<ActionGroup orientation={props.orientation} >
220-
<Item data-testid="button-1">Click me 1</Item>
221-
<Item data-testid="button-2">Click me 2</Item>
222-
<Item data-testid="button-3">Click me 3</Item>
220+
<Item data-testid="button-1" key="1">Click me 1</Item>
221+
<Item data-testid="button-2" key="">Click me 2</Item>
222+
<Item data-testid="button-3" key="3">Click me 3</Item>
223223
</ActionGroup>
224224
</Provider>
225225
);

‎packages/@react-spectrum/menu/stories/MenuTrigger.stories.tsx

Lines changed: 10 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -477,6 +477,16 @@ storiesOf('MenuTrigger', module)
477477
</div>
478478
</>
479479
)
480+
)
481+
.add(
482+
'with falsy key',
483+
() => render(
484+
<Menu onAction={action('onAction')}>
485+
<Item key="1">One</Item>
486+
<Item key="">Two</Item>
487+
<Item key="3">Three</Item>
488+
</Menu>
489+
)
480490
);
481491

482492
let customMenuItem = (item) => {

‎packages/@react-spectrum/menu/test/MenuTrigger.test.js

Lines changed: 38 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -319,6 +319,44 @@ describe('MenuTrigger', function () {
319319
let selectedItem = menuItems[0];
320320
expect(selectedItem).toBe(document.activeElement);
321321
});
322+
323+
it.each`
324+
Name | Component | props
325+
${'MenuTrigger'} | ${MenuTrigger} | ${{}}
326+
`('$Name moves focus via ArrowDown and ArrowUp', function ({Component, props}) {
327+
let tree = render(
328+
<Provider theme={theme}>
329+
<div data-testid="scrollable">
330+
<MenuTrigger>
331+
<Button>
332+
{triggerText}
333+
</Button>
334+
<Menu>
335+
<Item key="1">One</Item>
336+
<Item key="">Two</Item>
337+
<Item key="3">Three</Item>
338+
</Menu>
339+
</MenuTrigger>
340+
</div>
341+
</Provider>
342+
);
343+
344+
let button = tree.getByRole('button');
345+
fireEvent.keyDown(button, {key: 'ArrowDown', code: 40, charCode: 40});
346+
let menu = tree.getByRole('menu');
347+
let menuItems = within(menu).getAllByRole('menuitem');
348+
let selectedItem = menuItems[0];
349+
expect(selectedItem).toBe(document.activeElement);
350+
351+
fireEvent.keyDown(menu, {key: 'ArrowDown', code: 40, charCode: 40});
352+
expect(menuItems[1]).toBe(document.activeElement);
353+
354+
fireEvent.keyDown(menu, {key: 'ArrowDown', code: 40, charCode: 40});
355+
expect(menuItems[2]).toBe(document.activeElement);
356+
357+
fireEvent.keyDown(menu, {key: 'ArrowUp', code: 38, charCode: 38});
358+
expect(menuItems[1]).toBe(document.activeElement);
359+
});
322360
});
323361

324362
describe('menu popover closing behavior', function () {

‎packages/@react-spectrum/picker/test/Picker.test.js

Lines changed: 54 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -287,6 +287,59 @@ describe('Picker', function () {
287287
expect(document.activeElement).toBe(items[2]);
288288
});
289289

290+
it('can change item focus with arrow keys, even for item key=""', function () {
291+
let onOpenChange = jest.fn();
292+
let {getByRole} = render(
293+
<Provider theme={theme}>
294+
<Picker label="Test" onOpenChange={onOpenChange}>
295+
<Item key="1">One</Item>
296+
<Item key="">Two</Item>
297+
<Item key="3">Three</Item>
298+
</Picker>
299+
</Provider>
300+
);
301+
302+
expect(() => getByRole('listbox')).toThrow();
303+
304+
let picker = getByRole('button');
305+
act(() => {fireEvent.keyDown(picker, {key: 'ArrowDown'});});
306+
act(() => {fireEvent.keyUp(picker, {key: 'ArrowDown'});});
307+
act(() => jest.runAllTimers());
308+
309+
let listbox = getByRole('listbox');
310+
expect(listbox).toBeVisible();
311+
expect(onOpenChange).toBeCalledTimes(1);
312+
expect(onOpenChange).toHaveBeenCalledWith(true);
313+
expect(picker).toHaveAttribute('aria-expanded', 'true');
314+
expect(picker).toHaveAttribute('aria-controls', listbox.id);
315+
316+
let items = within(listbox).getAllByRole('option');
317+
expect(items.length).toBe(3);
318+
expect(items[0]).toHaveTextContent('One');
319+
expect(items[1]).toHaveTextContent('Two');
320+
expect(items[2]).toHaveTextContent('Three');
321+
322+
expect(document.activeElement).toBe(items[0]);
323+
324+
act(() => {fireEvent.keyDown(listbox, {key: 'ArrowDown'});});
325+
act(() => {fireEvent.keyUp(listbox, {key: 'ArrowDown'});});
326+
act(() => jest.runAllTimers());
327+
328+
expect(document.activeElement).toBe(items[1]);
329+
330+
act(() => {fireEvent.keyDown(listbox, {key: 'ArrowDown'});});
331+
act(() => {fireEvent.keyUp(listbox, {key: 'ArrowDown'});});
332+
act(() => jest.runAllTimers());
333+
334+
expect(document.activeElement).toBe(items[2]);
335+
336+
act(() => {fireEvent.keyDown(listbox, {key: 'ArrowUp'});});
337+
act(() => {fireEvent.keyUp(listbox, {key: 'ArrowUp'});});
338+
act(() => jest.runAllTimers());
339+
340+
expect(document.activeElement).toBe(items[1]);
341+
});
342+
290343
it('supports controlled open state', function () {
291344
let onOpenChange = jest.fn();
292345
let {getByRole, getByLabelText} = render(
@@ -926,7 +979,7 @@ describe('Picker', function () {
926979
</Picker>
927980
</Provider>
928981
);
929-
982+
930983
let picker = getByRole('button');
931984
expect(picker).toHaveTextContent('Select an option…');
932985
act(() => triggerPress(picker));

‎packages/@react-spectrum/tabs/stories/Tabs.stories.js

Lines changed: 41 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -24,6 +24,10 @@ storiesOf('Tabs', module)
2424
'Default',
2525
() => render()
2626
)
27+
.add(
28+
'with falsy item key',
29+
() => renderWithFalsyKey()
30+
)
2731
.add(
2832
'defaultSelectedKey: val2',
2933
() => render({defaultSelectedKey: 'val2'})
@@ -161,3 +165,40 @@ function renderWithIcons(props = {}) {
161165
</Tabs>
162166
);
163167
}
168+
169+
function renderWithFalsyKey(props = {}) {
170+
return (
171+
<Tabs {...props} maxWidth={500} onSelectionChange={action('onSelectionChange')}>
172+
<Item title="Tab 1" key="">
173+
<Content margin="size-160">
174+
<Heading>Tab Body 1</Heading>
175+
<Text>
176+
Dolore ex esse laboris elit magna esse sunt. Pariatur in veniam Lorem est occaecat do magna nisi mollit ipsum sit adipisicing fugiat ex. Pariatur ullamco exercitation ea qui adipisicing.
177+
Id cupidatat aute id ut excepteur exercitation magna pariatur. Mollit irure irure reprehenderit pariatur eiusmod proident Lorem deserunt duis cillum mollit. Do reprehenderit sit cupidatat quis laborum in do culpa nisi ipsum. Velit aliquip commodo ea ipsum incididunt culpa nostrud deserunt incididunt exercitation. In quis proident sit ad dolore tempor. Eiusmod pariatur quis commodo labore cupidatat cillum enim eiusmod voluptate laborum culpa. Laborum cupidatat incididunt velit voluptate incididunt occaecat quis do.
178+
Consequat adipisicing irure Lorem commodo officia sint id. Velit sit magna aliquip eiusmod non id deserunt. Magna veniam ad consequat dolor cupidatat esse enim Lorem ullamco. Anim excepteur consectetur id in. Mollit laboris duis labore enim duis esse reprehenderit.
179+
</Text>
180+
</Content>
181+
</Item>
182+
<Item title="Tab 2" key="val2">
183+
<Content margin="size-160">
184+
<Heading>Tab Body 2</Heading>
185+
<Text>
186+
Dolore ex esse laboris elit magna esse sunt. Pariatur in veniam Lorem est occaecat do magna nisi mollit ipsum sit adipisicing fugiat ex. Pariatur ullamco exercitation ea qui adipisicing.
187+
Id cupidatat aute id ut excepteur exercitation magna pariatur. Mollit irure irure reprehenderit pariatur eiusmod proident Lorem deserunt duis cillum mollit. Do reprehenderit sit cupidatat quis laborum in do culpa nisi ipsum. Velit aliquip commodo ea ipsum incididunt culpa nostrud deserunt incididunt exercitation. In quis proident sit ad dolore tempor. Eiusmod pariatur quis commodo labore cupidatat cillum enim eiusmod voluptate laborum culpa. Laborum cupidatat incididunt velit voluptate incididunt occaecat quis do.
188+
Consequat adipisicing irure Lorem commodo officia sint id. Velit sit magna aliquip eiusmod non id deserunt. Magna veniam ad consequat dolor cupidatat esse enim Lorem ullamco. Anim excepteur consectetur id in. Mollit laboris duis labore enim duis esse reprehenderit.
189+
</Text>
190+
</Content>
191+
</Item>
192+
<Item title="Tab 3" key="val3">
193+
<Content margin="size-160">
194+
<Heading>Tab Body 3</Heading>
195+
<Text>
196+
Dolore ex esse laboris elit magna esse sunt. Pariatur in veniam Lorem est occaecat do magna nisi mollit ipsum sit adipisicing fugiat ex. Pariatur ullamco exercitation ea qui adipisicing.
197+
Id cupidatat aute id ut excepteur exercitation magna pariatur. Mollit irure irure reprehenderit pariatur eiusmod proident Lorem deserunt duis cillum mollit. Do reprehenderit sit cupidatat quis laborum in do culpa nisi ipsum. Velit aliquip commodo ea ipsum incididunt culpa nostrud deserunt incididunt exercitation. In quis proident sit ad dolore tempor. Eiusmod pariatur quis commodo labore cupidatat cillum enim eiusmod voluptate laborum culpa. Laborum cupidatat incididunt velit voluptate incididunt occaecat quis do.
198+
Consequat adipisicing irure Lorem commodo officia sint id. Velit sit magna aliquip eiusmod non id deserunt. Magna veniam ad consequat dolor cupidatat esse enim Lorem ullamco. Anim excepteur consectetur id in. Mollit laboris duis labore enim duis esse reprehenderit.
199+
</Text>
200+
</Content>
201+
</Item>
202+
</Tabs>
203+
);
204+
}

0 commit comments

Comments
 (0)