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

Can't cleanly pass touch/click events to panzoom children #473

Closed
michaeljonathanblack opened this issue Apr 7, 2020 · 8 comments
Closed
Labels

Comments

@michaeljonathanblack
Copy link

Describe the bug

On latest devices, e.g. desktop Chrome or iOS Safari on iPhone X, click events to the panzoom element are still passed down to children, and we can add "isZooming" detection code using the panzoom events to determine if we want to handle the click events or not, and everything's great.

Unfortuantely, in older devices, e.g. iPhone SE iOS v12.4 on Safari, I'm seeing the click events are eaten by event.preventDefault(). This can be avoided using the excludeClass, however we have so many clickable children that it breaks the pinch/zoom behavior more often than not.

I'm not quite sure what pattern to use when handling many clickable children, or if there's a different library that makes more sense for our use-case.

Your environment

  • Version of panzoom
    4.1.x
  • Browser and browser version
    Various

Expected behavior

Pinching to zoom doesn't click child elements and doesn't behave differently if child elements are under part of the gesture.

Clicking on child elements should trigger their click events.

Actual behavior

Pinching to zoom behaves erratically with excludeChildren on since there are so many clickable elements.

Child elements are unfortunately only clickable with this enabled on older devices.

@michaeljonathanblack
Copy link
Author

michaeljonathanblack commented Apr 8, 2020

Interesting, digging into panzoom a little, I can see you're managing three different sets of events:


    var events;
    if (typeof window.PointerEvent === 'function') {
        events = {
            down: 'pointerdown',
            move: 'pointermove',
            up: 'pointerup pointerleave pointercancel'
        };
    }
    else if (typeof window.TouchEvent === 'function') {
        events = {
            down: 'touchstart',
            move: 'touchmove',
            up: 'touchend touchcancel'
        };
    }
    else {
        events = {
            down: 'mousedown',
            move: 'mousemove',
            up: 'mouseup mouseleave'
        };
    }

And my guess is that event.preventDefault() for pointerdown doesn't affect click, whereas touchstart on an older device would, and that's why we're seeing different behavior.

My temptation is to use this same block used above to change what type of event we're listening for so the propagation is the same, but that seems a little fragile.

I'd prefer that panzoom would optionally pass click-like events down as clicks to its children, perhaps? I could try creating a PR around that, if that makes sense.

e.g. Panzoom({ propagateClicks: true })

@timmywil
Copy link
Owner

timmywil commented Apr 8, 2020

Thanks for opening an issue. I don't think it's possible to pass the events back down to children in any clean way. The event has already propagated from the children as events bubble up by nature. However, your first instinct of binding to the same event as panzoom is probably the right way to go. But instead of copying that block, I could expose that events variable and then you can use event.stopImmediatePropagation.

@timmywil
Copy link
Owner

timmywil commented Apr 8, 2020

The events have been exposed on master so you don't need to copy the block.

I realized that if you want to allow dragging without calling preventDefault, you can use the handleStartEvent option as well. The event is passed to that function and the type is available at event.type.

@michaeljonathanblack
Copy link
Author

michaeljonathanblack commented Apr 8, 2020

Wow, thanks for the quick turn-around @timmywil! I'll experiment with using the same events.

@michaeljonathanblack
Copy link
Author

michaeljonathanblack commented Apr 8, 2020

It is clear to me after a little testing that I don't want to be calling event.stopImmediatePropagation as I want panzoom to pan and zoom correctly, irrespective of elements that I also want to be clickable.

Any element within the panzoom canvas that stops propagation (e.g. excludeClass) results in unwanted behaviors when a user pinches partially or entirely within those elements, either stopping or snapping the pan.

And if they start swiping on top of those elements to pan, they won't pan at all—it's not a good user experience.

@michaeljonathanblack
Copy link
Author

Ah, I finally figured it out! Sorry for all the rubber ducking in the comments.

Parent calls to event.preventDefault() affecting child events was really confusing me, until it occurred to me that touchstart is an event before click and can therefore prevent it. And that's why lining up my events with panzoom will work, since I'll be using the supported events and won't run into side-effects by way of using click everywhere.

Thanks again for the help.

@timmywil
Copy link
Owner

timmywil commented Apr 8, 2020

Glad you figured it out!

mattnathan pushed a commit to vanti-dev/panzoom that referenced this issue Jun 17, 2020
timmywil added a commit that referenced this issue Jul 7, 2020
# [4.2.0](4.1.0...4.2.0) (2020-07-07)

### Features

* **events:** expose event names used in the current browser ([3e0dfac](3e0dfac)), closes [#473](#473)
* **options:** add touchAction option for setting the touch-action style ([ae70878](ae70878)), closes [#494](#494) [#492](#492)
@timmywil
Copy link
Owner

timmywil commented Jul 7, 2020

🎉 This issue has been resolved in version 4.2.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

timmywil added a commit that referenced this issue Aug 6, 2020
# [4.2.0](4.1.0...4.2.0) (2020-07-07)

### Features

* **events:** expose event names used in the current browser ([3e0dfac](3e0dfac)), closes [#473](#473)
* **options:** add touchAction option for setting the touch-action style ([ae70878](ae70878)), closes [#494](#494) [#492](#492)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

2 participants