core(menu): Implement w3c menu pattern for ix-menu#2573
Conversation
🦋 Changeset detectedLatest commit: 0f2746b 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 |
✅ 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.
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughImplements menubar keyboard navigation and ARIA updates for ChangesMenubar accessibility implementation
🎯 4 (Complex) | ⏱️ ~60 minutes
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 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 |
…nto feat/4304-menu-a11y
Improved **ix-menu** accessibility by implementing the w3c menubar pattern, adding better keyboard navigation and screenreader support.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
packages/core/src/components.d.ts (1)
2595-2690:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winAdd the required
@sincedocs for the new public API.
setTabIndexandmenuCategoryLabelare new externally visible API surface, but these generated typings don’t carry the version metadata this repo requires for new props/methods.As per coding guidelines, new public API additions must include JSDoc with a
@sincetag in the source declarations so the generated types stay consistent. ``Also applies to: 9107-9170, 11757-11771
🤖 Prompt for 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. In `@packages/core/src/components.d.ts` around lines 2595 - 2690, The generated typings are missing `@since` tags for newly added public API; add JSDoc `@since` entries to the source declarations for the methods/props named setTabIndex and menuCategoryLabel so the generator emits the version metadata. Locate the component/source declaration files that define the IxMenuCategory/IxMenuItem APIs (the functions/properties setTabIndex and menuCategoryLabel) and prepend appropriate /** `@since` x.y.z */ JSDoc to each symbol (method signature for setTabIndex and the menuCategoryLabel prop declaration) using the correct release version.Source: Coding guidelines
packages/core/src/components/menu-category/test/menu-category.ct.ts (1)
9-9:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winMissing accessibility test using
makeAxeBuilder.Per coding guidelines, every
.ct.tsfile must include an'accessibility'test usingmakeAxeBuilderto verify thataccessibilityScanResults.violationsreturns an empty array. This test file has a'renders'test but is missing the required accessibility test.💡 Suggested accessibility test
import { makeAxeBuilder } from '`@utils/test`'; regressionTest('accessibility', async ({ mount, page }) => { await mount(` <ix-application> <ix-menu> <ix-menu-category label="Category label"> <ix-menu-item>Test Item 1</ix-menu-item> <ix-menu-item>Test Item 2</ix-menu-item> </ix-menu-category> </ix-menu> </ix-application> `); const element = page.locator('ix-menu-category'); await expect(element).toHaveClass(/hydrated/); const results = await makeAxeBuilder({ page }).analyze(); expect(results.violations).toHaveLength(0); });🤖 Prompt for 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. In `@packages/core/src/components/menu-category/test/menu-category.ct.ts` at line 9, Add a new regressionTest named "accessibility" to menu-category.ct.ts that uses makeAxeBuilder to scan the mounted component; specifically, mount the same markup used by the existing "renders" test (an ix-application with ix-menu and ix-menu-category containing two ix-menu-item children), await the component to hydrate via page.locator('ix-menu-category'), run makeAxeBuilder({ page }).analyze(), and assert that results.violations has length 0 using expect; ensure you import makeAxeBuilder from '`@utils/test`' and keep the test signature consistent with the existing regressionTest(async ({ mount, page }) => { ... }).Source: Coding guidelines
packages/core/src/components/toast/tests/toast.ct.ts (2)
19-171:⚠️ Potential issue | 🟠 Major | ⚡ Quick winAdd the required axe accessibility regression for this
.ct.tsfile.This file has a
'renders'test, but it still lacks the mandatory'accessibility'test usingmakeAxeBuilder()and an emptyaccessibilityScanResults.violationsassertion. That leaves the menu-role change in this path uncovered by the repo’s required a11y gate. As per coding guidelines, "Ensure accessibility is tested with component tests using axe, verifying thataccessibilityScanResults.violationsreturns an empty array", "Every test file should include an'accessibility'test usingmakeAxeBuilder", and "Every test file should include a'renders'test verifying basic hydration".🤖 Prompt for 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. In `@packages/core/src/components/toast/tests/toast.ct.ts` around lines 19 - 171, Add a new regressionTest named 'accessibility' that mounts the component (reuse the same mount pattern used in the 'renders' test), uses makeAxeBuilder(page) to run an axe scan (include the toast element/selector), stores the result in accessibilityScanResults, and assert accessibilityScanResults.violations is an empty array; reference the existing regressionTest helper and makeAxeBuilder, and place the test alongside the other tests in this file so the repo's a11y gate is satisfied.Source: Coding guidelines
71-72:⚠️ Potential issue | 🟠 Major | ⚡ Quick winReplace the fixed sleeps with state-based waits.
These
waitForTimeout(...)calls make the toast tests timing-sensitive and violate the component-test rules. Please wait on observable state instead—e.g. toast/menu visibility, attribute changes, orexpect.poll()for the pause/resume API—so the test follows the component rather than the clock. As per coding guidelines, "Don't use arbitrary timeouts orwaitForTimeoutin tests" and "Useexpect.poll()when a value requires async resolution or retries, but prefer Playwright's built-in auto-retry assertions whenever possible".Also applies to: 107-108, 115-116
🤖 Prompt for 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. In `@packages/core/src/components/toast/tests/toast.ct.ts` around lines 71 - 72, The tests currently use page.waitForTimeout(...) (e.g., the fixed 500ms sleeps) which makes them timing-sensitive; replace each sleep with state-based waits that observe the component: use Playwright's auto-retry assertions like await expect(menuLocator).toBeVisible() / toBeHidden() or await expect(toastLocator).toHaveAttribute(...) for attribute changes, and use expect.poll() for the pause/resume API checks instead of fixed timeouts; update the places calling page.waitForTimeout to wait on the toast/menu locators or the pause/resume observable state (referencing the existing page.waitForTimeout calls, the toast/menu locators, and the pause/resume checks) so tests wait on component state not the clock.Source: Coding guidelines
🤖 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 @.changeset/fair-falcons-search.md:
- Line 5: Update the changeset description string that currently reads "Improved
**ix-menu** accessibility by implementing the w3c menubar pattern, adding better
keyboard navigation and screenreader suppport." — fix the typo "suppport" →
"support" and capitalize the acronym "w3c" → "W3C" so the sentence becomes
"Improved **ix-menu** accessibility by implementing the W3C menubar pattern,
adding better keyboard navigation and screenreader support." Reference the same
changeset entry text in .changeset/fair-falcons-search.md to make the edits.
- Around line 1-5: The changeset for ix-menu is marked as patch but the
component adds new public API (new `@Prop/`@Event/@Method and exported interfaces
under packages/core/src/components/menu/*), so update
.changeset/fair-falcons-search.md to change the release type from patch to minor
and clearly list the `@siemens/ix` package; only add a "Fixes `#4304`" footer if the
PR/commit is actually linked to issue `#4304` (confirm the PR/issue connection
before adding that line).
In `@packages/core/src/components/menu-category/menu-category.tsx`:
- Around line 34-38: The helper createSequentialId currently increments its
parameter (sequenceId++) and callers also pass a post-incremented variable,
causing a redundant mutation; change createSequentialId to not mutate its input
(return `${prefix}-${sequenceId}`) and leave call-sites such as the usage with
categorySequenceId in menu-category.tsx and similar in menu-item.tsx using
sequenceId++ intact so the increment only happens at the call site; update the
createSequentialId implementation (and any docs/tests) to reflect the
non-mutating behavior.
In `@packages/core/src/components/menu-item/menu-item.tsx`:
- Around line 130-134: The new public API surface includes setTabIndex() (the
async method decorated with `@Method`() that sets hostTabIndex) but lacks
JSDoc/@since; either make it intentionally public by adding a JSDoc block above
setTabIndex() that documents the method and includes a `@since` tag with the
correct version, description and intended usage, or if it is internal plumbing
for roving tabindex, remove the `@Method`() decorator (and keep the method private
or rename to _setTabIndex) so it does not appear on the Stencil public surface;
update references to setTabIndex accordingly.
- Around line 218-233: getAriaLabel can produce "undefined …" when neither label
nor menuCategoryLabel is set; update getAriaLabel to compute a safe baseLabel
(e.g., const baseLabel = this.label ?? this.menuCategoryLabel ??
this.hostElement.textContent?.trim()) and use that instead of this.label when
building the distinct-tooltip branch; if baseLabel is falsy, return just
this.tooltipText (or undefined if tooltipText is also falsy) so you never
concatenate "undefined" with the tooltipText. Ensure changes are made inside the
getAriaLabel method and reference inheritAriaAttributes, tooltipText, label,
menuCategoryLabel, and hostElement.textContent.
In `@packages/core/src/components/menu/menu.tsx`:
- Around line 639-666: Fix the spelling typo in the comment above
updateMenuItemPositionMetadata: change "sepparated" to "separated" to improve
readability; locate the comment that reads "// item positions and menu size has
to be set manually because slotted items and utility controls are sepparated
into two groups" and update it accordingly (function:
updateMenuItemPositionMetadata, variable: allMenuItems).
- Around line 753-756: The switch branch handling 'End' indexes the last element
with items[items.length - 1]; change it to use the modern .at(-1) accessor for
clarity and safety: update the call to this.updateRovingTabIndex(items,
items.at(-1) ? items.length - 1 : items.length - 1) (or keep the index logic but
use items.at(-1) for retrieving the element to focus) and replace
items[items.length - 1].focus() with const last = items.at(-1); if (last) {
this.updateRovingTabIndex(items, items.length - 1); last.focus(); } to avoid
manual length arithmetic and null access in the case handlers.
---
Outside diff comments:
In `@packages/core/src/components.d.ts`:
- Around line 2595-2690: The generated typings are missing `@since` tags for newly
added public API; add JSDoc `@since` entries to the source declarations for the
methods/props named setTabIndex and menuCategoryLabel so the generator emits the
version metadata. Locate the component/source declaration files that define the
IxMenuCategory/IxMenuItem APIs (the functions/properties setTabIndex and
menuCategoryLabel) and prepend appropriate /** `@since` x.y.z */ JSDoc to each
symbol (method signature for setTabIndex and the menuCategoryLabel prop
declaration) using the correct release version.
In `@packages/core/src/components/menu-category/test/menu-category.ct.ts`:
- Line 9: Add a new regressionTest named "accessibility" to menu-category.ct.ts
that uses makeAxeBuilder to scan the mounted component; specifically, mount the
same markup used by the existing "renders" test (an ix-application with ix-menu
and ix-menu-category containing two ix-menu-item children), await the component
to hydrate via page.locator('ix-menu-category'), run makeAxeBuilder({ page
}).analyze(), and assert that results.violations has length 0 using expect;
ensure you import makeAxeBuilder from '`@utils/test`' and keep the test signature
consistent with the existing regressionTest(async ({ mount, page }) => { ... }).
In `@packages/core/src/components/toast/tests/toast.ct.ts`:
- Around line 19-171: Add a new regressionTest named 'accessibility' that mounts
the component (reuse the same mount pattern used in the 'renders' test), uses
makeAxeBuilder(page) to run an axe scan (include the toast element/selector),
stores the result in accessibilityScanResults, and assert
accessibilityScanResults.violations is an empty array; reference the existing
regressionTest helper and makeAxeBuilder, and place the test alongside the other
tests in this file so the repo's a11y gate is satisfied.
- Around line 71-72: The tests currently use page.waitForTimeout(...) (e.g., the
fixed 500ms sleeps) which makes them timing-sensitive; replace each sleep with
state-based waits that observe the component: use Playwright's auto-retry
assertions like await expect(menuLocator).toBeVisible() / toBeHidden() or await
expect(toastLocator).toHaveAttribute(...) for attribute changes, and use
expect.poll() for the pause/resume API checks instead of fixed timeouts; update
the places calling page.waitForTimeout to wait on the toast/menu locators or the
pause/resume observable state (referencing the existing page.waitForTimeout
calls, the toast/menu locators, and the pause/resume checks) so tests wait on
component state not the clock.
🪄 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: d0b8b7e6-499a-484c-8a08-0f77eb6d5033
⛔ Files ignored due to path filters (4)
packages/react/src/components/components.server.tsis excluded by!packages/react/src/components/**packages/vue/src/components/ix-menu-item.tsis excluded by!packages/vue/src/components/**testing/visual-testing/__screenshots__/tests/menu/menu.e2e.ts/menu-link---should-render-menu-items-with-a-href-1-chromium---classic-dark-linux.pngis excluded by!**/*.pngtesting/visual-testing/__screenshots__/tests/menu/menu.e2e.ts/menu-link---should-render-menu-items-with-a-href-1-chromium---classic-light-linux.pngis excluded by!**/*.png
📒 Files selected for processing (10)
.changeset/fair-falcons-search.mdpackages/core/src/components.d.tspackages/core/src/components/menu-category/menu-category.tsxpackages/core/src/components/menu-category/test/menu-category.ct.tspackages/core/src/components/menu-item/menu-item.tsxpackages/core/src/components/menu/menu.scsspackages/core/src/components/menu/menu.tsxpackages/core/src/components/toast/tests/toast.ct.tstesting/visual-testing/tests/menu-about-news/menu-about-news.e2e.tstesting/visual-testing/tests/menu/link/index.html
|
remaining sonar issues are not relevant:
|
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
packages/core/src/components/menu/menu.tsx (1)
110-145: 📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick winVersion the new
ix-menuprops in the source JSDoc.
i18nAriaLabelMenuandi18nNavigationHintare new public props, but their source JSDoc omits@since, so the generated API docs cannot show when they were introduced. Please add the correct version tags here before regenerating the typings. As per path instructions,packages/core/**:For new component/public API additions (component, prop, method, event), require JSDoc@SInCEtags and flag missing/incorrect versions.🤖 Prompt for 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. In `@packages/core/src/components/menu/menu.tsx` around lines 110 - 145, The new public props on the ix-menu component are missing source JSDoc version tags, so update the JSDoc for i18nAriaLabelMenu and i18nNavigationHint in menu.tsx to include the correct `@since` annotations before regenerating typings. Use the existing prop declarations in the Menu component to locate the additions, and ensure the tags match the version when these props were introduced so the generated API docs can surface them properly.Source: Path instructions
packages/core/src/components.d.ts (1)
2618-2618: 📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick winAdd
@sincemetadata to the newix-menu-item/ix-menu-categoryAPIs.The generated declarations expose
setTabIndex()andmenuCategoryLabelwithout any JSDoc version metadata. Because these are new public APIs, the source component members need the correct@sincetags before regenerating this file. As per path instructions,packages/core/**:For new component/public API additions (component, prop, method, event), require JSDoc@SInCEtags and flag missing/incorrect versions.Also applies to: 2689-2689, 2699-2699
🤖 Prompt for 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. In `@packages/core/src/components.d.ts` at line 2618, The generated declarations for the new ix-menu-item and ix-menu-category public APIs are missing version metadata, so update the source component members before regenerating this file. Add the correct JSDoc `@since` tags to the relevant public API symbols, including setTabIndex() and menuCategoryLabel, and verify the component/member declarations in ix-menu-item and ix-menu-category carry the proper version annotations so the generated typings include them.Source: Path instructions
🤖 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/angular/src/components.ts`:
- Around line 1561-1569: The Angular proxy metadata is out of sync with the
Stencil component contract: this wrapper only adds ix-menu inputs, but the new
ix-menu-item.menuCategoryLabel, ix-menu-item.setTabIndex(), and
ix-menu-category.setTabIndex() APIs are missing from the generated proxies.
Regenerate the Angular output target so the components.ts metadata for
ix-menu-item and ix-menu-category includes the new inputs/methods, rather than
editing the wrapper manually.
In `@packages/core/src/components/menu/menu.tsx`:
- Around line 650-655: The bottom-slot mutation path in updateBottomSlotState
only refreshes hasBottomSlotItems, so changes to bottom content can leave the
overflow/shadow state stale. Update this method in menu.tsx to also re-run
handleOverflowIndicator() after bottom-slot children change, so
isMenuItemsOverflow and the shadow flags are recalculated immediately whenever
onSlotchange detects a bottom-slot update.
---
Outside diff comments:
In `@packages/core/src/components.d.ts`:
- Line 2618: The generated declarations for the new ix-menu-item and
ix-menu-category public APIs are missing version metadata, so update the source
component members before regenerating this file. Add the correct JSDoc `@since`
tags to the relevant public API symbols, including setTabIndex() and
menuCategoryLabel, and verify the component/member declarations in ix-menu-item
and ix-menu-category carry the proper version annotations so the generated
typings include them.
In `@packages/core/src/components/menu/menu.tsx`:
- Around line 110-145: The new public props on the ix-menu component are missing
source JSDoc version tags, so update the JSDoc for i18nAriaLabelMenu and
i18nNavigationHint in menu.tsx to include the correct `@since` annotations before
regenerating typings. Use the existing prop declarations in the Menu component
to locate the additions, and ensure the tags match the version when these props
were introduced so the generated API docs can surface them properly.
🪄 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: 83a09914-6f53-4428-a76a-86d9ab549234
⛔ Files ignored due to path filters (3)
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-menu.tsis excluded by!packages/vue/src/components/**
📒 Files selected for processing (4)
packages/angular/src/components.tspackages/core/src/components.d.tspackages/core/src/components/menu/menu.scsspackages/core/src/components/menu/menu.tsx
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
packages/core/src/components/menu/menu.tsx (2)
691-699: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winClear stale ARIA position metadata from categories too.
itemscan includeix-menu-category, but the cleanup query only removesaria-posinset/aria-setsizefromix-menu-item. Hidden/disabled categories can keep stale set positions after recomputation.Proposed fix
const allMenuItems = [ ...Array.from( this.hostElement.querySelectorAll<HTMLIxMenuItemElement>('ix-menu-item') ), + ...Array.from( + this.hostElement.querySelectorAll<HTMLIxMenuCategoryElement>( + 'ix-menu-category' + ) + ), ...Array.from( this.hostElement.shadowRoot?.querySelectorAll<HTMLIxMenuItemElement>( '.menu-utility-controls > ix-menu-item'🤖 Prompt for 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. In `@packages/core/src/components/menu/menu.tsx` around lines 691 - 699, The stale ARIA cleanup in menu recomputation only targets ix-menu-item elements, so hidden or disabled ix-menu-category nodes can keep outdated aria-posinset and aria-setsize values. Update the cleanup logic in the menu component around allMenuItems to include ix-menu-category alongside ix-menu-item when clearing position metadata, and ensure the recompute path applies to both item types consistently.
316-320: 🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy liftKeep the active menu item, not the menubar container, as the tab stop.
resetRovingTabIndex()makes every item-1while.menu-navigationremainstabIndex={0}, so tabbing into the menu focuses the composite container without an active menuitem. Either keep one item tabbable for the roving-tabindex model, or fully switch to container focus witharia-activedescendant.As per path instructions, prioritize accessibility regressions; the PR objective is to implement the W3C menubar pattern.
Also applies to: 673-684, 916-920
🤖 Prompt for 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. In `@packages/core/src/components/menu/menu.tsx` around lines 316 - 320, The menubar focus model is incorrect because `resetRovingTabIndex()` sets all menu items to -1 while `.menu-navigation` stays tabbable, so focus lands on the container instead of an active `menuitem`. Update `menu.tsx` in `componentDidLoad` and the related roving-tabindex logic used by `getAllFocusableItems()`/`resetRovingTabIndex()` so one item remains the tab stop, or else switch the `menu-navigation` pattern fully to container focus with `aria-activedescendant`. Make the same accessibility-focused fix wherever this tab index setup is repeated in the menu component.Source: Path instructions
🤖 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/menu/menu.tsx`:
- Around line 110-120: The new public i18n props on Menu need JSDoc `@since` tags
added to their comments so the generated API docs include the upcoming version.
Update the documentation above the i18nAriaLabelMenu and i18nNavigationHint
`@Prop` declarations in menu.tsx, keeping the tags aligned with these new public
API additions and using the correct upcoming version.
---
Outside diff comments:
In `@packages/core/src/components/menu/menu.tsx`:
- Around line 691-699: The stale ARIA cleanup in menu recomputation only targets
ix-menu-item elements, so hidden or disabled ix-menu-category nodes can keep
outdated aria-posinset and aria-setsize values. Update the cleanup logic in the
menu component around allMenuItems to include ix-menu-category alongside
ix-menu-item when clearing position metadata, and ensure the recompute path
applies to both item types consistently.
- Around line 316-320: The menubar focus model is incorrect because
`resetRovingTabIndex()` sets all menu items to -1 while `.menu-navigation` stays
tabbable, so focus lands on the container instead of an active `menuitem`.
Update `menu.tsx` in `componentDidLoad` and the related roving-tabindex logic
used by `getAllFocusableItems()`/`resetRovingTabIndex()` so one item remains the
tab stop, or else switch the `menu-navigation` pattern fully to container focus
with `aria-activedescendant`. Make the same accessibility-focused fix wherever
this tab index setup is repeated in the menu component.
🪄 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: ec0cb808-f49c-4187-89cc-f9ce74c606f7
📒 Files selected for processing (1)
packages/core/src/components/menu/menu.tsx
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/core/src/components/menu/menu.tsx (1)
728-749: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winExclude non-rendered default items from roving navigation.
When the menu is collapsed on
sm, the default slot is not rendered anymore (Lines 934-936), butgetAllFocusableItems()still walks the light DOM andisVisible()treats those entries as navigable. Arrow/Home/End can then move focus to invisible items, andaria-posinset/aria-setsizeare computed against the wrong set.♻️ Suggested change
private getAllFocusableItems(): ( | HTMLIxMenuItemElement | HTMLIxMenuCategoryElement )[] { const isNavigable = (el: HTMLElement) => !el.hasAttribute('disabled') && !el.hasAttribute('hidden') && this.isVisible(el); const lightItems = Array.from( this.hostElement.querySelectorAll< HTMLIxMenuItemElement | HTMLIxMenuCategoryElement >(':scope > ix-menu-item, :scope > ix-menu-category') - ).filter(isNavigable); + ).filter((el) => { + if (!isNavigable(el)) { + return false; + } + + if (!this.isHiddenFromViewport()) { + return true; + } + + const slot = el.getAttribute('slot'); + return slot === 'home' || slot === 'bottom'; + }); const utilityItems = Array.from( this.hostElement.shadowRoot?.querySelectorAll<HTMLIxMenuItemElement>( '.menu-utility-controls > ix-menu-item' ) ?? []🤖 Prompt for 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. In `@packages/core/src/components/menu/menu.tsx` around lines 728 - 749, Exclude non-rendered default slot items from roving navigation in getAllFocusableItems(); when the menu is collapsed on sm and the default content is not rendered, light DOM query results should be filtered out so Arrow/Home/End and aria-posinset/aria-setsize only use actually rendered items. Update the logic in menu.tsx around getAllFocusableItems() and the related visibility checks so only rendered, navigable items from the active slot/shadow DOM are returned.
🤖 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.d.ts`:
- Around line 3170-3171: The generated public types still include IxPlayground
even though ix-playground is marked as development-only in stencil.config.ts via
excludeDevelopmentComponents. Update the component type generation so
IxPlayground is omitted from components.d.ts for the public distribution, and
verify the exclusion is applied wherever the public API typings are assembled.
---
Outside diff comments:
In `@packages/core/src/components/menu/menu.tsx`:
- Around line 728-749: Exclude non-rendered default slot items from roving
navigation in getAllFocusableItems(); when the menu is collapsed on sm and the
default content is not rendered, light DOM query results should be filtered out
so Arrow/Home/End and aria-posinset/aria-setsize only use actually rendered
items. Update the logic in menu.tsx around getAllFocusableItems() and the
related visibility checks so only rendered, navigable items from the active
slot/shadow DOM are returned.
🪄 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: c37cf98f-ea97-4983-825b-51140feb8ca5
⛔ Files ignored due to path filters (2)
packages/react/src/components/components.server.tsis excluded by!packages/react/src/components/**packages/vue/src/components/ix-menu.tsis excluded by!packages/vue/src/components/**
📒 Files selected for processing (3)
packages/core/src/components.d.tspackages/core/src/components/menu/menu-expand-icon.tsxpackages/core/src/components/menu/menu.tsx
|
regarding open sonar issues: |
|
2 participants
IX-4304
💡 What is the current behavior?
Currently ix-menu does not have proper keyboard-navigation according to w3c menubar pattern: https://www.w3.org/WAI/ARIA/apg/patterns/menubar/examples/menubar-navigation/
🆕 What is the new behavior?
Implements the w3c keyboard-navigation and roles for better screenreader support.
🏁 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
Summary by CodeRabbit
ix-menuand enhanced menubar support with rovingtabIndex, improved Arrow/Home/End navigation, and utility controls grouping.setTabIndex()APIs for menu items/categories and improved category labeling support; added typings forix-playground.aria-expanded.menuitemroles and interactions.