fix(picker): don't handle pointerdown for touch devices

[lehelen19]

Description

Don't handle pointerdown for touch devices from the picker's desktop controller, which applies to devices such as iPads. This fixes an issue where pointerdown would retrigger the picker to close early, causing inconsistent behavior.

For more context, see conversation here: https://cclight.slack.com/archives/CESK60MQD/p1729107196370669

Related issue(s)

• [Bug]: Picker does not open consistently on iPad devices #4841

Motivation and context

Fixes sp-picker bug on iPad devices.

How has this been tested?

Tested on Express mobile app on iPad and in Storybook on local dev environment.

• Test case 1

  1. Go to Storybook picker or action menu
  2. Open picker on iPad

• Did it pass in Desktop?

• Did it pass in Mobile?

• Did it pass in iPad?

Screenshots (if appropriate)

Types of changes

• Bug fix (non-breaking change which fixes an issue)
• New feature (non-breaking change which adds functionality)
• Breaking change (fix or feature that would cause existing functionality to change)
• Chore (minor updates related to the tooling or maintenance of the repository, does not impact compiled assets)

Checklist

• I have signed the Adobe Open Source CLA.
• My code follows the code style of this project.
• If my change required a change to the documentation, I have updated the documentation in this pull request.
• I have read the CONTRIBUTING document.
• I have added tests to cover my changes.
• All new and existing tests passed.
• I have reviewed at the Accessibility Practices for this feature, see: Aria Practices

Best practices

This repository uses conventional commit syntax for each commit message; note that the GitHub UI does not use this by default so be cautious when accepting suggested changes. Avoid the "Update branch" button on the pull request and opt instead for rebasing your branch against main.

[coveralls]

Pull Request Test Coverage Report for Build 11727934956

Details

• 1 of 1 (100.0%) changed or added relevant line in 1 file are covered.
• No unchanged relevant lines lost coverage.
• Overall coverage increased (+0.003%) to 98.189%

---

Totals
Change from base Build 11727845238:	0.003%
Covered Lines:	32313
Relevant Lines:	32735

---

💛 - Coveralls

[Rajdeepc]

Thanks for working on this! We would appreciate if you can surface up an isolated test for this touch event!

[rubencarvalho]

Here's a couple of thoughts:

• Can you reproduce the bug in an isolated environement (e.g., outside of Express, in our website or Stroybook)?
• I can't reproduce this on my side, neither using an iPad Pro on iOS 17 (BrowserStack), nor on my personal, older iOS16 iPad.
• Ideally, we would cover this with a test (a non-evergreen one).
• Maybe there are other approaches for a solution, however I can't be sure they work as I can't reproduce them, here are some ideas:

You could try logging in our cleanup section to see if preventNextToggle = 'no' is happening too late or too early on the iPad.

this.preventNextToggle = 'no';  // could we be delaying this too much?

Our current handleActivate logic is trying to prevent double toggling, but it might be too strict for touch devices. You could try to adjust this logic to always allow toggle on touch interactions:

public override handleActivate(event?: Event): void {
   
    ...

    if (event?.type === 'click' && event instanceof PointerEvent && event.pointerType === 'touch') {
        // If the event is a touch event, let us try to always trigger the toggle
        this.host.toggle();
        return;
    }

    if (event?.type === 'click' && this.open !== this.pointerdownState) {
        return;
    }
    this.host.toggle();
}

And lastly, because I lack the understanding of how events work on the iPad, you could check whether click events are being fired in addition to pointerdown and if those click events are causing early closure of the picker.

[lehelen19]

Here's a couple of thoughts:

• Can you reproduce the bug in an isolated environement (e.g., outside of Express, in our website or Stroybook)?
• I can't reproduce this on my side, neither using an iPad Pro on iOS 17 (BrowserStack), nor on my personal, older iOS16 iPad.
• Ideally, we would cover this with a test (a non-evergreen one).
• Maybe there are other approaches for a solution, however I can't be sure they work as I can't reproduce them, here are some ideas:

You could try logging in our cleanup section to see if preventNextToggle = 'no' is happening too late or too early on the iPad.

this.preventNextToggle = 'no';  // could we be delaying this too much?

Our current handleActivate logic is trying to prevent double toggling, but it might be too strict for touch devices. You could try to adjust this logic to always allow toggle on touch interactions:

public override handleActivate(event?: Event): void {
   
    ...

    if (event?.type === 'click' && event instanceof PointerEvent && event.pointerType === 'touch') {
        // If the event is a touch event, let us try to always trigger the toggle
        this.host.toggle();
        return;
    }

    if (event?.type === 'click' && this.open !== this.pointerdownState) {
        return;
    }
    this.host.toggle();
}

And lastly, because I lack the understanding of how events work on the iPad, you could check whether click events are being fired in addition to pointerdown and if those click events are causing early closure of the picker.

Yes, this is reproduced in the Storybook and in the studio example for Action Menu and Picker as mentioned before in the Slack conversation. I used an iPad 12.9" iOS 17.4.1. It only reproduces on physical devices (I was unable to reproduce on Simulator).

I will be forwarding future investigation to the SWC-Express team, if that is okay. cc @Rocss

[mizgaionutalexandru]

I've been investigating this issue and it seems like the problem resides in popover's toggle events being fired like this on iPad:

1. ToggleEvent with oldState: "closed" and newState:"open"
2. ToggleEvent with oldState: "open" and newState:"closed"
3. ToggleEvent with oldState: "closed" and newState:"open"

When the bug reproduces (I was able to reproduce it in the Simulator, on iPad v17.4, as well as the real device), only the first 2 events trigger, making the popover stay closed.
On macOS safari only the first one is triggered, which is the intended behavior for all platforms I assume.

I haven't been able to find a fix other than the one provided here by Helen, but something else I've observed is that if we do not early return in the handlePointerdown method, the click event handler doesn't trigger.

For now this seems like the only approach that fixes the existing problem without affecting current behavior caught in unit tests. Could we go forward with this, @rubencarvalho ?

@Rajdeepc , how would you write a test for this scenario considering that it's doesn't always reproduce and it's specific on iPad?

[rubencarvalho]

I've been investigating this issue and it seems like the problem resides in popover's toggle events being fired like this on iPad:

1. ToggleEvent with oldState: "closed" and newState:"open"
2. ToggleEvent with oldState: "open" and newState:"closed"
3. ToggleEvent with oldState: "closed" and newState:"open"

When the bug reproduces (I was able to reproduce it in the Simulator, on iPad v17.4, as well as the real device), only the first 2 events trigger, making the popover stay closed. On macOS safari only the first one is triggered, which is the intended behavior for all platforms I assume.

I haven't been able to find a fix other than the one provided here by Helen, but something else I've observed is that if we do not early return in the handlePointerdown method, the click event handler doesn't trigger.

For now this seems like the only approach that fixes the existing problem without affecting current behavior caught in unit tests. Could we go forward with this, @rubencarvalho ?

@Rajdeepc , how would you write a test for this scenario considering that it's doesn't always reproduce and it's specific on iPad?

Thanks for the added context.
I dove a bit deeper into this. I guess we are filtering out pointerdown events on touch devices to avoid duplicated events (pointerdown immediately followed by click) that are causing these toggling issues on iPads.
The pointerdown logic is only really useful for desktop users to enable a “drag-to-select” interaction. This lets users press on the picker dropdown, drag to an option, and release to select without requiring an extra click. Since touch devices don’t need this functionality, handling only click events for them seems reasonable. I’m good with moving forward with this solution!

What do you think, @Rajdeepc?