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.

Sign up for GitHub

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

Jump to bottom

I don't exactly trust m.trust... 😉 #2310

Open
dead-claudia opened this issue Nov 26, 2018 · 9 comments
Open

I don't exactly trust m.trust... 😉 #2310

dead-claudia opened this issue Nov 26, 2018 · 9 comments
Assignees
@dead-claudia
Labels
Area: Core For anything dealing with Mithril core itself Type: Bug For bugs and any other unexpected breakage

Comments

@dead-claudia
Copy link
Member

dead-claudia commented Nov 26, 2018 •
edited
Loading

Expected Behavior

m.trust should be a bit more agnostic about its surrounding environment, and it shouldn't be so overly complicated.

Current Behavior

m.trust currently uses a really ugly series of hacks to render itself. It also only works with some HTML and SVG.

Possible Solution

Let's simplify it to something closer to this:

Edit: Simplify it further

var fragment = $doc.createDocumentFragment()
function createHTML(parent, vnode, nextSibling) {
	// Not using the proper parent makes the child element(s) vanish.
	//     var div = document.createElement("div")
	//     div.innerHTML = "<td>i</td><td>j</td>"
	//     console.log(div.innerHTML)
	// --> "ij", no <td> in sight.
	//
	// The below code does this generically regardless of namespace or tag name.
	var temp = parent.firstChild == null ? parent : $doc.createElementNS(parent.nodeName, parent.namespaceURI)
	temp.innerHTML = vnode.children
	vnode.dom = temp.firstChild
	vnode.domSize = temp.childNodes.length
	if (temp !== parent) {
		var child
		while (child = temp.firstChild) fragment.appendChild(child)
		insertNode(parent, fragment, nextSibling)
	}
}

This is significantly smaller (like about 100 bytes saved), but I need to benchmark the rendering improvement though (it's not tested there currently). It does implement the optimization of if no other children exist, it can just render directly to the node and avoid the indirection.

Steps to Reproduce (for bugs)

Context

Just noticed it was weird, and that we were mostly just doing the wrong thing here.

Your Environment

  • Version used:
  • Browser Name and version:
  • Operating System and version (desktop or mobile):
  • Link to your project:
@dead-claudia dead-claudia added Type: Bug For bugs and any other unexpected breakage Type: Enhancement For any feature request or suggestion that isn't a bug fix labels Nov 26, 2018
@dead-claudia dead-claudia self-assigned this Nov 26, 2018
@ChrisGitIt
Copy link

Hi @isiahmeadows,

do you have like any examples for a weird m.trust behavior? I'm using it for almost all my user generated markdown => html output and haven't experienced any problems ... which i actually expected. The only problem that i got is when using m.trust within an array, like:

m('div', [
  m('span),
  m.trush('<my><custom><html> ....')
])

@dead-claudia
Copy link
Member Author

@ChrisGitIt There's not any real pitfalls with the current implementation unless you're using MathML (which no special case exists for). This whole bug is just about future-proofing the relevant code so we don't have to update it for every little spec change and in case browsers start adding some non-standard whatever that needs "supported". It's also to simplify and optimize it.

@barneycarroll
Copy link
Member

Would this bring back script execution?

@dead-claudia
Copy link
Member Author

@barneycarroll No. We already use innerHTML, so this does nothing that doesn't already.

@dead-claudia
Copy link
Member Author

Although that "fixed" snippet in the bug description is incredibly wrong. parent isn't necessarily a DOM element - we'd need to pass the real parent along as well.

@StephanHoyer
Copy link
Member

@dead-claudia still want to do this?

@dead-claudia
Copy link
Member Author

I'd like to revisit it, but it's not high-priority.

@pygy
Copy link
Member

pygy commented May 31, 2022

@dead-claudia historically IIRC innerHTML was HTML-only, and we left out MathML because of lack of cross-browser support (and user demand). A parent-aware API is the correct solution.

Aside rather than passing the parent and nextSibling around, we may use the stack-managed "global" trick this could save some stack space and some perf.

@dead-claudia
Copy link
Member Author

@pygy I'm aware of that historical bit.

Aside rather than passing the parent and nextSibling around, we may use the stack-managed "global" trick this could save some stack space and some perf.

Stack space is cheap - we could go well over a thousand nodes deep without issue (we aren't computing the Ackermann function here). The concern is copying, but stuff near the top of the CPU stack are normally in either registers or L1 cache already (and if we're actively using it, that's where it belongs), so it's really a balancing act in practice.

Also, we'd have to basically do shotgun surgery on our render in order to put that in. I've already tried once years ago, and quickly ran into that issue. (Also, perf didn't impress, or else I would've filed a PR back then.)

@dead-claudia dead-claudia moved this to Low priority in Triage/bugs Sep 2, 2024
@dead-claudia dead-claudia added Area: Core For anything dealing with Mithril core itself and removed Type: Enhancement For any feature request or suggestion that isn't a bug fix labels Sep 2, 2024
@dead-claudia dead-claudia mentioned this issue Oct 13, 2024
Open
8 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@dead-claudia dead-claudia

Labels
Area: Core For anything dealing with Mithril core itself Type: Bug For bugs and any other unexpected breakage
Projects
Triage/bugs
Status: Low priority
Milestone
No milestone
Development

No branches or pull requests
5 participants
@pygy @StephanHoyer @barneycarroll @ChrisGitIt @dead-claudia