core(menu): Implement w3c menu pattern for ix-menu#2573
Conversation
|
✅ Deploy Preview for ix-storybook ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
![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 and a roving tab index mechanism for the ix-menu component, updating ix-menu-item and ix-menu-category to support dynamic tab index configuration. A critical review comment points out that using closest('ix-menu') to identify direct menu children is too broad and will break focusability for nested items in slots like ix-menu-settings, suggesting a direct parent check instead.
| const isDirectMenuChild = | ||
| !this.isHostedInsideCategory && !!this.hostElement.closest('ix-menu'); |
There was a problem hiding this comment.
Using closest('ix-menu') to determine if the item is a direct menu child is too broad. If an ix-menu-item is placed inside nested slots of ix-menu (such as ix-menu-settings or ix-menu-about), closest('ix-menu') will still return true. However, these nested items are not direct children and are not queried by getAllFocusableItems(), meaning their tab index will never be updated and they will remain permanently unfocusable (tabIndex = -1).
To fix this, check if the parent element is directly ix-menu instead of using closest.
| const isDirectMenuChild = | |
| !this.isHostedInsideCategory && !!this.hostElement.closest('ix-menu'); | |
| const isDirectMenuChild = | |
| !this.isHostedInsideCategory && | |
| this.hostElement.parentElement?.tagName?.toLowerCase() === 'ix-menu'; |
|
1 participant
💡 What is the current behavior?
GitHub Issue Number: #
🆕 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