Always use event capturing. Remove media events. #12919
Conversation
|
|
||
| expect(onError).toHaveBeenCalledTimes(1); | ||
| } finally { | ||
| container.remove(); | ||
| } |
There was a problem hiding this comment.
Worried this might be a breaking change, but the events only appear to capture with JSDOM if the element is attached to the document. Any ideas?
|
@sebmarkbage Do you think this could cause issues at FB with ordering of listeners? I don't remember what you bumped into last time we tried to change something there. |
|
ReactDOM: size: -0.6%, gzip: -0.5% Details of bundled changes.Comparing: c5a733e...fec5dd3 react-dom
Generated by 🚫 dangerJS |
|
There is a subtle behavior change when you use react events in combination with native Right now, when you have code like this: class extends Component {
componentDidMount() {
document.addEventListener('click', () => console.log('document bubble'))
document.addEventListener('click', () => console.log('document capture'), true)
}
render() {
return <div
onClick={() => console.log('element bubble')}
onClickCapture={() => console.log('element capture')} />;
}
}A click on the element will fire in the order you would expect (jsfiddle): This happens because React will add the event listener to the The same pattern will behave unexpectedly after the change since the now the capture listener that was added first, in our case the once by react, will be fired first. This would probably result in the following order: We do have code in PSPDFKit for Web that adds document listener in the capture phase which might be affected (Usually doing this to simulate a pointer capture since we use mouse and touch events instead. React got support for pointer events only as of recently 🙈). I'm sure we can work around that but I just wanted to point that out. Notes:
|
|
It seems like we should just do #2043. It comes up pretty often and is impactful to fix. |
|
@philipp-spiess Want to take a stab at it? |
|
@gaearon Sounds good 👍 |
|
Fair warning. #2043 is going to be very risky and take a lot of work, which might include many back and forth after breaking FB, unless you have your own very large code base that can flush out issues. The polyfills we use for onSelect etc. relies on it being global AFAIK and doing it differently might expose subtle breakages. I could imagine that it won't actually make it to a release until we do 17.0 for that reason. |
|
@sebmarkbage Thanks for pointing that out. I looked a bit into the whole topics and there seem to be a lot of pitfalls - That's probably also the reason why this did not land yet despite having a lot of work put into. One immediate thought I have is if #2043 would even be compatible with this PR. When we listen in the capture phase, the event order for nested react contexts will appear inverted. Pretty much what was pointed out for |
|
If we do #2043 we’ll need to start it behind a feature flag so we can gradually test it and switch over in 17. |
|
I don't have much to contribute other than to thank everyone for pointing out the issues with this approach. I'll dig into this again with those in mind. |
This commit changes event subscription such that all events are attached with capturing. This prevents the need to eagerly attach media event listeners to video, audio, and source tags, and simplifies the event subscription process. I think we can probably extend this to more event types; requiring much more testing. Until that time, this commit lays the foundation for that work by demonstrating that capturing is applicable in cases where bubbling does not occur.
nhunzaker
force-pushed
the
nh-remove-media-events
branch
from
4990eba
to
fec5dd3
Compare
|
@philipp-spiess Added your test case (and it fails exactly as you've laid out). Just for fun, I did a quick test to see what would happen if all event listeners were attached to the root container element: nhunzaker/react@nh-remove-media-events...nhunzaker:nh-root-attachment-test This change allows the test to pass, but leaves 11 test failures (neat that it's only 11 though):
It's possible that this has been discussed before, but it looks like we still need to attach certain events to the owner document for things like enter/leave and the onSelect polyfill. |
|
@nhunzaker Thanks for adding the test case 🎉 I think #8117 was the latest attempt to implement per-root-container listening if you need some context 🙂 Besides the issues with SelectEventPlugin (which are non-trivial on their own) I'm concerned about the comments regarding the bubble order for nested React instances. Let’s have a look at an example of one react app (“outer”) that mounts another react app (“inner”): https://codesandbox.io/s/vy9oywmjl3 Right now, the events will be dispatched in this order: Which, of course, is unexpected since capture events must fire before bubble events. Now with the changes outlined in #8117, that behavior would still be present. The reason here lies deeply in the fact that we only listen for one top level event (may it be in the bubble phase like on The order we see right now (the one I outlined above) is a good compromise since I’m sure that bubble listeners are much more common than capture listeners and we see the Now if we would naively listen for top level events on the root container element in the bubble phase, we’d see the same order since the inner container will receive the bubble event first and dispatch both phases before the outer container gets a chance. If we would combine this with your PR, the order would be inverted since the outer container would be the first to receive the event, resulting in: Both orders are wrong - although I’d say the latter would be an issue for more people. I have two ideas how that can be fixed but they're huge changes:
|
|
Sounds like a universal change to capture at the top has a lot of issues :)
I experimented with that a bit, motivated to fix some rendering deopts with scroll events (#9333). The challenge I'm having with the current event system is that I need some way to dedupe events. Otherwise React sends out a new synthetic event every step of a bubble (failing this test). I wonder if a good next step would be to outline a more formal description of how this currently works. From that, we could create a test suite that verifies those rules. Maybe a test suite is all we need. We'd probably need to add more to it over time as we uncover unanticipated changes in behavior. We could also start to compile a list of use cases React's event system doesn't support. I think one of the reasons there is concern attaching all events locally is performance. But maybe it's not so much slower, or there's some middle-ground that gives us a good balance of performance, behavior and simplicity. Right now it's hard for me to gauge the cost/benefit of the change within the limited context of my own use-cases. |
|
Thank you for your pull request and welcome to our community. We require contributors to sign our Contributor License Agreement, and we don't seem to have you on file. In order for us to review and merge your code, please sign up at https://code.facebook.com/cla. If you are contributing on behalf of someone else (eg your employer), the individual CLA may not be sufficient and your employer may need the corporate CLA signed. If you have received this in error or have any questions, please contact us at [email protected]. Thanks! |
|
Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Facebook open source project. Thanks! |
|
This PR was useful to uncover a lot of design challenges, but I think it's unrealistic to move forward with. |
|
@nhunzaker Should we extract some of the test cases that we've identified in here and merge it upstream? Maybe to document the expectations that we have about the event system - Does not mean we never change them 🙂 |
This is documenting the current order in which events are dispatched when interacting with native document listeners and other React apps. For more context, check out facebook#12919.
This is documenting the current order in which events are dispatched when interacting with native document listeners and other React apps. For more context, check out facebook#12919.
This is documenting the current order in which events are dispatched when interacting with native document listeners and other React apps. For more context, check out facebook#12919.
This is documenting the current order in which events are dispatched when interacting with native document listeners and other React apps. For more context, check out #12919.
This is documenting the current order in which events are dispatched when interacting with native document listeners and other React apps. For more context, check out facebook#12919.
What
I finally had a chance to circle back on what I learned investigating event listener attachment.
In short, we distinguished between captured and bubbled events, however I believe this is not necessary as of IE9+. Captured events do not appear to exhibit the same edge cases as bubbled events, allowing us to simplify event code.
Why do we want to do this?
I think we can probably drastically reduce the code for eagerly attaching listeners for events that do not bubble. So far, all events that do not bubble appear to capture on parent elements.
Longer term, I wonder if we can remove the
.bind()when attaching listeners, and maybe even remove top level types.More info
This commit changes event subscription such that all events are attached with capturing. This prevents the need to eagerly attach media event listeners to video, audio, and source tags, and simplifies the event subscription process.
I think we can probably extend this to more event types, maybe most/all eager event attachments, but that will require substantial testing. Until that time, this commit lays the foundation for that work by demonstrating that capturing is applicable in cases where bubbling does not occur.
Test plan
I believe these to be the key event types to test, however this change definitely affects all event listeners.
I've tested so far in: