From 41bbf609ae7ba20a0aaaec325a57b35c41528f2a Mon Sep 17 00:00:00 2001 From: Rajdeep Chandra Date: Tue, 29 Sep 2026 21:51:44 +0530 Subject: [PATCH 1/2] style(menu): conform to TypeScript, test, and Storybook style guides - Menu.ts: drop the PlacementController mention from the concrete class JSDoc. Per the base-vs-concrete guide, internal implementation classes belong in base-class (contributor-facing) JSDoc, not concrete-class JSDoc, which is consumer- and CEM-facing. Popover.ts already follows this. - menu.stories.ts: move the shared defaultItems template into its own HELPERS section rather than leaving it under PLAYGROUND STORY, matching the documented section order and popover.stories.ts's own layout. - menu.stories.ts: add the missing TriggerElement.storyName override. Without it Storybook renders the title as "Trigger Element" (title case), mismatching menu.mdx's "### Trigger element" heading. Both popover.stories.ts and tooltip.stories.ts set this on their own TriggerElement stories. - menu.test.ts: add descriptive messages to the specific ARIA-attribute and focus-target assertions, matching the sub-pattern already used in popover.test.ts. Co-Authored-By: Claude Opus 5 --- 2nd-gen/packages/swc/components/menu/Menu.ts | 13 ++++--- .../components/menu/stories/menu.stories.ts | 7 +++- .../swc/components/menu/test/menu.test.ts | 35 +++++++++++++++---- 3 files changed, 43 insertions(+), 12 deletions(-) diff --git a/2nd-gen/packages/swc/components/menu/Menu.ts b/2nd-gen/packages/swc/components/menu/Menu.ts index 6fda721c66c..8a273412a0e 100644 --- a/2nd-gen/packages/swc/components/menu/Menu.ts +++ b/2nd-gen/packages/swc/components/menu/Menu.ts @@ -17,10 +17,15 @@ import { MenuBase } from '@adobe/spectrum-wc-core/components/menu'; import styles from './menu.css'; /** - * A menu is a full menu-button host: an externally-referenced trigger opens a - * `PlacementController`-anchored surface containing a `role="menu"` list of - * `swc-menu-item` rows (`swc-menu-group` and `swc-divider` as a separator - * join in a later migration phase). + * A menu is a list of actions or options anchored to an externally-referenced + * trigger, following the menu-button pattern. `swc-menu-item` rows + * (`swc-menu-group` and `swc-divider` as a separator join in a later + * migration phase) render in a shadow-internal `role="menu"` surface. + * + * The behavior (trigger and ARIA wiring, positioning, keyboard navigation, + * and event dispatch) lives in `MenuBase`; this class supplies only the + * styles, the render template, and the shadow-DOM element getter it + * overrides. * * @element swc-menu * diff --git a/2nd-gen/packages/swc/components/menu/stories/menu.stories.ts b/2nd-gen/packages/swc/components/menu/stories/menu.stories.ts index 868312a9874..4f1204cac71 100644 --- a/2nd-gen/packages/swc/components/menu/stories/menu.stories.ts +++ b/2nd-gen/packages/swc/components/menu/stories/menu.stories.ts @@ -73,7 +73,7 @@ const meta: Meta = { export default meta; // ──────────────────── -// PLAYGROUND STORY +// HELPERS // ──────────────────── // Placeholders for `swc-menu-item`, which doesn't exist yet; matches its @@ -84,6 +84,10 @@ const defaultItems = html` Paste `; +// ──────────────────── +// PLAYGROUND STORY +// ──────────────────── + export const Playground: Story = { args: { open: false, @@ -272,6 +276,7 @@ export const TriggerElement: Story = { `; }, }; +TriggerElement.storyName = 'Trigger element'; // ──────────────────────────────── // ACCESSIBILITY STORIES diff --git a/2nd-gen/packages/swc/components/menu/test/menu.test.ts b/2nd-gen/packages/swc/components/menu/test/menu.test.ts index 2a8b321fb5f..20a9ed5dd6a 100644 --- a/2nd-gen/packages/swc/components/menu/test/menu.test.ts +++ b/2nd-gen/packages/swc/components/menu/test/menu.test.ts @@ -364,12 +364,21 @@ export const TriggerAriaWiringSwcButtonTest: Story = { 'wires aria-haspopup and aria-expanded onto the inner shadow button', async () => { expect(innerButton, 'swc-button has an inner button').toBeTruthy(); - expect(innerButton?.getAttribute('aria-haspopup')).toBe('menu'); - expect(innerButton?.getAttribute('aria-expanded')).toBe('false'); + expect( + innerButton?.getAttribute('aria-haspopup'), + 'inner button aria-haspopup' + ).toBe('menu'); + expect( + innerButton?.getAttribute('aria-expanded'), + 'inner button aria-expanded before open' + ).toBe('false'); menu.open = true; await menu.updateComplete; - expect(innerButton?.getAttribute('aria-expanded')).toBe('true'); + expect( + innerButton?.getAttribute('aria-expanded'), + 'inner button aria-expanded after open' + ).toBe('true'); menu.open = false; await menu.updateComplete; @@ -440,7 +449,10 @@ export const TriggerSwapClearsOldAriaTest: Story = { ) as HTMLButtonElement; await step('wires the initial for target', async () => { - expect(oldTrigger.getAttribute('aria-haspopup')).toBe('menu'); + expect( + oldTrigger.getAttribute('aria-haspopup'), + 'initial trigger aria-haspopup' + ).toBe('menu'); }); await step( @@ -596,7 +608,10 @@ export const FocusManagementTest: Story = { timeout: 1000, }); await waitFor( - () => expect(document.activeElement).toBe(getItems(canvasElement)[0]), + () => + expect(document.activeElement, 'focus moves to the first row').toBe( + getItems(canvasElement)[0] + ), { timeout: 1000 } ); }); @@ -666,7 +681,10 @@ export const TabTrapAndEnterActivateTest: Story = { await step('opens and focuses the first row', async () => { await userEvent.click(trigger); await waitFor( - () => expect(document.activeElement).toBe(getItems(canvasElement)[0]), + () => + expect(document.activeElement, 'focus moves to the first row').toBe( + getItems(canvasElement)[0] + ), { timeout: 1000 } ); }); @@ -711,7 +729,10 @@ export const ArrowKeyNavigationTest: Story = { await step('opens and focuses the first row', async () => { await userEvent.click(trigger); await waitFor( - () => expect(document.activeElement).toBe(getItems(canvasElement)[0]), + () => + expect(document.activeElement, 'focus moves to the first row').toBe( + getItems(canvasElement)[0] + ), { timeout: 1000 } ); }); From 0094699dad5f5ba69b5f82529f634c5bb919c53d Mon Sep 17 00:00:00 2001 From: Rajdeep Chandra Date: Wed, 30 Sep 2026 12:13:13 +0530 Subject: [PATCH 2/2] style(menu): address review feedback on the conformance pass - Menu.ts: add the missing `@since 2.0.0-beta.4` tag, matching the convention every other migrated concrete class already follows (Popover, Tooltip, Badge). The newest release tag is gen2-2.0.0-beta.3, so the next cut is beta.4. - menu.test.ts: message the last two ARIA assertions that were still bare, so the whole file follows one pattern rather than most of it. - menu.test.ts: drop the redundant `{ timeout: 1000 }` from all 35 waitFor calls. 1000ms is already testing-library's `asyncUtilTimeout` default and vitest.config.js does not override it, so these restated the default as noise; popover.test.ts passes no explicit timeout on any of its 32 waitFor calls. The single `{ timeout: 2000 }` is kept, since that one genuinely doubles the default while waiting on the exit transition to clear `actual-placement`. Co-Authored-By: Claude Opus 5 --- 2nd-gen/packages/swc/components/menu/Menu.ts | 1 + .../swc/components/menu/test/menu.test.ts | 177 ++++++------------ 2 files changed, 61 insertions(+), 117 deletions(-) diff --git a/2nd-gen/packages/swc/components/menu/Menu.ts b/2nd-gen/packages/swc/components/menu/Menu.ts index 8a273412a0e..e0c7fbff623 100644 --- a/2nd-gen/packages/swc/components/menu/Menu.ts +++ b/2nd-gen/packages/swc/components/menu/Menu.ts @@ -28,6 +28,7 @@ import styles from './menu.css'; * overrides. * * @element swc-menu + * @since 2.0.0-beta.4 * * @example * diff --git a/2nd-gen/packages/swc/components/menu/test/menu.test.ts b/2nd-gen/packages/swc/components/menu/test/menu.test.ts index 20a9ed5dd6a..d22b68f6810 100644 --- a/2nd-gen/packages/swc/components/menu/test/menu.test.ts +++ b/2nd-gen/packages/swc/components/menu/test/menu.test.ts @@ -153,13 +153,9 @@ export const PropertyMutationTest: Story = { await step('reflects open attribute after mutation', async () => { menu.open = true; - await waitFor(() => expect(menu.hasAttribute('open')).toBe(true), { - timeout: 1000, - }); + await waitFor(() => expect(menu.hasAttribute('open')).toBe(true)); menu.open = false; - await waitFor(() => expect(menu.hasAttribute('open')).toBe(false), { - timeout: 1000, - }); + await waitFor(() => expect(menu.hasAttribute('open')).toBe(false)); }); await step('renders in every valid size', async () => { @@ -256,9 +252,7 @@ export const StartsOpenNoPhantomEventTest: Story = { mount.appendChild(menu); await menu.updateComplete; - await waitFor(() => expect(isMenuOpen(menu)).toBe(true), { - timeout: 1000, - }); + await waitFor(() => expect(isMenuOpen(menu)).toBe(true)); expect( openFired, 'swc-open does not fire on the very first render' @@ -286,17 +280,13 @@ export const ClickToToggleTest: Story = { await step('clicking the trigger opens the menu', async () => { await userEvent.click(trigger); - await waitFor(() => expect(isMenuOpen(menu)).toBe(true), { - timeout: 1000, - }); + await waitFor(() => expect(isMenuOpen(menu)).toBe(true)); expect(menu.open, 'open is true after trigger click').toBe(true); }); await step('clicking the trigger again closes the menu', async () => { await userEvent.click(trigger); - await waitFor(() => expect(isMenuOpen(menu)).toBe(false), { - timeout: 1000, - }); + await waitFor(() => expect(isMenuOpen(menu)).toBe(false)); expect(menu.open, 'open is false after second trigger click').toBe(false); }); }, @@ -423,9 +413,7 @@ export const TriggerElementOverrideTest: Story = { 'click on the triggerElement target toggles the menu', async () => { await userEvent.click(correct); - await waitFor(() => expect(isMenuOpen(menu)).toBe(true), { - timeout: 1000, - }); + await waitFor(() => expect(isMenuOpen(menu)).toBe(true)); menu.open = false; await menu.updateComplete; } @@ -580,9 +568,7 @@ export const ShadowRootScopeTest: Story = { ).toBe('menu'); menu.open = true; - await waitFor(() => expect(isMenuOpen(menu)).toBe(true), { - timeout: 1000, - }); + await waitFor(() => expect(isMenuOpen(menu)).toBe(true)); menu.open = false; await menu.updateComplete; } @@ -604,23 +590,17 @@ export const FocusManagementTest: Story = { await step('opening moves focus to the first row', async () => { await userEvent.click(trigger); - await waitFor(() => expect(isMenuOpen(menu)).toBe(true), { - timeout: 1000, - }); - await waitFor( - () => - expect(document.activeElement, 'focus moves to the first row').toBe( - getItems(canvasElement)[0] - ), - { timeout: 1000 } + await waitFor(() => expect(isMenuOpen(menu)).toBe(true)); + await waitFor(() => + expect(document.activeElement, 'focus moves to the first row').toBe( + getItems(canvasElement)[0] + ) ); }); await step('closing restores focus to the trigger', async () => { menu.open = false; - await waitFor(() => expect(isMenuOpen(menu)).toBe(false), { - timeout: 1000, - }); + await waitFor(() => expect(isMenuOpen(menu)).toBe(false)); expect(document.activeElement, 'focus returns to the trigger').toBe( trigger ); @@ -644,9 +624,7 @@ export const FocusNotStolenOnCloseTest: Story = { 'does not move focus to the trigger when focus was not inside the menu at close', async () => { menu.open = true; - await waitFor(() => expect(isMenuOpen(menu)).toBe(true), { - timeout: 1000, - }); + await waitFor(() => expect(isMenuOpen(menu)).toBe(true)); elsewhere.focus(); expect(document.activeElement, 'focus moved to elsewhere').toBe( @@ -654,9 +632,7 @@ export const FocusNotStolenOnCloseTest: Story = { ); menu.open = false; - await waitFor(() => expect(isMenuOpen(menu)).toBe(false), { - timeout: 1000, - }); + await waitFor(() => expect(isMenuOpen(menu)).toBe(false)); expect( document.activeElement, 'focus remains on the externally-focused element' @@ -680,12 +656,10 @@ export const TabTrapAndEnterActivateTest: Story = { await step('opens and focuses the first row', async () => { await userEvent.click(trigger); - await waitFor( - () => - expect(document.activeElement, 'focus moves to the first row').toBe( - getItems(canvasElement)[0] - ), - { timeout: 1000 } + await waitFor(() => + expect(document.activeElement, 'focus moves to the first row').toBe( + getItems(canvasElement)[0] + ) ); }); @@ -709,9 +683,7 @@ export const TabTrapAndEnterActivateTest: Story = { let closeCount = 0; menu.addEventListener('swc-close', () => closeCount++); await userEvent.keyboard('{Enter}'); - await waitFor(() => expect(isMenuOpen(menu)).toBe(false), { - timeout: 1000, - }); + await waitFor(() => expect(isMenuOpen(menu)).toBe(false)); expect(menu.open, 'menu closes on Enter').toBe(false); expect(closeCount, 'swc-close fires exactly once').toBe(1); }); @@ -728,12 +700,10 @@ export const ArrowKeyNavigationTest: Story = { await step('opens and focuses the first row', async () => { await userEvent.click(trigger); - await waitFor( - () => - expect(document.activeElement, 'focus moves to the first row').toBe( - getItems(canvasElement)[0] - ), - { timeout: 1000 } + await waitFor(() => + expect(document.activeElement, 'focus moves to the first row').toBe( + getItems(canvasElement)[0] + ) ); }); @@ -830,9 +800,7 @@ export const ClickActivationTest: Story = { 'clicking inside the menu but not on a row does not close it', async () => { await userEvent.click(trigger); - await waitFor(() => expect(isMenuOpen(menu)).toBe(true), { - timeout: 1000, - }); + await waitFor(() => expect(isMenuOpen(menu)).toBe(true)); await userEvent.click(menu); await menu.updateComplete; @@ -845,9 +813,7 @@ export const ClickActivationTest: Story = { await step('clicking a row closes the menu', async () => { await userEvent.click(getItems(canvasElement)[1]); - await waitFor(() => expect(isMenuOpen(menu)).toBe(false), { - timeout: 1000, - }); + await waitFor(() => expect(isMenuOpen(menu)).toBe(false)); expect(menu.open, 'menu closes after clicking a row').toBe(false); }); @@ -943,24 +909,18 @@ export const PlacementControllerIntegrationTest: Story = { 'applies translate positioning via PlacementController when opened', async () => { menu.open = true; - await waitFor(() => expect(isMenuOpen(menu)).toBe(true), { - timeout: 1000, - }); - await waitFor(() => expect(surface()?.style.translate).toBeTruthy(), { - timeout: 1000, - }); + await waitFor(() => expect(isMenuOpen(menu)).toBe(true)); + await waitFor(() => expect(surface()?.style.translate).toBeTruthy()); } ); await step( 'sets the actual-placement attribute to a valid physical side', async () => { - await waitFor( - () => - expect(['top', 'bottom', 'left', 'right']).toContain( - menu.getAttribute('actual-placement') - ), - { timeout: 1000 } + await waitFor(() => + expect(['top', 'bottom', 'left', 'right']).toContain( + menu.getAttribute('actual-placement') + ) ); } ); @@ -992,21 +952,17 @@ export const ReanchorOnPlacementChangeTest: Story = { 'reruns positioning when placement changes while open', async () => { menu.open = true; - await waitFor(() => expect(isMenuOpen(menu)).toBe(true), { - timeout: 1000, - }); - await waitFor( - () => expect(menu.getAttribute('actual-placement')).toBeTruthy(), - { timeout: 1000 } + await waitFor(() => expect(isMenuOpen(menu)).toBe(true)); + await waitFor(() => + expect(menu.getAttribute('actual-placement')).toBeTruthy() ); menu.removeAttribute('actual-placement'); menu.placement = 'top-end'; await menu.updateComplete; - await waitFor( - () => expect(menu.getAttribute('actual-placement')).toBeTruthy(), - { timeout: 1000 } + await waitFor(() => + expect(menu.getAttribute('actual-placement')).toBeTruthy() ); menu.open = false; @@ -1038,24 +994,20 @@ export const ReanchorOnPlacementChangeTest: Story = { }; menu.open = true; - await waitFor(() => expect(isMenuOpen(menu)).toBe(true), { - timeout: 1000, - }); + await waitFor(() => expect(isMenuOpen(menu)).toBe(true)); menu.removeAttribute('actual-placement'); menu.shouldFlip = false; await menu.updateComplete; - await waitFor( - () => expect(menu.getAttribute('actual-placement')).toBeTruthy(), - { timeout: 1000 } + await waitFor(() => + expect(menu.getAttribute('actual-placement')).toBeTruthy() ); menu.removeAttribute('actual-placement'); menu.for = 'reanchor-trigger-2'; await menu.updateComplete; - await waitFor( - () => expect(menu.getAttribute('actual-placement')).toBeTruthy(), - { timeout: 1000 } + await waitFor(() => + expect(menu.getAttribute('actual-placement')).toBeTruthy() ); expect( surfaceDistanceTo(trigger2), @@ -1065,9 +1017,8 @@ export const ReanchorOnPlacementChangeTest: Story = { menu.removeAttribute('actual-placement'); menu.triggerElement = trigger1; await menu.updateComplete; - await waitFor( - () => expect(menu.getAttribute('actual-placement')).toBeTruthy(), - { timeout: 1000 } + await waitFor(() => + expect(menu.getAttribute('actual-placement')).toBeTruthy() ); expect( surfaceDistanceTo(trigger1), @@ -1144,9 +1095,7 @@ export const NativeCloseReconciliationTest: Story = { 'syncs the open property down when the surface closes without open being set first', async () => { menu.open = true; - await waitFor(() => expect(isMenuOpen(menu)).toBe(true), { - timeout: 1000, - }); + await waitFor(() => expect(isMenuOpen(menu)).toBe(true)); // Simulates the surface closing on its own (e.g. native light-dismiss) // by calling hidePopover() directly instead of going through the @@ -1155,7 +1104,7 @@ export const NativeCloseReconciliationTest: Story = { // trusted Escape/outside-click would (covered end-to-end with real // trusted input in menu.a11y.spec.ts). surface().hidePopover(); - await waitFor(() => expect(menu.open).toBe(false), { timeout: 1000 }); + await waitFor(() => expect(menu.open).toBe(false)); expect(isMenuOpen(menu), 'surface is closed').toBe(false); } ); @@ -1171,9 +1120,7 @@ export const KeydownWhileClosedTest: Story = { 'a keydown that arrives just as the menu closes is a no-op', async () => { menu.open = true; - await waitFor(() => expect(isMenuOpen(menu)).toBe(true), { - timeout: 1000, - }); + await waitFor(() => expect(isMenuOpen(menu)).toBe(true)); // Setting `open` updates the property synchronously; the document // keydown listener isn't removed until the deferred close reaction @@ -1195,9 +1142,7 @@ export const KeydownWhileClosedTest: Story = { 'Tab is not trapped once the menu has closed' ).toBe(false); - await waitFor(() => expect(isMenuOpen(menu)).toBe(false), { - timeout: 1000, - }); + await waitFor(() => expect(isMenuOpen(menu)).toBe(false)); } ); }, @@ -1219,7 +1164,10 @@ export const TriggerBecomesUnresolvedTest: Story = { ) as HTMLButtonElement; await step('wires the initial valid trigger', async () => { - expect(oldTrigger.getAttribute('aria-haspopup')).toBe('menu'); + expect( + oldTrigger.getAttribute('aria-haspopup'), + 'initial trigger aria-haspopup' + ).toBe('menu'); }); await step( @@ -1264,9 +1212,7 @@ export const OpensWithNoItemsTest: Story = { let threw = false; try { menu.open = true; - await waitFor(() => expect(isMenuOpen(menu)).toBe(true), { - timeout: 1000, - }); + await waitFor(() => expect(isMenuOpen(menu)).toBe(true)); } catch { threw = true; } @@ -1289,9 +1235,7 @@ export const DismissibleStackTest: Story = { await step('registers as the top dismissible while open', async () => { menu.open = true; - await waitFor(() => expect(isMenuOpen(menu)).toBe(true), { - timeout: 1000, - }); + await waitFor(() => expect(isMenuOpen(menu)).toBe(true)); expect( isTopDismissible(menu), 'menu is the top dismissible while open' @@ -1300,9 +1244,7 @@ export const DismissibleStackTest: Story = { await step('unregisters once closed', async () => { menu.open = false; - await waitFor(() => expect(isMenuOpen(menu)).toBe(false), { - timeout: 1000, - }); + await waitFor(() => expect(isMenuOpen(menu)).toBe(false)); expect( isTopDismissible(menu), 'menu is no longer a dismissible once closed' @@ -1335,7 +1277,10 @@ export const DisconnectCleanupTest: Story = { await menu.updateComplete; await step('wires the trigger while connected', async () => { - expect(trigger.getAttribute('aria-haspopup')).toBe('menu'); + expect( + trigger.getAttribute('aria-haspopup'), + 'trigger aria-haspopup' + ).toBe('menu'); }); await step('disconnects while closed', async () => { @@ -1374,9 +1319,7 @@ export const DisconnectCleanupTest: Story = { await menu2.updateComplete; menu2.open = true; - await waitFor(() => expect(isMenuOpen(menu2)).toBe(true), { - timeout: 1000, - }); + await waitFor(() => expect(isMenuOpen(menu2)).toBe(true)); expect( isTopDismissible(menu2), 'menu2 is the top dismissible while open'