JedWatson / react-tappable

Tappable component for React
http://jedwatson.github.io/react-tappable/
MIT License
863 stars 90 forks source link

onTap is fired with mouseup event #48

Closed slorber closed 7 years ago

slorber commented 9 years ago
    onMouseUp: function onMouseUp(event) {
        if (window._blockMouseEvents || !this._mouseDown) return;
        this.processEvent(event);
        this.props.onMouseUp && this.props.onMouseUp(event);
        this.props.onTap && this.props.onTap(event);
        this.endMouseEvent();
    },

onTap fired when touchStart or mouseDown is followed by touchEnd or mouseUp within the moveThreshold

This may be on purpose, but I kinda think it's weird and unexpected.

Couldn't we simply have a onClick instead? What if I want different behavior onTap if it's a touch or mouse event?

I don't expect any tap event to fire at all when the user is using a mouse.

slorber commented 9 years ago

@JedWatson I think onTap should rather never be fired for anything unrelated to mouse.

If you want to make it convenient to use in most cases, why not have a onTapOrClick for example.

Also

    onMouseUp: function onMouseUp(event) {
        if (window._blockMouseEvents || !this._mouseDown) return;
        this.processEvent(event);
        this.props.onMouseUp && this.props.onMouseUp(event);
        this.props.onTap && this.props.onTap(event);
        this.endMouseEvent();
    },

As you can see, on mouseUp, even if we call preventDefault() on the onMouseUp callback, the tap callback is still called.

Also remember the "order" of events:

touchstart
touchmove
touchend
mouseover
mousemove
mousedown
mouseup
click

I think it may be quite unexpected for a mouse event to trigger a tap event, as touch events are always fired before mouse events in any browser.

Simply making this behavior optional (no matter the default for me) would be great.

slorber commented 9 years ago

Also this makes weird things in term of event propagation.

For example, let's imagine a Tappable element that has other child elements. One of the childs has a onClick on which an action is done. Clicking on the child and calling preventDefault on that child onClick listener will still make the onTap of the parent be called, which is quite unexpected.

Also wondering why you fire onTap on mouseUp and not onClick as most people use click listeners instead of mouseUp, as it seems to be a recommendation of the W3c to use onClick in cases where both onClick and onMouseUp would lead to the same result.

slorber commented 9 years ago

Something nice would be able to disable the firing of all events like tap/pinch/press when it's triggered by a mouse action.

ferrannp commented 8 years ago

Hi, I agree... why to fire onMouseUp ? I think we have onClick for desktop and then onTap should just fire touchStart followed by touchEnd... This makes difficult to have onTap and onClick together (because we'll fire onMoseUp). Plus, it will even fire onMoseUp if I use the right button of my mouse.

@slorber do you have a workaround for this?

Edit: ok so I guess a workaround can be:

<Tappable onTap={this.handleClick} onMouseDown={function(e){return false;}}>
  <div onClick={this.handleClick}>
   ...
  </div>
</Tappable>

Any drawback with this approach?

slorber commented 8 years ago

@ferrannp we are currently running a fork of react-tappable where I commented the call // this.props.onTap && this.props.onTap(event);

dcousens commented 7 years ago

Changing onTap behaviour to not support mouseup would be too big a breaking change, and besides, for many, the ambiguity is why this library is useful.