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

Bug while calling e.preventDefault #46

Closed
slorber opened this issue Jul 21, 2015 · 7 comments
Closed

Bug while calling e.preventDefault #46

slorber opened this issue Jul 21, 2015 · 7 comments

Comments

@slorber
Copy link
Contributor

slorber commented Jul 21, 2015

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
Copy link
Contributor Author

slorber commented Jul 21, 2015

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
Copy link
Contributor Author

slorber commented Jul 21, 2015

See PR #47

@slorber
Copy link
Contributor Author

slorber commented Jul 21, 2015

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
Copy link
Contributor Author

slorber commented Jul 21, 2015

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
Copy link
Contributor Author

slorber commented Jul 21, 2015

@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 facebook/react#3389 (comment))

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

@slorber
Copy link
Contributor Author

slorber commented Jul 28, 2015

@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
Copy link
Owner

@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.

@slorber slorber closed this as completed Dec 6, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants