Skip to content

Fix insertAdjacentHTML and createContextualFragment not working on non-HTML documents #307

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

Merged
merged 9 commits into from
Jun 4, 2025

Conversation

Gudine
Copy link
Contributor

@Gudine Gudine commented May 29, 2025

The functions Element.insertAdjacentHTML and Range.createContextualFragment resulted in crashes when used in XML documents, as they both rely on functionality from the template HTML element, which only works in HTML documents. I fixed the issue by adapting the logic used in HTMLTemplateElement into a separate utility function.

To test this, I've moved some tests from html/element.js to interface/element.js, as they don't apply just to HTML elements.

I'm not sure how feasible it would be, and it is out of scope of this PR, but it might also make sense to make the interface tests run on all three document types, as such tests would've caught both errors fixed here.

@Gudine
Copy link
Contributor Author

Gudine commented Jun 2, 2025

Thanks for the suggestion! I implemented it to the best of my understanding, though it is still O(n) because of the parent loop.

Also, in the process I added JSDoc to the function, which ended up changing the type of createContextualFragment, but I can remove it if necessary.

@WebReflection
Copy link
Owner

it is still O(n) because of the parent loop.

there is O(n) and O(n) but the whole point in there is that you don't need to bloat RAM and pressure GC with clones and logic that is one shot away so this latest change is the way to go in LinkeDOM, if I took the everything is O(n) anyway approach in here it would be deadly slow, I hope this follow up makes sense to you ... I'll try to find time to review, check/test and publish this MR today or tomorrow.

@WebReflection WebReflection merged commit c578172 into WebReflection:main Jun 4, 2025
2 checks passed
@WebReflection
Copy link
Owner

@Gudine it's up and running: thank you 🥳

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

Successfully merging this pull request may close these issues.

2 participants