-
Notifications
You must be signed in to change notification settings - Fork 54
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 pan #47
fix pan #47
Conversation
Sorry for coming back to this so late. I was busy with my diploma. |
This PR test if the user device is a touch device and if it is, it then listens on the touch-* event handlers instead of the mouse-* event handlers. Since touch devices listens to both, I think this is just complicating something that already works fine. |
So we can close this? |
@Fuzzyma Not sure.I ran #46 through google translate and @GuoSirius says:
So I guess this PR fixes an issue where the touch screen device offset is wrong. const box = new Box(this.viewbox()).transform(
new Matrix()
.translate(p.x, p.y)
.scale(zoomAmount, 0, 0)
.translate(-focusP.x, -focusP.y)
)
this.viewbox(box) But that is exactly what is we have now. Unfortunately @GuoSirius does not mention in which browser or platform, @GuoSirius Can you tell us in which browser and touch device you see the issue so we can replicate your issue please? ```js // javascript code here... ``` |
Thanks @dotnetCarpenter to dig into that. @GuoSirius we need your help here :) |
@Fuzzyma @dotnetCarpenter My English is not very good : Android 7.x AND Windows 10 import TouchEmulator from 'hammer-touchemulator'
// If the mouseevent has the shiftKey property to true, it enables multi-touch
TouchEmulator() The above code is used to simulate touch screen operation. First, the single-finger translation is normally operated with the mouse at present, but the touch screen device cannot be translated. There is a modification in the Pull Request I submitted again, which is available now. Second, mouse wheel zooming is normal, but with double finger zooming, the current deviation is very serious, a little operation, you can't see the content, the code positioning is as follows: const pinchZoom = function (ev) {
ev.preventDefault()
const currentTouches = normalizeEvent(ev)
const zoom = this.zoom()
// Distance Formula
const lastDelta = Math.sqrt(
Math.pow(lastTouches[0].clientX - lastTouches[1].clientX, 2)
+ Math.pow(lastTouches[0].clientY - lastTouches[1].clientY, 2)
)
const currentDelta = Math.sqrt(
Math.pow(currentTouches[0].clientX - currentTouches[1].clientX, 2)
+ Math.pow(currentTouches[0].clientY - currentTouches[1].clientY, 2)
)
let zoomAmount = lastDelta / currentDelta
if ((zoom < zoomMin && zoomAmount > 1) || (zoom > zoomMax && zoomAmount < 1)) {
zoomAmount = 1
}
const currentFocus = {
x: currentTouches[0].clientX + 0.5 * (currentTouches[1].clientX - currentTouches[0].clientX),
y: currentTouches[0].clientY + 0.5 * (currentTouches[1].clientY - currentTouches[0].clientY)
}
const lastFocus = {
x: lastTouches[0].clientX + 0.5 * (lastTouches[1].clientX - lastTouches[0].clientX),
y: lastTouches[0].clientY + 0.5 * (lastTouches[1].clientY - lastTouches[0].clientY)
}
// start
const p = this.point(currentFocus.x, currentFocus.y)
const focusP = this.point(2 * currentFocus.x - lastFocus.x, 2 * currentFocus.y - lastFocus.y)
const box = new Box(this.viewbox()).transform(
new Matrix()
.translate(p.x, p.y)
.scale(zoomAmount, 0, 0)
.translate(-focusP.x, -focusP.y)
)
this.viewbox(box)
// end
lastTouches = currentTouches
this.dispatch('zoom', { box: box, focus: focusP })
} |
As far as I understand you @GuoSirius you want to be able to pan with only one finger. However, this is not an intended behavior. Instead you can pan with two fingers (this worked last time I tested it). |
Normally, single finger is used for translation and multi-finger is used for scaling, but now single finger cannot be translated, and multi-finger scaling drift is very serious. |
No, we dont want single finger for translation. Thats bad behavior in my opinion. Single finger is for native scroll. We dont want to touch that. Where do you have scaling drift? It never happened to me... |
@GuoSirius Could you create an online demonstration where we can see that multi-finger scaling drift is very serious? Then I can test on my android tablet. You can use jsfiddle, codepen or CodeSandbox to host your demonstration. Thank you for using markdown code blocks instead of images. Much appreciated. Also next time you want to update this Pull Request (PR), please create new commits and push to your branch. That way, we can see each change you make, in relation to our conversation. |
@Fuzzyma I did see exaggerated zooming on macbook touchpads. I think it is because of |
@msurguy I see that you have +1 this PR. Could you chime in with your use-case/issue that this PR is solving? |
Isn't zoomFactor only for the scroll wheel? There shouldn't be anything like that for pinchzoom |
@Fuzzyma You're right. It's been so long since I have looked at this, that I keep writing wrong stuff. Sorry... wait.. Doesn't I also just saw that there is another PR which depends on @GuoSirius Never mind the git commit - I see that you are already doing that. But you have added a copy of svg.panzoom.js to the root directory. That must be a mistake? |
Well I only tested this on my android phone were it was working. So could be, that there are problems with ios devices. |
Alright, I would like to have this in the next release but I have no idea where we ended up here :D. |
I think we need an example with this patch, so we can check on various devices. Can we use codepen.io / jsfiddle or similar? |
I would prefer codepen for this: https://codepen.io/fuzzyma/full/gOpwyzq |
But I am not sure how we can use the code in the PR in a codepen because its an esm module and using that on codepen is rather not possible as far as I know |
@dotnetCarpenter I compiled the PR locally and loaded it into a codepen here: https://codepen.io/fuzzyma/pen/poJEXMm Can we work with this testcase? |
After testing on my own device I found the issue @GuoSirius was talking about. This is a regression bug because it worked with old svg.js v2. I pushed a fix for that. The zoom with 2 fingers should work correctly now. You can also pan with 2 fingers. Panning with one finger however is still not possible and never was |
Maybe we should make it possible though as an option. Just to allow users to operate more freely with this plug in. |
Yes, pan with 1 finger on small screen is more useful. |
@snowyu in some applications where you have no scroll (google maps like) it is useful to pan with one finger. Its not neccecarily good on small screens. Imo you should never have one finger pan enabled when you also have scroll on that page |
@snowyu in such a case any application with scroll AND pan with one finger is doomed anyway :D |
@Fuzzyma Never give up anybody or any device 😃 |
fix #46