-
Notifications
You must be signed in to change notification settings - Fork 1.3k
feat: TextFields in GridList and Grid #8691
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
3a21e3b
88a79a3
ba8b475
35e6c07
0bfe346
69d47f2
8185304
8dc873a
572ace9
1b0de0a
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -10,13 +10,13 @@ | |
* governing permissions and limitations under the License. | ||
*/ | ||
|
||
import {AriaLabelingProps, DOMAttributes, DOMProps, Key, KeyboardDelegate, RefObject} from '@react-types/shared'; | ||
import {AriaLabelingProps, BaseEvent, DOMAttributes, DOMProps, Key, KeyboardDelegate, RefObject} from '@react-types/shared'; | ||
import {filterDOMProps, mergeProps, useId} from '@react-aria/utils'; | ||
import {GridCollection} from '@react-types/grid'; | ||
import {GridKeyboardDelegate} from './GridKeyboardDelegate'; | ||
import {gridMap} from './utils'; | ||
import {GridState} from '@react-stately/grid'; | ||
import {useCallback, useMemo} from 'react'; | ||
import {KeyboardEvent, useCallback, useMemo} from 'react'; | ||
import {useCollator, useLocale} from '@react-aria/i18n'; | ||
import {useGridSelectionAnnouncement} from './useGridSelectionAnnouncement'; | ||
import {useHasTabbableChild} from '@react-aria/focus'; | ||
|
@@ -64,7 +64,13 @@ export interface GridProps extends DOMProps, AriaLabelingProps { | |
*/ | ||
escapeKeyBehavior?: 'clearSelection' | 'none', | ||
/** Whether selection should occur on press up instead of press down. */ | ||
shouldSelectOnPressUp?: boolean | ||
shouldSelectOnPressUp?: boolean, | ||
/** | ||
* Whether keyboard navigation to focusable elements within grid list items is | ||
* via the left/right arrow keys or the tab key. | ||
* @default 'arrow' | ||
*/ | ||
keyboardNavigationBehavior?: 'arrow' | 'tab' | ||
} | ||
|
||
export interface GridAria { | ||
|
@@ -90,7 +96,8 @@ export function useGrid<T>(props: GridProps, state: GridState<T, GridCollection< | |
onRowAction, | ||
onCellAction, | ||
escapeKeyBehavior = 'clearSelection', | ||
shouldSelectOnPressUp | ||
shouldSelectOnPressUp, | ||
keyboardNavigationBehavior = 'arrow' | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. One of the insights while working on this was that "edit mode", when handling events in the scroll container, was pretty much just Raising here because I think it could make sense to lift this prop into grid state instead of forwarding via props, which could replace For impl, see useGridEditState & useGridState There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Edit: started playing around with resizable table columns, I'm definitely going to need to think more about this There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
I implemented an extended |
||
} = props; | ||
let {selectionManager: manager} = state; | ||
|
||
|
@@ -123,8 +130,45 @@ export function useGrid<T>(props: GridProps, state: GridState<T, GridCollection< | |
escapeKeyBehavior | ||
}); | ||
|
||
|
||
let originalOnKeyDown = collectionProps.onKeyDown; | ||
let onKeyDown = useCallback((e: BaseEvent<KeyboardEvent<any>>) => { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't follow what these event handlers are doing. Why the special cases? Seems quite specific to our implementation There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I was trying to avoid passing keyboardNavigationBehavior to useSelectableCollection I know this won't be the final logic for the PR, but it serves as a starting point since I'm unsure what we want to do here. So this shows where there are problematic event handlers. @nwidynski had this suggestion #8691 (comment) But there are a few ways we could approach this. The problematic behaviours that this is blocking are: |
||
if (keyboardNavigationBehavior === 'tab') { | ||
if (e.target === ref.current | ||
|| (e.target as HTMLElement).getAttribute('role') === 'gridcell' | ||
|| (e.target as HTMLElement).getAttribute('role') === 'row' | ||
|| (e.target as HTMLElement).getAttribute('role') === 'presentation' // special case for gridlist thanks to redispatch of keyboard event by gridcell, theoretically no other presentation elements should get focus | ||
|| (e.target instanceof HTMLInputElement && (e.target as HTMLInputElement).getAttribute('type') === 'checkbox') | ||
|| e.key === 'Tab') { | ||
originalOnKeyDown?.(e); | ||
} | ||
} | ||
if (keyboardNavigationBehavior !== 'tab') { | ||
originalOnKeyDown?.(e); | ||
} | ||
}, [originalOnKeyDown, keyboardNavigationBehavior, ref]); | ||
collectionProps.onKeyDown = onKeyDown; | ||
|
||
let originalOnKeyDownCapture = collectionProps.onKeyDownCapture; | ||
let onKeyDownCapture = useCallback((e: BaseEvent<KeyboardEvent<any>>) => { | ||
if (keyboardNavigationBehavior === 'tab') { | ||
if (e.target === ref.current | ||
|| (e.target as HTMLElement).getAttribute('role') === 'gridcell' | ||
|| (e.target as HTMLElement).getAttribute('role') === 'row' | ||
|| (e.target as HTMLElement).getAttribute('role') === 'presentation' // special case for gridlist thanks to redispatch of keyboard event by gridcell, theoretically no other presentation elements should get focus | ||
|| (e.target instanceof HTMLInputElement && (e.target as HTMLInputElement).getAttribute('type') === 'checkbox') | ||
|| e.key === 'Tab') { | ||
originalOnKeyDownCapture?.(e); | ||
} | ||
} | ||
if (keyboardNavigationBehavior !== 'tab') { | ||
originalOnKeyDownCapture?.(e); | ||
} | ||
}, [originalOnKeyDownCapture, keyboardNavigationBehavior, ref]); | ||
collectionProps.onKeyDownCapture = onKeyDownCapture; | ||
|
||
let id = useId(props.id); | ||
gridMap.set(state, {keyboardDelegate: delegate, actions: {onRowAction, onCellAction}, shouldSelectOnPressUp}); | ||
gridMap.set(state, {keyboardDelegate: delegate, keyboardNavigationBehavior, actions: {onRowAction, onCellAction}, shouldSelectOnPressUp}); | ||
|
||
let descriptionProps = useHighlightSelectionDescription({ | ||
selectionManager: manager, | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -13,7 +13,7 @@ | |
import {DOMAttributes, FocusableElement, Key, RefObject} from '@react-types/shared'; | ||
import {focusSafely, isFocusVisible} from '@react-aria/interactions'; | ||
import {getFocusableTreeWalker} from '@react-aria/focus'; | ||
import {getScrollParent, mergeProps, scrollIntoViewport} from '@react-aria/utils'; | ||
import {getScrollParent, isFocusable, mergeProps, scrollIntoViewport} from '@react-aria/utils'; | ||
import {GridCollection, GridNode} from '@react-types/grid'; | ||
import {gridMap} from './utils'; | ||
import {GridState} from '@react-stately/grid'; | ||
|
@@ -62,7 +62,10 @@ export function useGridCell<T, C extends GridCollection<T>>(props: GridCellProps | |
} = props; | ||
|
||
let {direction} = useLocale(); | ||
let {keyboardDelegate, actions: {onCellAction}} = gridMap.get(state)!; | ||
let {keyboardDelegate, keyboardNavigationBehavior, actions: {onCellAction}} = gridMap.get(state)!; | ||
if (keyboardNavigationBehavior === 'tab') { | ||
focusMode = 'cell'; | ||
} | ||
Comment on lines
+66
to
+68
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Should this be a hard override? IIRC, my impl had focusMode ??= keyboardNavigation === 'tab' ? 'cell' : 'child'; There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. a good point, hadn't thought this through yet There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Trying to think of the mixed use case, I was thinking maybe for the selection column in a table. But it's a little weird to mix the navigation styles, feels a little inconsistent even if i don't like the way the selection column ends up looking in React Spectrum. What was your reason for doing it not as a hard override? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Imagine a cell with a text field and "save" button. When pressing on the cell, wouldn't you want to have the textfield focused for convenience? |
||
|
||
// We need to track the key of the item at the time it was last focused so that we force | ||
// focus to go to the item when the DOM node is reused for a different item in a virtualizer. | ||
|
@@ -113,6 +116,10 @@ export function useGridCell<T, C extends GridCollection<T>>(props: GridCellProps | |
return; | ||
} | ||
|
||
if (keyboardNavigationBehavior === 'tab' && e.target !== ref.current) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. L115: I think There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm sorry, I don't follow your concern here, both are possible conditions for leaving this event handler early? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Nevermind, you are correct. I had removed this condition for the W3C focus wrapping feature. See here for impl. |
||
return; | ||
} | ||
|
||
let walker = getFocusableTreeWalker(ref.current); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I believe this should only walk tabbable, right? Same in L78. We need to add tests for disabled interactive content 👍 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is for cell level navigation, not inside the cell. In keyboardNavigationBehavior = tab, we've already exited if we were inside the cell, skipping this code. I think it is correct as it is, but maybe an example would be helpful. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. added a test for disabled interactive content There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I know this isn't related to edit, IIRC they were bugs I fixed while working on it. L78 is basically the focus strategy of the cell, which should honor whether a child is tabbable. L123 is for |
||
walker.currentNode = document.activeElement; | ||
|
||
|
@@ -171,7 +178,7 @@ export function useGridCell<T, C extends GridCollection<T>>(props: GridCellProps | |
? walker.previousNode() as FocusableElement | ||
: walker.nextNode() as FocusableElement; | ||
|
||
if (focusMode === 'child' && focusable === ref.current) { | ||
if ((focusMode === 'child' && focusable === ref.current) || (keyboardNavigationBehavior === 'tab')) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This will never happen because of the early return above right? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It happens if you are focused on the cell, the early return above is only for events inside the cell |
||
focusable = null; | ||
} | ||
|
||
|
@@ -186,6 +193,7 @@ export function useGridCell<T, C extends GridCollection<T>>(props: GridCellProps | |
// We prevent the capturing event from reaching children of the cell, e.g. pickers. | ||
// We want arrow keys to navigate to the next cell instead. We need to re-dispatch | ||
// the event from a higher parent so it still bubbles and gets handled by useSelectableCollection. | ||
// Note: This may dispatch on something that isn't the cell nor the row, so we will need to handle that in useGrid. | ||
ref.current.parentElement?.dispatchEvent( | ||
new KeyboardEvent(e.nativeEvent.type, e.nativeEvent) | ||
); | ||
|
@@ -224,6 +232,48 @@ export function useGridCell<T, C extends GridCollection<T>>(props: GridCellProps | |
} | ||
}; | ||
|
||
let onKeyDown = (e) => { | ||
if (!e.currentTarget.contains(e.target as Element) || !ref.current || !document.activeElement) { | ||
return; | ||
} | ||
|
||
switch (e.key) { | ||
case 'Tab': { | ||
if (keyboardNavigationBehavior === 'tab') { | ||
// If there is another focusable element within this item, stop propagation so the tab key | ||
// is handled by the browser and not by useSelectableCollection (which would take us out of the list). | ||
let walker = getFocusableTreeWalker(ref.current, {tabbable: true}); | ||
walker.currentNode = document.activeElement; | ||
let next = e.shiftKey ? walker.previousNode() : walker.nextNode(); | ||
|
||
if (next) { | ||
e.stopPropagation(); | ||
} | ||
} | ||
} | ||
} | ||
}; | ||
|
||
// Prevent pressing space to select the row. | ||
let originalOnKeyDown = itemProps.onKeyDown; | ||
itemProps.onKeyDown = (e) => { | ||
if (keyboardNavigationBehavior === 'tab' && e.key === ' ' && e.target !== ref.current) { | ||
e.stopPropagation(); | ||
return; | ||
} | ||
originalOnKeyDown?.(e); | ||
}; | ||
|
||
// Prevent clicking on a focusable element from selecting the row. | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This one and the one above seem like they should be handled by the event leaks - i.e. stopping propagation within the component that handles the press. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. agreed, these would go away, but this pr isn't fixing event leaks, see description
I'm working on event leaks in another local branch at the moment, but it has some interesting issues to solve still. |
||
let originalPointerDown = itemProps.onPointerDown; | ||
itemProps.onPointerDown = (e) => { | ||
if (keyboardNavigationBehavior === 'tab' && isFocusable(e.target as Element) && e.target !== ref.current) { | ||
e.stopPropagation(); | ||
return; | ||
} | ||
originalPointerDown?.(e); | ||
}; | ||
|
||
// Grid cells can have focusable elements inside them. In this case, focus should | ||
// be marshalled to that element rather than focusing the cell itself. | ||
let onFocus = (e) => { | ||
|
@@ -253,6 +303,7 @@ export function useGridCell<T, C extends GridCollection<T>>(props: GridCellProps | |
let gridCellProps: DOMAttributes = mergeProps(itemProps, { | ||
role: 'gridcell', | ||
onKeyDownCapture, | ||
onKeyDown, | ||
'aria-colspan': node.colSpan, | ||
'aria-colindex': node.colIndex != null ? node.colIndex + 1 : undefined, // aria-colindex is 1-based | ||
colSpan: isVirtualized ? undefined : node.colSpan, | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -12,6 +12,7 @@ | |
|
||
import { | ||
AriaLabelingProps, | ||
BaseEvent, | ||
CollectionBase, | ||
DisabledBehavior, | ||
DOMAttributes, | ||
|
@@ -24,6 +25,7 @@ import { | |
RefObject | ||
} from '@react-types/shared'; | ||
import {filterDOMProps, mergeProps, useId} from '@react-aria/utils'; | ||
import {KeyboardEvent, useCallback} from 'react'; | ||
import {listMap} from './utils'; | ||
import {ListState} from '@react-stately/list'; | ||
import {useGridSelectionAnnouncement, useHighlightSelectionDescription} from '@react-aria/grid'; | ||
|
@@ -144,6 +146,38 @@ export function useGridList<T>(props: AriaGridListOptions<T>, state: ListState<T | |
escapeKeyBehavior | ||
}); | ||
|
||
let originalOnKeyDown = listProps.onKeyDown; | ||
let onKeyDown = useCallback((e: BaseEvent<KeyboardEvent<any>>) => { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. same |
||
if (keyboardNavigationBehavior === 'tab') { | ||
if (e.target === ref.current | ||
|| (e.target as HTMLElement).getAttribute('role') === 'gridcell' | ||
|| (e.target as HTMLElement).getAttribute('role') === 'row' | ||
|| e.key === 'Tab') { | ||
originalOnKeyDown?.(e); | ||
} | ||
} | ||
if (keyboardNavigationBehavior !== 'tab') { | ||
originalOnKeyDown?.(e); | ||
} | ||
}, [originalOnKeyDown, keyboardNavigationBehavior, ref]); | ||
listProps.onKeyDown = onKeyDown; | ||
|
||
let originalOnKeyDownCapture = listProps.onKeyDownCapture; | ||
let onKeyDownCapture = useCallback((e: BaseEvent<KeyboardEvent<any>>) => { | ||
if (keyboardNavigationBehavior === 'tab') { | ||
if (e.target === ref.current | ||
|| (e.target as HTMLElement).getAttribute('role') === 'gridcell' | ||
|| (e.target as HTMLElement).getAttribute('role') === 'row' | ||
|| e.key === 'Tab') { | ||
originalOnKeyDownCapture?.(e); | ||
} | ||
} | ||
if (keyboardNavigationBehavior !== 'tab') { | ||
originalOnKeyDownCapture?.(e); | ||
} | ||
}, [originalOnKeyDownCapture, keyboardNavigationBehavior, ref]); | ||
listProps.onKeyDownCapture = onKeyDownCapture; | ||
|
||
let id = useId(props.id); | ||
listMap.set(state, {id, onAction, linkBehavior, keyboardNavigationBehavior, shouldSelectOnPressUp}); | ||
|
||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -10,7 +10,7 @@ | |
* governing permissions and limitations under the License. | ||
*/ | ||
|
||
import {chain, getScrollParent, mergeProps, scrollIntoViewport, useSlotId, useSyntheticLinkProps} from '@react-aria/utils'; | ||
import {chain, getScrollParent, isFocusable, mergeProps, scrollIntoViewport, useSlotId, useSyntheticLinkProps} from '@react-aria/utils'; | ||
import {DOMAttributes, FocusableElement, Key, RefObject, Node as RSNode} from '@react-types/shared'; | ||
import {focusSafely, getFocusableTreeWalker} from '@react-aria/focus'; | ||
import {getRowId, listMap} from './utils'; | ||
|
@@ -135,6 +135,10 @@ export function useGridListItem<T>(props: AriaGridListItemOptions, state: ListSt | |
return; | ||
} | ||
|
||
if (keyboardNavigationBehavior === 'tab' && e.target !== ref.current) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. There's some logic below that previously ran on arrowup/arrowdown with keyboardNavigationBehavior = tab. Noticed in CardView you can no longer press up/down to move to the next card when focused on an action menu. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If I allow those through, it'll conflict with cursor positioning in textfields. I think we'll have to solve event leaks to have that not collide. We'll also need to make it non-capturing. |
||
return; | ||
} | ||
|
||
let walker = getFocusableTreeWalker(ref.current); | ||
walker.currentNode = document.activeElement; | ||
|
||
|
@@ -227,22 +231,6 @@ export function useGridListItem<T>(props: AriaGridListItemOptions, state: ListSt | |
} | ||
}; | ||
|
||
let onFocus = (e) => { | ||
keyWhenFocused.current = node.key; | ||
if (e.target !== ref.current) { | ||
// useSelectableItem only handles setting the focused key when | ||
// the focused element is the row itself. We also want to | ||
// set the focused key when a child element receives focus. | ||
// If focus is currently visible (e.g. the user is navigating with the keyboard), | ||
// then skip this. We want to restore focus to the previously focused row | ||
// in that case since the list should act like a single tab stop. | ||
if (!isFocusVisible()) { | ||
state.selectionManager.setFocusedKey(node.key); | ||
} | ||
return; | ||
} | ||
}; | ||
|
||
let onKeyDown = (e) => { | ||
if (!e.currentTarget.contains(e.target as Element) || !ref.current || !document.activeElement) { | ||
return; | ||
|
@@ -265,6 +253,42 @@ export function useGridListItem<T>(props: AriaGridListItemOptions, state: ListSt | |
} | ||
}; | ||
|
||
// Prevent pressing space to select the row. | ||
let originalOnKeyDown = itemProps.onKeyDown; | ||
itemProps.onKeyDown = (e) => { | ||
if (keyboardNavigationBehavior === 'tab' && e.key === ' ' && e.target !== ref.current) { | ||
e.stopPropagation(); | ||
return; | ||
} | ||
originalOnKeyDown?.(e); | ||
}; | ||
|
||
// Prevent clicking on a focusable element from selecting the row. | ||
let originalPointerDown = itemProps.onPointerDown; | ||
itemProps.onPointerDown = (e) => { | ||
if (keyboardNavigationBehavior === 'tab' && isFocusable(e.target as Element) && e.target !== ref.current) { | ||
e.stopPropagation(); | ||
return; | ||
} | ||
originalPointerDown?.(e); | ||
}; | ||
|
||
let onFocus = (e) => { | ||
keyWhenFocused.current = node.key; | ||
if (e.target !== ref.current) { | ||
// useSelectableItem only handles setting the focused key when | ||
// the focused element is the row itself. We also want to | ||
// set the focused key when a child element receives focus. | ||
// If focus is currently visible (e.g. the user is navigating with the keyboard), | ||
// then skip this. We want to restore focus to the previously focused row | ||
// in that case since the list should act like a single tab stop. | ||
if (!isFocusVisible()) { | ||
state.selectionManager.setFocusedKey(node.key); | ||
} | ||
Comment on lines
+285
to
+287
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Just flagging that this is still missing a fix to maintain a consistent tab sequence. Re-entering the collection from the bottom after exiting may cause focus to land on the item instead of the field. Same goes for the For impl, see here There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yeah, I suspect this is related to #8686 as well |
||
return; | ||
} | ||
}; | ||
|
||
let syntheticLinkProps = useSyntheticLinkProps(node.props); | ||
let linkProps = itemStates.hasAction ? syntheticLinkProps : {}; | ||
// TODO: re-add when we get translations and fix this for iOS VO | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wondering if this API should be at the cell level instead of (or in addition to?) the whole grid. I could imagine a UI where only certain columns have textfields in them, and it would be nice if the other columns still automatically focused the content of the cell. For example, in the Reddit Table story in S2 with
keyboardNavigationBehavior="tab"
, now you have to tab to the links instead of just arrowing to them, but there are no inputs in that cell.Ideally we'd even automatically set a cell to tab behavior if we detected inputs in them. But I guess there will be cases with custom components where we'll need a prop too.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Linking related discussion #8691 (comment)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yeah, I don't love it either, I have to tab to the column header menu trigger now as well for resizing table, and the focus ring on selection checkbox cells looks funny.