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

Determine event target in Shadow DOM correctly #12163

Closed
wants to merge 5 commits into
base: master
from

Conversation

Projects
None yet
9 participants
@marionebl

marionebl commented Feb 6, 2018

Motivation

Fixes an issue with events not being delegated correctly when originating from an element inside a Shadow DOM root as described in #9242.

Included changes

  • Add handling of Events that are .composed as per MDN
  • Add basic test case for getEventTarget
  • Add fixture in fixtures/dom, as attachShadow is not supported in jsdom yet

@marionebl marionebl force-pushed the marionebl:fix-event-propagation-across-shadow-dom branch from 0406615 to d02aa87 Feb 6, 2018

@alubchuk

This comment has been minimized.

alubchuk commented Feb 9, 2018

Cool! Also waiting for this fix... Thx @marionebl !

@coddingBoy

This comment has been minimized.

coddingBoy commented Mar 28, 2018

the callback still does not invoke in web component

@MRDNZ

This comment has been minimized.

MRDNZ commented Apr 6, 2018

@marionebl Great fix! Solves all my problems with injecting a complete react client inside of the shadowRoot. Can we have some sort of time estimation would love to see this merged in!

@marionebl

This comment has been minimized.

marionebl commented Apr 6, 2018

@MRDNZ I am not a react contributor, so I can't promise when / if this gets merged at all, sorry.

@jquense

The change looks fine to me, i'm not super familar with the shadow-dom but this is minimal enough i think it's ok?

let React;
let ReactDOM;
describe('getEventTarget', () => {

This comment has been minimized.

@jquense

jquense Apr 6, 2018

Collaborator

I'm not sure this test suite tests much? I suppose it doesn't hurt but it doesn't really cover this bug, or the svg case. I'm not sure if it makes sense to add.

@@ -17,6 +17,12 @@ import {TEXT_NODE} from '../shared/HTMLNodeType';
function getEventTarget(nativeEvent) {
let target = nativeEvent.target || window;
// If composed / inside open shadow-dom use first item of composed path #9242
if (nativeEvent.composed) {
const path = nativeEvent.composedPath();

This comment has been minimized.

@jquense

jquense Apr 6, 2018

Collaborator

I'd add an additional safety check here that composedPath exists. Who knows what the compatibility matrix will look like here...

This comment has been minimized.

@marionebl

marionebl Apr 6, 2018

typeof nativeEvent.composedPath === "function" should do the trick?

return (
<FixtureSet title="Shadow DOM" description="">
<TestCase title="Event listeners in shadow-dom" relatedIssues="4963">

This comment has been minimized.

@jquense

jquense Apr 6, 2018

Collaborator

Does this test case fail without this PR? My understanding is the event would still fire, but the target would be wrong, the TestCase doesn't assert anything about the target, only that the event was seen.

This comment has been minimized.

@marionebl

marionebl Apr 6, 2018

The test case fails without the other changes in this PR

@MRDNZ

This comment has been minimized.

MRDNZ commented Apr 9, 2018

@jquense , @marionebl is there anything i can help you guys with to help speed up the process of getting this merged in?

marionebl added some commits Apr 9, 2018

@marionebl

This comment has been minimized.

marionebl commented Apr 9, 2018

@jquense Adressed your comments, thanks for the review!

@marionebl marionebl closed this Apr 9, 2018

@marionebl marionebl reopened this Apr 9, 2018

@marionebl

This comment has been minimized.

marionebl commented Apr 17, 2018

Anything else I can do to help this move along?

@robdodson

This comment has been minimized.

robdodson commented Apr 17, 2018

Sorry if this is a silly question...

Would it be possible instead to expose Event.composed and Event.composedPath() on the nativeEvent? Then it would match the actual native DOM event, which is what nativeEvent seems to aim to do?

Shadow DOM purposefully retargets events so as not to violate encapsulation. composedPath() is there for folks who know that they want to purposefully reach inside the encapsulation boundary to access something specific. Ideally folks shouldn't need it, but it does prove useful from time to time. It seems like short circuiting that design might be bad?

Instead, if composedPath() is available on nativeEvent, folks have the option of accessing the internal target, but it's something they choose to do.

@gaearon

This comment has been minimized.

Member

gaearon commented Apr 18, 2018

To be clear nativeEvent is exactly the DOM event. It’s exposed to people who need it but it’s not the main object given to React users.

The function changed in this PR is only used internally by React. It’s not a public API. But it determines what React users will see as a target on React’s event object. The native DOM event is also available on it as a property.

@robdodson

This comment has been minimized.

robdodson commented Apr 18, 2018

ah I see.

But it determines what React users will see as a target on React’s event object.

Does it also inform React about which handler to call? For example, if I render a React component inside of the shadow DOM, like so:

x-foo
  #shadow-root
    <ReactThing onClick={bar}>

Normally the DOM click event would be retargeted to x-foo, but does React need to know that it actually came from ReactThing—or, more specifically, the DOM elements created by ReactThing—to properly trigger its onClick? That's what I inferred from reading #9242.

If that's the case, would it be possible for React to use the element at composedPath()[0] to trigger handlers, but still give the user the retargeted shadow host as its SyntheticEvent.target?

I realize this might be a weird idea, but my thinking is that React, the library, has a very specific need to reach inside of shadow boundaries because of how it does event delegation. But if I'm just using React to write a component, then I shouldn't need to know about the encapsulated internals of another component. And if, for whatever reason, I do need to get at those internals, I can still use nativeEvent.composedPath()[0] to do so.

@robdodson

This comment has been minimized.

robdodson commented Apr 18, 2018

In other words, React would use composedPath()[0] to get at ReactThing and properly call its onClick. But looking at SyntheticEvent.target would give me x-foo.

marionebl added a commit to marionebl/jsplayground that referenced this pull request Apr 18, 2018

@jquense

This comment has been minimized.

Collaborator

jquense commented Apr 18, 2018

The event Synthetic event target will be the DOM node associated with the component that's used here, so in the case it will be whatever ReactThing renders if it's open, just like it'd be on the native event, which you can still inspect via syntheticEvent.nativeEvent

This looks good to me, @nhunzaker or @aweary care to add another sign off?

@robdodson

This comment has been minimized.

robdodson commented Apr 18, 2018

Ok that confirms what I was curious about, thank you @jquense.

I guess I still have concerns that by giving the developer the internal element as SyntheticEvent.target it's bypassing shadow DOM's encapsulation. Those elements are supposed to be private, implementation details which is why the event is retargeted to the shadow host. That's a key point of shadow DOM. If it's possible to set the target back to the shadow host after React has triggered handlers that would feel a bit better to me, though I realize it may be more work ☹️

@robdodson

This comment has been minimized.

robdodson commented Apr 18, 2018

Maybe I can give some examples to explain my thinking. I realized you may want two things here...

x-foo
  #shadow-root
    <ReactApp>
        <ReactThing onClick={bar}>
          <button>

In this scenario, it's totally fine for ReactApp's onClick to see the <button> inside of ReactThing because they're both in the same shadow scope. So setting syntheticEvent.target = compoundPath()[0] seems fine.

<ReactApp>
  <x-bar onClick={baz}>
    #shadow-root
      <button>

In this scenario, ReactApp's onClick should not be able to see the <button> inside of x-bar. So setting syntheticEvent.target = compoundPath()[0] would be bad. That's basically just breaking Shadow DOM.

Apologies if I'm totally misunderstanding 😬 I hope these examples explain my thinking a bit better.

@gaearon

This comment has been minimized.

Member

gaearon commented Apr 18, 2018

@jquense Would this not be a breaking change? Technically we're changing what e.target points to, even if it didn't point to something useful before. Or not?

@marionebl

This comment has been minimized.

marionebl commented Apr 18, 2018

In this scenario, ReactApp's onClick should not be able to see the inside of x-bar. So setting syntheticEvent.target = compoundPath()[0] would be bad.

@robdodson I think I get your point. I did not consider scenario 2 when implementing this.

@jquense Would this not be a breaking change? Technically we're changing what e.target points to, even if it didn't point to something useful before. Or not?

@gaearon In scenario 2 (shadow-dom inside React tree) this would be a breaking change pointing e.target to something different compared to the current implementation.
In scenario 1 (React tree inside shadow-dom) the current implementation does not execute the handlers at all, so there no contracts to break.
What do you think?


Could we solve this by decoupling the passed e.target from the decision which event handlers are called in handleTopLevel?

I haven't dug deep enough into the events page yet to know if this might work.

@gaearon

This comment has been minimized.

Member

gaearon commented Apr 18, 2018

the current implementation does not execute the handlers at all

In this case starting to execute them can also be a breaking change.

@AllenWilliamson

This comment has been minimized.

AllenWilliamson commented Apr 23, 2018

I'm excited for this to be merged but it seems that <select> elements aren't firing onChange events.

@jquense

This comment has been minimized.

Collaborator

jquense commented Apr 23, 2018

I guess I still have concerns that by giving the developer the internal element as SyntheticEvent.target it's bypassing shadow DOM's encapsulation.

My understanding tho is that this is how the Shadow DOM works, it makes the distinction between "closed" and "open", we are talking about "fixing" the case where the tree is open, but not doing what would natively happen.

@jquense Would this not be a breaking change?

@gaearon I think this falls in the gray area. I'm inclined to say it's not, since it's a fix to the existing behavior to bring it inline with how the native DOM events work. My (limited) understand of the shadow DOM here is this is definitely a fix for the case demonstrated in the Fixture, it's odd that the Box handlers don't fire at all but otherwise everything works.

I guess i have a few outstanding questions @marionebl

  • What is the behavior for mode: "closed" here for the same fixture?
    • I'd still expect Box to fire it's onClick.
  • I'd expect that in that case the target for the bubbled onClick is Shadow, not Box.

Maybe my questions are a bit off, I suppose it probably doesn't make any sense for a component like this to accept children if it's closed BUT we do want to make sure that since all events bubble, that the targets are correct for both modes. My hunch is that we may need fancier logic for that tho...

<Box />
</Shadow>
</TestCase>
</FixtureSet>

This comment has been minimized.

@jquense

jquense Apr 23, 2018

Collaborator

we should probably also include a closed version that ensures documents/asserts the behavior in that case as well

@robdodson

This comment has been minimized.

robdodson commented Apr 24, 2018

My understanding tho is that this is how the Shadow DOM works, it makes the distinction between "closed" and "open", we are talking about "fixing" the case where the tree is open, but not doing what would natively happen.

Open shadow DOM still retargets the event to the shadow host to preserve encapsulation. It's just a bit less strict than closed shadow DOM—where you can't access the internal path at all.

Open shadow DOM example
Closed shadow DOM example

So for a closed shadow root, this approach won't work because composedPath()[0] is also retargeted to the shadow host.

we are talking about "fixing" the case where the tree is open, but not doing what would natively happen.

Even with open shadow roots, the goal is to avoid component consumers from knowing about/relying on the implementation details of a particular widget, which is why the retargeting happens. As @marionebl mentioned, if it's possible to decouple the synthetic event target that developers see, from the target used by React to call handlers, that could be a workable solution.

@jquense

This comment has been minimized.

Collaborator

jquense commented Apr 24, 2018

Ahhhhh ok, thanks for the rundown @robdodson that is very helpful.

if it's possible to decouple the synthetic event target that developers see, from the target used by React to call handlers, that could be a workable solution.

This is possible but i'm not sure there is one good place to do this...What happens is getEventTarget is used in a few places for a few different reasons. The relevant one is here:

let targetInst = getClosestInstanceFromNode(nativeEventTarget);

ReactDOM starts with a native event and derives the appropriate component instances to use in order to call the handlers. It also uses the target here to pass through to event plugins which (generally) pass it through to the Synthetic event.

We could decouple that logic in at the line i linked above, the trick is to get the instance related to the composedPath so listeners are called, but pass through the original target to plugins so that is used as the main target.

I'm not sure tho if that will just work, my hunch is it's gonna get messy and require some check of this logic in a few places which wouldn't be ideal. The other concern here is that currentTarget uses the calling instance, which will be ReactThing and then Shadow, etc as it bubbles up, which will be wrong. Not to mention that the event will fire twice for ReactThing, which also isn't great. Honestly i'm not sure that ShadowDOM behavior can be implemented without specific event dispatching logic, that is currently in a platform agnostic place. It may be that React has to differ from how the DOM works here :?

@balloob balloob referenced this pull request May 29, 2018

Merged

Add scheduler panel #1146

@marionebl

This comment has been minimized.

marionebl commented Nov 5, 2018

Closing as this naive fix won't do it.

@marionebl marionebl closed this Nov 5, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment