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

Attach event per react container root, rather than on the document #2043

Open
chenglou opened this issue Aug 14, 2014 · 7 comments

Comments

@chenglou
Copy link
Contributor

commented Aug 14, 2014

@nathansobo will that help your event perf issues a bit? I'm not familiar with atom's plugin infrastructure, But this'll help if you have <Editor/><Plugin1/> (two renderComponents). Doesn't help if you have <Editor><Plugin1/></Editor> though, but I have some ideas to optimize events a bit more.

This is nonetheless an ok idea, I think.
@spicyj

@chenglou chenglou self-assigned this Aug 14, 2014

@nathansobo

This comment has been minimized.

Copy link

commented Aug 14, 2014

@chenglou That would help our situation with the editor a bit, yes. But what's more important for Atom I think is the ability to integrate React with HTML custom elements without requiring the use of the shadow DOM. Seems like #1711 would help that, and it seems like this idea might be necessary to achieve that as well?

@sophiebits

This comment has been minimized.

Copy link
Collaborator

commented Aug 14, 2014

@nathansobo Can you clarify what you mean, "without requiring the use of the shadow DOM"? I'm a bit uninformed about how HTML custom elements work right now.

chenglou added a commit to chenglou/react that referenced this issue Aug 20, 2014

[RFC] Per React container event listening/dispatching
Work in progress. `enterleave` plugin (and maybe `analyticsPlugin`) is broken because it relied on the old behavior. But wanted to put this out here for suggestions. There are also some comments that need to be changed, I'll do it when the code is finalized.

If we attach the event listening/dispatching at container level, it'll benefit the case of `<Editor/><Plugin1/>` (both are container roots), since `Plugin1` won't disturb `Editor`.

We also detach those listeners now. There wasn't really a need in the past.

Fixes facebook#2043
Should help with facebook#1964
@syranide

This comment has been minimized.

Copy link
Contributor

commented Sep 29, 2014

@chenglou It seems we have your #2050 to track this. Closing, reopen if you disagree :)

@syranide syranide closed this Sep 29, 2014

@sophiebits

This comment has been minimized.

Copy link
Collaborator

commented Sep 29, 2014

@syranide I don't see why we should close issues just because there's a PR open – what if we don't go with that particular implementation? We may still want to make the same fix.

@syranide

This comment has been minimized.

Copy link
Contributor

commented Sep 29, 2014

@spicyj Fair enough :)

@syranide syranide reopened this Sep 29, 2014

fcsonline added a commit to fcsonline/react that referenced this issue Jun 20, 2016

Listen DOM events in each React container
Current React implementation is attaching events to `document` level.
This is breaking bubbling DOM behaviour not letting users to play
properly with methods like `stopPropagtion`.

Also attaching events to containers instead of `document` integrates
better with other Javascript frameworks.

With this change we start attaching events at container level.

Fixes:
facebook#2043
facebook#4335

fcsonline added a commit to fcsonline/react that referenced this issue Jun 20, 2016

Listen DOM events in each React container
Current React implementation is attaching events to `document` level.
This is breaking bubbling DOM behaviour not letting users to play
properly with methods like `stopPropagtion`.

Also attaching events to containers instead of `document` integrates
better with other Javascript frameworks.

With this change we start attaching events at container level.

Fixes:
facebook#2043
facebook#4335

fcsonline added a commit to fcsonline/react that referenced this issue Jun 20, 2016

Listen DOM events in each React container
Current React implementation is attaching events to `document` level.
This is breaking bubbling DOM behaviour not letting users to play
properly with methods like `stopPropagtion`.

Also attaching events to containers instead of `document` integrates
better with other Javascript frameworks.

With this change we start attaching events at container level.

Fixes:
facebook#2043
facebook#4335

fcsonline added a commit to fcsonline/react that referenced this issue Jun 20, 2016

Listen DOM events in each React container
Current React implementation is attaching events to `document` level.
This is breaking bubbling DOM behaviour not letting users to play
properly with methods like `stopPropagation`.

Also attaching events to containers instead of `document` integrates
better with other Javascript frameworks.

With this change we start attaching events at container level.

Fixes:
facebook#2043
facebook#4335
@gaearon

This comment has been minimized.

Copy link
Member

commented Nov 21, 2017

As far as I can tell #8117 was the latest attempt to do this. It has requested changes (that weren't addressed). So if somebody wants to pick this up they'll probably want to read the discussion there.

@kentcdodds

This comment has been minimized.

Copy link
Contributor

commented Apr 2, 2018

Just FYI, this effort may get new life due to: testing-library/react-testing-library#35

If I can't push this along myself, I'll try to encourage others to do so. Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
7 participants
You can’t perform that action at this time.