Skip to content

Commit 1f4503d

Browse files
gethinwebstergethinwebster
authored
fix: Open button dropdown when pressing up/down arrow keys (#3821)
1 parent e41e149 commit 1f4503d

File tree

5 files changed

+126
-83
lines changed

5 files changed

+126
-83
lines changed

‎src/button-dropdown/__tests__/button-dropdown-keyboard.test.tsx

Lines changed: 90 additions & 69 deletions
Original file line numberDiff line numberDiff line change
@@ -29,93 +29,114 @@ const items: ButtonDropdownProps.Items = [
2929
onClickSpy = jest.fn();
3030
onFollowSpy = jest.fn();
3131
wrapper = renderButtonDropdown({ ...props, items, onItemClick: onClickSpy, onItemFollow: onFollowSpy });
32-
act(() => wrapper.findNativeButton().keydown(KeyCode.enter));
3332
});
3433

35-
test('should close the dropdown when escape is pressed', () => {
36-
expect(wrapper.findOpenDropdown()).not.toBe(null);
37-
act(() => wrapper.findOpenDropdown()!.keydown(KeyCode.escape));
34+
test('should open the dropdown and focus first item when pressing down arrow key', () => {
3835
expect(wrapper.findOpenDropdown()).toBe(null);
39-
});
40-
test('should select the next item if "down" is pressed', () => {
41-
expect(wrapper.findHighlightedItem()!.getElement()).toHaveTextContent('item1');
42-
act(() => wrapper.findOpenDropdown()!.keydown(KeyCode.down));
43-
expect(wrapper.findHighlightedItem()!.getElement()).toHaveTextContent('item2');
44-
});
45-
46-
test('should select the previous item if "up" is pressed', () => {
47-
act(() => wrapper.findOpenDropdown()!.keydown(KeyCode.down));
48-
act(() => wrapper.findOpenDropdown()!.keydown(KeyCode.up));
36+
act(() => wrapper.findTriggerButton()!.keydown(KeyCode.down));
37+
expect(wrapper.findOpenDropdown()).toBeTruthy();
4938
expect(wrapper.findHighlightedItem()!.getElement()).toHaveTextContent('item1');
5039
});
5140

52-
test('should include disabled items', () => {
53-
act(() => wrapper.findOpenDropdown()!.keydown(KeyCode.down));
54-
act(() => wrapper.findOpenDropdown()!.keydown(KeyCode.down));
55-
expect(wrapper.findHighlightedItem()!.getElement()).toHaveTextContent('item3');
56-
});
57-
58-
test('should call onClick on enter', () => {
59-
act(() => wrapper.findOpenDropdown()!.keydown(KeyCode.enter));
60-
expect(onClickSpy).toHaveBeenCalledTimes(1);
61-
});
62-
63-
test('should not call onClick on enter a disabled item', () => {
64-
act(() => wrapper.findOpenDropdown()!.keydown(KeyCode.down));
65-
act(() => wrapper.findOpenDropdown()!.keydown(KeyCode.down));
66-
act(() => wrapper.findOpenDropdown()!.keydown(KeyCode.enter));
67-
expect(onClickSpy).not.toHaveBeenCalled();
68-
});
69-
70-
test('should call onClick on space', () => {
71-
act(() => wrapper.findOpenDropdown()!.keydown(KeyCode.space));
72-
act(() => wrapper.findOpenDropdown()!.keyup(KeyCode.space));
73-
expect(onClickSpy).toHaveBeenCalledTimes(1);
41+
test('should open the dropdown and focus last item when pressing up arrow key', () => {
42+
expect(wrapper.findOpenDropdown()).toBe(null);
43+
act(() => wrapper.findTriggerButton()!.keydown(KeyCode.up));
44+
expect(wrapper.findOpenDropdown()).toBeTruthy();
45+
expect(wrapper.findHighlightedItem()!.getElement()).toHaveTextContent('item5');
7446
});
7547

76-
test('should not call onClick on space a disabled item', () => {
77-
act(() => wrapper.findOpenDropdown()!.keydown(KeyCode.down));
78-
act(() => wrapper.findOpenDropdown()!.keydown(KeyCode.down));
79-
act(() => wrapper.findOpenDropdown()!.keydown(KeyCode.space));
80-
expect(onClickSpy).not.toHaveBeenCalled();
81-
});
48+
describe('when dropdown is open', () => {
49+
beforeEach(() => {
50+
act(() => wrapper.findNativeButton().keydown(KeyCode.enter));
51+
});
52+
53+
test('should close the dropdown when escape is pressed', () => {
54+
expect(wrapper.findOpenDropdown()).not.toBe(null);
55+
act(() => wrapper.findOpenDropdown()!.keydown(KeyCode.escape));
56+
expect(wrapper.findOpenDropdown()).toBe(null);
57+
});
58+
test('should select the next item if "down" is pressed', () => {
59+
expect(wrapper.findHighlightedItem()!.getElement()).toHaveTextContent('item1');
60+
act(() => wrapper.findOpenDropdown()!.keydown(KeyCode.down));
61+
expect(wrapper.findHighlightedItem()!.getElement()).toHaveTextContent('item2');
62+
});
8263

83-
test('should call onFollow on space if item has href', () => {
84-
act(() => wrapper.findOpenDropdown()!.keydown(KeyCode.down));
85-
act(() => wrapper.findOpenDropdown()!.keydown(KeyCode.space));
86-
act(() => wrapper.findOpenDropdown()!.keyup(KeyCode.space));
87-
expect(onFollowSpy).toHaveBeenCalledTimes(1);
88-
});
64+
test('should select the previous item if "up" is pressed', () => {
65+
act(() => wrapper.findOpenDropdown()!.keydown(KeyCode.down));
66+
act(() => wrapper.findOpenDropdown()!.keydown(KeyCode.up));
67+
expect(wrapper.findHighlightedItem()!.getElement()).toHaveTextContent('item1');
68+
});
8969

90-
test.each([KeyCode.enter, KeyCode.space])(
91-
'should fire event correctly when items with checkbox pressed using key=%s',
92-
keyCode => {
70+
test('should include disabled items', () => {
9371
act(() => wrapper.findOpenDropdown()!.keydown(KeyCode.down));
9472
act(() => wrapper.findOpenDropdown()!.keydown(KeyCode.down));
73+
expect(wrapper.findHighlightedItem()!.getElement()).toHaveTextContent('item3');
74+
});
75+
76+
test('should call onClick on enter', () => {
77+
act(() => wrapper.findOpenDropdown()!.keydown(KeyCode.enter));
78+
expect(onClickSpy).toHaveBeenCalledTimes(1);
79+
});
80+
81+
test('should not call onClick on enter a disabled item', () => {
9582
act(() => wrapper.findOpenDropdown()!.keydown(KeyCode.down));
9683
act(() => wrapper.findOpenDropdown()!.keydown(KeyCode.down));
84+
act(() => wrapper.findOpenDropdown()!.keydown(KeyCode.enter));
85+
expect(onClickSpy).not.toHaveBeenCalled();
86+
});
9787

98-
// Fire keydown on the 5th element, checkbox should be false after click
99-
act(() => wrapper.findItems()[4]!.keydown(keyCode));
100-
// Space handling is triggered on keyup
101-
if (keyCode === KeyCode.space) {
102-
act(() => wrapper.findItems()[4]!.keyup(keyCode));
103-
}
88+
test('should call onClick on space', () => {
89+
act(() => wrapper.findOpenDropdown()!.keydown(KeyCode.space));
90+
act(() => wrapper.findOpenDropdown()!.keyup(KeyCode.space));
10491
expect(onClickSpy).toHaveBeenCalledTimes(1);
105-
expect(onClickSpy).toHaveBeenCalledWith(expect.objectContaining({ detail: { id: 'i5', checked: false } }));
92+
});
10693

107-
// Open button dropdown again
108-
act(() => wrapper.findNativeButton().keydown(KeyCode.enter));
94+
test('should not call onClick on space a disabled item', () => {
95+
act(() => wrapper.findOpenDropdown()!.keydown(KeyCode.down));
96+
act(() => wrapper.findOpenDropdown()!.keydown(KeyCode.down));
97+
act(() => wrapper.findOpenDropdown()!.keydown(KeyCode.space));
98+
expect(onClickSpy).not.toHaveBeenCalled();
99+
});
109100

110-
// Fire keydown on the 1st element, checked should be undefined
111-
act(() => wrapper.findItems()[0]!.keydown(keyCode));
112-
// Space handling is triggered on keyup
113-
if (keyCode === KeyCode.space) {
114-
act(() => wrapper.findItems()[0]!.keyup(keyCode));
101+
test('should call onFollow on space if item has href', () => {
102+
act(() => wrapper.findOpenDropdown()!.keydown(KeyCode.down));
103+
act(() => wrapper.findOpenDropdown()!.keydown(KeyCode.space));
104+
act(() => wrapper.findOpenDropdown()!.keyup(KeyCode.space));
105+
expect(onFollowSpy).toHaveBeenCalledTimes(1);
106+
});
107+
108+
test.each([KeyCode.enter, KeyCode.space])(
109+
'should fire event correctly when items with checkbox pressed using key=%s',
110+
keyCode => {
111+
act(() => wrapper.findOpenDropdown()!.keydown(KeyCode.down));
112+
act(() => wrapper.findOpenDropdown()!.keydown(KeyCode.down));
113+
act(() => wrapper.findOpenDropdown()!.keydown(KeyCode.down));
114+
act(() => wrapper.findOpenDropdown()!.keydown(KeyCode.down));
115+
116+
// Fire keydown on the 5th element, checkbox should be false after click
117+
act(() => wrapper.findItems()[4]!.keydown(keyCode));
118+
// Space handling is triggered on keyup
119+
if (keyCode === KeyCode.space) {
120+
act(() => wrapper.findItems()[4]!.keyup(keyCode));
121+
}
122+
expect(onClickSpy).toHaveBeenCalledTimes(1);
123+
expect(onClickSpy).toHaveBeenCalledWith(expect.objectContaining({ detail: { id: 'i5', checked: false } }));
124+
125+
// Open button dropdown again
126+
act(() => wrapper.findNativeButton().keydown(KeyCode.enter));
127+
128+
// Fire keydown on the 1st element, checked should be undefined
129+
act(() => wrapper.findItems()[0]!.keydown(keyCode));
130+
// Space handling is triggered on keyup
131+
if (keyCode === KeyCode.space) {
132+
act(() => wrapper.findItems()[0]!.keyup(keyCode));
133+
}
134+
expect(onClickSpy).toHaveBeenCalledTimes(2);
135+
expect(onClickSpy).toHaveBeenCalledWith(
136+
expect.objectContaining({ detail: { id: 'i1', checked: undefined } })
137+
);
115138
}
116-
expect(onClickSpy).toHaveBeenCalledTimes(2);
117-
expect(onClickSpy).toHaveBeenCalledWith(expect.objectContaining({ detail: { id: 'i1', checked: undefined } }));
118-
}
119-
);
139+
);
140+
});
120141
});
121142
});

‎src/button-dropdown/__tests__/create-items-tree.test.ts

Lines changed: 10 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -63,4 +63,14 @@ describe('create-items-tree util', () => {
6363
expect(tree.getSequentialIndex([2, 0], -1)).toEqual([2]);
6464
expect(tree.getSequentialIndex([0, 0], -1)).toEqual(null);
6565
});
66+
67+
test('increment with looping', () => {
68+
const tree = createItemsTree(items);
69+
expect(tree.getSequentialIndex([4, 1], 1, true)).toEqual([0]);
70+
});
71+
72+
test('decrement with looping', () => {
73+
const tree = createItemsTree(items);
74+
expect(tree.getSequentialIndex([0, 0], -1, true)).toEqual([4, 1]);
75+
});
6676
});

‎src/button-dropdown/utils/create-items-tree.ts

Lines changed: 11 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -16,7 +16,7 @@ interface ItemsTreeApi {
1616
// in the tree (referential comparison), or an error will be thrown
1717
getItemIndex: (item: ButtonDropdownProps.ItemOrGroup) => TreeIndex;
1818
// Returns the index of next or previous sequential node or null if out of bounds
19-
getSequentialIndex: (index: TreeIndex, direction: -1 | 1) => TreeIndex | null;
19+
getSequentialIndex: (index: TreeIndex, direction: -1 | 1, loop?: boolean) => TreeIndex | null;
2020
// Returns parent tree index of a given item or null if no parent is present
2121
getParentIndex: (item: ButtonDropdownProps.ItemOrGroup) => TreeIndex | null;
2222
}
@@ -48,11 +48,19 @@ export default function createItemsTree(items: ButtonDropdownProps.Items): Items
4848

4949
return parseIndex(indexKey);
5050
},
51-
getSequentialIndex: (index: TreeIndex, direction: -1 | 1): TreeIndex | null => {
51+
getSequentialIndex: (index: TreeIndex, direction: -1 | 1, loop = false): TreeIndex | null => {
5252
const indexKey = stringifyIndex(index);
5353
const position = flatIndices.indexOf(indexKey);
5454

55-
const nextIndexKey = flatIndices[position + direction];
55+
let nextIndex = position + direction;
56+
if (loop) {
57+
if (nextIndex < 0) {
58+
nextIndex = flatIndices.length - 1;
59+
} else if (nextIndex >= flatIndices.length) {
60+
nextIndex = 0;
61+
}
62+
}
63+
const nextIndexKey = flatIndices[nextIndex];
5664

5765
if (!nextIndexKey) {
5866
return null;

‎src/button-dropdown/utils/use-button-dropdown.ts

Lines changed: 12 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -85,12 +85,6 @@ export function useButtonDropdown({
8585
closeDropdown();
8686
};
8787

88-
const doVerticalNavigation = (direction: -1 | 1) => {
89-
if (isOpen) {
90-
moveHighlight(direction);
91-
}
92-
};
93-
9488
const openAndSelectFirst = (event: React.KeyboardEvent) => {
9589
toggleDropdown();
9690
event.preventDefault();
@@ -129,12 +123,22 @@ export function useButtonDropdown({
129123
setIsUsingMouse(false);
130124
switch (event.keyCode) {
131125
case KeyCode.down: {
132-
doVerticalNavigation(1);
126+
if (!isOpen) {
127+
toggleDropdown();
128+
moveHighlight(1, true);
129+
} else {
130+
moveHighlight(1);
131+
}
133132
event.preventDefault();
134133
break;
135134
}
136135
case KeyCode.up: {
137-
doVerticalNavigation(-1);
136+
if (!isOpen) {
137+
toggleDropdown();
138+
moveHighlight(-1, true);
139+
} else {
140+
moveHighlight(-1);
141+
}
138142
event.preventDefault();
139143
break;
140144
}

‎src/button-dropdown/utils/use-highlighted-menu.ts

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -14,7 +14,7 @@ interface UseHighlightedMenuOptions {
1414
}
1515

1616
interface UseHighlightedMenuApi extends HighlightProps {
17-
moveHighlight: (direction: -1 | 1) => void;
17+
moveHighlight: (direction: -1 | 1, loop?: boolean) => void;
1818
expandGroup: (group?: ButtonDropdownProps.ItemGroup) => void;
1919
collapseGroup: () => void;
2020
reset: () => void;
@@ -61,9 +61,9 @@ export default function useHighlightedMenu({
6161
);
6262

6363
const moveHighlight = useCallback(
64-
(direction: -1 | 1) => {
64+
(direction: -1 | 1, loop?: boolean) => {
6565
const getNext = (index: TreeIndex) => {
66-
const nextIndex = getSequentialIndex(index, direction);
66+
const nextIndex = getSequentialIndex(index, direction, loop);
6767
const item = getItem(nextIndex || [-1]);
6868

6969
if (!nextIndex || !item) {

0 commit comments

Comments
 (0)