-
Notifications
You must be signed in to change notification settings - Fork 1.6k
fix: #8686 prevent useGridCell from overriding child focus restoration loops #10228
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
Changes from 26 commits
11a8867
7e8bcfe
adbba17
bce1556
24667bf
e9c5635
cf9fb80
6a6f158
70b8a96
04dda2d
2bce577
ce7ac24
480b06f
0cf561f
4228eb9
d5fe949
eb1152a
11f3ad3
4bb3a9d
75b2056
983bad4
e3b8dc3
e6fac10
8e6faf0
b04e589
e113f83
4328795
3d00705
11857eb
801acd2
7b0b5e1
49ded82
2861c45
4f8f75e
566dcd8
ed78d21
c7a59d8
c8adffe
5d03aa1
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 |
|---|---|---|
|
|
@@ -106,20 +106,42 @@ export function useGridCell<T, C extends GridCollection<T>>( | |
| // focus to go to the item when the DOM node is reused for a different item in a virtualizer. | ||
| let keyWhenFocused = useRef<Key | null>(null); | ||
|
|
||
| // Tracks the specific focusable child that was last focused within this cell. | ||
| let lastFocusedChild = useRef<FocusableElement | null>(null); | ||
|
|
||
| // Handles focusing the cell. If there is a focusable child, | ||
| // it is focused, otherwise the cell itself is focused. | ||
| let focus = () => { | ||
| if (ref.current) { | ||
| let treeWalker = getFocusableTreeWalker(ref.current); | ||
| if (focusMode === 'child') { | ||
| let activeElement = getActiveElement(getOwnerDocument(ref.current)); | ||
|
|
||
| // If focus is already on a focusable child within the cell, early return so we don't shift focus | ||
| if ( | ||
| isFocusWithin(ref.current) && | ||
| ref.current !== getActiveElement(getOwnerDocument(ref.current)) | ||
| ) { | ||
| if (isFocusWithin(ref.current) && ref.current !== activeElement) { | ||
| return; | ||
| } | ||
|
|
||
| // Focus counts as "disrupted" (as opposed to a fresh keyboard entry from a | ||
| // sibling row/cell) when it's on the cell itself or fell back to body/nothing, | ||
| // e.g. because an overlay closed and removed the element that had focus. In | ||
| // that case, restore the child that was last focused instead of defaulting to | ||
| // childFocusStrategy's first/last child. | ||
| let ownerDocument = getOwnerDocument(ref.current); | ||
| let isDisrupted = | ||
|
snowystinger marked this conversation as resolved.
Outdated
|
||
| ref.current === activeElement || !activeElement || activeElement === ownerDocument.body; | ||
| if (isDisrupted) { | ||
| let lastChild = lastFocusedChild.current; | ||
| if ( | ||
| lastChild && | ||
| keyWhenFocused.current === node.key && | ||
| nodeContains(ref.current, lastChild) | ||
| ) { | ||
| focusSafely(lastChild); | ||
| return; | ||
| } | ||
| } | ||
|
|
||
| let focusable = | ||
| state.selectionManager.childFocusStrategy === 'last' | ||
| ? last(treeWalker) | ||
|
|
@@ -309,7 +331,7 @@ export function useGridCell<T, C extends GridCollection<T>>( | |
|
|
||
| // 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 => { | ||
| let onFocus = (e: FocusEvent) => { | ||
| keyWhenFocused.current = node.key; | ||
| if (getEventTarget(e) !== ref.current) { | ||
| // useSelectableItem only handles setting the focused key when | ||
|
|
@@ -318,6 +340,19 @@ export function useGridCell<T, C extends GridCollection<T>>( | |
| // 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/cell | ||
| // in that case since the table should act like a single tab stop. | ||
|
|
||
| // Remember which child was focused so that if something later forces focus | ||
| // back to this cell (e.g. a closing overlay), focus() can restore it directly | ||
| // instead of falling back to the first/last focusable child. Only do this for | ||
| // actual DOM descendants of the cell -- portalled content (e.g. a dialog opened | ||
| // from within the cell) can still reach this handler because React dispatches | ||
| // events along the component tree rather than the DOM tree for portals, even | ||
| // though it isn't really inside this cell in the DOM. | ||
|
Member
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 not sure what the portalled comment is about? the line below
Contributor
Author
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. Yes, the nodeContains(ref.current, target) check prevents that. It ensures we remember only elements that belong to this cell, so focus is restored to the correct button when the dialog closes.
snowystinger marked this conversation as resolved.
Outdated
|
||
| let target = getEventTarget(e) as FocusableElement; | ||
| if (ref.current && nodeContains(ref.current, target)) { | ||
| lastFocusedChild.current = target; | ||
| } | ||
|
|
||
| if (!isFocusVisible()) { | ||
| state.selectionManager.setFocusedKey(node.key); | ||
| } | ||
|
|
||
Uh oh!
There was an error while loading. Please reload this page.