Bubbling event from OutPortal

[alessandro308]

Follow-up from #13

It is a POC that the proposed solution actually works. Now we need to find a way to generalise the caught events. The main issue is to understand, without creating an explicit mapping, how to catch the add event listeners for every React event and then dispatch a new event to the node.element.

The events in React are quite a lot: https://reactjs.org/docs/events.html#supported-events

[CLAassistant]

Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you all sign our Contributor License Agreement before we can accept your contribution.
1 out of 2 committers have signed the CLA.

✅ pimterry
❌ alessandro308
_{You have signed the CLA already but the status is still pending? Let us recheck it.}

[pimterry]

Hi @alessandro308, thanks this looks really promising!

I think there's one issue here though: it's taking React events (the events that React passes to React-managed event handlers like onClick) and passing them into the DOM event API (element.dispatchEvent).

These two events models aren't an exact match, and I don't think that's going to work for all cases. For example, React's onChange handlers are actually called by DOM input events, and many props on events are transformed slightly by React for compatibility between browsers.

I think we probably need to do this entirely underneath React, just using pure DOM events - i.e. using element.addEventListener rather than an onClick prop.

Can you test that out? If that works, it might also give us a solution for the "catch every event" problem, because that's apparently possible with the DOM but not with React.

[alessandro308]

The cons is that we need an extra div to catch the events...

[pimterry]

We do need a wrapper somewhere, I agree, and adding an extra div would be a breaking change that it'd definitely be good to avoid...

There is an existing wrapper div though, and I think we should be able to re-use that for this too. It's available as portalNode.element in most places (it's created and added to the portal node data here).

The way this works (as far as I can remember...) is:

• We manually create a wrapper div (or a <g> for SVGs) when a portal node is created (createHtmlPortalNode)
• When you pass the node to an InPortal, we use React's normal portals to render the InPortal children into that manually created wrapper div.
• When you pass the node to an OutPortal, we replace the contents of that OutPortal with the wrapper div (which therefore brings all the InPortal content with it).
• Because of this implementation, React's portal logic is currently listening to this div, capturing events there, and redirecting them back to the InPortal. We want to capture events here ourselves first, and then re-dispatch them on portalNode.element.parentNode instead.

To be honest it's been a long time since I touched this, but as far as I can tell that existing wrapper is the direct parent of the content at all times, and that's what we should be hooking into for this. Does that make sense?

[alessandro308]

I tried to implement the logic to catch all the events. I think it actually works, even without the wrapper using only the actual existing elements

[pimterry]

Sorry for the delay, this week has been kind of hectic, but I will look at this properly as soon as I can! I think realistically that'll probably be early next week now.

At first glance it all looks sensible to me, I just need to find some time to have a real play around with it and dig into the edge cases.

[pimterry]

This example doesn't seem to work as I was expecting. On my machine, I can still see the events bubbling up into the InPortal.

I've pushed a WIP commit here which extends this story slightly to demonstrate the issue. When I click the 'A' element it shows the 'OutPortal' wrapper event, but when I click the 'expensive' element, it shows the 'InPortal' wrapper event.

With the change we're aiming for, I think it should always show the OutPortal event, right? No events should ever bubble out of the InPortal.

[pimterry]

I'm not sure this is the right place to handle this, which I think might cause the current story behaviour. This method will fire when the InPortal's props change, but that doesn't really happen (the only InPortal prop in this example is node, which is memoized).

Adding some logging & playing with this, it seems that the if test here is always false in this story, so the event redirection never takes effect.

I think you probably want to put this in the portalNode.mount/unmount methods - those always get called any time the node is moved from one place to another, so that's a good place to set up & tear down any event redirection.

[adri1wald]

Hey I'm going to try to implement this. It's a critical requirement for our usecase of react-reverse-portal as we have functionality where the user can drop over elements coming out of an OutPortal, but the dnd handlers are on the parent of the OutPortal

Edit: I can't seem to figure it out :')

Edit 2: I can get the event to the OutPortal parent but only by cloning it, like

e.stopPropagation()
if (parent) {
  parent.dispatchEvent(new e.constructor(e.type, e))
}

This removes necessary properties for the drag event though. Without cloning I get Failed to execute 'dispatchEvent' on 'EventTarget': The event is already being dispatched.

[pimterry]

Ok! If you want to take a shot at this I'm happy to review it and merge it if you can get something working.

This removes necessary properties for the drag event though

Which properties specifically? Can we just clone them manually?

Could we just manually clone all event properties with Object.assign? Could use some kind of blocklist to drop any known-problematic ones, but I would expect that to work for most cases.

[adri1wald]

Hey sorry I never followed up on this. In the end I moved the parent of the OutPortal, which carried the dnd handlers, into the InPortal and that solved my problem.