JedWatson / react-tappable

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

Bug while calling e.preventDefault #46

Closed slorber closed 7 years ago

slorber commented 9 years ago

I have this: <Tappable onTap={this.onTap}>{element}</Tappable>;

When my listener is:

    onTap: function(e) {
        console.error("tap!",e);
        e.preventDefault();
        this.props.onTap(e);
    },

I get an error:

tap! SyntheticTouchEvent {dispatchConfig: null, dispatchMarker: null, nativeEvent: null, type: null, target: null…} touchable.jsx:22
Uncaught TypeError: Cannot read property 'preventDefault' of null 

It is not the e that is null, but calling e.preventDefault(); calls this internal SyntheticTouchEvent function:

  preventDefault: function() {
    this.defaultPrevented = true;
    var event = this.nativeEvent;
    if (event.preventDefault) {
      event.preventDefault();
    } else {
      event.returnValue = false;
    }
    this.isDefaultPrevented = emptyFunction.thatReturnsTrue;
  },

This is related to how React use pooling on Synthetic events and because onTap is called asynchronously un the setState callback

slorber commented 9 years ago

The React Synthetic events loose their native events while handled in async callbacks because I think they are resetted and put in the pool.

    endTouch: function endTouch(event, callback) {
        this.cancelPressDetection();
        this.clearActiveTimeout();
        if (event && this.props.onTouchEnd) {
            this.props.onTouchEnd(event);
        }
        this._initialTouch = null;
        this._lastTouch = null;
        if (this.state.isActive) {
            this.setState({
                isActive: false
            }, callback);
        } else if (callback) {
            callback();
        }
    },

The problem is because the callback of setState() is called asynchronously.

This works fine for me:

    endTouch: function endTouch(event, callback) {
        this.cancelPressDetection();
        this.clearActiveTimeout();
        if (event && this.props.onTouchEnd) {
            this.props.onTouchEnd(event);
        }
        this._initialTouch = null;
        this._lastTouch = null;
        if (this.state.isActive) {
            callback();
            this.setState({
                isActive: false
            });
        } else if (callback) {
            callback();
        }
    },
slorber commented 9 years ago

See PR https://github.com/JedWatson/react-tappable/pull/47

slorber commented 9 years ago

Also I think it should be documented that preventDefault is already called on the event before the onTap is called so it means I don't need to call it myself

slorber commented 9 years ago

A temporary workaround is to access the nativeEvent through the "onTouchEnd" callback which is called synchronously

So if you want to stop propagation of the touchend event in onTap (preventDefault is done, but not stopPropagation), instead you can do:

<Tappable onTap={this.props.onTap} onTouchEnd={function(e) { e.stopPropagation() }}>{element}</Tappable>;

I think this should be possible to handle directly in onTap(e) and this is what my PR solves

slorber commented 9 years ago

@JedWatson notice that when using React.Perf I'm also having an error in Perf code that only appears when using onTap of tappable (see https://github.com/facebook/react/issues/3389#issuecomment-123347416)

Surprisingly, my PR also solves this problem (but I absolutly don't know why)

slorber commented 9 years ago

@JedWatson btw another possible fix if you still want to use the setState callback (not sure it's a good idea however) you can use event.persist()

JedWatson commented 9 years ago

@slorber this should be resolved now that your PR has been merged, let me know if you can still reproduce any issues relating to it.