Add support for inline SVG element #104
Conversation
crates/html-validation/src/lib.rs
Outdated
| // https://developer.mozilla.org/en-US/docs/Web/SVG/Element | ||
| // a hashmap of `(tag, is_self_closing)` | ||
| static ref SVG_NAMESPACED_TAGS: HashMap<&'static str, bool> = [ | ||
| ("a", true), |
There was a problem hiding this comment.
This might affect the tag in HTML.
There was a problem hiding this comment.
Yeah as this stands now this seems like we'd use create_element_ns for anchor tags even when they aren't inside of an SVG
crates/html-validation/src/lib.rs
Outdated
| ("set", true), | ||
| ("solidcolor", true), | ||
| ("stop", true), | ||
| ("style", false), |
There was a problem hiding this comment.
This might also affect the <style> tag in HTML, there is a need for the parser to determine if this is inside of an element.
crates/html-validation/src/lib.rs
Outdated
| ("polyline", true), | ||
| ("radialGradient", false), | ||
| ("rect", true), | ||
| ("script", false), |
There was a problem hiding this comment.
This might also affect the <script> tag in html.
There was a problem hiding this comment.
Awesome stuff! Thanks so much for diving into this - very excited to see SVG support!
Left some small notes - mainly around adding a couple of tests and tweaks to ensure that this works both now and going forwards.
Note that the test files contain instructions for how to run them but you'll probably need to do
cargo install wasm-pack -f --git https://github.com/rustwasm/wasm-pack first since wasm-pack hasn't published the changes that allow you to run individual tests yet.
crates/virtual-node/src/lib.rs
Outdated
| @@ -217,7 +217,11 @@ impl VElement { | |||
| pub fn create_element_node(&self) -> CreatedNode<Element> { | |||
| let document = web_sys::window().unwrap().document().unwrap(); | |||
|
|
|||
| let element = document.create_element(&self.tag).unwrap(); | |||
| let element = if let Some(ns) = html_validation::get_namespace(&self.tag){ | |||
There was a problem hiding this comment.
We'll want a new test in the create_element tests that shows that this works for an SVG tag.
And also one that shows that anchor tags still work
There was a problem hiding this comment.
BTW, using the create_element for svg elements still attached the svg and its children to the DOM, but it somehow not shown( or misbehaving) in the browser.
crates/html-validation/src/lib.rs
Outdated
| /// Return the namespace of this tag | ||
| /// returns the svg namespace as of now, | ||
| /// any other tags will have a `None` value | ||
| pub fn get_namespace(tag: &str) -> Option<String> { |
There was a problem hiding this comment.
Option<String> feels a little off to me here since every tag has a namespace.
For now can we instead have a is_svg_namespace (tag: &str) -> bool and make use of that in our create_element_node function? This way we don't need to say that None really means HTML
There was a problem hiding this comment.
Yep, that make sense.
crates/html-validation/src/lib.rs
Outdated
| ("animateTransform", true), | ||
| ("circle", true), | ||
| ("clipPath",false), | ||
| //("color-profile",), |
There was a problem hiding this comment.
For everything that is commented out in here mind leaving a comment explaining why the item is commented out?
There was a problem hiding this comment.
I should file an issue how are tags that has a - (dash) in between are done? How is it also done in attributes?
crates/html-validation/src/lib.rs
Outdated
| // https://developer.mozilla.org/en-US/docs/Web/SVG/Element | ||
| // a hashmap of `(tag, is_self_closing)` | ||
| static ref SVG_NAMESPACED_TAGS: HashMap<&'static str, bool> = [ | ||
| ("a", true), |
There was a problem hiding this comment.
Yeah as this stands now this seems like we'd use create_element_ns for anchor tags even when they aren't inside of an SVG
|
You'll want to rebase on top of the latest master branch since I pushed up some conflicts in eb0c6de |
| } | ||
| /// Return the namespace of this tag | ||
| /// returns the svg namespace as of now, | ||
| /// any other tags will have a `None` value |
There was a problem hiding this comment.
looks like this comment needs a quick update since we no longer use None
|
Hmm looks like tests are failing Is |
|
My local test was failing. I just fixed the test to match the expected output. |
|
Thanks! Merging |
This change is