minwork / use-long-press

React hook for detecting click (or tap) and hold event
MIT License
120 stars 12 forks source link

Safari iOS: `onCancel` with `canceled-by-timeout` called after the callback #45

Closed powelski closed 1 year ago

powelski commented 1 year ago

I'm using this library and on desktop it works fine, but in Safari iOS it triggers the callback normally and then it fires onCancel (reason: canceled-by-timeout) if I don't hold for a long time, above 1s. I'm using default threshold of 400.

Not sure why that happens.

powelski commented 1 year ago

I tested it and it looks like it's detect: "both" doing that. I thought the library would prevent calling both touch and mouse callbacks at the same event (touch event wouldn't allow for mouse event), but it's not the case.

powelski commented 1 year ago

For now I solved it by setting using a boolean ref value:

const doubleLongPressLock = useRef(false);
const longPressBind = useLongPress(
    event => {
        // My code
    },
    {
        detect: LongPressDetectEvents.BOTH,
        onStart: event => {
            if (event.type === "touchstart") {
                doubleLongPressLock.current = true;
            }
        },
        onCancel: event => {
            if (event.type === "mouseup" && doubleLongPressLock.current) {
                doubleLongPressLock.current = false;
                return;
            }

            // My code
        },
    }
);
minwork commented 1 year ago

Hey, it seems it is related to #5. Thanks for including piece of the code of how are you handling it. I will try to address this case in v3.

powelski commented 1 year ago

Hi @minwork for the record, I found an easier approach - I called event.preventDefault() in onCancel and onFinish. It seems to be doing fine, I haven't found it causing any broken native behavior. It makes touch events skip the mouse events.

Would be nice if the library made sure that mouse events are not called when detect: "touch" is set.

minwork commented 1 year ago

Good point, also I will probably drop support for detect: "both" since it causes unnecessary confusion and edge cases.

ascrazy commented 1 year ago

Good point, also I will probably drop support for detect: "both" since it causes unnecessary confusion and edge cases.

I've seen your suggestion to use touch screen detection to decide which events to listen to

const bind = useLongPress(() => console.log('Long pressed'), {
  detect: isTouchDevice ? 'touch' : 'mouse',
})

Though I don't quite get if there's going to be a viable workaround for laptops with touchscreen (there seem to be plenty on the market) or tablets with a mouse (Microsoft Surface, iPad with magic keyboard and a mouse)?

minwork commented 1 year ago

Based on my knowledge often touch also simulates click which caused problems when 'both' option was set because it triggered both mouse and touch events in an unpredictable manner (see #5). Therefore using detect: 'mouse' should be enough for those type of devices and shouldn't trigger edge cases, but it needs to be verified.

Also I plan to add support for PointerEvents which according to MDN addresses this particular problem:

Much of today's web content assumes the user's pointing device will be a mouse. However, since many devices support other types of pointing input devices, such as pen/stylus and touch surfaces, extensions to the existing pointing device event models are needed. Pointer events address that need.

minwork commented 1 year ago

@powelski @ascrazy FYI I've added Pointer Events implementation in use-long-press@3.0.0-rc.1 so you can try it out before it goes live.

ascrazy commented 1 year ago

Hi @minwork,

Thank you for the quick feedback. I can confirm detect=pointer from 3.0.0-rc.1 seem to be working fine across devices I have at my hand

minwork commented 1 year ago

Version 3 which addresses this issue is now available at use-long-press@latest