Allow overriding xmlns #268
Conversation
A foreignObject within an svg tag should not lose its SVG_NAMESPACE, because it is still an SVG element. In addition, children of a foreignObject element should be allowed to specify their xmlns, since it is likely they will be some other NS than the foreignObject, e.g. xhtml. This PR allows elements to specify their xmlns, when needed.
Note: requires tweaking crank.js to accept xmlns: bikeshaving/crank#268
|
Thank you for being proactive with this PR! I will review it ASAP and I apologize about any blockers. |
There was a problem hiding this comment.
Thanks for the PR! Some general thoughts:
- I’m not overly familiar with the way the
xmlnsattribute is meant to work, and there is no corresponding MDN page. I’m curious about the use-case / difficulties you’re running into. - The natural place for tests would be
test/svg.tsx - If there are broader implications for how
xmlnsattribute should work outside offoreignObjectthere might be some additional work that we have to do in thecreate()method? Maybe not?
src/dom.ts
Outdated
| xmlns = undefined; | ||
| break; | ||
| case "svg": | ||
| xmlns = SVG_NAMESPACE; | ||
| break; | ||
| } | ||
|
|
||
| return xmlns; | ||
| return props?.xmlns ?? xmlns; |
There was a problem hiding this comment.
The optional chain operator ?. is unnecessary because props will always be defined. And as a stylistic thing, I’d prefer a boolean short-circuit over a nullish coalescing operator ?? insofar as the codebase doesn’t use it yet.
|
Ok, updated based on your comments @brainkim. Thanks for the feedback & for considering this change. |
There was a problem hiding this comment.
Ahhh, I’m getting a better sense of the problem. My assumption when throwing together <foreignObject> support was that the element took an xmlns attribute <foreignObject xmlns="whatever">, and its children would all be under one namespace or another. But normal behavior is to assume the children are SVG.
So there doesn’t need to be special casing for the foreignObject element, and we can rely on the xmlns prop.
TODO (for me):
- Double-check that this is what the HTML5 spec actually indicates. (Closest official spec reference I could find https://svgwg.org/svg2-draft/embedded.html#InterfaceSVGForeignObjectElement)
- Check that the HTML string renderer doesn’t need similar changes. (HTML renderer has no need for xmlns stuff)
|
F- IT, I’LL FIX THE LINT ERROR ON MAIN |
|
Shipped in 0.5.6 |
|
Thanks! |
A foreignObject element within an svg tag should not lose its SVG_NAMESPACE, because it is still an SVG element.
In addition, children of a foreignObject element should be allowed to specify their xmlns, since it is likely they will be of some other namespace than the foreignObject, e.g. a child div would need to be xhtml.
This PR allows such child elements to specify their xmlns, when needed.