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

Fix double-firing submit events #12877

Merged
merged 1 commit into from
May 22, 2018
Merged

Fix double-firing submit events #12877

merged 1 commit into from
May 22, 2018

Conversation

sophiebits
Copy link
Collaborator

We were adding a listener at the root when we weren't meant to. Blames to e96dc14.

This now alerts once (at FORM) instead of twice (at FORM, #document):

var Hello = class extends React.Component {
  render() {
    return (
      <form onSubmit={(e) => {e.preventDefault(); alert('hi ' + e.nativeEvent.currentTarget.nodeName);}}>
        <button>hi</button>
      </form>
    );
  }
};

We were adding a listener at the root when we weren't meant to. Blames to e96dc14.

This now alerts once (at FORM) instead of twice (at FORM, #document):

```
var Hello = class extends React.Component {
  render() {
    return (
      <form onSubmit={(e) => {e.preventDefault(); alert('hi ' + e.nativeEvent.currentTarget.nodeName);}}>
        <button>hi</button>
      </form>
    );
  }
};
```
@sophiebits
Copy link
Collaborator Author

sophiebits commented May 22, 2018

@philipp-spiess Looks like your refactor introduced this. Previously we added this listener only if it's present in topLevelTypes, which topSubmit and topReset weren't.

Neither were any of the media events – to my eyes the ones we shouldn't add top-level listeners for are:

pause play rateChange reset seeked submit volumeChange
abort canPlay canPlayThrough durationChange emptied encrypted ended error loadedData loadedMetadata playing progress seeking stalled suspend timeUpdate waiting

These are the ones that are in the interactive + noninteractive arrays in SimpleEventPlugin that were not in the old topLevelTypes. I am only adding submit and reset here because I believe they are the only ones that can ever bubble natively to the root.

Going to merge this now as an urgent fix, but would you be able to submit a less hacky fix for this and tests to make sure we don't regress? Should be doable with native event dispatching. Maybe we can add a fixture that tests this too.

@sophiebits sophiebits merged commit ad27845 into facebook:master May 22, 2018
@gaearon
Copy link
Collaborator

gaearon commented May 22, 2018

I misunderstood this line, sorry.

#12629 (comment)

I thought it meant “all valid top-level type constants” and forgot topLevelTypes was something else.

@gaearon
Copy link
Collaborator

gaearon commented May 22, 2018

#12878

@philipp-spiess
Copy link
Contributor

What a silly mistake - sorry for the troubles 🤦‍♀️ Thank you @sophiebits for pining me on this so I can follow along - I really appreciate that! And great work @gaearon on fixing this up so quickly and writing high quality tests.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants