Lusito / tsx-dom

Lightweight DOM Libraries
https://lusito.github.io/tsx-dom/
MIT License
49 stars 17 forks source link

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

Open danielhjacobs opened 3 months ago

danielhjacobs commented 3 months ago

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.

Lusito commented 2 months ago

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>
));
danielhjacobs commented 2 months ago

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 commented 2 months ago

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.

jeffrom commented 2 months ago

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.