Revert "Rely on bubbling for submit and reset events (#13358)"

[gaearon]

This reverts commit 725e499.

It broke registration on Workplace (thankfully end-to-end tests caught this before the rollout 😛).

I'm not sure why yet. We'll need to dig in deeper. The symptom is that the form that's normally controlled from JavaScript was always being submitted with default action. I guess one way this could happen is the React handler does e.preventDefault(), but there's a native DOM handler in the middle that does e.stopPropagation(). This would have no effect before this change, but with this change it would actually prevent the React handler from reaching the document. That's my hypothesis anyway. I don't know if this is what happened.

This means we probably can't make more changes to how native/React event mapping works without putting them behind feature flags. There is too much legacy code that can break unintentionally.

[nhunzaker]

Bizarre.

[nhunzaker]

I guess one way this could happen is the React handler does e.preventDefault(), but there's a native DOM handler in the middle that does e.stopPropagation(). This would have no effect before this change, but with this change it would actually prevent the React handler from reaching the document.

This makes sense to me, and is reproduceable:
https://codepen.io/nhunzaker/pen/VGLmwX

I wonder if we should have a test for this. We need to make sure to maintain resilience to code that could intercept events before they enter React.

[jquense]

Is there anyway for non-fb folks to make use of feature flags without building from source? I'd love to see some strategy for being able to start making progress on these sort of DOM issues, at the moment it feels fairly futile, since almost any change breaks some obscure thing somewhere, generally in FB's codebase where we can't help identify and mitigate :P

[philipp-spiess]

Oh no I broke submit again. 😐

[nhunzaker]

@philipp-spiess we're in this together 😱. Let's make sure to get test coverage on this (or leave a note in the source code).

Also I'm starting to think that we should catalog event binding rules. Like:

Document:
- Mouse events (enter-leave plugin needs to know about mouse enter/leave outside of React)

Local:
- Form submit/reset (man-in-the-middle code can use event.stopPropagation() to prevent React handlers from preventing default action)

[gaearon]

I wonder if we should have a test for this. We need to make sure to maintain resilience to code that could intercept events before they enter React.

It seems weird that form events are special in this regard though, doesn't it? Is there a particular reason why it makes sense? Otherwise it just sounds like something we should fix in the product code.

Is there anyway for non-fb folks to make use of feature flags without building from source?

Not really — but why does this block the progress? Do you mean in the sense of testing being more difficult? We could publish the version with feature flags flipped with a different forked name.

[pull-bot]

ReactDOM: size: 🔺+0.1%, gzip: 🔺+0.2%

Details of bundled changes.

Comparing: 90c92c7...4b465ee

react-dom

File	Filesize Diff	Gzip Diff	Prev Size	Current Size	Prev Gzip	Current Gzip	ENV
react-dom.development.js	0.0%	0.0%	649 KB	649.31 KB	152.36 KB	152.4 KB	UMD_DEV
react-dom.production.min.js	🔺+0.1%	🔺+0.2%	95.3 KB	95.42 KB	31.17 KB	31.21 KB	UMD_PROD
react-dom.development.js	0.0%	0.0%	645.14 KB	645.45 KB	151.24 KB	151.28 KB	NODE_DEV
react-dom.production.min.js	🔺+0.1%	🔺+0.1%	95.29 KB	95.41 KB	30.89 KB	30.92 KB	NODE_PROD
ReactDOM-dev.js	0.0%	0.0%	652.13 KB	652.45 KB	149.31 KB	149.35 KB	FB_WWW_DEV
ReactDOM-prod.js	🔺+0.2%	🔺+0.1%	287.78 KB	288.24 KB	53.3 KB	53.34 KB	FB_WWW_PROD
react-dom.profiling.min.js	+0.1%	+0.1%	96.49 KB	96.6 KB	31.29 KB	31.32 KB	NODE_PROFILING
ReactDOM-profiling.js	+0.2%	+0.1%	290.13 KB	290.59 KB	53.91 KB	53.95 KB	FB_WWW_PROFILING

Generated by 🚫 dangerJS

[nhunzaker]

It seems weird that form events are special in this regard though, doesn't it? Is there a particular reason why it makes sense? Otherwise it just sounds like something we should fix in the product code.

It does feel like a special case. Preventing form submission feels more critical to me than having external code block a mouse or keyboard event. I just assumed we'd want to keep it to avoid the problem beyond Workplace moving forward.

If we don't care about this edge case, there isn't a reason to add test coverage. Still, it sets up an expectation that is different for FB products and external use. Event system updates are on a few DOM team member's minds right now, we'll just need to keep that in mind until Workplace (or any other FB products) are updated.

[gaearon]

OK, I'm good with having a test for this.

[jquense]

Not really — but why does this block the progress?

well it's tough if you want feedback or carnary-in-the-coal-mine feedback from folks outside of, or with different use-cases than fb.

Maybe that's less the worry, and its more that there is this giant, implicit and opaque req that we not break anything in fb's codebase (a totally reasonable req btw). Given that most DOM work is currently being quasi-spearheaded by non-fb contributors it's hard to make progress when the iteration loop is "back and forth on the PR for a while" -> "merge" -> break fb and revert -> feel bad

I don't know what the alternative might be, independent react-dom versions would be great here maybe? give that most/many changes to RD break someone it's better stagnating to only get these sorts of fixes every 6m - 1yr when react takes a major bump yeah?

I guess all i'm saying is i'd love to help do some work a path forward for RD changes w/r/t incidental breaking changes in a way that is reasonable and safe, but also doesn't keep RD from evolving 👍

[gaearon]

Maybe that's less the worry, and its more that there is this giant, implicit and opaque req that we not break anything in fb's codebase (a totally reasonable req btw).

Sorry if this wasn't clear. This is not a requirement per se if you have a migration strategy. The only reason I reverted the PR is because I need to unblock the sync which we need to do as soon as possible.

If this broke FB there's a high chance it will break somebody else, so it's not suitable for a patch release. That said, if the change makes sense, we should figure out a migration strategy and then put it behind a feature flag in the next major. Then we can test this migration strategy internally (which would give us confidence that it would work in open source too) and switch over.

Given that most DOM work is currently being quasi-spearheaded by non-fb contributors it's hard to make progress when the iteration loop is "back and forth on the PR for a while" -> "merge" -> break fb and revert -> feel bad

Nobody needs to feel bad, it's a normal cycle. We learned more. Again, if this broke us in a critical way (website registration almost broke) it will likely break other companies in a similarly bad way. Working forms are critical. I think it's hugely helpful we've identified this issue now. I don't see it as "stopping progress". It means this change, if useful, needs to be an explicitly breaking one, and we need to come up with a strategy for how to find out the cases where it breaks, and warn.

give that most/many changes to RD break someone it's better stagnating to only get these sorts of fixes every 6m - 1yr when react takes a major bump yeah?

My intention was to add a feature flag soon, and start getting those breaking changes (with a clear migration strategy in). Yes, I think it would be irresponsible to make big breaking changes to the DOM parts more often than once a year. However, I agree that we shouldn't try to merge them at the last minute either. That's why I want to introduce a feature flag for them, and gradually work with you towards sensible new behavior for a few different issues, while using FB to dogfood them and ensure they're solid.

[gaearon]

To put it another way: the requirement isn't that we never break FB. If some FB code is problematic we're happy to merge and fix our product code to remove the issue.

However, in this case the breakage will likely affect others too (so it's not safe to get it out as a patch), for a piece of infra that's usually critical (e.g. signup forms). We also don't have a good strategy for how to determine the callsites that would break.

It doesn't mean the change can't be done. But if the risk is high we need to be very intentional about why we're doing this, and hide it behind a major.

[jquense]

I don't see it as "stopping progress". It means this change, if useful, needs to be an explicitly breaking one, and we need to come up with a strategy for how to find out the cases where it breaks, and warn.

Totally sorry I wasn't trying to be negative 👍sorry. I think all i mean is it'd be nice to have a clear sense for the non-fb folks what the next steps are in these cases. How feature flags help or don't and how we can follow up given the new knowledge and verify that the strategy going forward is a good or not. I know personally i've hit this sort of unconvered issue, had PR's reverted, and wasn't really sure how to move forward. I didn't mean to come off as complaining, i'm wondering how/if we can codify some manner of loose process for dealing with these things, so less stuff is stuck on ya'lls plate

[jquense]

i'd just add that I think this is even more important in RD's case that we have clear plans for migrations and breaking bug fixes, because it's so easy to break something! I think we'd all love to figure out how to be less afraid of the code base while also responsibly not moving everyone's cheese :P

[gaearon]

Sorry I didn’t communicate this clearly.

For the past week I’ve been going over the isssue tracker, merging low hanging fruit and investigating stalled issues. We’ve mostly run out of them, so now I’m doing a sync to get about a month of bugfixes running at FB. Once that’s landed and it works for a wile it will give more confidence to cut 16.4.3.

After 16.4.3, my plan is to focus on high impact DOM changes for several weeks. I’m thinking about stuff like:

• attaching event handlers per root
• removing syncing of attributes and properties
• beginning to prepare for onChange polyfilling changes

Those items are all complicated but I want to give them a chance because they’re at the root of so many issues. So solving them now would be great. But they would require a major.

Since we can’t break open source and will need a few months to be confident in these changes, I want to add a feature flags. All big changes to DOM behavior will go behind the feature flags. We’ll run relevant tests in both modes. We’ll use FB as a way to dogfood those changes.

The reason to add feature flags is precisely to allow this iteration without reverting anything. For example we can turn on the attachEventsToRoots feature flag just for our team first. Fix all issues we see. Then bump it to 1% engineers. If something’s broken, we don’t need to revert — just dial down the feature flag back to our team. Eventually as we roll the change out widely we know it’s solid. And we can do this for every change.

Finally when we start releasing 17 alphas we can set new feature flags to true for them. And then delete old code.