-
Notifications
You must be signed in to change notification settings - Fork 235
fix(menu): ipad menu item click #5901
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.
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
base: main
Are you sure you want to change the base?
Conversation
🦋 Changeset detectedLatest commit: dd2df56 The changes in this PR will be included in the next version bump. This PR includes changesets to release 78 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
3ece1a2 to
7523d6a
Compare
📚 Branch Preview Links🔍 First Generation Visual Regression Test ResultsWhen a visual regression test fails (or has previously failed while working on this branch), its results can be found in the following URLs:
Deployed to Azure Blob Storage: If the changes are expected, update the |
| expect(el.optionsMenu.shouldSupportDragAndSelect).to.be.true; | ||
| }); | ||
|
|
||
| it('dispatches change event when menu item is clicked on touch device', async () => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.

Description
Issue
Menu items (
<sp-menu-item>) in<sp-action-menu>were not registering click events on iPad devices, while the same items wrapped in<sp-menu-group>worked correctly. This issue was reported in GitHub Issue #5706.Symptoms
<sp-action-menu>don't fire click eventspointerdownandpointerupevents fire correctly<sp-menu-group>Root cause
The issue stemmed from an incorrect device detection strategy that didn't properly account for tablet devices with touch input.
Technical details
Device detection query
This query only matched devices with:
iPad devices don't match because their screen width is typically 768px-1024px.
The shouldSupportDragAndSelect flag
In
packages/picker/src/InteractionController.ts, the code set:Since iPad didn't match
IS_MOBILE,shouldSupportDragAndSelectwas set totrue.Event handling conflict
In
packages/menu/src/Menu.ts:When
shouldSupportDragAndSelect = true:handlePointerupprocesses the selection and setspointerUpTargethandleClicksees thatpointerUpTarget === event.targetand returns earlyWhy menu-group worked
When menu items are wrapped in
<sp-menu-group>, the additional DOM wrapper causes theevent.targetin the click handler to sometimes differ from thepointerUpTargetset in the pointerup handler, allowing the click to proceed.Solution
We introduced a new
IS_TOUCH_DEVICEmedia query that detects any device with touch input, regardless of screen size:This query matches:
The
PickerBaseclass now includes anisTouchDeviceproperty:The menu's
shouldSupportDragAndSelectproperty is set directly in the template when the menu is rendered:This ensures that on all touch devices (including iPads), the menu uses click events instead of the drag-and-select pattern, preventing the event handling conflict.
Motivation and context
Related issue(s)
Screenshots (if appropriate)
Author's checklist
Reviewer's checklist
patch,minor, ormajorfeaturesManual review test cases
iPad Menu Item click test
Descriptive Test Statement
Device review