Remove e.preventDefault from handleTouchEnd

[danbovey]

As far as I'm aware, this isn't needed. Fixes #98.

[vkbansal]

Are you sure, this is not needed? This should have been added for a reason.

[danbovey]

It works for my use case. And all tests still pass.

[vkbansal]

@danbovey I agree that it works for your use case, but it should not break for hundreds of others people who depend on this package. I hope you understand my concern.

[Seldaek]

I would also like it removed to be honest, we haven't seen any adverse effect to removing it, but maybe @bjarkig82 can tell us more as he added it in e0df3b3

[bjarkig82]

I was working on adding a feature that would trigger the context menu if the element is pressed for sertain amount of time. Have you tested this using a touch screen on a windows device? They fire the left click event if the screen is touched for half a second or so. If I remember correctly, I added this to prevent the default browser context menu to appear over the custom context menu in these cases.

Sorry about the horrible commit message habit :-)

[Seldaek]

Maybe a better solution then would be to keep track of the state of the touch action and make sure that we prevent default only if we showed a contextmenu, but if not then we let the default action occur in touchend:

Something like:

• on touchstart event set this.touchHandled = false;
• in touchstart timeout callback, set this.touchHandled=true; then call this.handleContextClick(event);
• then in touchend check if this.touchHandled === true before calling e.preventDefault()?

[bjarkig82]

I agree, that would be a more elegant solution :-)

[danbovey]

Thanks @bjarkig82, at least it does have a purpose being there right now. I've updated this PR with @Seldaek's solution 🎉

[Seldaek]

This shouldn't be static but a regular instance property if you address it with this below. i.e. just remove the static keyword and all is fine.

[Seldaek]

This should be inside the lambda function at line 57, otherwise it'll always be true not just when the touch actually triggered the "hold-to-display" functionality, and we're back at the point where preventDefault fires every time.

[Seldaek]

Thanks for updating it :) Hopefully now it's safe enough to be merged.

[Seldaek]

Woops, that's still not good I'm afraid.. To be fair the short syntax is easy to get confused by, but it should be expanded to this:

             this.touchstartTimeoutId = setTimeout(
                 () => {
                     this.handleContextClick(event);
                     this.touchHandled = true;
                 },
                 this.props.holdToDisplay
             );

[danbovey]

I wanted to, I hate comma separated stuff

[Seldaek]

But setTimeout takes two args, a callback and then a timeout (this.props.holdToDisplay). The way you fixed it now you left the timeout in the middle of the callback, so that's not gonna work :)

[danbovey]

Woops, yeah, this is why you should add curly brackets and semicolons as soon as you start doing more than one thing.

[Seldaek]

Right now you have:

            this.touchstartTimeoutId = setTimeout(
                () => {
                    this.handleContextClick(event),
                    this.props.holdToDisplay,
                    this.touchHandled = true
                }
            );

But this essentially does setTimeout(function() { ... }); without timeout so it executes instantly. The right version should be:

             this.touchstartTimeoutId = setTimeout(
                 () => {
                     this.handleContextClick(event);
                     this.touchHandled = true;
                 },
                 this.props.holdToDisplay
             );

[danbovey]

After all of this, I'm not sure it fixing the problem. Running it in Chrome Dev tools which converts clicks to touch events and it's still cancelling regular click events.

[danbovey]

Pushed these changes to my app and running it on Android Chrome = same results

[Seldaek]

There is still one issue with the PR I think, in the beginning of handleTouchstart it should set this.touchHandled = false always, otherwise as soon as you once open the context menu then touchHandled remains true always and the preventDefault always trigger from that point on.

Curious if you can try if that fixes it or not..

[danbovey]

@Seldaek that last change fixes it! The file changes make sense to me now 😃

Tested on Chrome Dev Tools and Android and it appears to work.

[Seldaek]

Nice good to hear :)