Skip to content

Conversation

@nicolasbrugneaux
Copy link

I created a chrome extension with react. And when inserting in the DOM, the styles would collapse with the page.

The solution I found was the most elegant was to create a shadow root and render my react component inside it.

Problem is that the events were not bound properly because of shadow boundaries. Here are my modifications :)

Otherwise they're still bound to the document and not the shadowroot.
@zpao
Copy link
Member

zpao commented Dec 1, 2014

Hmm, this is going to mean we walk up the whole DOM for every event listener we add, which is suboptimal (especially since we add listeners progressively as they get used). A couple things…

  1. At the very least we should keep the fast path fast. Was there a reason you stopped using container.ownerDocument when there is no shadow root?
  2. We should probably just use a while loop instead of recursion.
  3. I'm slightly concerned that this isn't going to play nicely when mixing a page with shadow roots and regular React usage (there may be some caching at play that will screw things up, not totally sure though)
  4. cc @sebmarkbage

@nicolasbrugneaux
Copy link
Author

Hey, hi :)
Thanks for the response!
This is indeed a very naive implementation that fitted my needs for now, I don't have much experience with react yet, but the fact that I couldn't play around with a shadowroot was really annoying.

I can rework it and maybe add some tests 🔭
But I don't see why for the number 3, the node to which we attach the event shouldn't be affected if not located in a shadoroot, no?

What would you suggest instead of iterating until we find if the node is indeed in a shadow dom or not?
I didn't find another way to determine it so far.

@nicolasbrugneaux
Copy link
Author

Also, what's wrong with recursion in this case? Is there any risk to reach the stack limit? 😭

Uses a while loop instead of recursivly calling `parentNode`. Reuses node.ownerDocument when shadowRoot isn't supported.
@zpao
Copy link
Member

zpao commented Dec 1, 2014

We probably won't hit the stack limit. My concern is that I'm not sure if engines would do tail call optimizations and eliminate the need for creating all the things functions need. There is definitely overhead in that unless engines are smart (I think most are but I'm not certain).

@nicolasbrugneaux
Copy link
Author

I changed it to an iterative way. 👍
The solution I see is described in the w3c specs: http://www.w3.org/TR/shadow-dom/#h4_attributes, but so far it seems like chrome did not implement the host property on the shadow elements

@nicolasbrugneaux
Copy link
Author

I realize that might sound crazy, but how about storing in the react element if it has is "shadow react" element? If the property is false, we bind to container.ownerDocument else to container.previouslyStoredShadowRoot at least the parentNodes lookup will be done once/element instead of once/event?
Or I'm completely wrong 😃

@sebmarkbage
Copy link
Collaborator

We want to do document wide delegation and use event.path to dispatch the correct handler.

// TODO: Re-enable event.path handling

However, this spec has been changing so we temporarily disabled it.

@sebmarkbage sebmarkbage closed this Oct 6, 2015
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.

4 participants