Skip to content

Commit a0a88da

Browse files
LFDanLuLFDanLu
authored
ListView dnd testing followups (#3181)
* fixing listview dnd issues with disabled rows and selected items removed from the collection fixes an issue where disabled rows were focusable via screen reader when in a drag session. Also fixes an issue where drag and drop events would contain all selected keys even if said keys dont exist in the draggable collection * calculating isFocused in useEffect in case manager.focusedKey is updated in a side effect fixes case where the wrong key is focused after dropping into a list. useDroppableCollection updates focused key in a useLayoutEffect which causes the render calculated isFocused in useSelectableItem to be out of date and causes the wrong item to get focused * adding test for focusing the dropped item * attempting to fix build on 16/17 * minimum amount of await run all timer calls * updating to clear promise queue * adding tab index check
1 parent 7eb6c62 commit a0a88da

File tree

5 files changed

+232
-23
lines changed

5 files changed

+232
-23
lines changed

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

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -135,24 +135,24 @@ export function useSelectableItem(options: SelectableItemOptions): SelectableIte
135135
};
136136

137137
// Focus the associated DOM node when this item becomes the focusedKey
138-
let isFocused = key === manager.focusedKey;
139138
useEffect(() => {
139+
let isFocused = key === manager.focusedKey;
140140
if (isFocused && manager.isFocused && !shouldUseVirtualFocus && document.activeElement !== ref.current) {
141141
if (focus) {
142142
focus();
143143
} else {
144144
focusSafely(ref.current);
145145
}
146146
}
147-
}, [ref, isFocused, manager.focusedKey, manager.childFocusStrategy, manager.isFocused, shouldUseVirtualFocus]);
147+
}, [ref, key, manager.focusedKey, manager.childFocusStrategy, manager.isFocused, shouldUseVirtualFocus]);
148148

149149
// Set tabIndex to 0 if the element is focused, or -1 otherwise so that only the last focused
150150
// item is tabbable. If using virtual focus, don't set a tabIndex at all so that VoiceOver
151151
// on iOS 14 doesn't try to move real DOM focus to the item anyway.
152152
let itemProps: SelectableItemAria['itemProps'] = {};
153153
if (!shouldUseVirtualFocus) {
154154
itemProps = {
155-
tabIndex: isFocused ? 0 : -1,
155+
tabIndex: key === manager.focusedKey ? 0 : -1,
156156
onFocus(e) {
157157
if (e.target === ref.current) {
158158
manager.setFocusedKey(key);

‎packages/@react-spectrum/list/src/ListViewItem.tsx

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -126,10 +126,11 @@ export function ListViewItem<T>(props: ListViewItemProps<T>) {
126126
let showCheckbox = state.selectionManager.selectionMode !== 'none' && state.selectionManager.selectionBehavior === 'toggle';
127127
let {visuallyHiddenProps} = useVisuallyHidden();
128128

129+
let dropProps = isDroppable ? droppableItem?.dropProps : {'aria-hidden': droppableItem?.dropProps['aria-hidden']};
129130
const mergedProps = mergeProps(
130131
rowProps,
131132
draggableItem?.dragProps,
132-
isDroppable && droppableItem?.dropProps,
133+
dropProps,
133134
hoverProps,
134135
focusWithinProps,
135136
focusProps

‎packages/@react-spectrum/list/stories/ListView.stories.tsx

Lines changed: 25 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -712,7 +712,14 @@ export function ReorderExample(props) {
712712
}
713713

714714
export function DragIntoItemExample(props) {
715-
let onDropAction = action('onDrop');
715+
let {
716+
listViewProps = {},
717+
dragHookOptions = {},
718+
dropHookOptions = {}
719+
} = props;
720+
let {onDragStart, onDragEnd} = dragHookOptions;
721+
let {onDrop} = dropHookOptions;
722+
let onDropAction = chain(action('onDrop'), onDrop);
716723

717724
let list = useListData({
718725
initialItems: [
@@ -746,8 +753,8 @@ export function DragIntoItemExample(props) {
746753
};
747754
});
748755
},
749-
onDragStart: action('dragStart'),
750-
onDragEnd: action('dragEnd')
756+
onDragStart: chain(action('dragStart'), onDragStart),
757+
onDragEnd: chain(action('dragEnd'), onDragEnd)
751758
});
752759

753760
let dropHooks = useDropHooks({
@@ -792,7 +799,7 @@ export function DragIntoItemExample(props) {
792799
disabledKeys={['2']}
793800
dragHooks={dragHooks}
794801
dropHooks={dropHooks}
795-
{...props}>
802+
{...listViewProps}>
796803
{(item: any) => (
797804
<Item textValue={item.textValue} hasChildItems={item.type === 'folder'}>
798805
<Text>{item.type === 'folder' ? 'Drop items here' : `Item ${item.textValue}`}</Text>
@@ -932,7 +939,14 @@ export function DragBetweenListsExample(props) {
932939
}
933940

934941
export function DragBetweenListsRootOnlyExample(props) {
935-
let onDropAction = action('onDrop');
942+
let {
943+
listViewProps = {},
944+
dragHookOptions = {},
945+
dropHookOptions = {}
946+
} = props;
947+
let {onDragStart, onDragEnd} = dragHookOptions;
948+
let {onDrop} = dropHookOptions;
949+
let onDropAction = chain(action('onDrop'), onDrop);
936950

937951
let list1 = useListData({
938952
initialItems: props.items1 || itemList1
@@ -960,8 +974,8 @@ export function DragBetweenListsRootOnlyExample(props) {
960974
};
961975
});
962976
},
963-
onDragStart: action('dragStart'),
964-
onDragEnd: action('dragEnd')
977+
onDragStart: chain(action('dragStart'), onDragStart),
978+
onDragEnd: chain(action('dragEnd'), onDragEnd)
965979
});
966980

967981
let dragHooksSecond = useDragHooks({
@@ -974,8 +988,8 @@ export function DragBetweenListsRootOnlyExample(props) {
974988
};
975989
});
976990
},
977-
onDragStart: action('dragStart'),
978-
onDragEnd: action('dragEnd')
991+
onDragStart: chain(action('dragStart'), onDragStart),
992+
onDragEnd: chain(action('dragEnd'), onDragEnd)
979993
});
980994

981995
let dropHooksFirst = useDropHooks({
@@ -1053,7 +1067,7 @@ export function DragBetweenListsRootOnlyExample(props) {
10531067
disabledKeys={['2']}
10541068
dragHooks={dragHooksFirst}
10551069
dropHooks={dropHooksFirst}
1056-
{...props}>
1070+
{...listViewProps}>
10571071
{(item: any) => (
10581072
<Item>
10591073
{item.textValue}
@@ -1071,7 +1085,7 @@ export function DragBetweenListsRootOnlyExample(props) {
10711085
disabledKeys={['2']}
10721086
dragHooks={dragHooksSecond}
10731087
dropHooks={dropHooksSecond}
1074-
{...props}>
1088+
{...listViewProps}>
10751089
{(item: any) => (
10761090
<Item>
10771091
{item.textValue}

‎packages/@react-spectrum/list/test/ListView.test.js

Lines changed: 201 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -11,12 +11,12 @@
1111
*/
1212

1313
jest.mock('@react-aria/live-announcer');
14-
import {act, fireEvent, render as renderComponent, within} from '@testing-library/react';
14+
import {act, fireEvent, render as renderComponent, waitFor, within} from '@testing-library/react';
1515
import {ActionButton} from '@react-spectrum/button';
1616
import {announce} from '@react-aria/live-announcer';
1717
import {CUSTOM_DRAG_TYPE} from '@react-aria/dnd/src/constants';
1818
import {DataTransfer, DataTransferItem, DragEvent} from '@react-aria/dnd/test/mocks';
19-
import {DragExample} from '../stories/ListView.stories';
19+
import {DragBetweenListsRootOnlyExample, DragExample, DragIntoItemExample, ReorderExample} from '../stories/ListView.stories';
2020
import {Droppable} from '@react-aria/dnd/test/examples';
2121
import {installPointerEvent, triggerPress} from '@react-spectrum/test-utils';
2222
import {Item, ListView} from '../src';
@@ -1359,8 +1359,6 @@ describe('ListView', function () {
13591359
let cellText = getAllByText(cell.textContent);
13601360
expect(cellText).toHaveLength(1);
13611361

1362-
// Need raf to be async so the drag preview shows up properly
1363-
jest.spyOn(window, 'requestAnimationFrame').mockImplementation(cb => setTimeout(cb, 0));
13641362
let dataTransfer = new DataTransfer();
13651363

13661364
fireEvent.pointerDown(cell, {pointerType: 'mouse', button: 0, pointerId: 1, clientX: 5, clientY: 5});
@@ -1514,7 +1512,7 @@ describe('ListView', function () {
15141512
act(() => jest.runAllTimers());
15151513
expect(onDrop).toHaveBeenCalledTimes(1);
15161514

1517-
expect(await onDrop.mock.calls[0][0].items.length).toBe(4);
1515+
expect(await onDrop.mock.calls[0][0].items).toHaveLength(4);
15181516
expect(await onDrop.mock.calls[0][0].items[0].getText('text/plain')).toBe('Adobe Photoshop');
15191517
expect(await onDrop.mock.calls[0][0].items[1].getText('text/plain')).toBe('Adobe XD');
15201518
expect(await onDrop.mock.calls[0][0].items[2].getText('text/plain')).toBe('Documents');
@@ -1681,7 +1679,7 @@ describe('ListView', function () {
16811679

16821680
expect(onDrop).toHaveBeenCalledTimes(1);
16831681

1684-
expect(await onDrop.mock.calls[0][0].items.length).toBe(4);
1682+
expect(await onDrop.mock.calls[0][0].items).toHaveLength(4);
16851683
expect(await onDrop.mock.calls[0][0].items[0].getText('text/plain')).toBe('Adobe Photoshop');
16861684
expect(await onDrop.mock.calls[0][0].items[1].getText('text/plain')).toBe('Adobe XD');
16871685
expect(await onDrop.mock.calls[0][0].items[2].getText('text/plain')).toBe('Documents');
@@ -1849,8 +1847,179 @@ describe('ListView', function () {
18491847
expect(onSelectionChange).toHaveBeenCalledTimes(0);
18501848
});
18511849

1850+
it('should only count the selected keys that exist in the collection when dragging and dropping', async function () {
1851+
let {getAllByRole} = render(
1852+
<DragIntoItemExample dragHookOptions={{onDragStart, onDragEnd}} listViewProps={{onSelectionChange, disabledKeys: []}} dropHookOptions={{onDrop}} />
1853+
);
1854+
1855+
userEvent.tab();
1856+
let rows = getAllByRole('row');
1857+
expect(rows).toHaveLength(7);
1858+
let droppable = rows[0];
1859+
moveFocus('ArrowDown');
1860+
fireEvent.keyDown(document.activeElement, {key: 'Enter'});
1861+
fireEvent.keyUp(document.activeElement, {key: 'Enter'});
1862+
moveFocus('ArrowDown');
1863+
fireEvent.keyDown(document.activeElement, {key: 'Enter'});
1864+
fireEvent.keyUp(document.activeElement, {key: 'Enter'});
1865+
moveFocus('ArrowDown');
1866+
fireEvent.keyDown(document.activeElement, {key: 'Enter'});
1867+
fireEvent.keyUp(document.activeElement, {key: 'Enter'});
1868+
1869+
expect(new Set(onSelectionChange.mock.calls[2][0])).toEqual(new Set(['1', '2', '3']));
1870+
let draghandle = within(rows[3]).getAllByRole('button')[0];
1871+
expect(draghandle).toBeTruthy();
1872+
expect(draghandle).toHaveAttribute('draggable', 'true');
1873+
1874+
moveFocus('ArrowRight');
1875+
fireEvent.keyDown(draghandle, {key: 'Enter'});
1876+
fireEvent.keyUp(draghandle, {key: 'Enter'});
1877+
1878+
expect(onDragStart).toHaveBeenCalledTimes(1);
1879+
expect(onDragStart).toHaveBeenCalledWith({
1880+
type: 'dragstart',
1881+
keys: new Set(['1', '2', '3']),
1882+
x: 50,
1883+
y: 25
1884+
});
1885+
1886+
act(() => jest.runAllTimers());
1887+
expect(document.activeElement).toBe(droppable);
1888+
fireEvent.keyDown(droppable, {key: 'Enter'});
1889+
fireEvent.keyUp(droppable, {key: 'Enter'});
1890+
await act(async () => Promise.resolve());
1891+
act(() => jest.runAllTimers());
1892+
1893+
expect(onDrop).toHaveBeenCalledTimes(1);
1894+
expect(await onDrop.mock.calls[0][0].items).toHaveLength(3);
1895+
expect(onDragEnd).toHaveBeenCalledTimes(1);
1896+
expect(onDragEnd).toHaveBeenCalledWith({
1897+
type: 'dragend',
1898+
keys: new Set(['1', '2', '3']),
1899+
x: 50,
1900+
y: 25,
1901+
dropOperation: 'move'
1902+
});
1903+
onSelectionChange.mockClear();
1904+
onDragStart.mockClear();
1905+
1906+
rows = getAllByRole('row');
1907+
expect(rows).toHaveLength(4);
1908+
1909+
// Select the folder and perform a drag. Drag start shouldn't include the previously selected items
1910+
moveFocus('ArrowDown');
1911+
fireEvent.keyDown(droppable, {key: 'Enter'});
1912+
fireEvent.keyUp(droppable, {key: 'Enter'});
1913+
// Selection change event still has all keys
1914+
expect(new Set(onSelectionChange.mock.calls[0][0])).toEqual(new Set(['1', '2', '3', '0']));
1915+
1916+
draghandle = within(rows[0]).getAllByRole('button')[0];
1917+
expect(draghandle).toBeTruthy();
1918+
expect(draghandle).toHaveAttribute('draggable', 'true');
1919+
moveFocus('ArrowRight');
1920+
fireEvent.keyDown(draghandle, {key: 'Enter'});
1921+
fireEvent.keyUp(draghandle, {key: 'Enter'});
1922+
act(() => jest.runAllTimers());
1923+
1924+
expect(onDragStart).toHaveBeenCalledTimes(1);
1925+
expect(onDragStart).toHaveBeenCalledWith({
1926+
type: 'dragstart',
1927+
keys: new Set(['0']),
1928+
x: 50,
1929+
y: 25
1930+
});
1931+
1932+
fireEvent.keyDown(document.body, {key: 'Escape'});
1933+
fireEvent.keyUp(document.body, {key: 'Escape'});
1934+
});
1935+
1936+
it('should automatically focus the newly added dropped item', async function () {
1937+
let {getAllByRole} = render(
1938+
<DragBetweenListsRootOnlyExample dragHookOptions={{onDragStart, onDragEnd}} listViewProps={{onSelectionChange, disabledKeys: []}} dropHookOptions={{onDrop}} />
1939+
);
1940+
1941+
let grids = getAllByRole('grid');
1942+
expect(grids).toHaveLength(2);
1943+
let firstListRows = within(grids[0]).getAllByRole('row');
1944+
let draggedCell = within(firstListRows[0]).getByRole('gridcell');
1945+
let secondListRows = within(grids[1]).getAllByRole('row');
1946+
expect(firstListRows).toHaveLength(6);
1947+
expect(secondListRows).toHaveLength(6);
1948+
1949+
let dataTransfer = new DataTransfer();
1950+
fireEvent.pointerDown(draggedCell, {pointerType: 'mouse', button: 0, pointerId: 1, clientX: 0, clientY: 0});
1951+
fireEvent(draggedCell, new DragEvent('dragstart', {dataTransfer, clientX: 0, clientY: 0}));
1952+
1953+
act(() => jest.runAllTimers());
1954+
expect(onDragStart).toHaveBeenCalledTimes(1);
1955+
fireEvent.pointerMove(draggedCell, {pointerType: 'mouse', button: 0, pointerId: 1, clientX: 1, clientY: 1});
1956+
fireEvent(draggedCell, new DragEvent('drag', {dataTransfer, clientX: 1, clientY: 1}));
1957+
fireEvent(grids[1], new DragEvent('dragover', {dataTransfer, clientX: 1, clientY: 1}));
1958+
fireEvent.pointerUp(draggedCell, {pointerType: 'mouse', button: 0, pointerId: 1, clientX: 1, clientY: 1});
1959+
fireEvent(draggedCell, new DragEvent('dragend', {dataTransfer, clientX: 1, clientY: 1}));
1960+
expect(onDragEnd).toHaveBeenCalledTimes(1);
1961+
fireEvent(grids[1], new DragEvent('drop', {dataTransfer, clientX: 1, clientY: 1}));
1962+
1963+
await waitFor(() => expect(within(grids[1]).getAllByRole('row')).toHaveLength(7), {interval: 10});
1964+
expect(onDrop).toHaveBeenCalledTimes(1);
1965+
1966+
grids = getAllByRole('grid');
1967+
firstListRows = within(grids[0]).getAllByRole('row');
1968+
secondListRows = within(grids[1]).getAllByRole('row');
1969+
expect(firstListRows).toHaveLength(5);
1970+
expect(secondListRows).toHaveLength(7);
1971+
1972+
// The newly added row in the second list should be the active element
1973+
expect(secondListRows[6]).toBe(document.activeElement);
1974+
expect(secondListRows[6]).toHaveTextContent('Item One');
1975+
1976+
for (let [index, row] of secondListRows.entries()) {
1977+
if (index !== 6) {
1978+
expect(row).toHaveAttribute('tabIndex', '-1');
1979+
} else {
1980+
expect(row).toHaveAttribute('tabIndex', '0');
1981+
}
1982+
}
1983+
1984+
draggedCell = firstListRows[3];
1985+
dataTransfer = new DataTransfer();
1986+
fireEvent.pointerDown(draggedCell, {pointerType: 'mouse', button: 0, pointerId: 1, clientX: 0, clientY: 0});
1987+
fireEvent(draggedCell, new DragEvent('dragstart', {dataTransfer, clientX: 0, clientY: 0}));
1988+
1989+
act(() => jest.runAllTimers());
1990+
expect(onDragStart).toHaveBeenCalledTimes(2);
1991+
fireEvent.pointerMove(draggedCell, {pointerType: 'mouse', button: 0, pointerId: 1, clientX: 1, clientY: 1});
1992+
fireEvent(draggedCell, new DragEvent('drag', {dataTransfer, clientX: 1, clientY: 1}));
1993+
fireEvent(grids[1], new DragEvent('dragover', {dataTransfer, clientX: 1, clientY: 2}));
1994+
fireEvent.pointerUp(draggedCell, {pointerType: 'mouse', button: 0, pointerId: 1, clientX: 1, clientY: 1});
1995+
fireEvent(draggedCell, new DragEvent('dragend', {dataTransfer, clientX: 1, clientY: 1}));
1996+
expect(onDragEnd).toHaveBeenCalledTimes(2);
1997+
fireEvent(grids[1], new DragEvent('drop', {dataTransfer, clientX: 1, clientY: 1}));
1998+
1999+
await waitFor(() => expect(within(grids[1]).getAllByRole('row')).toHaveLength(8), {interval: 10});
2000+
expect(onDrop).toHaveBeenCalledTimes(2);
2001+
2002+
grids = getAllByRole('grid');
2003+
firstListRows = within(grids[0]).getAllByRole('row');
2004+
secondListRows = within(grids[1]).getAllByRole('row');
2005+
expect(firstListRows).toHaveLength(4);
2006+
expect(secondListRows).toHaveLength(8);
2007+
2008+
// The 2nd newly added row in the second list should still be the active element
2009+
expect(secondListRows[7]).toBe(document.activeElement);
2010+
expect(secondListRows[7]).toHaveTextContent('Item Five');
2011+
2012+
for (let [index, row] of secondListRows.entries()) {
2013+
if (index !== 7) {
2014+
expect(row).toHaveAttribute('tabIndex', '-1');
2015+
} else {
2016+
expect(row).toHaveAttribute('tabIndex', '0');
2017+
}
2018+
}
2019+
});
2020+
18522021
describe('accessibility', function () {
1853-
it('drag handle should reflect the correct number of draggable rows', async function () {
2022+
it('drag handle should reflect the correct number of draggable rows', function () {
18542023

18552024
let {getAllByRole} = render(
18562025
<DraggableListView listViewProps={{defaultSelectedKeys: ['a', 'b', 'c']}} />
@@ -1885,6 +2054,31 @@ describe('ListView', function () {
18852054
expect(dragButtonB).toHaveAttribute('aria-label', 'Drag 3 selected items');
18862055
expect(dragButtonD).toHaveAttribute('aria-label', 'Drag 3 selected items');
18872056
});
2057+
2058+
it('disabled rows and invalid drop targets should become aria-hidden when keyboard drag session starts', function () {
2059+
let {getAllByRole} = render(
2060+
<ReorderExample listViewProps={{disabledKeys: ['2']}} />
2061+
);
2062+
2063+
let rows = getAllByRole('row');
2064+
for (let row of rows) {
2065+
expect(row).not.toHaveAttribute('aria-hidden');
2066+
}
2067+
2068+
let row = rows[0];
2069+
let cell = within(row).getByRole('gridcell');
2070+
let draghandle = within(cell).getAllByRole('button')[0];
2071+
expect(row).toHaveAttribute('draggable', 'true');
2072+
fireEvent.keyDown(draghandle, {key: 'Enter'});
2073+
fireEvent.keyUp(draghandle, {key: 'Enter'});
2074+
2075+
for (let row of rows) {
2076+
expect(row).toHaveAttribute('aria-hidden', 'true');
2077+
}
2078+
2079+
fireEvent.keyDown(document.body, {key: 'Escape'});
2080+
fireEvent.keyUp(document.body, {key: 'Escape'});
2081+
});
18882082
});
18892083
});
18902084
});

‎packages/@react-stately/dnd/src/useDraggableCollectionState.ts

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -52,7 +52,7 @@ export function useDraggableCollectionState(props: DraggableCollectionOptions):
5252
// item is dragged. This matches native macOS behavior.
5353
let keys = new Set(
5454
selectionManager.isSelected(key)
55-
? new Set([...selectionManager.selectedKeys])
55+
? new Set([...selectionManager.selectedKeys].filter(key => !!collection.getItem(key)))
5656
: []
5757
);
5858

0 commit comments

Comments
 (0)