From 483d7872d39daa34c8d245539a35bcddf965a90f Mon Sep 17 00:00:00 2001 From: Rajdeep Chandra Date: Fri, 24 Oct 2025 10:32:22 +0530 Subject: [PATCH 1/2] fix(menu): multi-select keyboard selection and esc key closing --- packages/menu/src/Menu.ts | 17 +-- packages/menu/src/MenuItem.ts | 7 +- packages/menu/stories/menu.stories.ts | 159 ++++++++++++++++++++++++++ 3 files changed, 171 insertions(+), 12 deletions(-) diff --git a/packages/menu/src/Menu.ts b/packages/menu/src/Menu.ts index 28cbfb6f74..eb24f56ca6 100644 --- a/packages/menu/src/Menu.ts +++ b/packages/menu/src/Menu.ts @@ -103,30 +103,30 @@ export class Menu extends SizedMixin(SpectrumElement, { noDefaultSize: true }) { /** * Minimum vertical movement (in pixels) required to trigger scrolling detection. - * + * * This threshold is consistent with other components in the design system: * - Card component uses 10px for click vs. drag detection * - Menu component uses 10px for scroll vs. selection detection - * + * * The 10px threshold is carefully chosen to: * - Allow for natural finger tremor and accidental touches * - Distinguish between intentional scroll gestures and taps * - Provide consistent behavior across the platform - * + * * @see {@link packages/card/src/Card.ts} for similar threshold usage */ private scrollThreshold = 10; // pixels /** * Maximum time (in milliseconds) for a movement to be considered scrolling. - * + * * This threshold is consistent with other timing values in the design system: * - Longpress duration: 300ms (ActionButton, LongpressController) * - Scroll detection: 300ms (Menu component) - * + * * Quick movements within this timeframe are likely intentional scrolls, * while slower movements are more likely taps or selections. - * + * * @see {@link packages/action-button/src/ActionButton.ts} for longpress duration * @see {@link packages/overlay/src/LongpressController.ts} for longpress duration */ @@ -576,7 +576,7 @@ export class Menu extends SizedMixin(SpectrumElement, { noDefaultSize: true }) { * Resets the scrolling state after a short delay (100ms) to allow for * any final touch events to be processed. This delay prevents immediate * state changes that could interfere with the selection logic. - * + * * The 100ms delay is consistent with the design system's approach to * touch event handling and ensures that any final touch events or * gesture recognition can complete before the scrolling state is reset. @@ -886,8 +886,9 @@ export class Menu extends SizedMixin(SpectrumElement, { noDefaultSize: true }) { } if (key === ' ' || key === 'Enter') { event.preventDefault(); + // The click() method already triggers selection via handlePointerBasedSelection + // so we don't need to call selectOrToggleItem again here. root?.focusElement?.click(); - if (root) this.selectOrToggleItem(root); return; } this.navigateBetweenRelatedMenus(event as MenuItemKeydownEvent); diff --git a/packages/menu/src/MenuItem.ts b/packages/menu/src/MenuItem.ts index 75de1e0de3..1de4b44574 100644 --- a/packages/menu/src/MenuItem.ts +++ b/packages/menu/src/MenuItem.ts @@ -583,10 +583,9 @@ export class MenuItem extends LikeAnchor( const openSubmenuKey = this.hasSubmenu && !this.open && [' ', 'Enter'].includes(key); if (target === this) { - if ( - ['ArrowLeft', 'ArrowRight', 'Escape'].includes(key) || - openSubmenuKey - ) + // Only prevent default for arrow keys and submenu-related keys. + // Don't prevent Escape - let it bubble to close overlays. + if (['ArrowLeft', 'ArrowRight'].includes(key) || openSubmenuKey) event.preventDefault(); this.dispatchEvent( new MenuItemKeydownEvent({ root: this, event: event }) diff --git a/packages/menu/stories/menu.stories.ts b/packages/menu/stories/menu.stories.ts index 992d1a1d1a..b71ef72790 100644 --- a/packages/menu/stories/menu.stories.ts +++ b/packages/menu/stories/menu.stories.ts @@ -591,3 +591,162 @@ InputsWithMenu.parameters = { // Disables Chromatic's snapshotting on a global level chromatic: { disableSnapshot: true }, }; + +export const menuWithMultipleSelection = (): TemplateResult => { + import('@spectrum-web-components/picker-button/sp-picker-button.js'); + import('@spectrum-web-components/overlay/sp-overlay.js'); + import('@spectrum-web-components/popover/sp-popover.js'); + + function handleKeydown(event: KeyboardEvent): void { + const logEl = document.querySelector('.debug-log'); + if (logEl) { + const time = new Date().toLocaleTimeString(); + const info = `[${time}] Key: "${event.key}", Target: ${(event.target as HTMLElement).tagName}, defaultPrevented: ${event.defaultPrevented}`; + logEl.textContent = info + '\n' + logEl.textContent; + } + } + + setTimeout(() => { + const menu = document.querySelector( + 'sp-menu[selects="multiple"]' + ) as Menu; + const overlay = document.querySelector('sp-overlay'); + if (menu && overlay) { + menu.addEventListener('keydown', handleKeydown, { capture: true }); + console.log('Debug: Menu found, attaching keydown listener'); + console.log('Debug: Menu selects=', menu.getAttribute('selects')); + console.log('Debug: Menu childItems=', menu.childItems); + + // Log focus changes + menu.addEventListener('focusin', () => { + console.log( + '🎯 FOCUSIN on menu, activeElement:', + document.activeElement + ); + }); + + // Also listen on document for all keydown events + document.addEventListener( + 'keydown', + (event: KeyboardEvent) => { + if (event.key === ' ' || event.key === 'Enter') { + console.log( + '⌨️ Space/Enter pressed, activeElement:', + document.activeElement?.tagName + ); + } + if (event.key === 'Escape') { + console.log( + '🚪 Escape pressed, overlay open:', + overlay.open, + 'activeElement:', + document.activeElement?.tagName + ); + } + }, + { capture: true } + ); + + // Listen for overlay events + overlay.addEventListener('sp-opened', () => { + console.log('✅ Overlay opened'); + }); + overlay.addEventListener('sp-closed', () => { + console.log('❌ Overlay closed'); + }); + } + }, 1000); + + return html` + +
+
+ Instructions: +
+ 1. Click the picker button below to open the menu +
+ 2. Use + Arrow Up/Down + to navigate +
+ 3. Press + Space + or + Enter + to toggle selection +
+ 4. Press + Escape + to close +
+ 5. Watch the debug log below for keyboard events +
+ + + Select multiple options + + + + + { + console.log( + 'Selected:', + (event.target as Menu).selected + ); + }} + > + Option 1 + Option 2 + Option 3 + + + + +
Debug log will appear here...
+
+ `; +}; + +menuWithMultipleSelection.parameters = { + chromatic: { disableSnapshot: true }, +}; + +menuWithMultipleSelection.story = { + name: 'Picker Button with Multiple Selection', +}; From c37162c7aa8a1bcb397d1a3b2139ac391bb60d8a Mon Sep 17 00:00:00 2001 From: Rajdeep Chandra Date: Fri, 24 Oct 2025 16:03:14 +0530 Subject: [PATCH 2/2] fix: resetting esc key handler --- packages/menu/src/MenuItem.ts | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/packages/menu/src/MenuItem.ts b/packages/menu/src/MenuItem.ts index 1de4b44574..75de1e0de3 100644 --- a/packages/menu/src/MenuItem.ts +++ b/packages/menu/src/MenuItem.ts @@ -583,9 +583,10 @@ export class MenuItem extends LikeAnchor( const openSubmenuKey = this.hasSubmenu && !this.open && [' ', 'Enter'].includes(key); if (target === this) { - // Only prevent default for arrow keys and submenu-related keys. - // Don't prevent Escape - let it bubble to close overlays. - if (['ArrowLeft', 'ArrowRight'].includes(key) || openSubmenuKey) + if ( + ['ArrowLeft', 'ArrowRight', 'Escape'].includes(key) || + openSubmenuKey + ) event.preventDefault(); this.dispatchEvent( new MenuItemKeydownEvent({ root: this, event: event })