Skip to content

Conversation

@Rajdeepc
Copy link
Contributor

@Rajdeepc Rajdeepc commented Oct 27, 2025

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

  • On iPad (and other tablets):
    • Direct menu items inside <sp-action-menu> don't fire click events
    • pointerdown and pointerup events fire correctly
    • Works fine when menu items are wrapped in <sp-menu-group>
  • On desktop and mobile phones:
    • Everything works as expected

Root cause

The issue stemmed from an incorrect device detection strategy that didn't properly account for tablet devices with touch input.

Technical details

  1. Device detection query

    // Old IS_MOBILE definition
    const IS_MOBILE =
        '(max-width: 743px) and (hover: none) and (pointer: coarse)';

    This query only matched devices with:

    • Screen width ≤ 743px AND
    • Touch input (hover: none, pointer: coarse)

    iPad devices don't match because their screen width is typically 768px-1024px.

  2. The shouldSupportDragAndSelect flag

    In packages/picker/src/InteractionController.ts, the code set:

    this.host.optionsMenu.shouldSupportDragAndSelect =
        !this.host.isMobile.matches;

    Since iPad didn't match IS_MOBILE, shouldSupportDragAndSelect was set to true.

  3. Event handling conflict

    In packages/menu/src/Menu.ts:

    private handlePointerup(event: Event): void {
        this.handleTouchEnd();
    
        if (!this.shouldSupportDragAndSelect) {
            return; // Selection handled by click event
        }
        this.pointerUpTarget = event.target;
        this.handlePointerBasedSelection(event);
    }
    
    private handleClick(event: Event): void {
        if (this.pointerUpTarget === event.target) {
            this.pointerUpTarget = null;
            return; // EARLY RETURN - Click is ignored!
        }
        this.handlePointerBasedSelection(event);
    }

    When shouldSupportDragAndSelect = true:

    • handlePointerup processes the selection and sets pointerUpTarget
    • handleClick sees that pointerUpTarget === event.target and returns early
    • The click event is never processed!
  4. Why menu-group worked

    When menu items are wrapped in <sp-menu-group>, the additional DOM wrapper causes the event.target in the click handler to sometimes differ from the pointerUpTarget set in the pointerup handler, allowing the click to proceed.

Solution

We introduced a new IS_TOUCH_DEVICE media query that detects any device with touch input, regardless of screen size:

export const IS_TOUCH_DEVICE = '(hover: none) and (pointer: coarse)';

This query matches:

  • Mobile phones (screen width ≤ 743px)
  • Tablets like iPad (screen width 744px-1366px)
  • Any other device with touch input

The PickerBase class now includes an isTouchDevice property:

public isTouchDevice = new MatchMediaController(this, IS_TOUCH_DEVICE);

The menu's shouldSupportDragAndSelect property is set directly in the template when the menu is rendered:

protected get renderMenu(): TemplateResult {
    const menu = html`
        <sp-menu
            ...
            .shouldSupportDragAndSelect=${!this.isTouchDevice.matches}
            ...
        >
            <slot></slot>
        </sp-menu>
    `;
    ...
}

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

  • I have read the CONTRIBUTING and PULL_REQUESTS documents.
  • I have reviewed at the Accessibility Practices for this feature, see: Aria Practices
  • I have added automated tests to cover my changes.
  • I have included a well-written changeset if my change needs to be published.
  • I have included updated documentation if my change required it.

Reviewer's checklist

  • Includes a Github Issue with appropriate flag or Jira ticket number without a link
  • Includes thoughtfully written changeset if changes suggested include patch, minor, or major features
  • Automated tests cover all use cases and follow best practices for writing
  • Validated on all supported browsers
  • All VRTs are approved before the author can update Golden Hash

Manual review test cases

Device review

  • Did it pass in Desktop?
  • Did it pass in (emulated) Mobile?
  • Did it pass in (emulated) iPad?

@changeset-bot
Copy link

changeset-bot bot commented Oct 27, 2025

⚠️ No Changeset found

Latest commit: 4110d45

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

@github-actions
Copy link
Contributor

📚 Branch Preview

🔍 Visual Regression Test Results

When 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: pr-5829

If the changes are expected, update the current_golden_images_cache hash in the circleci config to accept the new images. Instructions are included in that file.
If the changes are unexpected, you can investigate the cause of the differences and update the code accordingly.

@github-actions
Copy link
Contributor

Tachometer results

Currently, no packages are changed by this PR...

@coveralls
Copy link
Collaborator

Pull Request Test Coverage Report for Build 18830708544

Details

  • 4 of 4 (100.0%) changed or added relevant lines in 2 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage increased (+0.002%) to 97.966%

Totals Coverage Status
Change from base Build 18789962718: 0.002%
Covered Lines: 34244
Relevant Lines: 34773

💛 - Coveralls

@Rajdeepc Rajdeepc added the Status: WIP PR is a work in progress or draft label Oct 27, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Status: WIP PR is a work in progress or draft

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Bug]: Menu items in action menus do not dispatch click events on tablets

3 participants