Skip to content

fix(menu): multi-select keyboard selection and esc key closing #5826

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Draft
wants to merge 2 commits into
base: main
Choose a base branch
from
Draft

fix(menu): multi-select keyboard selection and esc key closing #5826

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
483d787
fix(menu): multi-select keyboard selection and esc key closing
Oct 24, 2025
c37162c
fix: resetting esc key handler
Oct 24, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
17 changes: 9 additions & 8 deletions packages/menu/src/Menu.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
*/
Expand Down Expand Up @@ -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.
Expand Down Expand Up @@ -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);
Expand Down
159 changes: 159 additions & 0 deletions packages/menu/stories/menu.stories.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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`
<style>
.multi-select-demo {
display: flex;
flex-direction: column;
gap: 16px;
padding: 20px;
}
.status {
padding: 12px;
background: var(--spectrum-gray-100);
border-radius: 4px;
font-family: monospace;
font-size: 12px;
}
.instructions {
padding: 12px;
background: var(--spectrum-blue-100);
border-radius: 4px;
font-size: 14px;
}
.debug-log {
padding: 12px;
background: var(--spectrum-yellow-100);
border-radius: 4px;
font-family: monospace;
font-size: 11px;
max-height: 150px;
overflow-y: auto;
white-space: pre-wrap;
}
</style>
<div class="multi-select-demo">
<div class="instructions">
<strong>Instructions:</strong>
<br />
1. Click the picker button below to open the menu
<br />
2. Use
<kbd>Arrow Up/Down</kbd>
to navigate
<br />
3. Press
<kbd>Space</kbd>
or
<kbd>Enter</kbd>
to toggle selection
<br />
4. Press
<kbd>Escape</kbd>
to close
<br />
5. Watch the debug log below for keyboard events
</div>

<sp-picker-button id="multi-select-button">
<span slot="label">Select multiple options</span>
</sp-picker-button>

<sp-overlay
trigger="multi-select-button@click"
placement="bottom-start"
>
<sp-popover>
<sp-menu
selects="multiple"
label="Select options"
@change=${(event: Event) => {
console.log(
'Selected:',
(event.target as Menu).selected
);
}}
>
<sp-menu-item value="option1">Option 1</sp-menu-item>
<sp-menu-item value="option2">Option 2</sp-menu-item>
<sp-menu-item value="option3">Option 3</sp-menu-item>
</sp-menu>
</sp-popover>
</sp-overlay>

<div class="debug-log">Debug log will appear here...</div>
</div>
`;
};

menuWithMultipleSelection.parameters = {
chromatic: { disableSnapshot: true },
};

menuWithMultipleSelection.story = {
name: 'Picker Button with Multiple Selection',
};
Loading