split button focus behavior #2137
Conversation
|
| if (index < 0) return; | ||
| const items = this.dropdownItems; | ||
| items.forEach((item, i) => | ||
| item.setAttribute('tabindex', i === index ? '0' : '-1') |
There was a problem hiding this comment.
Why do we need to change the tabIndex?
There was a problem hiding this comment.
When the dropdown is closed by pressing the Tab key, we set the tabindex of all dropdown items to -1. This removes them from the natural tab order of the page. It ensures that a user cannot accidentally Tab into the items of a closed dropdown. Focus should move to the next focusable element outside of the split-button component, which is the standard and expected keyboard navigation behavior.
| if (event.key !== 'Tab') { | ||
| return; | ||
| } | ||
| this.dropdownItems.forEach((item) => item.setAttribute('tabindex', '-1')); |
There was a problem hiding this comment.
Why do we need to set the tabIndex?
There was a problem hiding this comment.
When Shift+Tab is pressed, the browser's default behavior is to move focus to the previous focusable element, which in this case is the split-button's dropdown trigger (the anchorButton).
However, the desired behavior is to close the dropdown and move focus to the element before the entire split-button component.
To achieve this, we temporarily set tabindex="-1" on the anchorButton, making it non-focusable. This allows the browser's default Shift+Tab action to skip the anchorButton and move focus to the correct preceding element.
The tabindex is immediately restored within a requestAnimationFrame callback so that the anchorButton is focusable again for future interactions.
| return; | ||
| } | ||
| this.dropdownItems.forEach((item) => item.setAttribute('tabindex', '-1')); | ||
| this.dropdownElement!.show = false; |
There was a problem hiding this comment.
Why is this necessary?
| const actionButton = this.hostElement.shadowRoot?.querySelector( | ||
| 'ix-button, ix-icon-button:not(.anchor)' | ||
| ) as HTMLElement | null; | ||
|
|
||
| const anchorButton = this.hostElement.shadowRoot?.querySelector( |
There was a problem hiding this comment.
Getter would be better IMO.
|
/gemini review |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces keyboard navigation for the split button's dropdown, which is a great enhancement for accessibility. The implementation uses an ArrowFocusController for arrow key navigation and a custom keydown handler for the Tab key. I've found a critical issue with the usage of ArrowFocusController that prevents the arrow key navigation from working, and another potential runtime error if the dropdown is empty. I've left detailed comments with suggestions on how to fix them. The new tests are well-structured and will be very helpful in verifying the fixes.
| this.arrowFocusController = new ArrowFocusController( | ||
| this.dropdownItems, | ||
| this.dropdownElement!, | ||
| (index: number) => this.focusDropdownItem(index) | ||
| ); |
There was a problem hiding this comment.
The ArrowFocusController will not work as expected here. Its getActiveIndex method compares document.activeElement directly with the items in the list. However, your focusDropdownItem function focuses an inner <button> element inside the ix-dropdown-item's shadow DOM. This means document.activeElement will be this inner button, not the ix-dropdown-item element itself. As a result, getActiveIndex will always return -1, breaking the arrow key navigation.
To fix this, ArrowFocusController needs to be updated to correctly find the active item, for example by checking if an item contains(document.activeElement). Since utils/focus.ts is not part of this PR, you might need to include it or find a workaround.
The new tests in split-button-keyboard.ct.ts are also likely to fail because they expect the ix-dropdown-item host to be focused (ix-dropdown-item:focus), which is not what happens.
| const item = items[index]; | ||
| requestAnimationFrame(() => { | ||
| item.shadowRoot?.querySelector('button')?.focus(); | ||
| }); |
There was a problem hiding this comment.
The code attempts to access item.shadowRoot without checking if item is defined. If the split button has no dropdown items, this.dropdownItems will be empty, and items[index] will be undefined. This will cause a runtime error on item.shadowRoot. Please add a guard to ensure item exists before using it.
const item = items[index];
if (item) {
requestAnimationFrame(() => {
item.shadowRoot?.querySelector('button')?.focus();
});
}
nuke-ellington
left a comment
nuke-ellington
left a comment
There was a problem hiding this comment.
Although not part of the Figma spec, ArrowDown/Up should also open the dropdown (compare WCAG).
|
|
PR will be closed regarding general refactoring of keyboard navigation #2268 |
3 participants
💡 What is the current behavior?
GitHub Issue Number: #
Jira issue number: IX-3051
🆕 What is the new behavior?
🏁 Checklist
A pull request can only be merged if all of these conditions are met (where applicable):
pnpm test)pnpm lint)pnpm build, changes pushed)👨💻 Help & support