tsx-dom icon indicating copy to clipboard operation
tsx-dom copied to clipboard

tsx-dom should use document.createElementNS if the closest svg has an xmlns

Open danielhjacobs opened this issue 1 year ago • 4 comments

According to the definition of closest from https://developer.mozilla.org/en-US/docs/Web/API/Element/closest

See here: https://github.com/Lusito/tsx-dom/blob/7440919ac2811bc8b88c397c6bc7d90efd8c28b8/packages/tsx-dom/src/utils.ts#L22

I already mentioned this in https://github.com/Lusito/tsx-dom/issues/22#issuecomment-2241749275, but it's really a separate issue.

danielhjacobs avatar Jul 23 '24 18:07 danielhjacobs

So here are my thoughts on this:

As tsx-dom works in a way, that creates the children without knowing what the parents are, we never know what namespace needs to be used.

Option 1: renderJSX

It might work by rewriting it, so that the JSX-Syntax doesn't return the HTMLElement, but rather a virtual dom and then a renderJSX function would create the actual dom. But that would change the project drastically.

Option 2: Whitelist

So I'm wondering if, instead, it might be an option to have a whitelist of SVG-Only element names, which will automatically add the SVG namespace. That wouldn't work with shared element names like video, audio, canvas, iframe, a, style, title (not sure if that list is complete). So those elements would still have to specify xmlns manually.. though I am not sure if there actually is a difference for those elements. Not sure what exactly changes for createElementNS under the hood

Option 3: Prefix for elements

It might be an option to make a shortcut like <svg:a>, but I'm not sure I like that idea or if it might create issues down the road with other users.

Option 4: Submodule import

Another alternative might be to not use the new (import-less) JSX transform and instead do something like this for the entire file:

// MyIcon.tsx
import { h } from "tsx-dom/svg"; //notice the submodule import

const MyIcon = () => (
    <svg>...</svg>
);

This would only work for the whole file though.

Option 5A: setDefaultNamespace

And finally, it might be a possibility to expose a setDefaultNamespace function, so that you can do this:

const MyIcon = () => {
    try {
        setDefaultNamespace("http://www.w3.org/2000/svg");
        return (
            <svg>...</svg>
        );
    } finally {
        setDefaultNamespace(null);
    }
}

Option 5B: withNamespace

Or (maybe in addition to 5A)with a helper-function, which does it automatically for you:

const MyIcon = () => withNamespace("http://www.w3.org/2000/svg", () => (
    <svg>...</svg>
));

Lusito avatar Aug 17 '24 10:08 Lusito

Option 1: renderJSX

Too big a change, seems too complex and different, almost like creating a different project

Option 2: Whitelist

Not a bad idea, but needs to be combined with one of the other options too. If you look at https://developer.mozilla.org/en-US/docs/Web/SVG/Element/script, it is different from https://developer.mozilla.org/en-US/docs/Web/HTML/Element/script, where the prior has href while the latter has src.

Option 3: Prefix for elements

Could combine with option 2 to make a good solution. The downside is no ability to specify multiple namespaces, as if both of these options were used and none of the others the namespace would likely always be assumed to be "http://www.w3.org/2000/svg". However, I've never specified different namespaces when working with svg elements myself and doubt it's very common.

Option 4: Submodule import

This would only work for the whole file though.

The particular project I'm referring to would need to further split the tsx components in that case. That may be true for others too. See https://github.com/ruffle-rs/ruffle/blob/master/web/packages/core/src/internal/ui/container.tsx, which contains some SVG elements inside some HTML elements.

Option 5A: setDefaultNamespace

Would this only work on SVG elements? If so, the file I linked above would still need changes (though that's fine), I'm just curious what your thought would be there.

Option 5B: withNamespace

Same question as above. I will say this helper function feels a bit nicer as the package user needs to change less code.

I lean towards options 2 and 3 or option 5 (B or both). I'll ask others.

danielhjacobs avatar Aug 19 '24 18:08 danielhjacobs

Option 6 is to say that requiring xmlns inside svg elements is expected, suggest it whenever using tsx-dom even though it's not required by normal HTML, and as stated in https://github.com/Lusito/tsx-dom/issues/22#issuecomment-2243050512 suggest adding xmlns to every SVGElement in the library (so the current situation but without a need to expand type definitions). That's a bit hacky though, and potentially a lot of repeated references to xmlns, so I don't like that idea very much.

danielhjacobs avatar Aug 19 '24 18:08 danielhjacobs

Would an option be to use withNamespace internally on svg tags after the existing check? That wouldn't require any API changes and would be minimally breaking. I think this approach would also just work for most users who want to use svg, assuming they follow the spec and wrap all svg elements in an svg tag.

My opinion of the existing choices would be for adding withNamespace. I also like 2, but it sounds larger code-wise than 5B and also still leaves the user to do extra work when there is overlap between html and svg element tags.

jeffrom avatar Aug 28 '24 18:08 jeffrom

Sorry, this completely slipped my mind. I'll make sure that something comes up with the next major version. Probably something with option 5 a/b. Maybe even something like push/popDefaultNamespace, so the previous namespace would be restored.

@danielhjacobs setDefaultNamespace would work for everthing, not just SVG.

@jeffrom If I'm getting your idea correcly, that would not work. At the point where the svg tag gets created internally, all children have already been created, so the problem already exists.

One idea arround this would be to have a custom component with a render function like this:

const MyIcon = () => (
     <Svg /* add normal props */ render={() => (
         <rect width="200" height="100" x="10" y="10" rx="20" ry="20" fill="blue" />
     )} />
);

That way, the Svg Component can call setDefaultNamespace before calling the render function.

Or:

const MyIcon = () => (
     <XmlNs namespace="http://www.w3.org/2000/svg" render={() => (
         <svg>...</svg>
     )} />
);

Could even provide both and have <Svg> as an alias for <XmlNs namespace="http://www.w3.org/2000/svg">

Lusito avatar Mar 21 '25 20:03 Lusito

Hmm, due to the lack of fragments, the Svg component is not a nice option, but the other APIs work nicely.

You can see this on the next branch if you want to try: https://github.com/Lusito/tsx-dom/blob/next/packages/tsx-dom/src/namespace.spec.tsx

Lusito avatar Mar 21 '25 22:03 Lusito

For tsx-dom-ssr, I can actually use a simpler approach:

  • svg elements will have an implied namespace set to "http://www.w3.org/2000/svg" if not specified otherwise via xmlns attribute.
  • Any element created as a child of an element that is being created with an xmlns will inherit that namespace unless it has an xmlns attribute specified manually or it is an svg element.

So SVG elements just work without any namespace specification: https://github.com/Lusito/tsx-dom/blob/next/packages/tsx-dom-ssr/src/svg.spec.tsx

Lusito avatar Mar 22 '25 13:03 Lusito