-
Notifications
You must be signed in to change notification settings - Fork 18
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
Comments
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: renderJSXIt might work by rewriting it, so that the JSX-Syntax doesn't return the HTMLElement, but rather a virtual dom and then a Option 2: WhitelistSo 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 elementsIt might be an option to make a shortcut like Option 4: Submodule importAnother 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: setDefaultNamespaceAnd finally, it might be a possibility to expose a const MyIcon = () => {
try {
setDefaultNamespace("http://www.w3.org/2000/svg");
return (
<svg>...</svg>
);
} finally {
setDefaultNamespace(null);
}
} Option 5B: withNamespaceOr (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>
)); |
Option 1: renderJSXToo big a change, seems too complex and different, almost like creating a different project Option 2: WhitelistNot 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 elementsCould 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
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: setDefaultNamespaceWould 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: withNamespaceSame 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. |
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. |
Would an option be to use My opinion of the existing choices would be for adding |
According to the definition of closest from https://developer.mozilla.org/en-US/docs/Web/API/Element/closest
See here:
tsx-dom/packages/tsx-dom/src/utils.ts
Line 22 in 7440919
I already mentioned this in #22 (comment), but it's really a separate issue.
The text was updated successfully, but these errors were encountered: