fix(picker): menuitem focus when opened via mouse (#5358)

nikkimk · Rajdeep Chandra · caseyisonit · web-flow · commit 3c3bc2bed9f1 · 2025-04-23T13:47:53.000Z
* fix(picker): only set focus on first item if keyboard

* test(picker): updated tests are WIP

* chore: added test to check for focus when closed via keyboard

* chore: fix picker test for returning focus

* chore: fix picker test for returning focus linted

* test(action-group): adjusts timeout

* test(action-group): adjusts timeout

* test(action-group): adjusts timeout

* fix(action-menu): make sure keyboard can focus even if mouse toggles menu

* fix(action-menu): make sure keyboard can focus even if mouse toggles menu

* chore(picker): added changeset

---------

Co-authored-by: Rajdeep Chandra &lt;rajdeepchandra@Rajdeeps-MacBook-Pro-2.local&gt;
Co-authored-by: Casey Eickhoff &lt;48574582+caseyisonit@users.noreply.github.com&gt;
diff --git a/.changeset/fluffy-sides-cross.md b/.changeset/fluffy-sides-cross.md
@@ -0,0 +1,11 @@
+---
+'@spectrum-web-components/picker': patch
+---
+
+`PickerBase`(used in `<sp-picker>` and `sp-action-menu>`):
+
+Fixes focus so that it is not set on `<sp-menu-item>` elements when opened via mouse.
+
+A keyboard interaction is the only interaction that should set focus on an `<sp-menu-item>` when the menu is opened. A user with a mouse would expect the focus to stay where the mouse is.
+
+Fixes: #2950
diff --git a/packages/action-group/test/action-group.test.ts b/packages/action-group/test/action-group.test.ts
@@ -177,11 +177,7 @@ describe('ActionGroup', () => {
         );
 
         await elementUpdated(el);
-
-        await nextFrame();
-        await nextFrame();
-        await nextFrame();
-        await nextFrame();
+        await waitUntil(() => el.children.length === 4);
 
         // press Tab to focus into the action-group
         await sendKeys({ press: 'Tab' });
@@ -269,7 +265,7 @@ describe('ActionGroup', () => {
 
         await elementUpdated(el);
 
-        await aTimeout(500);
+        await aTimeout(100);
 
         // get the bounding box of the first button
         const firstButton = el.querySelector('#first') as ActionButton;
@@ -285,9 +281,7 @@ describe('ActionGroup', () => {
                 },
             ],
         });
-
-        await elementUpdated(el);
-        await aTimeout(500);
+        await elementUpdated(firstButton);
 
         // expect all the elements of the focus group to have a tabIndex of -1 except the first button because it is focused using mouse
         expect(
@@ -318,7 +312,6 @@ describe('ActionGroup', () => {
         });
 
         await elementUpdated(el);
-        await aTimeout(500);
 
         // expect the first button to have a tabIndex of 0 and everything else to have a tabIndex of -1
         expect(
@@ -338,9 +331,13 @@ describe('ActionGroup', () => {
             'mouse2: should not be focused on the fourth button'
         ).to.equal(-1);
 
+        await elementUpdated(el);
+
         // get the bounding box of the action-menu
         const actionMenu = el.querySelector('#action-menu') as ActionMenu;
         const actionMenuRect = actionMenu.getBoundingClientRect();
+
+        const opened = oneEvent(el.children[3] as ActionMenu, 'sp-opened');
         sendMouse({
             steps: [
                 {
@@ -352,20 +349,19 @@ describe('ActionGroup', () => {
                 },
             ],
         });
-
+        await opened;
         await elementUpdated(el);
 
-        const opened = oneEvent(el.children[3] as ActionMenu, 'sp-opened');
-        await opened;
+        expect(actionMenu).to.equal(document.activeElement);
+        const closed = oneEvent(el.children[3] as ActionMenu, 'sp-closed');
 
         // use keyboard to navigate to the second menu item and select it
         await sendKeys({ press: 'ArrowDown' });
+        expect(actionMenu.children[0]).to.equal(document.activeElement);
         await sendKeys({ press: 'Enter' });
 
-        const closed = oneEvent(el.children[3] as ActionMenu, 'sp-closed');
         await closed;
 
-        await aTimeout(500);
         expect(
             (el.children[0] as ActionButton)?.tabIndex,
             'final: should NOT be focused on the first button'
diff --git a/packages/picker/src/Picker.ts b/packages/picker/src/Picker.ts
@@ -311,7 +311,20 @@ export class PickerBase extends SizedMixin(SpectrumElement, {
     };
 
     protected async keyboardOpen(): Promise<void> {
-        this.toggle(true);
+        // if the menu is not open, we need to toggle it and wait for it to open to focus on the first selected item
+        if (!this.open || !this.strategy.open) {
+            this.addEventListener(
+                'sp-opened',
+                () => this.optionsMenu?.focusOnFirstSelectedItem(),
+                {
+                    once: true,
+                }
+            );
+            this.toggle(true);
+        } else {
+            // if the menu is already open, we need to focus on the first selected item
+            this.optionsMenu?.focusOnFirstSelectedItem();
+        }
     }
 
     protected async setValueFromItem(
@@ -373,14 +386,6 @@ export class PickerBase extends SizedMixin(SpectrumElement, {
             return;
         }
         const open = typeof target !== 'undefined' ? target : !this.open;
-        if (open && !this.open)
-            this.addEventListener(
-                'sp-opened',
-                () => this.optionsMenu?.focusOnFirstSelectedItem(),
-                {
-                    once: true,
-                }
-            );
 
         this.open = open;
         if (this.strategy) {
diff --git a/packages/picker/test/index.ts b/packages/picker/test/index.ts
@@ -544,7 +544,7 @@ export function runPickerTests(): void {
                 'first item should not be visually focused after closing'
             ).to.be.false;
         });
-        it('opens, on click, with visible focus on a menu item', async () => {
+        it('opens, on click, with visible focus NOT on a menu item', async () => {
             await nextFrame();
             await nextFrame();
             const firstItem = el.querySelector('sp-menu-item') as MenuItem;
@@ -566,7 +566,9 @@ export function runPickerTests(): void {
             await opened;
 
             expect(el.open, 'open?').to.be.true;
-            expect(firstItem.focused, 'firstItem focused?').to.be.true;
+            expect(firstItem.focused, 'firstItem focused after click?').to.be
+                .false;
+            expect(firstItem).to.not.equal(document.activeElement);
         });
         it('opens and selects in a single pointer button interaction', async () => {
             await nextFrame();
@@ -837,7 +839,20 @@ export function runPickerTests(): void {
             ) as MenuItem;
 
             const opened = oneEvent(el, 'sp-opened');
-            el.click();
+
+            const boundingRect = el.button.getBoundingClientRect();
+            sendMouse({
+                steps: [
+                    {
+                        type: 'click',
+                        position: [
+                            boundingRect.x + boundingRect.width / 2,
+                            boundingRect.y + boundingRect.height / 2,
+                        ],
+                    },
+                ],
+            });
+
             await opened;
             await elementUpdated(el);
 
@@ -880,6 +895,7 @@ export function runPickerTests(): void {
                 input.focus();
             });
 
+            // Clicking on an item in the picker triggers a change event
             secondItem.click();
             await waitUntil(
                 () => document.activeElement === input,
@@ -1369,7 +1385,7 @@ export function runPickerTests(): void {
             // we should have SAFARI_FOCUS_RING_CLASS in the classList
             expect(
                 button.classList.contains(SAFARI_FOCUS_RING_CLASS),
-                'has focus ring?'
+                'button has focus ring?'
             ).to.be.true;
 
             // picker should still have focus
@@ -1396,20 +1412,48 @@ export function runPickerTests(): void {
             });
             await opened;
 
+            expect(firstItem.focused, 'firstItem focused?').to.be.true;
+
             // Make a selection again
             closed = oneEvent(el, 'sp-closed');
-            firstItem.click();
+            await sendKeys({
+                press: 'Enter',
+            });
             await closed;
 
             await elementUpdated(el);
 
             // expect the tray to be closed
             expect(el.open, 'open?').to.be.false;
+            // Test focus behavior when using keyboard to close
+            expect(
+                document.activeElement,
+                'focus should be on picker after keyboard close'
+            ).to.equal(el);
+
+            // Verify that focus is maintained on the picker element when closed via keyboard
+            expect(
+                el.contains(document.activeElement) ||
+                    el === document.activeElement,
+                'focus should remain within picker component after keyboard close'
+            ).to.be.true;
+
+            // Click elsewhere to remove focus completely
+            await sendMouse({
+                steps: [
+                    {
+                        type: 'click',
+                        position: [0, 0],
+                    },
+                ],
+            });
 
+            // Now picker should not have focus
+            expect(document.activeElement).not.to.equal(el);
             // we should not have SAFARI_FOCUS_RING_CLASS in the classList
             expect(
                 button.classList.contains(SAFARI_FOCUS_RING_CLASS),
-                'has focus ring?'
+                'has focus ring again?'
             ).to.be.false;
         });
         it('does not close on document scroll', async () => {
