Skip to content

Make events propagate through shadow DOMs. #4150

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

Merged
merged 1 commit into from
Jul 2, 2015
Merged

Conversation

jimfb
Copy link
Contributor

@jimfb jimfb commented Jun 16, 2015

Make events propagate through shadow DOMs. This is necessary for WebComponents, which often render into shadow DOMs, through which events may need to propagate.

@jimfb
Copy link
Contributor Author

jimfb commented Jun 16, 2015

cc @sebmarkbage

}
var path = bookKeeping.nativeEvent.path;
var topLevelTargetID;
if (path != null) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can we split these branches into two functions? That avoids some nesting hell and also allows us to put a name to this code which is built-in documentation! (Also avoids the need to call the iterator variable j instead of i.)

@jimfb jimfb force-pushed the event-path branch 3 times, most recently from d5f82af to 1df093e Compare June 18, 2015 13:11
@jimfb
Copy link
Contributor Author

jimfb commented Jun 18, 2015

@sebmarkbage new diff is up.

@jimfb jimfb force-pushed the event-path branch 2 times, most recently from dee3e35 to a923bb8 Compare June 18, 2015 13:33
@sebmarkbage sebmarkbage mentioned this pull request Jun 18, 2015
25 tasks
@gurkerl83
Copy link

Thanks for doing this, no more workarounds! I hope this will get merged into the next release of react!

module.exports = window.WebComponents;
if (typeof module !== 'undefined') {
module.exports = window.WebComponents;
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why was this hack needed again? There are no examples here so it shouldn't be needed.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

For the unit tests run in debug mode, we load webcomponents in the wrapper html and module isn't defined.

@nicolasbrugneaux
Copy link

Thanks for this!! :)

@ziahamza
Copy link

ziahamza commented Jul 1, 2015

Really appreciate this pull request! Hope it gets merged in the 0.14 timeframe!

@sophiebits sophiebits added this to the 0.14 milestone Jul 1, 2015
@jimfb
Copy link
Contributor Author

jimfb commented Jul 2, 2015

Ran the perf test, event handling took between one tenth and two tenths of a millisecond. The new implementation looked ~2 hundredths of a millisecond slower, but I could totally believe that's within the noise of the measurement. I think that's within a reasonable range (certainly for a beta release, we can try to optimize later if we feel it's necessary). As per conversation with @sebmarkbage the perf test was the only thing blocking this, so I'm going to mark this as accepted as per that conversation.

jimfb added a commit that referenced this pull request Jul 2, 2015
Make events propagate through shadow DOMs.
@jimfb jimfb merged commit 851378b into facebook:master Jul 2, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants