Skip to content
Draft
Show file tree
Hide file tree
Changes from all commits
Commits
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
100 changes: 93 additions & 7 deletions packages/menu/src/Menu.ts
Original file line number Diff line number Diff line change
Expand Up @@ -72,6 +72,16 @@ export class Menu extends SizedMixin(SpectrumElement, { noDefaultSize: true }) {

protected rovingTabindexController?: RovingTabindexController<MenuItem>;

/**
* Tracks the last hovered menu item for keyboard navigation
*/
private hoveredItem?: MenuItem;

/**
* Tracks whether we have a global keyboard listener active
*/
private hasGlobalKeyListener = false;

/**
* iPad scroll detection properties
*
Expand Down Expand Up @@ -103,30 +113,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 @@ -431,6 +441,26 @@ export class Menu extends SizedMixin(SpectrumElement, { noDefaultSize: true }) {
new RovingTabindexController<MenuItem>(this, {
direction: 'vertical',
focusInIndex: (elements: MenuItem[] | undefined) => {
// If we have a hovered item and no current focus, use it for transition
if (
this.hoveredItem &&
elements?.includes(this.hoveredItem) &&
!this.matches(':focus-within')
) {
const hoveredIndex = elements.indexOf(
this.hoveredItem
);
if (
hoveredIndex !== -1 &&
!this.hoveredItem.disabled
) {
// Clear hovered item after using it for focus transition
this.hoveredItem = undefined;
return hoveredIndex;
}
}

// Fall back to original logic (selected item or first enabled)
let firstEnabledIndex = -1;
const firstSelectedIndex = elements?.findIndex(
(el, index) => {
Expand Down Expand Up @@ -524,6 +554,51 @@ export class Menu extends SizedMixin(SpectrumElement, { noDefaultSize: true }) {
}
}

/**
* Sets the hovered item for keyboard navigation without stealing focus
*/
public setHoveredItem(item: MenuItem): void {
this.hoveredItem = item;

// Add global keyboard listener when we have a hovered item
if (item && !this.hasGlobalKeyListener) {
this.hasGlobalKeyListener = true;
document.addEventListener(
'keydown',
this.handleGlobalKeydown,
true
);
}
}

/**
* Handle global keyboard events for hover-to-focus transition
*/
private handleGlobalKeydown = (event: KeyboardEvent): void => {
if (
[
'ArrowUp',
'ArrowDown',
'ArrowLeft',
'ArrowRight',
'Home',
'End',
].includes(event.key)
) {
if (this.hoveredItem) {
// Focus the menu to enable keyboard navigation
this.focus();
// Clear the global listener
this.hasGlobalKeyListener = false;
document.removeEventListener(
'keydown',
this.handleGlobalKeydown,
true
);
}
}
};

/**
* Handles touchstart events for iPad scroll detection.
*
Expand Down Expand Up @@ -576,7 +651,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 @@ -1089,6 +1164,17 @@ export class Menu extends SizedMixin(SpectrumElement, { noDefaultSize: true }) {
this.selectedItemsMap.clear();
this.childItemSet.clear();
this.descendentOverlays = new Map<Overlay, Overlay>();

// Clean up global listener if active
if (this.hasGlobalKeyListener) {
document.removeEventListener(
'keydown',
this.handleGlobalKeydown,
true
);
this.hasGlobalKeyListener = false;
}

super.disconnectedCallback();
}

Expand Down
3 changes: 2 additions & 1 deletion packages/menu/src/MenuItem.ts
Original file line number Diff line number Diff line change
Expand Up @@ -482,7 +482,8 @@ export class MenuItem extends LikeAnchor(
handleMouseover(event: MouseEvent): void {
const target = event.target as HTMLElement;
if (target === this) {
this.focus();
// Track hovered item for keyboard navigation without stealing focus
this.menuData.parentMenu?.setHoveredItem(this);
this.focused = false;
}
}
Expand Down
107 changes: 107 additions & 0 deletions packages/menu/stories/menu.stories.ts
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,11 @@ import '@spectrum-web-components/icons-workflow/icons/sp-icon-export.js';
import '@spectrum-web-components/icons-workflow/icons/sp-icon-folder-open.js';
import '@spectrum-web-components/icons-workflow/icons/sp-icon-share.js';
import '@spectrum-web-components/icons-workflow/icons/sp-icon-show-menu.js';
import '@spectrum-web-components/search/sp-search.js';
import '@spectrum-web-components/textfield/sp-textfield.js';
import '@spectrum-web-components/number-field/sp-number-field.js';
import '@spectrum-web-components/combobox/sp-combobox.js';
import '@spectrum-web-components/color-field/sp-color-field.js';

export default {
component: 'sp-menu',
Expand Down Expand Up @@ -484,3 +489,105 @@ export const dynamicRemoval = (): TemplateResult => {
</sp-menu>
`;
};

export const InputsWithMenu = (): TemplateResult => {
return html`
<div style="padding: 20px; max-width: 600px;">
<h3>Input Focus Demo</h3>
<p>
Try typing in any input field below, then hover over the menu
items. The input should maintain focus and not be interrupted.
This demonstrates the fix for focus stealing from all supported
input types.
</p>

<div
style="display: grid; gap: 16px; grid-template-columns: 1fr 1fr; margin-bottom: 20px;"
>
<!-- Search Input -->
<div>
<label for="demo-search">Search:</label>
<sp-search
id="demo-search"
placeholder="Search input..."
style="width: 100%; margin-top: 8px;"
></sp-search>
</div>

<!-- Textfield Input -->
<div>
<label for="demo-textfield">Textfield:</label>
<sp-textfield
id="demo-textfield"
placeholder="Textfield input..."
style="width: 100%; margin-top: 8px;"
></sp-textfield>
</div>

<!-- Number Field Input -->
<div>
<label for="demo-number">Number Field:</label>
<sp-number-field
id="demo-number"
placeholder="Number input..."
style="width: 100%; margin-top: 8px;"
></sp-number-field>
</div>

<!-- Combobox Input -->
<div>
<label for="demo-combobox">Combobox:</label>
<sp-combobox
id="demo-combobox"
placeholder="Combobox input..."
style="width: 100%; margin-top: 8px;"
></sp-combobox>
</div>

<!-- Color Field Input -->
<div>
<label for="demo-color">Color Field:</label>
<sp-color-field
id="demo-color"
placeholder="Color input..."
style="width: 100%; margin-top: 8px;"
></sp-color-field>
</div>

<!-- Native Input -->
<div>
<label for="demo-native">Native Input:</label>
<input
id="demo-native"
placeholder="Native input..."
style="width: 100%; margin-top: 8px; padding: 8px; border: 1px solid #ccc; border-radius: 4px;"
/>
</div>
</div>

<sp-popover open>
<sp-menu>
<sp-menu-item>Search Results</sp-menu-item>
<sp-menu-item>Recent Searches</sp-menu-item>
<sp-menu-item>Saved Searches</sp-menu-item>
<sp-menu-item>Advanced Search</sp-menu-item>
<sp-menu-item>Search Settings</sp-menu-item>
<sp-menu-item>Clear History</sp-menu-item>
</sp-menu>
</sp-popover>
</div>
`;
};

InputsWithMenu.parameters = {
tags: ['!dev'],
};

InputsWithMenu.swc_vrt = {
skip: true,
};

InputsWithMenu.parameters = {
// Disables Chromatic's snapshotting on a global level
chromatic: { disableSnapshot: true },
};
27 changes: 19 additions & 8 deletions packages/menu/test/menu.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -82,7 +82,6 @@ describe('Menu', () => {
<sp-menu-item disabled>Disabled item 2</sp-menu-item>
</sp-menu>
`);
const firstItem = el.querySelector('sp-menu-item') as MenuItem;

await elementUpdated(el);
expect(document.activeElement).to.not.equal(el);
Expand All @@ -91,7 +90,6 @@ describe('Menu', () => {
await elementUpdated(el);
expect(document.activeElement).to.not.equal(el);
expect(focusinSpy.callCount).to.equal(0);
firstItem.focus();
await elementUpdated(el);
expect(document.activeElement).to.not.equal(el);
expect(focusinSpy.callCount).to.equal(0);
Expand Down Expand Up @@ -338,24 +336,37 @@ describe('Menu', () => {
expect(document.activeElement).to.equal(firstItem);
expect(firstItem.focused, 'first item focused').to.be.true;

// Hover over second item - should not steal focus
await sendMouseTo(secondItem);
await elementUpdated(el);

// Active element should remain on first item since focus is not stolen on hover
expect(document.activeElement, 'active element after hover').to.equal(
secondItem
firstItem
);
expect(document.activeElement).to.equal(secondItem);
expect(firstItem.focused, 'first item should still be focused').to.be
.true;
expect(
secondItem.focused,
'second item should not have focus styling on hover'
).to.be.false;

await sendKeys({ press: 'ArrowUp' });
// Test keyboard navigation still works
await sendKeys({ press: 'ArrowDown' });
await elementUpdated(el);

expect(document.activeElement).to.equal(firstItem);
expect(
firstItem.focused,
'first item should have focus styling after keyboard'
document.activeElement,
'active element after arrow down'
).to.equal(secondItem);
expect(
secondItem.focused,
'second item should be focused after keyboard'
).to.be.true;
expect(
firstItem.focused,
'first item should not be focused after keyboard'
).to.be.false;
});

it('handle focus and late descendant additions', async () => {
Expand Down
Loading