Skip to content

Commit 044c690

Browse files
snowystingersnowystinger
committed
enable textfield inside table tests and fix logic
1 parent 3bc865f commit 044c690

File tree

5 files changed

+82
-55
lines changed

5 files changed

+82
-55
lines changed

‎packages/@react-aria/grid/src/useGridCell.ts‎

Lines changed: 20 additions & 19 deletions
Original file line numberDiff line numberDiff line change
@@ -113,25 +113,26 @@ export function useGridCell<T, C extends GridCollection<T>>(props: GridCellProps
113113
return;
114114
}
115115

116-
if (e.target !== ref.current && e.key !== 'ArrowLeft' && e.key !== 'ArrowRight' && e.key !== 'ArrowUp' && e.key !== 'ArrowDown') {
117-
if (e.key === 'Tab' && ref.current.contains(e.target as Element)) {
118-
let cellWalker = getFocusableTreeWalker(ref.current, {tabbable: true});
119-
if (e.shiftKey) {
120-
cellWalker.currentNode = ref.current;
121-
let isFirstFocusable = cellWalker.firstChild() === e.target;
122-
if (!isFirstFocusable) {
123-
e.stopPropagation();
124-
}
125-
} else {
126-
cellWalker.currentNode = ref.current;
127-
let isLastFocusable = cellWalker.lastChild() === e.target;
128-
if (!isLastFocusable) {
129-
e.stopPropagation();
130-
}
131-
}
132-
}
133-
return;
134-
}
116+
// TODO: keyboard handler should only stop propagation on keys we intend to handle, not ALL keys, that way we don't have to call continue then call stop else where but only *sometimes* depending on the order
117+
// if (e.target !== ref.current && e.key !== 'ArrowLeft' && e.key !== 'ArrowRight' && e.key !== 'ArrowUp' && e.key !== 'ArrowDown') {
118+
// if (e.key === 'Tab' && ref.current.contains(e.target as Element)) {
119+
// let cellWalker = getFocusableTreeWalker(ref.current, {tabbable: true});
120+
// if (e.shiftKey) {
121+
// cellWalker.currentNode = ref.current;
122+
// let isFirstFocusable = cellWalker.firstChild() === e.target;
123+
// if (!isFirstFocusable) {
124+
// e.stopPropagation();
125+
// }
126+
// } else {
127+
// cellWalker.currentNode = ref.current;
128+
// let isLastFocusable = cellWalker.lastChild() === e.target;
129+
// if (!isLastFocusable) {
130+
// e.stopPropagation();
131+
// }
132+
// }
133+
// }
134+
// return;
135+
// }
135136

136137
let walker = getFocusableTreeWalker(ref.current);
137138
walker.currentNode = document.activeElement;

‎packages/@react-aria/grid/test/useGrid.test.js‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -60,7 +60,7 @@ describe('useGrid', () => {
6060
await user.keyboard('[ArrowRight]');
6161
expect(document.activeElement).toBe(tree.getAllByRole('switch')[0]);
6262

63-
await user.tab();
63+
await user.keyboard('[ArrowRight]');
6464
expect(document.activeElement).toBe(tree.getAllByRole('switch')[1]);
6565

6666
await user.keyboard('[ArrowRight]');
@@ -71,7 +71,7 @@ describe('useGrid', () => {
7171

7272
act(() => tree.getAllByRole('switch')[1].focus());
7373

74-
await user.tab({shift: true});
74+
await user.keyboard('[ArrowLeft]');
7575
expect(document.activeElement).toBe(tree.getAllByRole('switch')[0]);
7676

7777
await user.keyboard('[ArrowLeft]');

‎packages/@react-aria/textfield/src/useTextField.ts‎

Lines changed: 48 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -135,31 +135,47 @@ export function useTextField<T extends TextFieldIntrinsicElements = DefaultEleme
135135
if ((e.key === 'ArrowLeft' || e.key === 'ArrowUp' || e.key === 'Home')
136136
&& (e.target as HTMLInputElement).selectionStart === 0
137137
&& (e.target as HTMLInputElement).selectionEnd === 0) {
138-
e.continuePropagation();
139-
}
140-
if ((e.key === 'ArrowRight' || e.key === 'ArrowDown' || e.key === 'End')
138+
if (e.isPropagationStopped()) {
139+
e.continuePropagation();
140+
}
141+
} else if ((e.key === 'ArrowRight' || e.key === 'ArrowDown' || e.key === 'End')
141142
&& (e.target as HTMLInputElement).selectionStart === (e.target as HTMLInputElement).value.length
142143
&& (e.target as HTMLInputElement).selectionEnd === (e.target as HTMLInputElement).value.length) {
143-
e.continuePropagation();
144+
if (e.isPropagationStopped()) {
145+
e.continuePropagation();
146+
}
147+
} else if (KEYS_TO_CONTINUE_PROPAGATION.has(e.key)) {
148+
if (e.isPropagationStopped()) {
149+
e.continuePropagation();
150+
}
151+
} else {
152+
if (!e.isPropagationStopped()) {
153+
e.stopPropagation();
154+
}
144155
}
145-
if (KEYS_TO_CONTINUE_PROPAGATION.has(e.key)) {
146-
e.continuePropagation();
147-
};
148156
onKeyDownProp?.(e);
149157
}, [onKeyDownProp]);
150158
let onKeyUp = useCallback((e: BaseEvent<KeyboardEvent<any>>) => {
151159
if ((e.key === 'ArrowLeft' || e.key === 'ArrowUp')
152160
&& (e.target as HTMLInputElement).selectionStart === 0
153161
&& (e.target as HTMLInputElement).selectionEnd === 0) {
154-
e.continuePropagation();
155-
}
156-
if ((e.key === 'ArrowRight' || e.key === 'ArrowDown')
162+
if (e.isPropagationStopped()) {
163+
e.continuePropagation();
164+
}
165+
} else if ((e.key === 'ArrowRight' || e.key === 'ArrowDown')
157166
&& (e.target as HTMLInputElement).selectionStart === (e.target as HTMLInputElement).value.length
158167
&& (e.target as HTMLInputElement).selectionEnd === (e.target as HTMLInputElement).value.length) {
159-
e.continuePropagation();
160-
}
161-
if (KEYS_TO_CONTINUE_PROPAGATION.has(e.key)) {
162-
e.continuePropagation();
168+
if (e.isPropagationStopped()) {
169+
e.continuePropagation();
170+
}
171+
} else if (KEYS_TO_CONTINUE_PROPAGATION.has(e.key)) {
172+
if (e.isPropagationStopped()) {
173+
e.continuePropagation();
174+
}
175+
} else {
176+
if (!e.isPropagationStopped()) {
177+
e.stopPropagation();
178+
}
163179
};
164180
onKeyUpProp?.(e);
165181
}, [onKeyUpProp]);
@@ -233,6 +249,24 @@ export function useTextField<T extends TextFieldIntrinsicElements = DefaultEleme
233249
spellCheck: props.spellCheck,
234250
[parseInt(React.version, 10) >= 17 ? 'enterKeyHint' : 'enterkeyhint']: props.enterKeyHint,
235251

252+
// TODO: Always?? or only if we're inside a grid? in which case maybe I should do this in all of Grid hooks by checking if target instance of HTMLInputElement?
253+
onPointerDown: (e) => {
254+
e.stopPropagation();
255+
props.onKeyDown?.(e);
256+
},
257+
onPointerUp: (e) => {
258+
e.stopPropagation();
259+
props.onKeyUp?.(e);
260+
},
261+
onClick: (e) => {
262+
e.stopPropagation();
263+
props.onClick?.(e);
264+
},
265+
onDoubleClick: (e) => {
266+
e.stopPropagation();
267+
props.onDoubleClick?.(e);
268+
},
269+
236270
// Clipboard events
237271
onCopy: props.onCopy,
238272
onCut: props.onCut,

‎packages/@react-spectrum/table/test/TableTests.js‎

Lines changed: 1 addition & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -1815,7 +1815,6 @@ export let tableTests = () => {
18151815

18161816
// Simulate tabbing within the table
18171817
await user.tab();
1818-
await user.tab();
18191818

18201819
let after = tree.getByTestId('after');
18211820
expect(document.activeElement).toBe(after);
@@ -4252,8 +4251,7 @@ export let tableTests = () => {
42524251

42534252
expect(document.activeElement).toEqual(input);
42544253

4255-
fireEvent.keyDown(input, {key: 'Escape', code: 27, charCode: 27});
4256-
fireEvent.keyUp(input, {key: 'Escape', code: 27, charCode: 27});
4254+
await user.keyboard('{Escape}');
42574255
act(() => {
42584256
jest.runAllTimers();
42594257
});

‎packages/react-aria-components/test/Table.test.js‎

Lines changed: 11 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -2655,7 +2655,7 @@ describe('Table', () => {
26552655
});
26562656
});
26572657

2658-
describe.skip('Editable fields in cells', () => {
2658+
describe('Editable fields in cells', () => {
26592659
describe.each(['none', 'single', 'multiple'])('selectionMode: %s', (selectionMode) => {
26602660
it('should support editing a textfield in a cell in a table with keyboard interactions', async () => {
26612661
let {getByRole, getAllByRole} = render(
@@ -2716,15 +2716,14 @@ describe('Table', () => {
27162716
await user.keyboard('{ArrowRight}');
27172717
await user.keyboard('{ArrowRight}');
27182718
if (selectionMode === 'none') {
2719-
expect(tableTester.cells({element: tableTester.rows[0]})[2]).toHaveFocus();
2719+
expect(inputs[0]).toHaveFocus();
27202720
} else {
27212721
// in selection modes, account for extra checkbox column
27222722
await user.keyboard('{ArrowRight}');
2723-
expect(tableTester.cells({element: tableTester.rows[0]})[3]).toHaveFocus();
2723+
expect(inputs[0]).toHaveFocus();
27242724
}
2725-
expect(inputs[0]).toHaveFocus();
27262725
await user.keyboard('{ArrowRight}');
2727-
expect(inputs[0]).toHaveFocus();
2726+
expect(inputs[1]).toHaveFocus();
27282727
await user.keyboard('{ArrowLeft}');
27292728
expect(inputs[0]).toHaveFocus();
27302729
// Type a string that would trigger a typeahead or selection if we weren't in a textfield
@@ -2733,10 +2732,10 @@ describe('Table', () => {
27332732
expect(inputs[0]).toHaveFocus();
27342733

27352734
// Navigate to second textfield in first row
2736-
await user.tab();
2737-
expect(inputs[1]).toHaveFocus();
27382735
await user.keyboard('{ArrowRight}');
27392736
expect(inputs[1]).toHaveFocus();
2737+
await user.keyboard('{ArrowRight}');
2738+
expect(tableTester.rows[0]).toHaveFocus();
27402739
await user.keyboard('{ArrowLeft}');
27412740
expect(inputs[1]).toHaveFocus();
27422741
// Type a string that would trigger a typeahead or selection if we weren't in a textfield
@@ -2750,11 +2749,7 @@ describe('Table', () => {
27502749
// Come back to the table, we should remember roughly where we were, in this case, on the cell containing the input.
27512750
// We may want this to focus the input itself instead of the cell.
27522751
await user.tab({shift: true});
2753-
if (selectionMode === 'none') {
2754-
expect(tableTester.cells({element: tableTester.rows[0]})[2]).toHaveFocus();
2755-
} else {
2756-
expect(tableTester.cells({element: tableTester.rows[0]})[3]).toHaveFocus();
2757-
}
2752+
expect(inputs[0]).toHaveFocus(); // TODO: this should be the second input if we were on it previously, but it's always the first input right now
27582753
});
27592754

27602755
describe('pointer interactions', () => {
@@ -2815,7 +2810,7 @@ describe('Table', () => {
28152810
await user.click(inputs[0]);
28162811
expect(inputs[0]).toHaveFocus();
28172812
await user.keyboard('{ArrowRight}');
2818-
expect(inputs[0]).toHaveFocus();
2813+
expect(inputs[1]).toHaveFocus();
28192814
await user.keyboard('{ArrowLeft}');
28202815
expect(inputs[0]).toHaveFocus();
28212816
// Type a string that would trigger a typeahead or selection if we weren't in a textfield
@@ -2827,7 +2822,7 @@ describe('Table', () => {
28272822
await user.click(inputs[1]);
28282823
expect(inputs[1]).toHaveFocus();
28292824
await user.keyboard('{ArrowRight}');
2830-
expect(inputs[1]).toHaveFocus();
2825+
expect(tableTester.rows[0]).toHaveFocus();
28312826
await user.keyboard('{ArrowLeft}');
28322827
expect(inputs[1]).toHaveFocus();
28332828
// Type a string that would trigger a typeahead or selection if we weren't in a textfield
@@ -2888,16 +2883,15 @@ describe('Table', () => {
28882883
await user.keyboard('{ArrowRight}');
28892884
expect(inputs[1]).toHaveFocus();
28902885
await user.keyboard('{ArrowRight}');
2891-
expect(inputs[1]).toHaveFocus();
2886+
expect(tableTester.rows[0]).toHaveFocus();
28922887
await user.keyboard('{ArrowLeft}');
28932888
expect(inputs[1]).toHaveFocus();
28942889
// Type a string that would trigger a typeahead or selection if we weren't in a textfield
28952890
await user.keyboard('B ');
28962891
expect(tableTester.selectedRows).toHaveLength(0);
28972892
expect(inputs[1]).toHaveFocus();
28982893

2899-
await user.tab({shift: true});
2900-
2894+
// TODO: correct behaviour? selection cursor is where it would be if you pressed down, so it doesn't do anything, so should it be allowed to navigate cells now?
29012895
await user.keyboard('{ArrowDown}');
29022896
await user.tab();
29032897
expect(button).toHaveFocus();

0 commit comments

Comments
 (0)