Skip to content
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.

Sign up for GitHub

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

Jump to bottom

fix: support Enter to go from a folded listbox to a row #1219

Merged
merged 3 commits into from
Apr 13, 2023
Merged

fix: support Enter to go from a folded listbox to a row #1219

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
6019e74
fix: support Enter -> Enter to go from a collapsed listbox to a row
quanho Apr 13, 2023
ba19e51
fix: fix Enter to focus on the first row
quanho Apr 13, 2023
109135e
fix: update tabIndex
quanho Apr 13, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion apis/nucleus/src/components/listbox/components/ListBoxSearch.jsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -115,7 +115,7 @@ export default function ListBoxSearch({

function focusRow(container) {
const row = container?.querySelector('.last-focused') || container?.querySelector('[role="row"]:first-child');
row.setAttribute('tabIndex', 0);
row.setAttribute('tabIndex', -1);
row?.focus();
}

Expand Down
6 changes: 3 additions & 3 deletions apis/nucleus/src/components/listbox/components/useTempKeyboard.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,7 +38,7 @@ export default function useTempKeyboard({ containerRef, enabled }) {
removeLastFocused(containerRef.current);
if (resetFocus && vizCell) {
// Move focus to the viz's cell.
vizCell.setAttribute('tabIndex', 0);
vizCell.setAttribute('tabIndex', -1);
vizCell.focus();
}
},
Expand All @@ -53,12 +53,12 @@ export default function useTempKeyboard({ containerRef, enabled }) {
const fieldElement = c?.querySelector('.value.selector, .value, .ActionsToolbar-item button');

const elementToFocus = searchField || lastSelectedRow || fieldElement;
elementToFocus?.setAttribute('tabIndex', 0);
elementToFocus?.setAttribute('tabIndex', -1);
quanho marked this conversation as resolved.
Show resolved Hide resolved
elementToFocus?.focus();
},
focusSelection() {
const confirmButton = document.querySelector('.actions-toolbar-default-actions .actions-toolbar-confirm');
confirmButton?.setAttribute('tabIndex', 0);
confirmButton?.setAttribute('tabIndex', -1);
confirmButton?.focus();
},
};
Expand Down
11 changes: 9 additions & 2 deletions apis/nucleus/src/components/listbox/interactions/listbox-keyboard-navigation.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,7 @@ const focusSearch = (container) => {

const focusRow = (container) => {
const row = container?.querySelector('.value.last-focused, .value.selector, .value');
row?.setAttribute('tabIndex', 0);
row?.setAttribute('tabIndex', -1);
row?.focus();
return row;
};
Expand Down Expand Up @@ -180,7 +180,7 @@ export function getListboxInlineKeyboardNavigation({
// 3. Blur row and focus the listbox container.
keyboard.blur();
const c = currentTarget.closest('.listbox-container');
c.setAttribute('tabIndex', 0);
c.setAttribute('tabIndex', -1);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this is unnecessary, what happens if you remove it? Do we ever set it to 0 elsewhere?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We do not set it 0 to elsewhere but to make it focusable we should set tabIndex to -1.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I do think it needed to focus on it when keyboardNavigation is false (I think it would have no tabIndex without it)
but I am also questioning if it should be focused in that case.

c?.focus();
}
};
Expand Down Expand Up @@ -217,6 +217,13 @@ export function getListboxInlineKeyboardNavigation({
prevent();
break;
case KEYS.ENTER:
if (inSelection) {
focusSearch(container) || focusRow(container);
} else {
break;
}
prevent();
break;
case KEYS.SPACE:
if (!event.target.classList.contains('listbox-container')) {
break; // don't mess with keydown handlers within the listbox (e.g. row seletion)
Expand Down