feat!: Align namespace-switching logic to the HTML living standard tree construction rules#18
Conversation
wooorm
left a comment
There was a problem hiding this comment.
Thanks for your patience!
Looks like a good addition. Some code nits. I also fixed CI, so you can rebase, then it should work!
| tagName, | ||
| currentNamespace, | ||
| properties | ||
| ) |
There was a problem hiding this comment.
If there is such tight coupling, between the call site and the utility function, 3 parameters, 2 return fields, plus only called once, does it really make sense to split them?
There was a problem hiding this comment.
IIRC I split this to reduce the function's complexity. Merged together, the function becomes quite large. That's really the only reason to keep it split, but I can merge it in if you prefer that.
FlippingBinary
left a comment
There was a problem hiding this comment.
Okay, I've made changes based on your feedback. The instrumented document is gone, I refactored the trees in just my tests to use the hast convenience functions when able, and created a helper function to serialize the tree into pairs of namespace and localname strings. That all made the tests so much cleaner.
Also, I had neglected to explicitly test whether html elements are created with createElementNS() when the namespace is set and createElement() when it isn't, so I added tests for that. That will protect against accidentally using createElementNS() for every element, which might cause breaking behavior downstream. Those tests avoid instrumenting document by inferring the creation method from whether the tag's local name became lowercased. That matches the spec, but it's not necessarily intuitive so I mention it in a comment.
I think it's ready for your review again whenever you're available!
| tagName, | ||
| currentNamespace, | ||
| properties | ||
| ) |
There was a problem hiding this comment.
IIRC I split this to reduce the function's complexity. Merged together, the function becomes quite large. That's really the only reason to keep it split, but I can merge it in if you prefer that.
Initial checklist
Description of changes
This pull request is intended to align the namespace-switching logic to the HTML Living Standard § 13.2.6. I needed this done because I was running into a problem when child elements of
foreignObjectwere being created in the SVG namespace instead of the HTML namespace. Since I needed this fixed for my own project whether it gets accepted to the official release or not, I wrote the code without opening an issue. Now that I've got it working, I'm sharing what I've got so far. I hope that's okay.In particular, this changes namespace selection in the following cases:
SVG: In the SVG namespace, the HTML namespace is selected for child elements of
<foreignObject>,<desc>, and<title>.MathML: In the HTML namespace, the MathML namespace is selected for
<math>elements. In the MathML namespace, the HTML namespace is selected for<annotation-xml>elements that have anencodingattribute that matches eithertext/htmlorapplication/xhtml+xml. The standard calls for a text integration point on<mi>,<mo>,<mn>,<ms>, and<mtext>, but I couldn't confidently say if that means the namespace should switch to HTML, so I didn't add special handling for those.Explicit Overrides: The xmlns property values on individual elements override the implicitly inherited namespace, not just the root element.
Two existing tests would fail because of this work, but I tried to follow the convention that was established in the
serializeNodeToHtmlString()helper function by adding the MathML namespace to the string replacement chain. Downstream code that is closely coupled to the previous namespace selection behavior could break, so I would consider this to be a breaking change. I also added a few new tests to assert the intended namespaces rather than modifying existing tests to do the same. Maybe the tests could be consolidated, but I wanted to leave the existing tests as-is to improve confidence in the impact of this pull request.