Skip to content
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

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

Open
danielhjacobs opened this issue Jul 23, 2024 · 4 comments

Comments

@danielhjacobs
Copy link
Contributor

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

See here:

if (attrs?.xmlns) return document.createElementNS(attrs.xmlns as string, tag, options) as SVGElement;

I already mentioned this in #22 (comment), but it's really a separate issue.

@Lusito
Copy link
Owner

Lusito commented Aug 17, 2024

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
Copy link
Contributor Author

danielhjacobs commented Aug 19, 2024

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
Copy link
Contributor Author

danielhjacobs commented Aug 19, 2024

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 #22 (comment) 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
Copy link

jeffrom commented Aug 28, 2024

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants