[Rajdeepc]

Phases 2–4 for swc-menu-item, one commit each. API and a11y only; no row chrome (next story).

What's here

Commit	Files
Phase 2 — structure	core/components/menu/MenuItem.types.ts, swc/components/menu/{menu-item.css,swc-menu-item.ts}, barrel exports
Phase 3 — API	core/components/menu/MenuItem.base.ts, swc/components/menu/MenuItem.ts, stories
Phase 4 — a11y	role + aria-disabled wiring in MenuItem.base.ts, tests

Public surface: size (s/m/l/xl, default m), disabled, and three named slots: label, visual, description. That's it. Enter and Space activate a row by dispatching a click, so keyboard and pointer activation reach the same consumer handler.

What to review

1. DisabledMixin is deliberately not used — MenuItem.base.ts

It produces the right ARIA, but it also writes tabindex, which FocusgroupNavigationController owns for these rows. I mapped disabled → aria-disabled by hand instead (3 lines). This settles Q20 via the fallback agreed in the plan review. Review question: agree the mixin should stay unused here?

2. role is set in connectedCallback, not render — MenuItem.base.ts

swc-menu collects rows on slotchange, which can run before the row first renders, so the role can't wait for one. An author-supplied role wins, so menuitemcheckbox/menuitemradio can arrive later without touching this. Review question: is connectedCallback the right hook?

3. Authoring ergonomics of the label slot — settled in review

Named label slot kept. I briefly moved it to the default slot on a misreading of React Spectrum S2, which in fact takes Text slot="label" alongside Text slot="description"; reverted. With description shipping beside it, an unnamed label is ambiguous and has no handle for grid placement in the row layout. Closes Q23.

4. The activation fix reaches into swc-menu — menu/Menu.base.ts

Raised in review: only Enter was handled, and it merely closed the menu without dispatching a click, so a consumer's @click on a row never ran from the keyboard. MenuItemBase now turns Enter and Space into a this.click(); swc-menu treats Space like Enter and skips aria-disabled rows for keyboard and pointer activation. That last part is a behaviour change to swc-menu: clicking a disabled row used to close the menu. Menu's own 29 tests still pass. Review question: is that swc-menu change in scope here, or should it be split out?

5. Breaking changes, implemented by omission — nothing to read, but worth confirming nothing is missing

No href/target/download/rel/referrerpolicy (link rows → swc-menu-link). No selected/value (menuitem can't carry aria-selected; selection → checkbox/radio rows via aria-checked). No hasSubmenu/open, setRole(), dispatchUpdate(). icon slot renamed to visual; the label stays in the default slot. All 45 --mod-menu-item-* dropped.

6. description joins the accessible name — open question for a11y review

A menuitem names itself from its contents, so a row's description is announced as part of its name, not as a separate description. 1st-gen behaves identically, so this is parity. Logged as Q25 in the plan. Review question: is name-from-contents right here, or is it worth naming the row from its label alone?

Verified

• Build, eslint, prettier clean; full gen2 suite green (1253 tests), menu-item 8/8, menu 29/29
• Neither new test is vacuous, confirmed by breaking the code and watching them fail:
  • disabled-row reachability fails when skipDisabled is set to true in Menu.base.ts
  • keyboard activation fails with the consumer click handler runs: expected false to be true when the row's keydown listener is dropped, and with the menu stays open: expected false to be true when the disabled guard is removed

Not verified — needs a human

The ticket's screen reader matrix (JAWS / NVDA / VoiceOver) and WCAG 2.2 AA sign-off. I can't run those, and the automated tests above don't substitute for them. This phase isn't complete until someone does that pass.

On the ticket's paths

The ticket says 2nd-gen/packages/swc/components/menu/menu-item/ with menu-item.ts / menu-item.base.ts. It predates the gen2 rename, and no gen2 component nests a sub-element in its own subdirectory. It was right that rows belong under menu/ though, which the first three commits got wrong and the last one fixes: rows now sit beside Menu.ts, the way AccordionItem.ts sits beside Accordion.ts and Tab.ts beside Tabs.ts. There is no separate Menu item Storybook entry any more; rows are documented in menu.mdx and storied, tested and VRT'd in the menu's own files. The base classes stay in core, not swc. The a11y doc is at 03_components/menu-item/, not under menu/.

Author's checklist

• I have read the CONTRIBUTING and PULL_REQUESTS documents.
• I have reviewed at the Accessibility Practices for this feature, see: Aria Practices
• I have added automated tests to cover my changes.
• I have included a well-written changeset if my change needs to be published.
• I have included updated documentation if my change required it.

Accessibility testing checklist

• Keyboard — open Components → Menu item → Docs → States. The story renders the menu already open, so there is no trigger to click, and note that Menu → States is a different story, about the menu's own open state. Arrow through the rows: focus should start on the first row and reach the disabled row rather than skip it. Chrome paints no ring on the first row after a mouse open; that is :focus-visible, not misplaced focus, and the row chrome story fixes it.
• Screen reader — JAWS, NVDA, VoiceOver on the same story. Expect each row announced as a menu item with its label, and the disabled row announced as unavailable. Outstanding.

[changeset-bot]

⚠️ No Changeset found

Latest commit: b199d3f

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

[github-actions]

📚 Branch Preview Links

• Storybook (Gen2, dev mode)
• Storybook (Gen2, prod mode)
• Documentation Site (Gen1)
• Storybook (Gen1)

🔍 Gen1 Visual Regression Test Results

When a visual regression test fails (or has previously failed while working on this branch), its results can be found in the following URLs:

• Spectrum | Light | Medium | LTR
• Spectrum | Dark | Large | RTL
• Express | Light | Medium | LTR
• Express | Dark | Large | RTL
• Spectrum-two | Light | Medium | LTR
• Spectrum-two | Dark | Large | RTL
• High Contrast Mode | Medium | LTR

Deployed to Azure Blob Storage: pr-6874

If the changes are expected, update the current_golden_images_cache hash in the circleci config to accept the new images. Instructions are included in that file.
If the changes are unexpected, you can investigate the cause of the differences and update the code accordingly.

[aramos-adobe]

[nit] The PR description suggests we should look at Components → Menu item → States but that exists in Menu 😁

[aramos-adobe]

Screen.Recording.2026-10-28.at.11.47.29.AM.mov

Clicking the trigger then pressing the down arrow key moves to the second menu item not the first. But if you tab to the button trigger, press Enter | Space then press Down arrow key it moves to the first menu item

Edit:
It shows the first item as focused in Safari at least

Screen.Recording.2026-10-28.at.12.41.05.PM.mov

[rubencarvalho]

I am ok with skipping DisabledMixin here, and connectedCallback for the role makes sense to me. left two questions, nothing huge

[rubencarvalho]

also, regarding file structure, this is showing up as a top level component in the Storybook, wouldn't we want it inside the menu?

I guess this is a bigger question - but it feels like we've been taking that approach for all other components

[Rajdeepc]

also, regarding file structure, this is showing up as a top level component in the Storybook, wouldn't we want it inside the menu?

I guess this is a bigger question - but it feels like we've been taking that approach for all other components

gen2 has two conventions, and I think I matched the wrong one. close-button, infield-button and color-handle get their own folder because they are reused across parents. AccordionItem and Tab sit beside their parent with one Storybook entry because they only exist inside it. A menu row is the second kind.

So rows now ship inside components/menu/: MenuItem.base.ts and MenuItem.types.ts in core, MenuItem.ts, menu-item.css and swc-menu-item.ts in swc, all re-exported from the menu barrels. The separate ./components/menu-item subpath export is gone, and there is no Menu item sidebar entry; rows are documented in menu.mdx under Item states.

[aramos-adobe]

Nice work on this 🚀

[rubencarvalho]

Requesting changes so it also dismisses my approval.

After the change of putting the menu item inside menu, we're not seeing the API of the menu item in the docs...

[5t3ph]

Also trigger handleTextSlotChange on first render in case slots are empty at first.

[Rajdeepc]

Done in 6b7e04dc5d, and it turned out to matter for more than safety: an entirely empty row fires no slotchange at all, so first render is the only thing that reaches the new missing-label warning. That is the case the test pins.

[5t3ph]

Could we write these to internals instead of this? Setting this.ariaLabelledByElements writes to the host's aria-labelledby, so it'll overwrite (or with null, remove) anything a consumer sets there. With internals they're just defaults and consumer attributes still win, same as _syncLabelling in the labelling mixin. Also swapped to assignedElements({ flatten: true }) so this still works if the slots get forwarded through another component.

Suggested change

	}
	@query('slot[name="label"]')
	private labelSlot?: HTMLSlotElement;
	@query('slot[name="description"]')
	private descriptionSlot?: HTMLSlotElement;
	protected handleTextSlotChange(): void {
	// `menuitem` names from contents, so without explicit references the
	// description is folded into the name instead of announced as a description.
	const label = this.labelSlot?.assignedElements({ flatten: true }) ?? [];
	const description =
	this.descriptionSlot?.assignedElements({ flatten: true }) ?? [];
	this.internals.ariaLabelledByElements = label.length > 0 ? label : null;
	this.internals.ariaDescribedByElements =
	description.length > 0 ? description : null;
	}

This means a switch for the host to have internals from attachInternals() (where role = 'menuitem' is set).

If you don't want to switch to internals, then this would be the alternative to not override consumer-set attributes, but @nikkimk should weigh-in that concern:

  // Skip when a consumer set IDREFs on the host. Element reflection leaves
  // the attribute as "", so a non-empty value means it isn't ours.
  if (!this.getAttribute('aria-labelledby')) {
    this.ariaLabelledByElements = label.length > 0 ? label : null;
  }
  if (!this.getAttribute('aria-describedby')) {
    this.ariaDescribedByElements = description.length > 0 ? description : null;
  }

One edge case: if there's a description but no label, the description still gets folded into the name. Might be worth a debug warning, or falling back to internals.ariaLabel.

[nikkimk]

I agree with @5t3ph's code suggestion here. I think if there is a description but no label, that would be a dev warning as we should always have a label.

[Rajdeepc]

All three taken in 6b7e04dc5d: references moved onto internals (the host keeps no aria-labelledby/aria-describedby, and a consumer value now survives, which a test pins), slots read via assignedElements({ flatten: true }), and a missing-label warning via the shared validateRequiredSlot rather than an ariaLabel fallback, per @nikkimk.

One deliberate omission: I kept role as setAttribute rather than moving it to internals.role, because on internals an author attribute would win and that reopens the role="menuitemcheckbox" override you asked me to remove in the comment above. Happy to move it if you would rather have the platform layering there too.

[Rajdeepc]

Thanks @5t3ph and @nikkimk for the review! It caught three issues I would otherwise have shipped, all fixed in 6b7e04dc5d.

• aria-labelledby clobbering: A genuine bug where writing to this overwrote consumer-provided values. internals correctly keeps these as overridable defaults.
• Flattened slots: assignedElements({ flatten: true }) handles forwarded slots through wrapper components, preventing rows from silently losing their names.
• First-render validation: Empty rows never fire slotchange, so the initial validation is necessary to catch missing labels. I also corrected the test to cover this case and verified it fails when the fix is removed.

I went with @nikkimk's shared validateRequiredSlot approach rather than an ariaLabel fallback, since silently falling back would hide authoring mistakes.

One open question: I kept role on setAttribute rather than internals.role, since an author attribute takes precedence on internals and reintroduces the menuitemcheckbox override. Happy to switch if we prefer platform layering here.

[rubencarvalho]

Left some notes inline. Couple of things: the @example in Menu.ts still uses default-slot rows, menu.mdx line 34 still says swc-menu-item doesn't exist yet (and line 75 doesn't mention Space).

Might also be worth a check in -Firefox- (actually on all browsers), I'm getting a weird behaviour for Space: focus is already back on the trigger before keyup, and then the browser has clicked the newly focused button on keyup before, which would reopen the menu.

[rubencarvalho]

this covers Enter/Space, but a mouse click on a disabled row still goes through. on the preview storybook (Item states), clicking the disabled row still runs a click listener on it, the menu just stays open. 1st-gen blocks this with a capture click listener (handleClickCapture in 1st-gen MenuItem.ts), could we do the same here? and maybe a test that actually clicks the disabled row, since the current one only presses Enter

[rubencarvalho]

this runs on document capture, so with a real Enter/Space the menu closes first (swc-close fires, focus goes back to the trigger) and only then does the row's keydown call click(). with the mouse it's row click first, then close. what if this branch only did preventDefault and the row's click closed the menu through handleItemActivate, so both paths match? (I think the tests can't see this, since events dispatched from script don't let the update run between listeners)

[rubencarvalho]

maybe ignore event.repeat here (after preventDefault)? if you hold Enter on a row it runs, focus goes back to the trigger, the next repeat reopens the menu, and the one after that runs row 1 again

[rubencarvalho]

this one still puts the text in the default slot, so these 15 rows render empty now and the long list overflow snapshot won't really show a long list. <span slot="label">${label}</span>?

[rubencarvalho]

swc-menu-item ships now, so a few spots in this guide are out of date: line 7 says sp-menu-item hasn't migrated, line 26 only imports swc-menu.js (the accordion and tabs guides list the child import too), and the After examples on 79 and 98 use <swc-menu-item>Deselect</swc-menu-item>, which renders an empty row now. could we fix those in this PR and leave the full row section for the docs phase?

[rubencarvalho]

swc is already on 2.0.0-beta.6, so this should probably be the next version

[rubencarvalho]

this is ticked, but there's no test for Space on a disabled row, or for Enter reaching an enabled row's click handler (TabTrapAndEnterClosesTest only checks that the menu closes)

[rubencarvalho]

tiny one: #open-questions doesn't exist here, the heading anchor is #blockers-and-open-questions

[rubencarvalho]

nit: this is tagged options but sits under the STATES banner, could move it up next to Sizes

[rubencarvalho]

nit: the jsdoc guide asks for each @slot on one line (07_jsdoc-standards.md). the CEM output looks fine today, same thing in MenuItem.ts