SVG support

[spautz]

This is an attempt to address issue #2 -- I've tested it locally, and added a storybook story for it, but I haven't tested it very extensively.

There are two noteworthy changes here:

• A string may be passed to createPortalNode to set the element type which will be used for the portalNode. I noticed that a few of the stories were passing this argument, but that it wasn't actually being used.
• On portalNode mount/unmount, if the content is an SVGElement then it will transfer its innerHTML through the portal instead of the dom elements themselves. This makes the repro example from issue Does not work with SVGs #2 work, and the innerHTML technique was suggested by several Stackoverflow threads, but I'm not sure whether it's the right way to do this.

Other approaches I tried, for the second bullet point:

• Swapping in the element's childNodes through the portal (instead of the element itself, or its innerHTML). This feels like it should be the right way to support elements in contexts where you can't have a <div>, and it did arrange everything correctly in the dom, but it still didn't render correctly.
• Deep-cloning the dom elements and appending them explicitly, instead of replacing the prior child. This gave the same result: everything looks right in the inspector, but the text in the svg doesn't render despite the element being in the right place.

[pimterry]

Thanks for taking a shot at this! It's quite tricky 😄. I think you're on the right track, but this isn't quite there yet.

I'm not totally sure, but I'd think it's likely that the rendering problems you're talking about with replaceChild might be caused by the document.createElement issue (see my comment). If you take a shot at fixing that, that might let you use replaceChild, and then this whole thing might get simpler.

Give it a go, happy to chat or help if that'd be useful. I'd recommend testing out some of the existing HTML tests with SVGs if you want to stress test this a little and see how it behaves.

[pimterry]

Viewing the story here in Firefox gives me a React warning:

Warning: The tag <text> is unrecognized in this browser. If you meant to render a React component, start its name with an uppercase letter.

I think that's because SVG elements are supposed to be created explicitly with document.createElementNS("http://www.w3.org/2000/svg", tagName), not with document.createElement(tagName). See https://stackoverflow.com/questions/39457888/after-creating-a-text-element-with-javascript-why-cant-i-getbbox and facebook/react#11899 (comment).

I'm not totally sure how React deals with this generally, but I'd guess that it has a list of SVG tag names, and calls the right method for each type. Happy to do that or something better, if you have any ideas?

[spautz]

Thanks for the feedback and investigation! I didn't realize things like video-in-svg were even possible. I'll do some more research and give the createElement/createElementNS stuff a try.

[spautz]

I think I have it working -- I'm going to do some cleanup and update this branch/pr shortly, if the general strategy seems sound.

• Instead of creating the portal's dom element within createPortalNode, it's created within mount()
• Inside mount() we're able to look at newParent to determine whether or not it's an SVG element -- so we can then either createElement or createElementNS as appropriate.
• The newly-created element gets saved to the element property of portalNode, and then instead of doing a replaceChild(portalNode, newPlaceholder) we do a replaceChild(portalNode.element, newPlaceholder) (and vice versa on unmount)
• The newly-created portal element can copy its parent's tagName -- so there's no need to send that to the constructor

With those changes the svg elements are going through replaceChild without any special checks, and without mutating the placeholder. DOM state appears to remain intact: I have a story which moves a foreignObject video around within the svg, and its playing state remains intact.

There is a potential breaking change:
Because the element is now created after the portalNode instance is created, the portalNode functionality (mount, unmount, setPortalProps, getInitialPortalProps) is no longer being attached to the element directly. Instead of portalNode being the element itself, it's a plain object which contains the element. This seems to work fine -- all of the stories still work -- but if somebody was mutating their portalNode (as suggested by the className note in the readme) then they'd have to mutate portalNode.element instead.

Two other notes:

• Firefox has an issue where it doesn't honor the alignmentBaseline property correctly for , which results in the text being rendered outside of its container when x and y aren't set explicitly for it.
• In addition to html and svg, React supports mathml: https://github.com/facebook/react/blob/b87aabdfe1b7461e7331abb3601d9e6bb27544bc/packages/react-dom/src/shared/DOMNamespaces.js#L8-L10 . It looks like support for that could be added in exactly the same way.

[spautz]

I ran into an issue with the InPortal/OutPortal render order: because OutPortal renders the placeholder and mounts the portalNode, only it can determine whether or not we need svg. So when InPortal renders first we don't yet know whether the portalNode element should be svg or non-svg -- and replaceChild only works properly when we render the right kind of element.

I've prototyped out a few different options, but none of them feels like the clear solution:

A) Require the caller to tell us whether or not we're dealing with SVG. This is easily done as an argument to createPortalNode().

This is an easy solution, but it feels like an undue burden on consumers -- especially since omitting the value would lead to a broken UI.

B) If the InPortal renders before the OutPortal, trigger a rerender of InPortal -- and by the time the rerender happens, OutPortal should have created either a svg element or a non-svg element. This can be done with a setState during render to queue up the rerender. (That causes a console error, as it's clearly an anti-pattern, but it works well in this case.)

This works well today, but I suspect it'll run into issues with asynchronous/interruptible rendering in the future. Putting the InPortal's setState inside a setTimeout removes the warning but causes a visible flicker.

C) Inspect the InPortal's children to guess whether or not we're dealing with svg. We'd need a list of tagName strings which should be treated as svg for this to work. (Right now, OutPortal looks at the element itself to decide whether or not it's svg, which is more reliable than just checking its name.)

This works in general, but there are a few edge cases which could be dangerous: <a> is valid both as svg and as plain html, for example, and in theory almost any tag name could be used as a custom element -- so relying on tag name doesn't seem like an ideal strategy to me.

D) If the InPortal renders before the OutPortal, attach a callback to the portalNode to rerender the InPortal. Then, once the OutPortal determines whether or not we're dealing with svg and creates the appropriate portal element, it can rerender the InPortal (which then works properly because the svg or non-svg portal element is in place). This is very similar to (B), but I think it'll work even with async rendering.

This feels kinda dirty to me, but it works well and emits no console errors or warnings. It's definitely a coupling between the two components, though, through a non-obvious channel. It should work fine with asynchronous/interruptible rendering, I think, and there is no visible flicker in the UI, so this is the best option I've found so far for dealing with this issue.

---

Any guidance or suggestions? (And apologies for the two walls of text; I know there's a lot here.)

[pimterry]

Wow, nice work! Thanks for all the investigation, this is great. I'm rushed off my feet right now, but I'll dig into this properly next week, looks like there's some promising options 👍

[spautz]

Just to float out a new thought, for when you're back: (and no hurry on any of this -- I appreciate your time)

After playing around a bit more, I'd like to propose another revision to this to:

• Allow the caller to specify the element type the portal is for, like in idea A: either 'div' or 'svg' (and that could be expanded for MathML in the future, or other cases like if desired.)
• If no type is specified, and OutPortal is rendered first, infer the type based on its parent's type, just like in this PR. No onReady callback needed.
• If no type is specified, and InPortal is rendered first, assume 'div' so that it works the same as before.
• If at any time an OutPortal detects that the portalNode is the wrong type, emit a warning or error. This would work just like the "infer the type based on its parent's type" behavior in this PR, except it could tell the user how to fix it.

Here's the reasoning, and the issues I ran into with the current PR:

• The onReady callback approach (idea D, and what's currently in this PR) delays mounting the InPortal contents until the OutPortal has been created, but only for its first render -- so if components inside the InPortal are recording timestamps or have side effects inside lifecycle methods or a useEffect, the results might seem unpredictable if the OutPortal isn't rendered right away, and if it gets moved or hidden later.
• The current approach of using the tagName of the OutPortal's parent works well for simple cases but if there's global styling for the tag (e.g. if inside a <button>) then the styling gets doubled and it looks awful. 😅 The prior approach of always using a <div> for was better for non-svg, I think -- so: element type instead of tagName.
• On the last bullet point above: instead of just emitting a warning, the portalNode could create a new element if it detects that the wrong type is being used. Unfortunately, that only works if the InPortal renders before the OutPortal: if the order is reversed then it'll be broken until the next render. There may also be side effects of replacing the portalNode element after initial render, like losing any previous dom state (although I wasn't able to make that happen).

I can update this with those changes, although it may be a few days.

[pimterry]

Ok, this is really interesting! Great stuff. I think:

• Your explanation of why onReady is difficult makes sense. I suspect there's a lot of InPortal/OutPortal rendering situations that will make any of those kind of deferred rendering tricky very difficult to do 100% reliably. This will get harder still as React does more concurrent rendering soon. Let's avoid this entirely.
• I think we can assume in general that users will be moving HTML content within different HTML portals, or SVG content between SVG portals, but they won't be mixing and matching the two for the same node. In most cases that can't work anyway - moving an SVG <image> into your HTML or an <img> into your SVG won't do what you expect. If they really need to do that, they could do it with HTML content by placing it as normal into HTML, and then wrapping any OutPortals within SVGs in <foreignObject>. But they probably shouldn't.
• Given that, I think we should strongly encourage users to be explicit on the portal node about which type they're using, so we always know in advance.
• Once we have that, the InPortal doesn't need to do anything special I think, that's easy.
• For the OutPortal:
  • It's cleaner if it knows in advance whether it's rendering into an SVG or HTML node, and the user always knows this, so we should make users be explicit here too.
  • When it mounts, it should check if the parent is an HTML or SVG element, and complain if this doesn't match what the user explicitly said.
  • It then needs just creates an appropriate placeholder for its type: <div> or <g> (no need to copy any parents, we always explicitly know the type in advance)
  • If a portal node is provided, we need to check that that's the right type too
• Because everything is now always the right type of element in each place, we can use replaceChild etc as normal and it should work fine, I think.
• You're right that we could warn if you get this wrong, and try to work around it. Since there's a high risk that it will behave unexpectedly though, I think we should go further: we should just throw an error, and refuse to render at all. AFAIK there's no good use case for rendering an HTML node in an SVG portal or vice versa, and even if it 'works' it may well have odd behaviour and edge cases. It would be better to guarantee that we always do the right thing.

In terms of doing this in practice, rather than using tagName and inferring from there, I propose we split the package up a little to have separate methods/components for each (sharing quite a bit of code ofc) so that you use it like:

import * as portals from 'react-reverse-portal';

const svgNode = portals.svg.createPortalNode();
const htmlNode = portals.html.createPortalNode();

// To define InPortals:
<portals.svg.InPortal node={svgNode} />
<portals.html.InPortal node={htmlNode} />

// To render an OutPortal:
<svg><portals.svg.OutPortal node={svgNode} /></svg>
<div><portals.html.OutPortal node={htmlNode} /></div>

If you know you only want one, you can do that with:

import { html as portals } from 'react-reverse-portal';

// Exactly the same API as today, works for any HTML:
const node = portals.createPortalNode(); // An HTML node
<portals.InPortal> // an HTML InPortal
<portals.OutPortal> // an HTML OutPortal

// Same thing works with import { svg as portals }, for the SVG-only case.

Last, to avoid breaking changes and for general convenience, we can keep the existing API on the root package, as an alias for portals.html, but deprecate it (users should use html/svg explicitly instead). There is an argument for keeping this, defaulted as HTML... I'm leaning the other way but I could be persuaded.

Advantages of all this explicitness:

• We never have to infer anything or defer one thing until we check something else or any of that, ever ever
• It's super easy to do the right thing, and only a tiny bit more friction (add { html as portals } to the import, or .svg/.html to each site where you use it).
• If you ever do the wrong thing, we know it for sure, and we can immediately make it super clear. We should be able to get TypeScript to enforce this at compile time too.
• We can still support tagName later if we want, without ever having to infer which names need creating as which kind of node

Does that make sense? Am I missing anything? You've done a lot of digging into the issues, of which there are quite a few, so it's hard to be sure I've covered everything! In general I think this will work though, and be fairly easy to use too.

[spautz]

Thanks! I agree that the explicitness seems best. I think I can update this PR with those changes by this weekend.

The only comment I have is on the entry -- whether the element type (html/svg) is best specified via the import (import { html as portals } ...) or as an argument to createPortalNode (as createPortalNode('html')).

There are only two lines of code different between html mode and svg mode (the createElement at init, and the error check at runtime), and if element type is an argument then we can just set a default to avoid breaking changes (const createPortalNode = (elementType = 'html') => ...). In either case we'll need to split the typings to be more explicit (HtmlPortalNode and SvgPortalNode?) so I think either route can be enforced at compile time. It feels to me like the elementType route might be less disruptive, but that's just opinion.

I've been playing around with other types as well (like Math and inline text) -- they seem to only affect the same two spots as svg, although I'm not 100% certain. The potential of supporting four element types seems possible to me, though -- and it should work either as 4 importable namespaces or as 4 allowed elementType values. I'm happy to submit whichever approach you think is best.

[pimterry]

Thanks! I agree that the explicitness seems best. I think I can update this PR with those changes by this weekend.

Ok, great! No hurry of course, whenever you have time.

The only comment I have is on the entry -- whether the element type (html/svg) is best specified via the import (import { html as portals } ...) or as an argument to createPortalNode (as createPortalNode('html')).

Yeah, it's quite debatable either way! If there are clear problems I'm definitely open to alternative approaches. There's a couple of reasons I'm leaning towards a fully namespaced API though:

• We want to know in advance whether it's SVG/HTML/other for OutPortals too, not just for createPortalNode. Right now InPortals don't care, but plausibly they might in future? You would need to set the type name separately for all of them, everywhere, e.g. <portals.OutPortal elementType='html' />. It just feels a bit annoying & repetitive, whilst with a namespace you can import everything once with import { html as portals }... and then safely use portals.* everywhere, as now.
• It makes it a bit more explicit to users that the two types of portal are fundamentally separate & incompatible if they're two APIs in separate namespaces, and it's worth trying to make that obvious, so that people don't get caught out by incompatibility later on.

Internally the two namespaces can still share almost all their code of course, totally agree they're very similar, it's just the exposed API that I'm thinking about here.

Overall I'm not really that worried about short-term disruption. For people who don't need SVG, they don't need to upgrade at all, so they don't care. For people who do need SVG they do need to upgrade, but they need to think about their portal types anyway.

I'm somewhat tempted to say we should even increase the disruption, just to avoid any confusion or compatibility issues. Instead of keeping the current API, we'd namespace everything, so no backward compatibility at all, and this PR would be the start of react-reverse-portals v2, where you must import either portals.html or portals.svg. Upgrading wouldn't be automatic, but existing users just need to do a find/replace on their imports from import * as portals to import { html as portals }, and then they're all good. What do you think?

I've been playing around with other types as well (like Math and inline text) -- they seem to only affect the same two spots as svg, although I'm not 100% certain

Awesome! That sounds great. I'm not exactly sure what you mean about inline text here though, can you explain a little more?

[spautz]

You would need to set the type name separately for all of them, everywhere, e.g. <portals.OutPortal elementType='html' />

I agree this would be very annoying, but I don't think it'd be necessary because the node prop has to be passed in: the node will have been given a type at creation, so if OutPortal or InPortal ever needed to change behavior based on node type, it will be able to look at that. (I think I communicated it poorly before, but at the moment even OutPortal doesn't care about the type: the html/svg behavior is encapsulated inside the node's mount())

Also, somewhat selfishly: being able to use the same InPortal and OutPortal for different types of nodes -- where each node has a firm, explicit type set at creation -- makes the project I'm working on a little cleaner. In it, I'm storing a collection of portalNodes and blindly rendering their portals with a bit of conditional logic. Applying the html/svg type at creation is easy, but if InPortal and OutPortal are namespaced for rendering as well then that's one more thing to track. It's certainly doable, just not as direct. :-)

It makes it a bit more explicit to users that the two types of portal are fundamentally separate & incompatible if they're two APIs in separate namespaces

Very good point. Here's a possible third idea that might fit both: what if the createPortalNode were split into separate namespaces or entry points, instead of the entire portals namespace? Something like one of these?

import { createHtmlPortalNode, InPortal, OutPortal } from 'react-reverse-portal';
const myPortalNode = createHtmlPortalNode();
// <InPortal node={myPortalNode}>

import { htmlNode, InPortal, OutPortal } from 'react-reverse-portal';
const myPortalNode = htmlNode.create();
// <InPortal node={myPortalNode}>
// with room to add other things later, like htmlNode.debug()

So then the portalNode creation is always fixed and explicit -- and the existing createPortalNode can be given a deprecation warning -- and because the node must be given to everything else we won't hit any situation where the type isn't known. It avoids any accidental/implicit defaults that a param would have, and the code is still easy to unify under the hood (and probably easier to make separate typings for, too). Consumers would still have some disruption when upgrading, but it'd be one find/replace even if they're using named imports instead of *.

Thoughts? As before, I'll go with whatever option you choose. Any of them will work okay for what I'm doing, in the end, but this seemed like a possible middle idea.

I'm not exactly sure what you mean about inline text here though, can you explain a little more?

It's mostly just a "hey this is neat" capability I was experimenting with. :-)

I was playing around with textNodes (instead of full elements) and what it might take to do a portal for text within a sentence, just to see how it might work. Because the portals currently use a <div> it breaks up the sentence unless you manually override its styles from outside.

I tried a few routes:

• Using createTextNode for the portalNode (instead of createElement) doesn't work because you can't make React portals on text nodes.
• If the portal is guaranteed to have exactly one child, you could replaceChild above the placeholder so that the portal itself gets replaced with whatever goes into the portal (i.e., instead of the portal rendering a <div> around whatever went into the portal, it only renders the single element that went into the portal). I think this worked even for a textNode -- so that the dom ended up with three textNodes in a row, in the sentence -- but it was very brittle since it was messing with nodes we didn't create: if something disturbed the OutPortal it would all come crashing down.
• Using a <span> instead of a <div> for the portal node was simple, easy, and worked on the first try. It lets you portal text (or other inline elements) directly into or out of a sentence -- and the implementation is exactly the same as for html/svg: you just change the document.createElement used to make the portal node and everything works.

So, inline elements don't seem like a common case to me, but if anybody did need to portal around inline elements instead of block elements then making an entry point or namespace for text seems straightforward.

[spautz]

I think the branch is updated with all the changes now: the onReady stuff is removed and the portalNode element is back to being created with the appropriate type within createPortalNode. I built out both approaches to exports, and it should be easy to remove/replace whichever one we don't need.

The typings took a bit of work, but I think it's all good now: I used a technique I'd done once elsewhere (with the shared code using unified types that aren't exported, while exporting only the specific types for the public api) but if there's a better way I'd love to learn it.

I've tested it pretty thoroughly, and everything seems to work as before.

[pimterry]

This is very cool. A couple of comments to discuss, but I'm definitely sold on the approach, and I'm happy to go for the create{Html,Svg}PortalNode API instead of namespacing.

The one last extra thing we need to do is to update the docs for this, but then it's probably good to go I think.

[spautz]

Notes on sending 'direct' content through the portal:

The idea and goal:

• Today the children of InPortal are wrapped in a portal node element (portalNode.element, the appropriately-namespaced DOM element created in create(Html|Svg)PortalNode), and then that gets rendered in place of OutPortal.
• If we could instead render the childNodes themselves -- without any namespaced wrapper -- then a lot of things seem like they'd work better.

Possible benefits:

• If you pass only a textNode into the portal, then only the textNode would come out -- no extra <div>, so inline elements work automatically. This was what I hoped to do initially.
• After digging through React's code (in this comment, above) it seems like the root cause behind the SVG-support issue is that same portal node element: the element from document.createElement('div') reset the namespace from SVG to HTML, which then resulted in SVG not working because React created the SVG elements in the HTML namespace. So, if we can directly render content from InPortal to OutPortal, without adding any wrapper, we'd avoid changing the namespace -- and so we wouldn't need namespace-specific portal nodes.

What I tried:

• Rendering only the contents sent into the portal, without the portalNode.element wrapper, is trivial when the portal has a single child: instead of using node.element we just use node.element.firstChild everywhere.
• Alternatively, instead of trying to use node.element.firstChild in place of node.element, it might work to use node.element.firstChild as the portal itself (instead of passing node.element to createPortal()).

Problem:

• node.element.firstChild works on mount and initial render, but the app errors when unmounting because it cannot remove that node.
• I traced this to removeChildFromContainer at ReactDOMHostConfig.js#L534, and that comes from the component's Fiber's stateNode to determine what to remove: ReactFiberCommitWork.js#L1251
• So, as near as I can tell, it tries to remove the DOM elements based on the state/arrangement they were in when they were rendered -- which (if correct) means trying to swap in the firstChild cannot possibly work because it wasn't rendered for the portal initially: node.element was.

Current conclusions and thoughts:

• Directly rendering the portal content seems like a nice idea, but we must use a real dom element for the portal's container (React doesn't support using a textNode), and it seems like that element must remain in the dom later (or else we get an error when React tries to remove it) -- so the intermediate element seems unavoidable.
• That basically brings us back full circle: the intermediate element needs to be created with the right namespace (and it is now), and that gives us everything we need for SVG or MathML to work. I'm not sure what else to try, but it feels like the current solution should be enough.

[pimterry]

it tries to remove the DOM elements based on the state/arrangement they were in when they were rendered -- which (if correct) means trying to swap in the firstChild cannot possibly work because it wasn't rendered for the portal initially: node.element was.

I think it could still work, as we can use componentWillUnmount to run code before react unmounts. In theory we can put the DOM back into the right state there, for whatever possible state it started in.

That said, doing that without a placeholder is very difficult, especially since other sibling node could be updated & changed in the meantime. It might be nice to do that one day but for now I don't think it's a big deal, I'm happy to leave it.

[pimterry]

Ok, I think this all looks good to go now. Anything essential left that you're aware of? If not, I'll merge and get this out as a breaking release, and we can finally close off #2 😄

[spautz]

Looking over the diff one last time, I noticed an accidental inaccuracy in the docs I wrote, where I'd blamed the SVG issue on browsers before that last bit of research. I just pushed to 6870bb5
remove that. I think that covers everything.

Unrelated to this PR, I'd prototyped portal nodes for <math> and <span> content, and can push that as a separate (non-breaking) PR if it'd be useful -- although I don't know whether or not either of those would ever actually be used. Chrome doesn't even support MathML without a polyfill.

In any case, thanks for all your help and time with this!

[pimterry]

Merged, tested in the HTTP Toolkit UI, added a couple of tweaks (we'd lost the portal generic types along the way), and now released as v2.0.0 🎉

I'm happy to look at a MathML etc PR too, if that'd be useful to you. Until there's somebody with a specific use case I don't think it's critical, but I have no problem with adding support for it anyway if you're keen to look into it.

Thanks again for all your hard work on this. It's a great result, I'm really happy with how it's all turned out! 👍 👍 👍