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

Replace method to get offset in touch event #13

Merged
merged 2 commits into from Sep 18, 2021

Conversation

jaarenhoevel
Copy link
Contributor

Loop through parent nodes to get offset from top of page since Element.getBoundingClientRect() returns offset to top of screen.

That way touch events work when scrolled down.

Loop through parent nodes to get offset from top of page since Element.getBoundingClientRect() returns offset to top of screen
@andrepxx
Copy link
Owner

You are right in that the current code is not correct. (Interesting that I didn't notice that this doesn't work.)

I think there is a much simpler solution that doesn't have to walk up the DOM tree though. Shouldn't adding window.scrollX and window.scrollY to rect.left and rect.top, respectively, give the absolute coordinates of the canvas on the document?

The other option would be to use touch.clientX and touch.clientY instead of touch.pageX and touch.pageY, which would give touch coordinates relative to view port instead of document origin.

I'm gonna experiment with this a bit and see if they work as expected and which of the solutions I prefer.

@jaarenhoevel
Copy link
Contributor Author

You're right, looping through DOM tree is not a clean approach.
IMO using touch.clientX and touch.clientY would be the easiest solution that seems to work just fine.

@andrepxx andrepxx merged commit 23399e3 into andrepxx:master Sep 18, 2021
@andrepxx
Copy link
Owner

Just tested this on an Android device and previously, the knob control would not work properly when the page was scrolled down, while now it does.

Thanks a lot for spotting that issue!

Interestingly, zooming the page (pinch-zoom) and then scrolling around on it to get the knob control into the viewport (at least on Android) does not trigger this issue, which is why I was unaware of it.

The zooming / panning on Android appears to be "virtual" somehow and does not change the client bounding rectangle.

@andrepxx
Copy link
Owner

One more thing: I noticed that you used your actual (gmail) e-mail address in the committer information. Having your e-mail address somewhere in public (i. e. in the commit history of a publicly accessible repository) can lead to more spam, because e-mail addresses get crawled.

In order not to expose your e-mail address, you should, under your GitHub account settings, category "e-mails", enable the options "Keep my e-mail address private" and "Block command-line pushed that expose my email".

On the same page, GitHub will also display an e-mail address of the form 7642349+andrepxx@users.noreply.github.com (this one is mine), which is what you should configure your Git client to use as author / committer information. This way, GitHub will still be able to associate the commit with your user account / profile, but your e-mail address won't be visible in the git log. You can then amend the commit (or use rebase interactive if there are more than one) and force-push to your branch.

Now, I already merged your commit to master, and of course the merged commit won't be affected. However, if you wish me to do so, I could offer to amend the commit and replace your committer address with the "...@users.noreply.github.com" one on the master as well. However, in order to do so, you'd have to send me this address (in a comment), since I currently don't know it.

Please tell me if you wish me to do this. Otherwise, I will just keep the commit history as it is.

Kind regards.

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.

None yet

2 participants