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

implement more intuitive zoom gesture behaviour #284

Merged

Conversation

Pascal-So
Copy link
Contributor

@Pascal-So Pascal-So commented May 4, 2024

fixes #283

This works nicely in chrome on android, here's a comparison between the old behaviour on the left and the version of this PR on the right:

chrome-old-new.webm

Note how the two touch locations now remain pinned to the underlying image locations.

Unfortunately this doesn't work entirely yet, since the behaviour in Firefox on android is now more annoying (this is fixed in a later commit, check the update below):

ff-new.webm

The problem here lies with the onpointermove events. When both pointers are moving simultaneously, it can happen that firefox only sends updates for one of the pointers for a while, which means that when the other pointer finally does get an update, it arrives as a large instantaneous jump. Chrome apparently sends a more even rate of updates. Note that I've only tested this on my android phone, I don't know what the behaviour on other browsers on other operating systems would be, but I could imagine a similar problem arising for more setups than just firefox on android.

I saw that the manuelstofer/pinch-zoom library listens to touchmove events rather than pointermove events, and when testing that library the zoom appeared smooth in all browsers. I'm guessing that the update rate is more consistent for touch events.

Since I believe that the experience on firefox is now worse than it was before this PR, I'm only sending the PR as a draft to see what you think about this and to then possibly later switch to touch events depending on what comes out of our discussion.

Feel free to ask if anything is unclear!

Copy link

stackblitz bot commented May 4, 2024

Review PR in StackBlitz Codeflow Run & review this pull request in StackBlitz Codeflow.

Copy link

netlify bot commented May 4, 2024

Deploy Preview for zoom-image-library ready!

Name Link
🔨 Latest commit 8d2ecc1
🔍 Latest deploy log https://app.netlify.com/sites/zoom-image-library/deploys/663d0c872e5cb900087f2617
😎 Deploy Preview https://deploy-preview-284--zoom-image-library.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

@Pascal-So
Copy link
Contributor Author

Pascal-So commented May 9, 2024

I just tried using touchmove instead of pointermove for the two-finger movements and the zoom is now just as smooth in mobile firefox as it is in mobile chrome:

ff-and-chrome-mostly-working.webm

One remaining problem is when a finger is removed and the other continues to move the image around. There we get a sudden jump.

remove-finger-jump.webm

To remedy this, I am tempted to completely separate the mouse and touch handling, i.e. getting rid of the pointer* events. The two-input case is only relevant for touch anyway, as you can't have two mouse inputs. For the one-input case, such edge cases might be easier to handle if the logic for the touch case is separate. I see that the double-tap to zoom functionality is already in a touchstart handler anyway, so there is precedent for this.

What do you think about how we should approach this?

@willnguyen1312
Copy link
Owner

willnguyen1312 commented May 9, 2024

Hi Pascal, I think your code looks great. Sorry from my end that I have never tested on Android device since I'm a Mac and iOS users. Hence, I missed the part that Firefox on Android doesn't work nicely 😅

One remaining problem is when a finger is removed and the other continues to move the image around. There we get a sudden jump

#287 should get this off your shoulder 😉 Please go ahead and get your code working as expected with the main branch. I can't wait to see what's coming out from you soon. The current zooming on mobile feels fantastic, thanks so much 🙂

@willnguyen1312
Copy link
Owner

With regard to converting pointer events to mouse and touch events in order to support browsers across OSs. I'm happy with this approach as long as it solves the challenges we are facing. The reason I chose pointer event in the first place because it's nicer to unify experience instead of having to switch context back and forth between mouse and touch devices 😅 Unfortunately, I missed the Firefox on Android part as mentioned above 😅

@Pascal-So
Copy link
Contributor Author

Pascal-So commented May 9, 2024

Thank you for the nice feedback! Yeah the way that Firefox sends lower update rates on pointer events than on touch events wasn't an issue before because it was kinda hidden by the fixed delta zooming, but now with the smooth approach it does make a difference 😅

I merged the current main branch in to my branch, thanks for fixing the jumping issue when letting go of one finger! Note that I didn't change the rest of all the pointer events to touch&mouse events now since you already provided a unified way of handling the jumping issue.

In my view this is now ready for review and merge

@Pascal-So Pascal-So marked this pull request as ready for review May 9, 2024 17:14
@willnguyen1312
Copy link
Owner

willnguyen1312 commented May 9, 2024

Hi Pascal, I tested on my device based on this link deployed preview from your code. It looks awesome. I just had one minor thing to ask for. Can you add changeset to your PR by running pnpm changeset command and input your change as below before we merge in? Thanks :)

image

@willnguyen1312 willnguyen1312 self-assigned this May 9, 2024
@Pascal-So
Copy link
Contributor Author

done, is the changeset okay like this?

Copy link
Owner

@willnguyen1312 willnguyen1312 left a comment

Choose a reason for hiding this comment

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

Thanks so much for your contribution, Pascal. I learned a lot from this PR ❤️

@willnguyen1312 willnguyen1312 merged commit 3688890 into willnguyen1312:main May 9, 2024
7 checks passed
@github-actions github-actions bot mentioned this pull request May 9, 2024
@Pascal-So
Copy link
Contributor Author

thanks for merging!

also, i just found a small detail that I want to improve about the double tap zoom behaviour, so don't spend too much effort on releasing everything yet 😅. PR incoming in a few minutes 😁

@willnguyen1312
Copy link
Owner

willnguyen1312 commented May 9, 2024

No worries. It's automatically executed by Github action. It's just a matter of few clicks from my side. The reason I asked for changset because it will collect you as contributor in the Changelog 😊

@Pascal-So
Copy link
Contributor Author

okay it's not gonna be today, looks a bit more tricky than I expected :D

@willnguyen1312
Copy link
Owner

Take your time buddy 😊 I noticed it's rather late in Zurich. Thanks for your meaningful collaboration, good night and catch you later 😊

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.

Mobile zoom too sensitive?
2 participants