Skip to content
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

Fix for touchStarted triggers twice among other issues with touchStarted/mousePressed on mobile #6740

Merged
merged 4 commits into from
Jan 23, 2024

Conversation

RandomGamingDev
Copy link
Contributor

@RandomGamingDev RandomGamingDev commented Jan 14, 2024

Resolves #6739
Note: This looks like nearly the same issue with #6737. We might have to check if this sort of issue applies to any other event listeners. For now, I'll list these 2 as separate issues and pull requests since I already created the first pair.

Changes:

Adds an instance variable called touchstart to p5 and changed p5.prototype._ontouchstart & p5.prototype._onmousedown to function as documented.

Screenshots of the change:

On touchscreen

  • mousePressed and touchStarted no longer both trigger if they're both present (same for mousePressed)
  • If either is without the other it doesn't triggers twice anymore
  • touchStarted now gets called when the mouse is pressed like described in the documentation

PR Checklist

@davepagurek
Copy link
Contributor

I also mentioned in #6738, but a comment explaining the flag would help. Other than that I think this approach makes sense.

I notice there was a branch for Safari that is now removed. I assume modern Safari doesn't need it any more, but do you know what version that started in?

Also tagging events stewards, who are probably more familiar than I am with the control flow of these events: @limzykenneth, @richardegil, @angelabelle, @littlejacinthe, @TanviKumar, @tuminzee

@RandomGamingDev
Copy link
Contributor Author

I notice there was a branch for Safari that is now removed. I assume modern Safari doesn't need it any more, but do you know what version that started in?

The Safari branch wasn't functioning properly in the first place, since it breaks the the behavior described before where it's supposed to call touchStarted() if there isn't a mousePressed() on not just Safari. Also, since Safari fails to call the touch events, it'll just do what the original code looked like it was supposed to do, which was use the mousedown event which prioritizes mousePressed() and then calls touchStarted() if it can't find the prior.

If we want to introduce something that fixes Safari only calling the mouse event handlers and not the touch screen ones on certain versions, we'll have to introduce something else to try and detect whether or not it's touchscreen.

For now, I think we should just merge the pull request and check whether or not we should try doing that in another issue and/or PR.

Safari Issue: https://bugs.webkit.org/show_bug.cgi?id=105406

@RandomGamingDev
Copy link
Contributor Author

I also mentioned in #6738, but a comment explaining the flag would help. Other than that I think this approach makes sense.

I've added the comment

@Garima3110
Copy link
Member

Hello everyone, I just want to add that I did the tests for this on Safari, and it all looks good. I also tried it out on Chrome on my Android phone, and Safari on my iPhone behaved the same way. So, it seems like things are working fine on both browsers.

Copy link
Contributor

@davepagurek davepagurek left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks everyone for helping check that this works as expected!

@davepagurek davepagurek merged commit b14f25e into processing:main Jan 23, 2024
2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

touchStarted triggers twice among other issues with touchStarted/mousePressed on mobile
3 participants