-
Notifications
You must be signed in to change notification settings - Fork 3.5k
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
Avoid transitioning from pinch until all fingers have been released. #6479
Conversation
@puckey, thanks for the pull request! Maintainers, we have a signed CLA from @puckey, so you can review this at any time.
I am a bot who helps you make Cesium awesome! Contributions to my configuration are welcome. 🌍 🌎 🌏 |
I do, sorry for the delay. I need to carve out a bit of time to try this on a touchscreen. |
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.
Thanks @puckey, this is certainly better behavior for the user. You've got one failing unit test, and I'm curious which strategy you'll take in fixing it. Also can you merge master into here when you get a chance? Thanks!
pinchEndAction.calls.reset(); | ||
|
||
// Putting another finger down should not trigger | ||
// PINCH_START: |
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.
This test fails, and indeed PINCH_START
is re-called when the user's touch count deviates from 2 (while remaining non-zero) and returns to 2, bringing pinch out of suspension. I'm not sure this is a bad thing, as the pinch is re-starting even though it never fully stopped.
Anyway the test should be fixed, not sure if by changing the test's expectation or by suppressing the pinch re-start event.
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.
Thanks – I will look into this next week!
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.
Since we aren't firing PINCH_END with a single touch up, it indeed doesn't make sense to fire PINCH_START when moving back to 2 fingers down.
I added a check to see if we are already pinching when touch count returns to 0.. All tests should be green and I merged in master.
Thanks @puckey. I will test on a touchscreen soon. With the latest changes I'd like to make sure that if I have two fingers down, and I lift one finger and place it down somewhere new, I don't get a sudden jump as if I had moved the finger from one place to the other. That my concern with skipping the intermediate PINCH_START event, but I'm not sure if it will happen. |
Yes, I had the same concern.. I built the demo to check this on my iPhone and it seemed fine – but another check is always a good idea! |
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.
Code looks good, tests pass, behavior seems nice on a touchscreen. Great work @puckey.
Updated CHANGES.md in a32d774. |
This fixes issue #6478: