Skip to content
This repository has been archived by the owner on May 27, 2018. It is now read-only.

use template.content #2

Open
kapouer opened this issue Apr 28, 2017 · 37 comments
Open

use template.content #2

kapouer opened this issue Apr 28, 2017 · 37 comments

Comments

@kapouer
Copy link

kapouer commented Apr 28, 2017

Because

template tags allow any HTML (even elements out of context)
@see https://developer.mozilla.org/en-US/docs/Web/HTML/Element/template

https://github.com/straker/html-tagged-template/blob/c06f23ac/index.js#L150-L157

@loilo
Copy link
Member

loilo commented Apr 30, 2017

Good point, thanks! Didn't actually think about that.

@loilo
Copy link
Member

loilo commented Apr 30, 2017

This actually raises another issue: The <template> element is also part of ES2015 and thus not available in IE11 and below.

I'm sure how to approach that. I'd probably

  • use HTMLTemplateElement.content whenever available

  • provide a compat method which receives a parameter that can override the internally created container tag name, so something like this would be possible:

    domify.compat('table') `<tr><td>Table cells, heck yeah!</td></tr>`

That solution is not particularly pretty, but I don't think there's a better one for browsers not supporting <template>s.

Also that means that this way, users can decide on their own if they need to do this for compatibility reasons (i.e. "if they do support IE11") or not.

@kapouer
Copy link
Author

kapouer commented Apr 30, 2017

There is actually a polyfill for the template element, to be released soon-ish i believe.

@loilo
Copy link
Member

loilo commented May 1, 2017

Right, but the polyfill does not (and probably cannot) fix the problem of out-of-context elements. It will fail having <tr>s as top level element just like the current domify version does.

I suppose this is just not fixable.

@loilo
Copy link
Member

loilo commented May 1, 2017

Though it is to add that jQuery manages to do that correctly, no matter the browser. I suppose they use some way more sophisticated algortihm though.

@kapouer
Copy link
Author

kapouer commented May 1, 2017

@loilo
Copy link
Member

loilo commented May 1, 2017

Also, we could just generally use a <table> as fallback container. Though formally invalid, It seems to render all non-<tr> children just fine.

However, before doing that we should figure out

  • if there are more cases like isolated <tr> nodes (which would make the always-<table>-solution not applicable).
  • if the <table> solution works in every browser.

@kapouer
Copy link
Author

kapouer commented May 1, 2017

Template engines just grep for and wrap them, then unwrap them.
Always wrapping in a table tag is going to make a lot of weird troubles like

> document.body.innerHTML = '<table><table></table></table>'
'<table><table></table></table>'
> document.body.innerHTML
'<table></table><table></table>'

@kapouer
Copy link
Author

kapouer commented May 1, 2017

@kapouer
Copy link
Author

kapouer commented May 1, 2017

Also... this problem is unrelated to using template.content, is it ? domify just had that problem before ?

@kapouer
Copy link
Author

kapouer commented May 1, 2017

(on IE11 i mean)

@loilo
Copy link
Member

loilo commented May 1, 2017

Okay, so wrapping in <table> doesn't work.

Well, this is kinda related to template.content in so far that currently the misbehaviour of not-working <tr> tags is consistent across browsers.

If we'd introduce a working way with template.content, I'd like to avoid inconsistent behaviour in different browsers as far as possible.

@kapouer
Copy link
Author

kapouer commented May 1, 2017

I'm pretty sure browsers supporting natively <template> tag support topmost <tr> tags (see for example this mdn doc).
The problem here is IE and maybe other old browsers. It's not up to domify-template-strings to fix that problem. However, fixing it in the template polyfill i refered to looks possible.

(EDIT: fixing bad english style)

@loilo
Copy link
Member

loilo commented May 1, 2017

Yes, <template> tag supporting browsers are not our problem here, they create toplevel <tr>s hands down.

I agree that fixing templates is not in the primary scope of this project. However, since we cannot point to a working polyfill instead, I'd still insist on having an IE11-compatible solution—probably the way jQuery did it.

I'd be happy to drop that fallback as soon as a working template polyfill is available. Maybe we should open a pull request over there at the polyfill.

@kapouer
Copy link
Author

kapouer commented May 1, 2017

on it :)

@kapouer
Copy link
Author

kapouer commented May 1, 2017

webcomponents/template#16

Granted, webcomponents/template is not well maintained at the moment.

@loilo
Copy link
Member

loilo commented May 1, 2017

Cool. I'm curious how responsive the folks over there are. Some issues didn't seem to get attention in quite a while.

@kapouer
Copy link
Author

kapouer commented May 1, 2017

They're not. At some point someone will carry on the duties and things will go faster. Hopefully...

@loilo
Copy link
Member

loilo commented May 1, 2017

We'll see.

However, you might have to revisit your contribution as it currently cannot handle some edge cases with multiple nodes.

Consider this example:

wrapMap('<!--Comment--><tr><td>Content</td></tr>')

This will return the default wrap map. Thus the <tr> probably (just telling from reading, didn't test it myself) will not be handled.

Maybe have another look at the jQuery source, they correctly handle all quirky edge cases I came up with:

// Correctly returns the comment and the row.
$('<!--Comment--><tr><td>Content</td></tr>')

// Correctly returns the comment and the row.
// Does not accidentally wrap `<tr>` in comment content with `<table>` tags.
$('<!--<tr><td>Comment</td></tr>--><tr><td>Content</td></tr>') // 

You may also want to add those two (or similar) cases to your tests there.

@kapouer
Copy link
Author

kapouer commented May 1, 2017

True, i think i just "fixed" one case, and not sure about the other. Added tests too.
I'm in TLWTDD (too late without test development driven) mode so i'll leave it for a moment now.

@loilo
Copy link
Member

loilo commented May 1, 2017

I also found this suspiciously not-upvoted snippet on StackOverflow that survives all cases I tried for the moment:

function createFragment(html){
    var tmpl = document.createElement('template');
    tmpl.innerHTML = html;
    if (tmpl.content == void 0){ // ie11
        var fragment = document.createDocumentFragment();
        var isTableEl = /^[^\S]*?<(t(?:head|body|foot|r|d|h))/i.test(html);
        tmpl.innerHTML = isTableEl ? '<table>'+html : html;
        var els        = isTableEl ? tmpl.querySelector(RegExp.$1).parentNode.childNodes : tmpl.childNodes;
        while(els[0]) fragment.appendChild(els[0]);
        return fragment;
    }
    return tmpl.content;
}

It creates a document fragment from arbitrary HTML code with template.content and an IE fallback in whopping 13 lines of code.

You may throw some test strings at it if you like, but it looks like a sane solution to me.

EDIT: No wonder that function magically worked if I'm only testing it in modern browsers...

@loilo
Copy link
Member

loilo commented May 1, 2017

The "more sophisticated algorithm" of jQuery seems to be this function, by the way. That one really goes deep into the HTML string.

@loilo
Copy link
Member

loilo commented May 1, 2017

Also, the Dojo Toolkit has such a method. Maybe we should get inspiration from their approach, since their toDom() method is, as opposed to jQuery's, designed for external usage (and thus probably easier to extract).

@kapouer
Copy link
Author

kapouer commented May 2, 2017

The one you point on jquery is actually going deep for <script> processing.
The Dojo method is very very similar to the one i copied from jquery. The main difference is using firstChild instead of lastChild when extracting the wrapped nodes - and a bit more tags, and IE8 compat (but really, targeting IE8 is insane here).

@loilo
Copy link
Member

loilo commented May 2, 2017

Agreed to the last, IE11 is all I'd target for.

It's pretty natural that the dojo method is similar to the jQuery one, they should basically do the same thing. I just mentioned it here because you seemed to have copied only parts of the jQuery method (which is the reason that more complex test cases like the mentioned ones with comments do fail). Just in case that it's easier to extract from there.

@kapouer
Copy link
Author

kapouer commented May 2, 2017

Not exactly, i'm now testing on IE11 and the problems with comments come from serialization (the innerHTML getter of the template polyfill), not from parsing :)

@loilo
Copy link
Member

loilo commented May 2, 2017

I might have taken a too short look on your implementation. Isn't your wrapMap function applied once to the HTML string?

@kapouer
Copy link
Author

kapouer commented May 2, 2017

Yes it is, but jquery and dojo do the same. jQuery is misleading: it always accept an array of elements to process, so you always find some iterator before actual stuff is being done.
Anyway, THROW TESTS AT ME ! i'm passing the ones you gave above now (and the original template ones).

@loilo
Copy link
Member

loilo commented May 2, 2017

It's cool, I guess we're pretty good to go already if the above tests do pass. 👍

@kapouer
Copy link
Author

kapouer commented May 2, 2017

I'm having a look at Edge, IE10, IE9, and some more, just in case.

@kapouer
Copy link
Author

kapouer commented May 2, 2017

IE10 is bad :( but i have a clue at what's going on.

@kapouer
Copy link
Author

kapouer commented May 2, 2017

IE10 pass tests now :) this template polyfill was made in a hurry...

@loilo
Copy link
Member

loilo commented May 2, 2017

If you got this all running, maybe consider publishing the solution as a standalone package. 👍

I haven't seen a good one so far and we could require it as a dependency here. Would definitely give us a better separation of concerns. :)

@loilo
Copy link
Member

loilo commented May 2, 2017

(What I mean: not the Template polyfill itself but a generic "convert-html-to-DOM" package.)

EDIT: Recovered my initial comment and removed all the followup gibberish that did not make any sense. :)

@kapouer
Copy link
Author

kapouer commented May 2, 2017

I won't have time for that, but i released "@kapouer/template" version 1.0.1 with IE9-10-11 compatibility.

@loilo
Copy link
Member

loilo commented May 2, 2017

Okay. I probably won't have too much time during the week either, but let's see what we can accomplish. Thanks for all your contributions so far! 👍

@kapouer
Copy link
Author

kapouer commented May 2, 2017

FYI i forked because i actually need to concentrate on a project with particular needs. I kept you in the authors/LICENSE. Also BTW you're missing a LICENSE file.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants