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

Rely on bubbling for submit and reset events #13358

Merged
merged 4 commits into from
Aug 10, 2018

Conversation

philipp-spiess
Copy link
Contributor

Fixes #12251

We can get rid of some special cases by relying on bubbling for submit and reset events instead of adding the events to the individual <form> elements. After reading #12251 I was curious and had to try that out. Here are my manual testing results:

Modern

✅ Chrome 67
✅ Firefox 61

✅ Edge 17
✅ Safari 12

Legacy

✅ Chromium 41
✅ Firefox ESR 52
✅ Safari 6
✅ Internet Explorer 9

(The browser list is inspired by #9301 (comment))

Note: This does not fix any bug and also only has minimal impact on the bundle size. I'm not sure if we really want it.

I used a custom fixture to test that if you want to reproduce. I don't think we want to add this though since I hardly believe that this behavior will ever change again and it would only make manual testing even more time consuming. Let me know if you think I should add it.

While looking in the repository for occurrences of the term submit, I also found a lonely test that that was written more than 5 years ago and probably never run until now. The behavior still works, although the API changed quite a bit over the years. Seems like this test was part of the initial public release already: 75897c2#diff-1bf5126edab96f3b7fea034cd3b0c742R31

In addition to all that, I also found out that the yarn lockfile for the DOM fixtures was outdated.

I found a test that was written more than 5 years ago and probably never
run until now. The behavior still works, although the API changed quite
a bit over the years.

Seems like this was part of the initial public release already:

facebook@75897c2#diff-1bf5126edab96f3b7fea034cd3b0c742R31
@philipp-spiess
Copy link
Contributor Author

Oh and FYI I have a background (#12877) in breaking sumbit events already once. So better be careful here 😐

@pull-bot
Copy link

pull-bot commented Aug 9, 2018

ReactDOM: size: -0.1%, gzip: -0.1%

Details of bundled changes.

Comparing: be4533a...425738e

react-dom

File Filesize Diff Gzip Diff Prev Size Current Size Prev Gzip Current Gzip ENV
react-dom.development.js -0.0% -0.0% 641.59 KB 641.27 KB 150.74 KB 150.69 KB UMD_DEV
react-dom.production.min.js -0.1% -0.1% 96.39 KB 96.27 KB 31.24 KB 31.2 KB UMD_PROD
react-dom.development.js -0.0% -0.0% 637.72 KB 637.41 KB 149.57 KB 149.53 KB NODE_DEV
react-dom.production.min.js -0.1% -0.1% 96.38 KB 96.27 KB 30.8 KB 30.78 KB NODE_PROD
ReactDOM-dev.js -0.0% -0.0% 644.86 KB 644.55 KB 147.8 KB 147.76 KB FB_WWW_DEV
ReactDOM-prod.js -0.1% -0.1% 279.26 KB 278.96 KB 52.29 KB 52.26 KB FB_WWW_PROD
react-dom.profiling.min.js -0.1% -0.1% 97.59 KB 97.47 KB 31.2 KB 31.18 KB NODE_PROFILING
ReactDOM-profiling.js -0.1% -0.1% 281.67 KB 281.37 KB 52.93 KB 52.9 KB FB_WWW_PROFILING

Generated by 🚫 dangerJS

Copy link
Contributor

@nhunzaker nhunzaker left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks right to me, and I'd like to get this change in. It safely removes another special case, which slowly makes it easier to work on event system code.

As far as I can tell, this existed to support IE8. I wasn't able to track this down in the source, but it is referenced in Meteor's article about their event system:

https://blog.meteor.com/browser-events-bubbling-capturing-and-delegation-14db28e924ae

I have faith in @philipp-spiess, but I can also do some extra testing tomorrow morning for good measure.

Separately: Should we just publish the browser list somewhere? It doesn't have to be official support, I'd just like to make it easier for contributors to stumble upon.

@@ -361,7 +361,6 @@ describe('ReactDOMEventListener', () => {
<audio {...mediaEvents}>
<source {...mediaEvents} />
</audio>
<form onReset={() => {}} onSubmit={() => {}} />
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For other reviewers: I was curious about this removal, but it's a part of a general test that asserts how listeners are attached:

try {
// We expect that mounting this tree will
// *not* attach handlers for any top-level events.
ReactDOM.render(
<div>
<video ref={videoRef} {...mediaEvents} onPlay={handleVideoPlay} />
<audio {...mediaEvents}>
<source {...mediaEvents} />
</audio>
</div>,
container,
);
// Also verify dispatching one of them works
videoRef.current.dispatchEvent(
new Event('play', {
bubbles: false,
}),
);
expect(handleVideoPlay).toHaveBeenCalledTimes(1);
} finally {
document.addEventListener = originalAddEventListener;
document.body.removeChild(container);
}

👍

@philipp-spiess
Copy link
Contributor Author

I have faith in @philipp-spiess

🙂

I've rebuild everything locally (including an additional console.log so I know I'm on the right version) and tested again on IE9 so I can sleep well tonight:

screen shot 2018-08-09 at 22 31 22

Separately: Should we just publish the browser list somewhere? It doesn't have to be official support, I'd just like to make it easier for contributors to stumble upon.

👍 I came across that by total accident today. Maybe we can mention some browsers to test in the contribution documentation?

@nhunzaker
Copy link
Contributor

This looks great on my end. Feel free to merge!

@philipp-spiess
Copy link
Contributor Author

@nhunzaker covered iOS and Android as well (we should probably add versions there to the browser list as well).

Let's give it a shot?! 😱 🎉

@philipp-spiess philipp-spiess merged commit 725e499 into facebook:master Aug 10, 2018
@philipp-spiess philipp-spiess deleted the bubbling-submit branch August 10, 2018 19:11
gaearon added a commit to gaearon/react that referenced this pull request Aug 22, 2018
gaearon added a commit that referenced this pull request Aug 22, 2018
philipp-spiess added a commit that referenced this pull request Aug 22, 2018
This PR includes a test that I've enabled in #13358 and another test that we've discussed in #13462 as well as some random cleanup while I'm at it.
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.

Does react still require non-toplevel submit handler?
4 participants