Skip to content
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

#451 fixes missing inner html in html template #506

Conversation

Mas0nShi
Copy link
Contributor

@Mas0nShi Mas0nShi commented Jun 15, 2022

Fixes #451

@Mas0nShi Mas0nShi changed the title 451 fixes missing inner html in html template #451 fixes missing inner html in html template Jun 15, 2022
@Mas0nShi
Copy link
Contributor Author

I can't find the better ways to process innerHTML, i overwrite innerHTML getter & setter , this doesn't seem like a good way to go, and if you have a better idea for solving this, reject it.

@capricorn86
Copy link
Owner

capricorn86 commented Jun 15, 2022

@Mas0nShi I don't have a better suggestion at the moment. Maybe we can improve this in the future by extracting setting and getting HTML to a utility.

We also need to sovle this for: get outerHTML(), set outerHTML() and getInnerHTML().

I also suggest that we replace get content() with just a property for performance.

I noticed that cloneNode() is wrong as well. It has to clone the content element.

My suggestion is to do this:

import Node from '../node/Node';
import HTMLElement from '../html-element/HTMLElement';
import IDocumentFragment from '../document-fragment/IDocumentFragment';
import INode from '../node/INode';
import IHTMLTemplateElement from './IHTMLTemplateElement';
import XMLParser from '../../xml-parser/XMLParser';
import XMLSerializer from '../../xml-serializer/XMLSerializer';
import DOMException from '../../exception/DOMException';

/**
 * HTML Template Element.
 *
 * Reference:
 * https://developer.mozilla.org/en-US/docs/Web/API/HTMLTemplateElement.
 */
export default class HTMLTemplateElement extends HTMLElement implements IHTMLTemplateElement {
	public readonly content: IDocumentFragment = this.ownerDocument.createDocumentFragment();

	/**
	 * @override
	 */
	public get innerHTML(): string {
		return this.getInnerHTML();
	}

	/**
	 * @override
	 */
	public set innerHTML(html: string) {
		for (const child of this.content.childNodes.slice()) {
			this.content.removeChild(child);
		}

		for (const node of XMLParser.parse(this.ownerDocument, html).childNodes.slice()) {
			this.content.appendChild(node);
		}
	}

	/**
	 * @override
	 */
	public get outerHTML(): string {
		return new XMLSerializer().serializeToString(this.content);
	}

	/**
	 * @override
	 */
	public set outerHTML(_html: string) {
		throw new DOMException(`Failed to set the 'outerHTML' property on 'Element': This element has no parent node.`);
	}

	/**
	 * @override
	 */
	public get previousSibling(): INode {
		return this.content.previousSibling;
	}

	/**
	 * @override
	 */
	public get nextSibling(): INode {
		return this.content.nextSibling;
	}

	/**
	 * @override
	 */
	public get firstChild(): INode {
		return this.content.firstChild;
	}

	/**
	 * @override
	 */
	public get lastChild(): INode {
		return this.content.lastChild;
	}

	/**
	 * @override
	 */
	public getInnerHTML(options?: { includeShadowRoots?: boolean }): string {
		const xmlSerializer = new XMLSerializer();
		let xml = '';
		for (const node of this.content.childNodes) {
			xml += xmlSerializer.serializeToString(node, options);
		}
		return xml;
	}

	/**
	 * @override
	 */
	public appendChild(node: INode): INode {
		return this.content.appendChild(node);
	}

	/**
	 * @override
	 */
	public removeChild(node: Node): INode {
		return this.content.removeChild(node);
	}

	/**
	 * @override
	 */
	public insertBefore(newNode: INode, referenceNode: INode): INode {
		return this.content.insertBefore(newNode, referenceNode);
	}

	/**
	 * @override
	 */
	public replaceChild(newChild: INode, oldChild: INode): INode {
		return this.content.replaceChild(newChild, oldChild);
	}

	/**
	 * @override
	 */
	public cloneNode(deep = false): IHTMLTemplateElement {
		const clone = <IHTMLTemplateElement>super.cloneNode(deep);
		(<IDocumentFragment>clone.content) = this.content.cloneNode(deep);
		return clone;
	}
}

Copy link
Contributor Author

@Mas0nShi Mas0nShi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

changes HTMLTemplateElement.ts

@Mas0nShi
Copy link
Contributor Author

replaceWith method is exists?

	/**
	 * @override
	 */
	public set outerHTML(html: string) {
		this.content.replaceWith(html);
	}

@Mas0nShi
Copy link
Contributor Author

maybe you can create a new PR for yourself? I will close this half-fixes PR.

@capricorn86
Copy link
Owner

replaceWith method is exists?

	/**
	 * @override
	 */
	public set outerHTML(html: string) {
		this.content.replaceWith(html);
	}

Here it would be enough to throw an exception as it is not possible to set outerHTML.

 	/**
 	 * @override
 	 */
 	public set outerHTML(_html: string) {
 		throw new DOMException(`Failed to set the 'outerHTML' property on 'Element': This element has no parent node.`);
 	}
```;

@capricorn86
Copy link
Owner

@Mas0nShi I have updated my code example

@Mas0nShi
Copy link
Contributor Author

@Mas0nShi I have updated my code example

copy that

@Mas0nShi
Copy link
Contributor Author

Mas0nShi commented Jun 15, 2022

Note
done. We may need more test units now.

@capricorn86 capricorn86 merged commit 7667c0a into capricorn86:master Jun 28, 2022
@capricorn86
Copy link
Owner

Good job! I will add some more unit tests 👍

@Mas0nShi Mas0nShi deleted the 451-fixes-missing-innerHTML-in-html-template branch June 28, 2022 10:22
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.

innerHTML broken for template elements
2 participants