Skip to content

Commit c9c838b

Browse files
ktaborssnowystingerLFDanLu
authored
Adding FocusRing to ListView (Virtualizer persistedKeys) (#3168)
* Adding FocusRing to ListView * a pass at ListView no focus and scroll modaility * reverting change prior to virtualizer improvement * fixing a merge conflict mistake * adding persisted keys to virtualizer for ListView FocusRing * handling table keys better * Update packages/@react-aria/virtualizer/src/Virtualizer.tsx Co-authored-by: Robert Snow <[email protected]> * Update packages/@react-stately/virtualizer/src/Virtualizer.ts Co-authored-by: Robert Snow <[email protected]> * missing semicolon * Update packages/@react-aria/virtualizer/src/Virtualizer.tsx Co-authored-by: Robert Snow <[email protected]> * fixingn lint errors (space and unneeded return statement * update to tests based on Virtualizer improvement * replacing recursion with while loop * syntax fix * adding listview scrolling and focus tests * messed up the arrow movement prior to last commit * calling updateSubViews and layoutInfos keeping order matching collection * fixing tests (updateSubviews still needs work) * cleaning up logic issues found by failing picker tests * storing some work in progress before taking a new path * persisted keys refactor for listview and picker (tableview needs work and picker test failure) * adding persistedKey checking to TableLayout * tests pass and added some comments * making persisted keys work with table cells * preventing different loops from looking at the same item Co-authored-by: Robert Snow <[email protected]> Co-authored-by: Daniel Lu <[email protected]>
1 parent 634fa94 commit c9c838b

File tree

8 files changed

+247
-78
lines changed

8 files changed

+247
-78
lines changed

‎packages/@react-aria/virtualizer/src/Virtualizer.tsx‎

Lines changed: 10 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -14,7 +14,7 @@ import {Collection} from '@react-types/shared';
1414
import {focusWithoutScrolling, mergeProps, useLayoutEffect} from '@react-aria/utils';
1515
import {getInteractionModality} from '@react-aria/interactions';
1616
import {Layout, Rect, ReusableView, useVirtualizerState, VirtualizerState} from '@react-stately/virtualizer';
17-
import React, {FocusEvent, HTMLAttributes, Key, ReactElement, RefObject, useCallback, useEffect, useRef} from 'react';
17+
import React, {FocusEvent, HTMLAttributes, Key, ReactElement, RefObject, useCallback, useEffect, useMemo, useRef} from 'react';
1818
import {ScrollView} from './ScrollView';
1919
import {VirtualizerItem} from './VirtualizerItem';
2020

@@ -145,6 +145,15 @@ export function useVirtualizer<T extends object, V, W>(props: VirtualizerOptions
145145
lastFocusedKey.current = focusedKey;
146146
}, [focusedKey, virtualizer.visibleRect.height, virtualizer, lastFocusedKey, scrollToItem]);
147147

148+
// Tell the virtualizer to persist the focusedKey and prevent it from being reused.
149+
// This is to prevent the FocusRing from setting focus to the ListView when a
150+
// focused ListViewItem is scrolled out of the Virtualizer visible rectangle.
151+
let focusedKeySet = useMemo((): Set<Key> => (
152+
focusedKey ? new Set([focusedKey]) : new Set()
153+
), [focusedKey]);
154+
155+
virtualizer.persistedKeys = focusedKeySet;
156+
148157
let onFocus = useCallback((e: FocusEvent) => {
149158
// If the focused item is scrolled out of view and is not in the DOM, the collection
150159
// will have tabIndex={0}. When tabbing in from outside, scroll the focused item into view.

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

Lines changed: 76 additions & 73 deletions
Original file line numberDiff line numberDiff line change
@@ -15,6 +15,7 @@ import type {DraggableCollectionState, DroppableCollectionState} from '@react-st
1515
import {DragHooks, DropHooks} from '@react-spectrum/dnd';
1616
import type {DroppableCollectionResult} from '@react-aria/dnd';
1717
import {filterDOMProps, useLayoutEffect} from '@react-aria/utils';
18+
import {FocusRing} from '@react-aria/focus';
1819
import InsertionIndicator from './InsertionIndicator';
1920
// @ts-ignore
2021
import intlMessages from '../intl/*.json';
@@ -226,82 +227,84 @@ function ListView<T extends object>(props: SpectrumListViewProps<T>, ref: DOMRef
226227

227228
return (
228229
<ListViewContext.Provider value={{state, dragState, dropState, dragHooks, dropHooks, onAction, isListDraggable, isListDroppable, layout, loadingState}}>
229-
<Virtualizer
230-
{...mergeProps(isListDroppable && droppableCollection?.collectionProps, gridProps)}
231-
{...filterDOMProps(otherProps)}
232-
{...gridProps}
233-
{...styleProps}
234-
isLoading={isLoading}
235-
onLoadMore={onLoadMore}
236-
ref={domRef}
237-
focusedKey={focusedKey}
238-
scrollDirection="vertical"
239-
className={
240-
classNames(
241-
listStyles,
242-
'react-spectrum-ListView',
243-
`react-spectrum-ListView--${density}`,
244-
'react-spectrum-ListView--emphasized',
245-
{
246-
'react-spectrum-ListView--quiet': isQuiet,
247-
'react-spectrum-ListView--loadingMore': loadingState === 'loadingMore',
248-
'react-spectrum-ListView--draggable': !!isListDraggable,
249-
'react-spectrum-ListView--dropTarget': !!isRootDropTarget,
250-
'react-spectrum-ListView--isVerticalScrollbarVisible': isVerticalScrollbarVisible,
251-
'react-spectrum-ListView--isHorizontalScrollbarVisible': isHorizontalScrollbarVisible,
252-
'react-spectrum-ListView--hasAnyChildren': hasAnyChildren,
253-
'react-spectrum-ListView--wrap': overflowMode === 'wrap'
254-
},
255-
styleProps.className
256-
)
257-
}
258-
layout={layout}
259-
collection={collection}
260-
transitionDuration={isLoading ? 160 : 220}>
261-
{(type, item) => {
262-
if (type === 'item') {
263-
return (
264-
<>
265-
{isListDroppable && collection.getKeyBefore(item.key) == null &&
266-
<RootDropIndicator key="root" />
267-
}
268-
{isListDroppable &&
269-
<InsertionIndicator
270-
key={`${item.key}-before`}
271-
target={{key: item.key, type: 'item', dropPosition: 'before'}} />
272-
}
273-
<ListViewItem item={item} isEmphasized hasActions={!!onAction} />
274-
{isListDroppable &&
275-
<InsertionIndicator
276-
key={`${item.key}-after`}
277-
target={{key: item.key, type: 'item', dropPosition: 'after'}}
278-
isPresentationOnly={collection.getKeyAfter(item.key) != null} />
230+
<FocusRing focusRingClass={classNames(listStyles, 'focus-ring')}>
231+
<Virtualizer
232+
{...mergeProps(isListDroppable && droppableCollection?.collectionProps, gridProps)}
233+
{...filterDOMProps(otherProps)}
234+
{...gridProps}
235+
{...styleProps}
236+
isLoading={isLoading}
237+
onLoadMore={onLoadMore}
238+
ref={domRef}
239+
focusedKey={focusedKey}
240+
scrollDirection="vertical"
241+
className={
242+
classNames(
243+
listStyles,
244+
'react-spectrum-ListView',
245+
`react-spectrum-ListView--${density}`,
246+
'react-spectrum-ListView--emphasized',
247+
{
248+
'react-spectrum-ListView--quiet': isQuiet,
249+
'react-spectrum-ListView--loadingMore': loadingState === 'loadingMore',
250+
'react-spectrum-ListView--draggable': !!isListDraggable,
251+
'react-spectrum-ListView--dropTarget': !!isRootDropTarget,
252+
'react-spectrum-ListView--isVerticalScrollbarVisible': isVerticalScrollbarVisible,
253+
'react-spectrum-ListView--isHorizontalScrollbarVisible': isHorizontalScrollbarVisible,
254+
'react-spectrum-ListView--hasAnyChildren': hasAnyChildren,
255+
'react-spectrum-ListView--wrap': overflowMode === 'wrap'
256+
},
257+
styleProps.className
258+
)
259+
}
260+
layout={layout}
261+
collection={collection}
262+
transitionDuration={isLoading ? 160 : 220}>
263+
{(type, item) => {
264+
if (type === 'item') {
265+
return (
266+
<>
267+
{isListDroppable && collection.getKeyBefore(item.key) == null &&
268+
<RootDropIndicator key="root" />
279269
}
280-
</>
281-
);
282-
} else if (type === 'loader') {
283-
return (
284-
<CenteredWrapper>
285-
<ProgressCircle
286-
isIndeterminate
287-
aria-label={collection.size > 0 ? stringFormatter.format('loadingMore') : stringFormatter.format('loading')} />
288-
</CenteredWrapper>
289-
);
290-
} else if (type === 'placeholder') {
291-
let emptyState = props.renderEmptyState ? props.renderEmptyState() : null;
292-
if (emptyState == null) {
293-
return null;
294-
}
270+
{isListDroppable &&
271+
<InsertionIndicator
272+
key={`${item.key}-before`}
273+
target={{key: item.key, type: 'item', dropPosition: 'before'}} />
274+
}
275+
<ListViewItem item={item} isEmphasized hasActions={!!onAction} />
276+
{isListDroppable &&
277+
<InsertionIndicator
278+
key={`${item.key}-after`}
279+
target={{key: item.key, type: 'item', dropPosition: 'after'}}
280+
isPresentationOnly={collection.getKeyAfter(item.key) != null} />
281+
}
282+
</>
283+
);
284+
} else if (type === 'loader') {
285+
return (
286+
<CenteredWrapper>
287+
<ProgressCircle
288+
isIndeterminate
289+
aria-label={collection.size > 0 ? stringFormatter.format('loadingMore') : stringFormatter.format('loading')} />
290+
</CenteredWrapper>
291+
);
292+
} else if (type === 'placeholder') {
293+
let emptyState = props.renderEmptyState ? props.renderEmptyState() : null;
294+
if (emptyState == null) {
295+
return null;
296+
}
295297

296-
return (
297-
<CenteredWrapper>
298-
{emptyState}
299-
</CenteredWrapper>
300-
);
301-
}
298+
return (
299+
<CenteredWrapper>
300+
{emptyState}
301+
</CenteredWrapper>
302+
);
303+
}
302304

303-
}}
304-
</Virtualizer>
305+
}}
306+
</Virtualizer>
307+
</FocusRing>
305308
{DragPreview && isListDraggable &&
306309
<DragPreview ref={preview}>
307310
{() => {

‎packages/@react-spectrum/list/src/styles.css‎

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -586,3 +586,10 @@
586586
opacity: 0.01;
587587
max-width: 0px;
588588
}
589+
590+
.react-spectrum-ListView {
591+
&:focus-ring {
592+
border-color: var(--spectrum-global-color-blue-500);
593+
box-shadow: inset 0 0 0 1px var(--spectrum-table-cell-border-color-key-focus);
594+
}
595+
}

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

Lines changed: 70 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -139,6 +139,8 @@ describe('ListView', function () {
139139
fireEvent.keyUp(document.activeElement, {key, ...opts});
140140
};
141141

142+
let focusRow = (tree, text) => act(() => getRow(tree, text).focus());
143+
142144
it('renders a static listview', function () {
143145
let {getByRole, getAllByRole} = render(
144146
<ListView aria-label="List" data-testid="test">
@@ -1311,5 +1313,73 @@ describe('ListView', function () {
13111313
expect(document.activeElement).toBe(getRow(tree, 'Item 0'));
13121314
expect(grid.scrollTop).toBe(0);
13131315
});
1316+
1317+
it('should scroll to a row when it is focused', function () {
1318+
let tree = render(
1319+
<ListView
1320+
width="250px"
1321+
height="60px"
1322+
aria-label="List"
1323+
data-testid="test"
1324+
selectionStyle="highlight"
1325+
selectionMode="multiple"
1326+
onSelectionChange={onSelectionChange}
1327+
items={[...Array(20).keys()].map(k => ({key: k, name: `Item ${k}`}))}>
1328+
{item => <Item>{item.name}</Item>}
1329+
</ListView>
1330+
);
1331+
let grid = tree.getByRole('grid');
1332+
Object.defineProperty(grid, 'clientHeight', {
1333+
get() {
1334+
return 60;
1335+
}
1336+
});
1337+
// fire resize so the new clientHeight is requested
1338+
act(() => {
1339+
fireEvent(window, new Event('resize'));
1340+
});
1341+
expect(grid.scrollTop).toBe(0);
1342+
1343+
focusRow(tree, 'Item 10');
1344+
expect(document.activeElement).toBe(getRow(tree, 'Item 10'));
1345+
1346+
expect(grid.scrollTop).toBe(380);
1347+
});
1348+
1349+
it('should scroll to a row when it is focused off screen', function () {
1350+
let tree = render(
1351+
<ListView
1352+
width="250px"
1353+
height="60px"
1354+
aria-label="List"
1355+
data-testid="test"
1356+
selectionStyle="highlight"
1357+
selectionMode="multiple"
1358+
onSelectionChange={onSelectionChange}
1359+
items={[...Array(20).keys()].map(k => ({key: k, name: `Item ${k}`}))}>
1360+
{item => <Item>{item.name}</Item>}
1361+
</ListView>
1362+
);
1363+
let body = tree.getByRole('grid');
1364+
1365+
let row = getRow(tree, 'Item 0');
1366+
act(() => row.focus());
1367+
expect(document.activeElement).toBe(row);
1368+
expect(body.scrollTop).toBe(0);
1369+
1370+
// When scrolling the focused item out of view, focus should remain on the item
1371+
body.scrollTop = 1000;
1372+
fireEvent.scroll(body);
1373+
1374+
expect(body.scrollTop).toBe(1000);
1375+
expect(document.activeElement).toBe(row);
1376+
// item isn't reused by virutalizer
1377+
expect(tree.queryByText('Item 0')).toBe(row.firstElementChild.firstElementChild.firstElementChild);
1378+
1379+
// Moving focus should scroll the new focused item into view
1380+
moveFocus('ArrowDown');
1381+
expect(body.scrollTop).toBe(0);
1382+
expect(document.activeElement).toBe(getRow(tree, 'Item 1'));
1383+
});
13141384
});
13151385
});

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

Lines changed: 6 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -1552,13 +1552,14 @@ describe('TableView', function () {
15521552
expect(document.activeElement).toBe(cell);
15531553
expect(body.scrollTop).toBe(0);
15541554

1555-
// When scrolling the focused item out of view, focus should move to the table itself
1555+
// When scrolling the focused item out of view, focus should remaind on the item,
1556+
// virtualizer keeps focused items from being reused
15561557
body.scrollTop = 1000;
15571558
fireEvent.scroll(body);
15581559

15591560
expect(body.scrollTop).toBe(1000);
1560-
expect(document.activeElement).toBe(tree.getByRole('grid'));
1561-
expect(tree.queryByText('Baz 5')).toBeNull();
1561+
expect(document.activeElement).toBe(cell);
1562+
expect(tree.queryByText('Baz 5')).toBe(cell.firstElementChild);
15621563

15631564
// Moving focus should scroll the new focused item into view
15641565
moveFocus('ArrowLeft');
@@ -1569,14 +1570,15 @@ describe('TableView', function () {
15691570
it('should not scroll when a column header receives focus', function () {
15701571
let tree = renderMany();
15711572
let body = tree.getByRole('grid').childNodes[1];
1573+
let cell = getCell(tree, 'Baz 5');
15721574

15731575
focusCell(tree, 'Baz 5');
15741576

15751577
body.scrollTop = 1000;
15761578
fireEvent.scroll(body);
15771579

15781580
expect(body.scrollTop).toBe(1000);
1579-
expect(document.activeElement).toBe(tree.getByRole('grid'));
1581+
expect(document.activeElement).toBe(cell);
15801582

15811583
focusCell(tree, 'Bar');
15821584
expect(document.activeElement).toHaveAttribute('role', 'columnheader');

‎packages/@react-stately/layout/src/ListLayout.ts‎

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -109,6 +109,12 @@ export class ListLayout<T> extends Layout<Node<T>> implements KeyboardDelegate {
109109
res.push(node.header);
110110
}
111111

112+
if (node.children) {
113+
addNodes(node.children);
114+
}
115+
} else if (this.virtualizer.isPersistedKey(node)) {
116+
res.push(node.layoutInfo);
117+
112118
if (node.children) {
113119
addNodes(node.children);
114120
}

‎packages/@react-stately/layout/src/TableLayout.ts‎

Lines changed: 28 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -298,17 +298,39 @@ export class TableLayout<T> extends ListLayout<T> {
298298
case 'rowgroup': {
299299
let firstVisibleRow = this.binarySearch(node.children, rect.topLeft, 'y');
300300
let lastVisibleRow = this.binarySearch(node.children, rect.bottomRight, 'y');
301+
// Check to see if a persisted key exists before the visible rows
302+
// This is for keeping focus on a row that scrolls out of view
303+
for (let h = 0; h < firstVisibleRow; h++) {
304+
if (this.virtualizer.isPersistedKey(node.children[h])) {
305+
res.push(node.children[h].layoutInfo);
306+
this.addVisibleLayoutInfos(res, node.children[h], rect);
307+
}
308+
}
301309
for (let i = firstVisibleRow; i <= lastVisibleRow; i++) {
302310
res.push(node.children[i].layoutInfo);
303311
this.addVisibleLayoutInfos(res, node.children[i], rect);
304312
}
313+
// Check to see if a persisted key exists after the visible rows
314+
for (let j = lastVisibleRow + 1; j < node.children.length; j++) {
315+
if (this.virtualizer.isPersistedKey(node.children[j])) {
316+
res.push(node.children[j].layoutInfo);
317+
this.addVisibleLayoutInfos(res, node.children[j], rect);
318+
}
319+
}
305320
break;
306321
}
307322
case 'headerrow':
308323
case 'row': {
309324
let firstVisibleCell = this.binarySearch(node.children, rect.topLeft, 'x');
310325
let lastVisibleCell = this.binarySearch(node.children, rect.topRight, 'x');
311326
let stickyIndex = 0;
327+
// Check to see if a persisted key exists before the visible cells
328+
// This is for keeping focus on a cell that scrolls out of view
329+
for (let h = 0; h < firstVisibleCell; h++) {
330+
if (this.virtualizer.isPersistedKey(node.children[h])) {
331+
res.push(node.children[h].layoutInfo);
332+
}
333+
}
312334
for (let i = firstVisibleCell; i <= lastVisibleCell; i++) {
313335
// Sticky columns and row headers are always in the DOM. Interleave these
314336
// with the visible range so that they are in the right order.
@@ -322,6 +344,12 @@ export class TableLayout<T> extends ListLayout<T> {
322344

323345
res.push(node.children[i].layoutInfo);
324346
}
347+
// Check to see if a persisted key exists after the visible cells
348+
for (let j = lastVisibleCell; j < node.children.length; j++) {
349+
if (this.virtualizer.isPersistedKey(node.children[j])) {
350+
res.push(node.children[j].layoutInfo);
351+
}
352+
}
325353

326354
while (stickyIndex < this.stickyColumnIndices.length) {
327355
let idx = this.stickyColumnIndices[stickyIndex++];

0 commit comments

Comments
 (0)