fix(picker): dispatch click events for menu items on touch devices (#5901)

iuliauta · Rajdeep Chandra · web-flow · commit ae61361da670 · 2025-12-03T15:26:04.000-05:00
* fix(menu): ipad menu item click

* test: menu now handle clicks properly

* docs: apply changeset formatting suggestions from review

* refactor: address PR review feedback
- Remove reactive-controllers changeset (not needed)
- Simplify picker changeset description
- Remove duplicate bindEvents() calls in tests
- Query menu items through optionsMenu to avoid shadow DOM issues
- Reset both isTouchDevice and isMobile in afterEach

* fix: use childItems.find() to access menu item in test
- Changed from querySelector to childItems.find() as menu items are tracked in the childItems array
- Changed assertion from to.not.be.null to to.not.be.undefined (find() returns undefined)

* fix: use direct index to access menu item in test
- Changed from find() to direct array access childItems[1]
- Simpler and more direct approach as suggested by reviewer

---------

Co-authored-by: Rajdeep Chandra &lt;rajdeepc@adobe.com&gt;
diff --git a/.changeset/brave-deer-pick.md b/.changeset/brave-deer-pick.md
@@ -0,0 +1,7 @@
+---
+'@spectrum-web-components/picker': patch
+---
+
+**Fixed**: click events are now dispatched from menu-items on touch devices
+
+- All touch devices (including iPads with screen widths >743px) now correctly use click events instead of drag-and-select behavior
diff --git a/1st-gen/packages/picker/src/InteractionController.ts b/1st-gen/packages/picker/src/InteractionController.ts
@@ -179,15 +179,6 @@ export class InteractionController implements ReactiveController {
 
     hostConnected(): void {
         this.init();
-        this.host.addEventListener('sp-opened', () => {
-            /**
-             * set shouldSupportDragAndSelect to false for mobile
-             * to prevent click event being captured behind the menu-tray
-             * we do this here because the menu gets reinitialized on overlay open
-             */
-            this.host.optionsMenu.shouldSupportDragAndSelect =
-                !this.host.isMobile.matches;
-        });
         this.host.addEventListener('sp-closed', () => {
             if (
                 !this.open &&
diff --git a/1st-gen/packages/picker/src/Picker.ts b/1st-gen/packages/picker/src/Picker.ts
@@ -49,6 +49,7 @@ import type { SlottableRequestEvent } from '@spectrum-web-components/overlay/src
 import { DependencyManagerController } from '@spectrum-web-components/reactive-controllers/src/DependencyManger.js';
 import {
     IS_MOBILE,
+    IS_TOUCH_DEVICE,
     MatchMediaController,
 } from '@spectrum-web-components/reactive-controllers/src/MatchMedia.js';
 import type { Tooltip } from '@spectrum-web-components/tooltip';
@@ -85,6 +86,8 @@ export class PickerBase extends SizedMixin(SpectrumElement, {
 
     public isMobile = new MatchMediaController(this, IS_MOBILE);
 
+    public isTouchDevice = new MatchMediaController(this, IS_TOUCH_DEVICE);
+
     public strategy!: DesktopController | MobileController;
 
     @state()
@@ -774,6 +777,7 @@ export class PickerBase extends SizedMixin(SpectrumElement, {
                 role=${this.listRole}
                 .selects=${this.selects}
                 .selected=${this.value ? [this.value] : []}
+                .shouldSupportDragAndSelect=${!this.isTouchDevice.matches}
                 size=${this.size}
                 @sp-menu-item-keydown=${this.handleEscape}
                 @sp-menu-item-added-or-updated=${this.shouldManageSelection}
diff --git a/1st-gen/packages/picker/test/picker-responsive.test.ts b/1st-gen/packages/picker/test/picker-responsive.test.ts
@@ -15,16 +15,19 @@ import {
     expect,
     fixture,
     html,
+    oneEvent,
     waitUntil,
 } from '@open-wc/testing';
 import '@spectrum-web-components/field-label/sp-field-label.js';
 import '@spectrum-web-components/menu/sp-menu-divider.js';
+import type { MenuItem } from '@spectrum-web-components/menu';
 import '@spectrum-web-components/menu/sp-menu-item.js';
 import { Picker } from '@spectrum-web-components/picker';
 import '@spectrum-web-components/picker/sync/sp-picker.js';
 import { spreadProps } from '../../../test/lit-helpers.js';
 import { Popover } from '@spectrum-web-components/popover';
 import { Tray } from '@spectrum-web-components/tray/src/Tray.js';
+import { spy } from 'sinon';
 
 describe('Picker, responsive', () => {
     let el: Picker;
@@ -172,4 +175,139 @@ describe('Picker, responsive', () => {
             expect(popover, 'popover').to.not.be.null;
         });
     });
+
+    describe('touch device detection', () => {
+        afterEach(() => {
+            // Reset touch device and mobile simulation.
+            if (el && el.isTouchDevice) {
+                el.isTouchDevice.matches = false;
+            }
+            if (el && el.isMobile) {
+                el.isMobile.matches = false;
+            }
+        });
+
+        it('sets shouldSupportDragAndSelect to false on touch devices', async () => {
+            el = await pickerFixture();
+            await elementUpdated(el);
+            /**
+             * This is a hack to set the `isTouchDevice` property to true
+             * so that we can test the touch device behavior.
+             */
+            el.isTouchDevice.matches = true;
+            await elementUpdated(el);
+
+            // Open the picker to initialize the menu.
+            const opened = oneEvent(el, 'sp-opened');
+            el.open = true;
+            await opened;
+
+            // Wait for menu to be ready.
+            await waitUntil(
+                () => el.optionsMenu && el.optionsMenu.childItems.length > 0,
+                'Menu should be initialized'
+            );
+
+            // Check that shouldSupportDragAndSelect is false.
+            expect(el.optionsMenu.shouldSupportDragAndSelect).to.be.false;
+        });
+
+        it('sets shouldSupportDragAndSelect to true on desktop devices', async () => {
+            el = await pickerFixture();
+            await elementUpdated(el);
+
+            // Ensure we're not on a touch device.
+            el.isTouchDevice.matches = false;
+            await elementUpdated(el);
+
+            // Open the picker to initialize the menu.
+            const opened = oneEvent(el, 'sp-opened');
+            el.open = true;
+            await opened;
+
+            // Wait for menu to be ready.
+            await waitUntil(
+                () => el.optionsMenu && el.optionsMenu.childItems.length > 0,
+                'Menu should be initialized'
+            );
+
+            // Check that shouldSupportDragAndSelect is true.
+            expect(el.optionsMenu.shouldSupportDragAndSelect).to.be.true;
+        });
+
+        it('dispatches change event when menu item is clicked on touch device', async () => {
+            el = await pickerFixture();
+            await elementUpdated(el);
+
+            const changeSpy = spy();
+            el.addEventListener('change', changeSpy);
+
+            /**
+             * This is a hack to set the `isTouchDevice` property to true
+             * so that we can test the iPad/tablet behavior.
+             */
+            el.isTouchDevice.matches = true;
+            await elementUpdated(el);
+
+            // Open the picker.
+            const opened = oneEvent(el, 'sp-opened');
+            el.open = true;
+            await opened;
+            await elementUpdated(el);
+
+            // Wait for menu to be ready.
+            await waitUntil(
+                () => el.optionsMenu && el.optionsMenu.childItems.length > 0,
+                'Menu should be initialized'
+            );
+
+            // Wait for menu to be fully updated.
+            await el.optionsMenu.updateComplete;
+            await elementUpdated(el.optionsMenu);
+
+            // Verify shouldSupportDragAndSelect is false on touch devices.
+            expect(el.optionsMenu.shouldSupportDragAndSelect).to.be.false;
+
+            // Get the second menu item (value="option-2") from childItems.
+            const menuItem = el.optionsMenu.childItems[1] as MenuItem;
+            expect(menuItem).to.not.be.null;
+            await elementUpdated(menuItem);
+
+            // Ensure menu is not in scrolling state (which would prevent selection).
+            el.optionsMenu.isScrolling = false;
+
+            // Click the menu item.
+            const closed = oneEvent(el, 'sp-closed');
+            menuItem.click();
+            await closed;
+
+            // Verify the change event was dispatched.
+            expect(changeSpy.callCount).to.equal(1);
+            expect(el.value).to.equal('option-2');
+        });
+
+        it('uses isTouchDevice instead of isMobile for shouldSupportDragAndSelect', async () => {
+            el = await pickerFixture();
+            await elementUpdated(el);
+
+            // Simulate iPad: isMobile is false but isTouchDevice is true.
+            el.isMobile.matches = false;
+            el.isTouchDevice.matches = true;
+            await elementUpdated(el);
+
+            // Open the picker.
+            const opened = oneEvent(el, 'sp-opened');
+            el.open = true;
+            await opened;
+
+            // Wait for menu to be ready.
+            await waitUntil(
+                () => el.optionsMenu && el.optionsMenu.childItems.length > 0,
+                'Menu should be initialized'
+            );
+
+            // The fix: shouldSupportDragAndSelect should be false based on isTouchDevice.
+            expect(el.optionsMenu.shouldSupportDragAndSelect).to.be.false;
+        });
+    });
 });
diff --git a/1st-gen/tools/reactive-controllers/src/MatchMedia.ts b/1st-gen/tools/reactive-controllers/src/MatchMedia.ts
@@ -14,6 +14,7 @@ import type { ReactiveController, ReactiveElement } from 'lit';
 export const DARK_MODE = '(prefers-color-scheme: dark)';
 export const IS_MOBILE =
     '(max-width: 743px) and (hover: none) and (pointer: coarse)';
+export const IS_TOUCH_DEVICE = '(hover: none) and (pointer: coarse)';
 
 export class MatchMediaController implements ReactiveController {
     key = Symbol('match-media-key');
