Fix table keyboard navigation bugs#1993
Conversation
LFDanLu
requested review from
dannify,
devongovett and
snowystinger
as code owners
| // TODO: This conflicts with the childFocusStrategy stuff set up below, but this behavior made more sense to me | ||
| // since it respects visual tab order by default. | ||
| // Perhaps state.selectionManager.childFocusStrategy should default to "undef" in useMultipleSelectionState | ||
| // so that I can do a childFocusStategy != null check here so it doesn't override what the user sets? | ||
|
|
||
| // If the current active element is a focusable child within the cell, early return. | ||
| // This means that we've alt/option tab forward/backward into the cell and we should preserve the current | ||
| // focused child element. | ||
| if (document.activeElement !== ref.current && ref.current.contains(document.activeElement)) { | ||
| return; | ||
| } |
There was a problem hiding this comment.
Open question here. Not entirely sure what the desired behavior here is with childFocusStrategy, kinda feel like useMultipleSelectionState shouldn't have a default focus strategy, instead setting it to null similar to how useMenuTriggerState does it. Then I could check a child focus strategy exists and perform the early return if one doesn't, allowing shift+option+tab to follow the visual tab order (currently behaves this way w/o these fixes)
| } else if (e.target !== ref.current && focusedKey != null && !isFocusWithin.current) { | ||
| let focusedView = virtualizer.getView(focusedKey); | ||
| // Otherwise if we are re-entering the collection and there is a previously focused key that is out of view, scroll it into view | ||
| // If it is in view already, preserve the current scroll position via scrollTo. This is to prevent the shift-tabbing into the | ||
| // collection from scrolling the focused key out of view due to useSelectableCollection focusing the last element in the table when tabbing out of the collection | ||
| if (!focusedView) { | ||
| virtualizer.scrollToItem(focusedKey, {duration: 0}); | ||
| } else { | ||
| let rect = virtualizer.getVisibleRect(); | ||
| virtualizer.scrollTo(new Point(rect.x, rect.y), 0); | ||
| } |
There was a problem hiding this comment.
Fixes "When I shift tab back into the table, it does another page down"
There was a problem hiding this comment.
Is the root cause here that the focused key actually changes when tabbing out of the table due to focusing the last focusable item? Should we instead prevent this from happening?
There was a problem hiding this comment.
So the focused key actually doesn't change when tabbing out of the table because the last focusable item is actually a checkbox for a row that is out of view (useGridCell actually doesn't update the focusedKey in this case because we are in keyboard modality and useSelectableItem doesn't either because e.target isn't the cell itself).
The root cause of the weird scrolling behavior from shift tabbing into the table is because the browser is moving focus to the last focusable element in the table which is the out of view checkbox, causing the scroll...
There was a problem hiding this comment.
Tried a bunch of stuff but this still feels like the best solution since it fixes the following two cases:
- stopping the table from scrolling to the last checkbox when shift tabbing back into the table
- properly scrolling a item back into view when tabbing into the table
The second one can be reproduced by doing the following:
- focus a cell in the table by clicking/tabbing into the table
- using your mouse/touchpad, scroll the cell just out of view by scrolling down a little bit (make sure not to scroll so far that the cell is removed from the DOM)
- click on an element outside of the table
- tab back into the table
Without the above code, tabbing back into the table would focus the first focusable item, aka the column's select all checkbox which results in the focused cell in the table not being scrolled back into view.
Any suggestions? I'm not sure how we would be able to pull this out of virtualizer
There was a problem hiding this comment.
An alternative would be to apply excludeFromTabOrder to the props returned by useTableSelectionCheckbox and useTableSelectAllCheckbox so that tabbing/shift-tabbing into the table moves focus to the previously focused cell that has tabindex=0.
EDIT: this doesn't take into account what cells might be passed to the user unfortunately, same problem will happen if any of the cell children have tabbable content. Ideally items that are out of visible view should have tabIndex = -1
There was a problem hiding this comment.
A possible solution that could replace the Virtualizer change here is perhaps something like this?
useEffect(() => {
let onKeyDown = (e) => {
// The consts here can be removed if we export them from FocusScope
const focusableElements = [
'input:not([disabled]):not([type=hidden])',
'select:not([disabled])',
'textarea:not([disabled])',
'button:not([disabled])',
'a[href]',
'area[href]',
'summary',
'iframe',
'object',
'embed',
'audio[controls]',
'video[controls]',
'[contenteditable]'
];
focusableElements.push('[tabindex]:not([tabindex="-1"]):not([disabled])');
const TABBABLE_ELEMENT_SELECTOR = focusableElements.join(':not([hidden]):not([tabindex="-1"]),');
if (e.key === 'Tab' && !ref.current.contains(e.target as HTMLElement)) {
let tabbableElements = Array.from(document.querySelectorAll(TABBABLE_ELEMENT_SELECTOR));
let index = tabbableElements.findIndex(node => node === e.target)
let nodeToFocus;
if (e.shiftKey) {
nodeToFocus = tabbableElements[index - 1];
} else {
nodeToFocus = tabbableElements[index + 1];
}
if (ref.current.contains(nodeToFocus) && manager.focusedKey) {
e.preventDefault();
manager.setFocused(true);
focusSafely(ref.current);
}
}
}
window.addEventListener('keydown', onKeyDown, true);
return () => {
window.removeEventListener('keydown', onKeyDown, true);
}
}, [])
Essentially we capture all keypresses that happen outside of the TableView. If we detect that the key press is Tab/Shift + Tab we then determine if focus would've shifted into the TableView and if so, prevent default behavior of the Tab key down so the browser doesn't shift the scroll position and then focus the TableView itself. The TableView then handles focusing the previously focused key itself via its existing useVirtualizer code (if selectedKey is out of view) or the useSelectableItem code (if selectedKey is already in view)
There was a problem hiding this comment.
Another idea is to mimic some of the code in useSelectableList used for scrolling the focusedKey into view: https://github.com/adobe/react-spectrum/blob/main/packages/@react-aria/selection/src/useSelectableList.ts#L114-L121. We could add similiar scrollIntoView in a capturing onFocus listener that detects when the focus is moved into the Table from outside. If so, then we bring the focused item into view but only if it isn't in view already and if isVirtualized is false. For the virtualized case, we keep the changes already made in this PR.
As an aside, we might want to also have the aforementioned code from useSelectableList added to useGrid since the table hooks don't automatically move the focused key into view as the user navigates via keyboard
Quick and dirty example code, might live in useGrid/useTable? Also might change the api of those hooks depending on what info is available :
// Move focused key into view as it changes
let bodyRef = useRef();
React.useEffect(() => {
// Would also need to check if !isVirtualized
if (state.selectionManager.focusedKey && bodyRef?.current) {
let element = bodyRef.current.querySelector(`[data-key="${state.selectionManager.focusedKey}"]`) as HTMLElement;
if (element) {
// This func comes from useSelectableList, not part of the snippet
scrollIntoView(bodyRef.current, element);
}
}
}, [bodyRef, state.selectionManager.focusedKey]);
let prevScroll = useRef(bodyRef.current?.scrollTop || 0);
// Bring focused key into view if focus enters the table from outside
React.useEffect(() => {
let onFocus = (e) => {
if (ref.current?.contains(e.target) && (!ref.current?.contains(e.relatedTarget))) {
if (state.selectionManager.state.focusedKey) {
let element = bodyRef.current.querySelector(`[data-key="${state.selectionManager.focusedKey}"]`) as HTMLElement;
if (element) {
// Need to track previous scroll position, but at the same time need to override the scroll position
let scrollContainerTop = bodyRef.current.offsetTop + prevScroll.current
let scrollContainerBottom = bodyRef.current.offsetTop +prevScroll.current + bodyRef.current.offsetHeight
let elementBottom = element.offsetTop + element.clientHeight;
// If focused key is out of view, scroll it into view when re-entering the table
if (!(elementBottom > scrollContainerTop && elementBottom < scrollContainerBottom)) {
scrollIntoView(bodyRef.current, element);
} else {
// If focusedkey is already in view, override the scroll that may happen from shift tabbing (browser will focus last focusable element in the table which may be out of view, causing a scroll)
bodyRef.current.scrollTop = prevScroll.current
}
}
}
}
}
window.addEventListener('focus', onFocus, true);
return () => {
window.removeEventListener('focus', onFocus, true);
}
}, [state, bodyRef.current])
// Save the previous scroll position
let onScroll = React.useCallback(() => {
prevScroll.current = bodyRef.current.scrollTop;
}, [bodyRef]);
|
Build successful! 🎉 |
|
Build successful! 🎉 |
LFDanLu
changed the title
|
Build successful! 🎉 |
|
Build successful! 🎉 |
|
Build successful! 🎉 |
seems to work if we switch to scrollToItem from TableView instead of always using Virtualizer.scrolltoItem
|
Build successful! 🎉 |
|
Build successful! 🎉 |
snowystinger
left a comment
snowystinger
left a comment
There was a problem hiding this comment.
lgtm in chrome/safari/ff
ktabors
left a comment
ktabors
left a comment
There was a problem hiding this comment.
LGTM
I testing it maintaining its focused key and that option/alt-tab doesn't work when the user is within the table.
|
Build successful! 🎉 |
5 participants
Closes #1208
✅ Pull Request Checklist:
📝 Test Instructions:
🧢 Your Project: