feat(core/dropdown): support roving tabindex keyboard navigation - #2659
feat(core/dropdown): support roving tabindex keyboard navigation#2659danielleroux wants to merge 8 commits into
Conversation
✅ Deploy Preview for ix-storybook ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
🦋 Changeset detectedLatest commit: 4bbd405 The changes in this PR will be included in the next version bump. This PR includes changesets to release 5 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
📝 WalkthroughWalkthroughAdds ChangesDropdown roving tabindex
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant User
participant IxDropdown
participant DropdownFocus
participant OverlayCoordinator
participant Popover
User->>IxDropdown: Open or navigate with keyboard
IxDropdown->>DropdownFocus: Initialize roving tabindex
DropdownFocus->>IxDropdown: Focus active item and update tabindex
User->>IxDropdown: Press Tab or Escape
IxDropdown->>OverlayCoordinator: Resolve hierarchy and dismissal
OverlayCoordinator->>Popover: Dismiss child or parent overlay as applicable
OverlayCoordinator->>IxDropdown: Return focus exit target
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![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 a new navigationMode property to ix-dropdown and ix-dropdown-button components, enabling a roving-tabindex keyboard navigation strategy alongside the default active-descendant mode. It also coordinates nested overlay hierarchies between dropdowns and popovers to ensure seamless keyboard navigation and focus management. Key feedback highlights several critical issues: undefined functions focusLast and focusFirst are used in dropdown.tsx which will cause runtime errors; closestPassShadow fails to traverse across shadow boundaries due to unhandled ShadowRoot nodes; and negative tabindex elements are not excluded when calculating sequential tab order. Additionally, it is recommended to use a Set to track visited IDs in hierarchy traversals to prevent potential infinite loops.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| if (node instanceof HTMLElement) { | ||
| if (node.matches(selector)) { | ||
| return node; | ||
| } else { | ||
| return closestPassShadow(node.parentNode!, selector); | ||
| } | ||
|
|
||
| return closestPassShadow(node.assignedSlot ?? node.parentNode, selector); | ||
| } | ||
|
|
||
| return closestPassShadow(node.parentNode!, selector); | ||
| return closestPassShadow(node.parentNode, selector); | ||
| } |
There was a problem hiding this comment.
The closestPassShadow function fails to traverse across shadow boundaries because it does not handle ShadowRoot nodes. When node is a ShadowRoot, node instanceof HTMLElement is false, and node.parentNode is null, causing the traversal to stop prematurely and return null. To fix this, we should explicitly check if the node is an instance of ShadowRoot and traverse to its host element.
if (node instanceof HTMLElement) {
if (node.matches(selector)) {
return node;
}
return closestPassShadow(node.assignedSlot ?? node.parentNode, selector);
}
if (node instanceof ShadowRoot) {
return closestPassShadow(node.host, selector);
}
return closestPassShadow(node.parentNode, selector);| if (event.key === 'ArrowUp' || event.key === 'End') { | ||
| focusLast(this.itemsHost); | ||
| } else { | ||
| focusFirst(this.itemsHost); | ||
| } |
There was a problem hiding this comment.
The functions focusLast and focusFirst are not imported or defined in this file, which will cause a runtime ReferenceError or compilation failure. They should be replaced with the imported focusLastDescendant and focusFirstDescendant utilities.
| if (event.key === 'ArrowUp' || event.key === 'End') { | |
| focusLast(this.itemsHost); | |
| } else { | |
| focusFirst(this.itemsHost); | |
| } | |
| if (event.key === 'ArrowUp' || event.key === 'End') { | |
| focusLastDescendant(this.itemsHost); | |
| } else { | |
| focusFirstDescendant(this.itemsHost); | |
| } |
| const focusableElements = queryElements( | ||
| document.body, | ||
| focusableQueryString | ||
| ).filter( | ||
| (element) => | ||
| closestPassShadow(element, 'ix-dropdown') === null && | ||
| element.getClientRects().length > 0 | ||
| ); |
There was a problem hiding this comment.
When calculating the sequential tab order to exit the dropdown, elements with a negative tabindex (such as tabindex="-1") should be excluded, as they are not part of the sequential keyboard navigation flow. Otherwise, pressing Tab could incorrectly move focus to an unfocusable element. Additionally, querying the entire document.body on every Tab key press can be a performance bottleneck in large applications, and standard query selectors do not traverse shadow DOM boundaries.
const focusableElements = queryElements(
document.body,
focusableQueryString
).filter(
(element) =>
element.tabIndex >= 0 &&
closestPassShadow(element, 'ix-dropdown') === null &&
element.getClientRects().length > 0
);
| private getRootDropdown(dropdown: DropdownInterface) { | ||
| let root = dropdown; | ||
| let parentId = this.getParentDropdownId(root.getId()); | ||
|
|
||
| while (parentId) { | ||
| const parent = this.registry.get(parentId); | ||
| if (!parent) { | ||
| break; | ||
| } | ||
| }); | ||
| root = parent; | ||
| parentId = this.getParentDropdownId(root.getId()); | ||
| } | ||
|
|
||
| return root; | ||
| } |
There was a problem hiding this comment.
To prevent potential infinite loops in case of circular parent-child relationships in the dropdown hierarchy, it is recommended to keep track of visited IDs using a Set.
private getRootDropdown(dropdown: DropdownInterface) {
let root = dropdown;
let parentId = this.getParentDropdownId(root.getId());
const visited = new Set<string>([root.getId()]);
while (parentId && !visited.has(parentId)) {
const parent = this.registry.get(parentId);
if (!parent) {
break;
}
root = parent;
visited.add(parentId);
parentId = this.getParentDropdownId(root.getId());
}
return root;
}| private getDropdownHierarchy() { | ||
| const hierarchy: Dropdown[] = [this]; | ||
| let parentId = dropdownController.getParentDropdownId(this.getId()); | ||
|
|
||
| while (parentId) { | ||
| const parent = dropdownController.getDropdownById(parentId) as Dropdown; | ||
| hierarchy.push(parent); | ||
| parentId = dropdownController.getParentDropdownId(parent.getId()); | ||
| } | ||
|
|
||
| return hierarchy; | ||
| } |
There was a problem hiding this comment.
To prevent potential infinite loops in case of circular parent-child relationships in the dropdown hierarchy, it is recommended to keep track of visited IDs using a Set.
private getDropdownHierarchy() {
const hierarchy: Dropdown[] = [this];
let parentId = dropdownController.getParentDropdownId(this.getId());
const visited = new Set<string>([this.getId()]);
while (parentId && !visited.has(parentId)) {
const parent = dropdownController.getDropdownById(parentId) as Dropdown;
if (!parent) {
break;
}
hierarchy.push(parent);
visited.add(parentId);
parentId = dropdownController.getParentDropdownId(parent.getId());
}
return hierarchy;
}
| private isDescendantOf(childKey: string, parentKey: string) { | ||
| let current = this.getParentKey(childKey); | ||
|
|
||
| while (current) { | ||
| if (current === parentKey) { | ||
| return true; | ||
| } | ||
| current = this.getParentKey(current); | ||
| } | ||
|
|
||
| return false; | ||
| } |
There was a problem hiding this comment.
To prevent potential infinite loops in case of circular parent-child relationships in the overlay hierarchy, it is recommended to keep track of visited IDs using a Set.
private isDescendantOf(childKey: string, parentKey: string) {
let current = this.getParentKey(childKey);
const visited = new Set<string>();
while (current && !visited.has(current)) {
if (current === parentKey) {
return true;
}
visited.add(current);
current = this.getParentKey(current);
}
return false;
}![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/core/src/components/dropdown-button/dropdown-button.ct.ts`:
- Around line 174-217: Add makeAxeBuilder() accessibility assertions to the
roving-tabindex regression tests in the dropdown-button test file, covering the
rendered dropdown in its new focus/tabindex state. Apply the same axe coverage
to the additional roving-tabindex test range, while preserving the existing
hydration and focus/ARIA behavior assertions.
In `@packages/core/src/components/dropdown/dropdown.tsx`:
- Around line 983-992: Replace the inline tab-order sorting in the dropdown’s
focusable-elements flow with the shared sortByTabOrder comparator from
focus-trap.ts. Export the existing helper if necessary, import and reuse it
here, and preserve the current positive-tabIndex ordering with document-order
tie-breaking.
In `@packages/core/src/components/dropdown/test/dropdown.ct.ts`:
- Around line 793-813: Add an accessibility check using makeAxeBuilder() to the
Roving tabindex navigation tests, covering the mounted
navigation-mode="roving-tabindex" dropdown fixture. Reuse the existing
regressionTest setup and assert the axe scan passes alongside the current focus
and tabindex behavior checks.
In `@packages/core/src/components/utils/nested-overlay/overlay-coordinator.ts`:
- Around line 153-164: In dismissCrossTypeChildren, split the
children.reverse().forEach(...) chain into separate statements: reverse children
first, then invoke forEach to dismiss each entry with the existing reason.
Preserve the current reverse-order dismissal behavior.
In `@packages/react-test-app/src/preview-examples/dropdown-roving-tabindex.tsx`:
- Line 18: Replace the anonymous default-export arrow function with a named
function component in the default export, preserving its existing JSX and
behavior so React DevTools and Fast Refresh can identify it.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: e1296fe7-18c4-4500-99e4-09a05f513a63
⛔ Files ignored due to path filters (9)
packages/angular/standalone/src/components.tsis excluded by!packages/angular/standalone/src/components.tspackages/react/src/components/components.server.tsis excluded by!packages/react/src/components/**packages/vue/src/components/ix-dropdown-button.tsis excluded by!packages/vue/src/components/**packages/vue/src/components/ix-dropdown.tsis excluded by!packages/vue/src/components/**testing/framework-tests/tests/generated/axe.tsis excluded by!**/generated/**testing/framework-tests/tests/generated/dropdown-roving-tabindex-axe.spec.tsis excluded by!**/generated/**testing/framework-tests/tests/generated/dropdown-roving-tabindex.spec.tsis excluded by!**/generated/**testing/framework-tests/tests/generated/index.tsis excluded by!**/generated/**testing/framework-tests/tests/generated/test-ids.autogenerated.tsis excluded by!**/generated/**
📒 Files selected for processing (33)
.changeset/dropdown-roving-tabindex.mdpackages/angular-standalone-test-app/src/app/app.routes.tspackages/angular-standalone-test-app/src/preview-examples/dropdown-roving-tabindex.tspackages/angular-test-app/src/app/app-routing.module.tspackages/angular-test-app/src/app/app.module.tspackages/angular-test-app/src/preview-examples/dropdown-roving-tabindex.tspackages/angular/src/components.tspackages/core/src/components.d.tspackages/core/src/components/dropdown-button/dropdown-button.ct.tspackages/core/src/components/dropdown-button/dropdown-button.tsxpackages/core/src/components/dropdown/dropdown-controller.tspackages/core/src/components/dropdown/dropdown-focus.tspackages/core/src/components/dropdown/dropdown.tsxpackages/core/src/components/dropdown/test/dropdown.ct.tspackages/core/src/components/popover/popover-controller.tspackages/core/src/components/popover/popover.tsxpackages/core/src/components/popover/test/popover-controller.spec.tspackages/core/src/components/popover/test/popover.ct.tspackages/core/src/components/utils/focus/focus-trap.tspackages/core/src/components/utils/nested-overlay/index.tspackages/core/src/components/utils/nested-overlay/nested-overlay-registry.tspackages/core/src/components/utils/nested-overlay/overlay-coordinator.tspackages/core/src/components/utils/nested-overlay/test/nested-overlay.spec.tspackages/core/src/components/utils/nested-overlay/test/overlay-coordinator.spec.tspackages/core/src/components/utils/overlay.tspackages/core/src/components/utils/shadow-dom.tspackages/core/src/components/utils/test/shadow-dom.spec.tspackages/html-test-app/src/preview-examples/dropdown-roving-tabindex.htmlpackages/react-test-app/src/main.tsxpackages/react-test-app/src/preview-examples/dropdown-roving-tabindex.tsxpackages/storybook-docs/src/stories/dropdown-button.stories.tspackages/vue-test-app/src/Root.vuepackages/vue-test-app/src/preview-examples/dropdown-roving-tabindex.vue
💤 Files with no reviewable changes (1)
- packages/core/src/components/utils/overlay.ts
|
1 participant
🆕 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)Related
👨💻 Help & support
Summary by CodeRabbit
New Features
navigationMode="roving-tabindex"to dropdowns and dropdown buttons (default remains the existing mode).data-ix-roving-item.Tab/Shift+Tabclose dropdowns and move focus appropriately;Escapedismisses nested overlays in the correct hierarchy.Bug Fixes
aria-activedescendanthandling) during mode changes and closing.