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 pan #47

Closed
wants to merge 4 commits into from
Closed

fix pan #47

wants to merge 4 commits into from

Conversation

GuoSirius
Copy link

fix #46

@Fuzzyma
Copy link
Member

Fuzzyma commented Oct 31, 2019

Sorry for coming back to this so late. I was busy with my diploma.
Help me out here. Your linked issue is in chinese. So what is the problem exactly what you solved here?

@dotnetCarpenter
Copy link
Member

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.
https://developer.mozilla.org/en-US/docs/Web/API/Touch_events/Supporting_both_TouchEvent_and_MouseEvent

@Fuzzyma
Copy link
Member

Fuzzyma commented Nov 2, 2019

So we can close this?

@dotnetCarpenter
Copy link
Member

@Fuzzyma Not sure.I ran #46 through google translate and @GuoSirius says:

The phenomenon is: when using panZoom directly on the root svg element, the mouse device zoom will not be offset, and the touch screen device offset is very serious.

The fixed code has been submitted, pull request.

So I guess this PR fixes an issue where the touch screen device offset is wrong.
@GuoSirius has posted a picture which highlights:

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, translate is wrong. So it's a needle in a haystack problem, without knowing what the needle is. 😞

@GuoSirius Can you tell us in which browser and touch device you see the issue so we can replicate your issue please?
And please do not post images of code. Use markdown code block instead.

```js
// javascript code here...
```

@dotnetCarpenter dotnetCarpenter added the needs more info Issue requires more information from poster label Nov 4, 2019
@Fuzzyma
Copy link
Member

Fuzzyma commented Nov 4, 2019

Thanks @dotnetCarpenter to dig into that. @GuoSirius we need your help here :)

@GuoSirius
Copy link
Author

Thanks @dotnetCarpenter to dig into that. @GuoSirius we need your help here :)

@Fuzzyma @dotnetCarpenter My English is not very good :

Android 7.x
Chrome 76.x
TouchDevice

AND

Windows 10
Chrome 76.x
MouseDevice

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 })
    }

@Fuzzyma
Copy link
Member

Fuzzyma commented Nov 6, 2019

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).
The reason why this is done is because of sideeffects with scrolling on mobile sites. It is really ugly to catch the touchevent for your own code when the user just wants to scroll down the page

@GuoSirius
Copy link
Author

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).
The reason why this is done is because of sideeffects with scrolling on mobile sites. It is really ugly to catch the touchevent for your own code when the user just wants to scroll down the page

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.

@Fuzzyma
Copy link
Member

Fuzzyma commented Nov 6, 2019

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...

@dotnetCarpenter
Copy link
Member

@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.

@dotnetCarpenter
Copy link
Member

@Fuzzyma I did see exaggerated zooming on macbook touchpads. I think it is because of zoomFactor.
I added zoomFactor after reading through svg-pan-zoom which at the time was a constant. I now see that it's changed to a configurable variable.
Perhaps we can just get rid of zoomFactor?

@dotnetCarpenter
Copy link
Member

@msurguy I see that you have +1 this PR. Could you chime in with your use-case/issue that this PR is solving?

@Fuzzyma
Copy link
Member

Fuzzyma commented Nov 6, 2019

Isn't zoomFactor only for the scroll wheel? There shouldn't be anything like that for pinchzoom

@dotnetCarpenter
Copy link
Member

@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 mousedown also gets triggered on touch devices after touchstart? I don't see pinchZoomStart removing the mousedown listener. But if (this.dispatch('pinchZoomStart', { event: ev }).defaultPrevented) { return } seems to be there so the user can manually stop zoom but it's not clear from the documentation and I'm not sure that it will work.

I also just saw that there is another PR which depends on zoomFactor - #45

@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?

@Fuzzyma
Copy link
Member

Fuzzyma commented Nov 6, 2019

Well I only tested this on my android phone were it was working. So could be, that there are problems with ios devices.
The default prevented stuff is exactly for the case you wrote and ofc it works (in supported browsers at least ^^)

@Fuzzyma
Copy link
Member

Fuzzyma commented Feb 19, 2020

Alright, I would like to have this in the next release but I have no idea where we ended up here :D.

@dotnetCarpenter
Copy link
Member

I think we need an example with this patch, so we can check on various devices. Can we use codepen.io / jsfiddle or similar?

@Fuzzyma
Copy link
Member

Fuzzyma commented Feb 20, 2020

I would prefer codepen for this: https://codepen.io/fuzzyma/full/gOpwyzq

@Fuzzyma
Copy link
Member

Fuzzyma commented Feb 20, 2020

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

@Fuzzyma
Copy link
Member

Fuzzyma commented Feb 21, 2020

@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?

Fuzzyma added a commit that referenced this pull request Feb 22, 2020
@Fuzzyma
Copy link
Member

Fuzzyma commented Feb 22, 2020

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

@Fuzzyma
Copy link
Member

Fuzzyma commented Feb 22, 2020

Maybe we should make it possible though as an option. Just to allow users to operate more freely with this plug in.

@snowyu
Copy link

snowyu commented Mar 14, 2020

Yes, pan with 1 finger on small screen is more useful.

@Fuzzyma
Copy link
Member

Fuzzyma commented Mar 14, 2020

@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
Copy link

snowyu commented Mar 15, 2020

@Fuzzyma

  • Small Device: small device

  • Big hand: Big Hand

@Fuzzyma
Copy link
Member

Fuzzyma commented Mar 15, 2020

@snowyu in such a case any application with scroll AND pan with one finger is doomed anyway :D

@snowyu
Copy link

snowyu commented Mar 17, 2020

@Fuzzyma Never give up anybody or any device 😃

@Fuzzyma Fuzzyma closed this Sep 15, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs more info Issue requires more information from poster
Projects
None yet
Development

Successfully merging this pull request may close these issues.

pan and pinch zoom has BUG in touch device,不能移动,缩放有误差
4 participants